Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OAuth2: Add samesite attribute support for all OAuth2 supported cookie types #37917

Conversation

Yueren-Wang
Copy link

@Yueren-Wang Yueren-Wang commented Jan 8, 2025

Commit Message: OAuth2: Add samesite attribute support for all OAuth2 supported cookie types

Additional Description: The SameSite attribute offers three values to control whether cookies are shared within the same site or across different sites. It's an optional setting, with a "Disabled" option that omits the SameSite attribute altogether. By default, this setting is disabled to ensure no changes are made to existing deployments, but operators now have the option to enable SameSite. The six cookies supporting SameSite attribute are:

  1. bearer_token_cookie
  2. hmac_cookie
  3. expires_cookie
  4. id_token_cookie
  5. refresh_token_cookie
  6. nonce_cookie

The samesite attribute value allowed are:

  1. Strict
  2. Lax
  3. None
  4. Disabled (Default, if no value is set in config)

The operator can also optionally do not specify any SameSite attributes for cookie. This will result DISABLED value to be set for all cookie's SameSite attribute value. in this case no same site attribute will be returned by filter.

The operator can also choose different same site attribute to be configured by different cookies. This means the SameSite attributes for different cookies listed above can be different. Also the operator can optionally specify SameSite attribute for some cookie but miss it for others. it is not mandatory to specify SameSite explicitly for all cookies

Risk Level: Medium
Testing: unit
Docs Changes: proto is documented
Release Notes: changelog entry pending, adding in next iteration

Copy link

CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @adisuissa
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

Caused by: #37917 was opened by Yueren-Wang.

see: more, trace.

@Yueren-Wang
Copy link
Author

ready for review. all unit tests added and passed

@Yueren-Wang Yueren-Wang changed the title OAuth2: Add samesite attribute support for all cookies OAuth2: Add samesite attribute support for all OAuth2 supported cookie types Jan 9, 2025
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
@Yueren-Wang Yueren-Wang force-pushed the yueren-wang-samesite-cookie-support branch from cf8679c to 5f0d302 Compare January 9, 2025 19:33
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist

🐱

Caused by: #37917 was synchronize by Yueren-Wang.

see: more, trace.

Yueren-Wang and others added 10 commits January 9, 2025 14:45
Signed-off-by: Yueren Wang <[email protected]>
…en-Wang/envoy into yueren-wang-samesite-cookie-support
…y#37832)

An LRU cache was introduced to cache `Envoy::Network::Address` instances
because they are expensive to create. These addresses are cached for
reading source and destination addresses from `recvmsg` and `recvmmsg`
calls on QUIC UDP sockets. The current size of the cache is 4 entries
for each IoHandle (i.e. each socket).

A locally run CPU profile of Envoy Mobile showed about 1.75% of CPU
cycles going towards querying and inserting into the
`quic::QuicLRUCache`.

Given the small number of elements in the cache, this commit uses a
`std::vector` data structure instead of `QuicLRUCache`. `QuicLRUCache`,
`std::vector`, and `std::deque` were compared using newly added
benchmark tests, and the following were the results:

QuicLRUCache:
```
-------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                               Time             CPU   Iterations
-------------------------------------------------------------------------------------------------------------------------
BM_GetOrCreateEnvoyAddressInstanceNoCache/iterations:1000                           31595 ns        31494 ns         1000
BM_GetOrCreateEnvoyAddressInstanceConnectedSocket/iterations:1000                    5538 ns         5538 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocket/iterations:1000                 38918 ns        38814 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocketLargerCache/iterations:1000      52969 ns        52846 ns         1000
```

std::deque:
```
-------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                               Time             CPU   Iterations
-------------------------------------------------------------------------------------------------------------------------
BM_GetOrCreateEnvoyAddressInstanceNoCache/iterations:1000                           31805 ns        31716 ns         1000
BM_GetOrCreateEnvoyAddressInstanceConnectedSocket/iterations:1000                    1553 ns         1550 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocket/iterations:1000                 27243 ns        27189 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocketLargerCache/iterations:1000      39335 ns        39235 ns         1000
```

std::vector:
```
-------------------------------------------------------------------------------------------------------------------------
Benchmark                                                                               Time             CPU   Iterations
-------------------------------------------------------------------------------------------------------------------------
BM_GetOrCreateEnvoyAddressInstanceNoCache/iterations:1000                           31960 ns        31892 ns         1000
BM_GetOrCreateEnvoyAddressInstanceConnectedSocket/iterations:1000                    1514 ns         1514 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocket/iterations:1000                 26361 ns        26261 ns         1000
BM_GetOrCreateEnvoyAddressInstanceUnconnectedSocketLargerCache/iterations:1000      43987 ns        43738 ns         1000
```

`std::vector` uses 3.5x less CPU cycles than `quic::QuicLRUCache` and
performs very slightly better than `std::deque` at small cache sizes. If
considering creating a bigger cache size (e.g. >= 50 entries),
`std::deque` may perform better and it's worth profiling, though in such
a situation, no cache at all seems to perform better than a cache.

Risk Level: low
Testing: unit and benchmark tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

---------

Signed-off-by: Ali Beyad <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
agrawroh and others added 28 commits January 9, 2025 14:51
)

## Description

This PR rewords the
[example](https://www.envoyproxy.io/docs/envoy/latest/intro/arch_overview/upstream/aggregate_cluster#load-balancing-example)
we have in the aggregate cluster to explain how the cluster level load
balancer selects the cluster.

Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
https://github.com/google/quiche/compare/71111c723..e599ad25d

```
$ git log 71111c723..e599ad25d --date=short --no-merges --format="%ad %al %s"

2025-01-06 martinduke Refresh MoQT integration test for FETCH and fix detected errors.
```

Risk Level: low
Testing: n/a
Docs Changes: n.a
Release Notes:  n/ah
Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Fix envoyproxy#36432

Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com>
Signed-off-by: Yueren Wang <[email protected]>
…nvoyproxy#37776)

Reapply the RBAC change by reverting
envoyproxy#35899.

The original commit is reverted for flaky RBAC integration test.

The fix is pretty simple by just removing the simulated time in the test
([diff](envoyproxy@d3297b4)),
I'm not sure of the root cause but the test does not really need the
simulated test.

Before the fix, the flaky test can be reproduced easily with
`--runs_per_test=1000`, After the test, I can no longer reproduce
the failure. To be on the safer side, I ran the test with
`--runs_per_test=10000` and still couldn't reproduce the test failure.

Test output:
```
 % bazel test //test/extensions/filters/network/rbac:integration_test --runs_per_test=10000  --test_timeout=60 --test_env=ENVOY_IP_TEST_VERSIONS=v4only
INFO: Invocation ID: c1ebbc18-4887-4626-9d19-e257d42fe4cd
INFO: Analyzed target //test/extensions/filters/network/rbac:integration_test (0 packages loaded, 19 targets configured).
INFO: Found 1 test target...
Target //test/extensions/filters/network/rbac:integration_test up-to-date:
  bazel-bin/test/extensions/filters/network/rbac/integration_test
INFO: Elapsed time: 2486.472s, Critical Path: 24.73s
INFO: 10001 processes: 1 internal, 10000 processwrapper-sandbox.
INFO: Build completed successfully, 10001 total actions

Executed 1 out of 1 test: 1 test passes.
```

---------

Signed-off-by: Yangmin Zhu <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…`wamr` -> 2.2.0 (envoyproxy#37868)

Commit Message: deps: Update `proxy_wasm_cpp_host` -> c4d7bb0,
`wasmtime` -> 24.0.2, `wamr` -> 2.2.0
Additional Description:
proxy-wasm/proxy-wasm-cpp-host@f199214...c4d7bb0
Risk Level: low
Testing: `bazel test test/...` passes, with `--define=wasm=v8`,
`--define=wasm=wamr`, and `--define=wasm=wasmtime`.
Docs Changes: None.
Release Notes: Mentioned new support for [Go
SDK](github.com/proxy-wasm/proxy-wasm-go-sdk) plugins.

Supercedes envoyproxy#36880 and envoyproxy#36857

---------

Signed-off-by: Matt Leon <[email protected]>
Signed-off-by: Keith Mattix II <[email protected]>
Co-authored-by: Ryan Northey <[email protected]>
Co-authored-by: Keith Mattix II <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Fix envoyproxy#37553

Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com>
Signed-off-by: Yueren Wang <[email protected]>
…proxy#37897)

Currently this file is getting the includes transitively though Abseil, and I am trying to remove the includes from Abseil without breaking Envoy.

Risk Level: low
Testing: N/A
Docs Changes: N/A
Release Notes: N/A
Platform Specific Features: N/A
Signed-off-by: Derek Mauro <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Commit Message: [mobile] Report network subtype when on cellular
Additional Description: Cellular networks will be further categorized as
2G/3G/4G/5G
Risk Level: low
Testing: unit tests
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: android only

Signed-off-by: Renjie Tang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
The ability to truncate a file is a prerequisite for the next round of
file-based cache.

---------

Signed-off-by: Raven Black <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…envoyproxy#37899)

I tested dynamic modules from rust and was getting a segfault. I noticed
that drop was getting called twice. It turns out that `onDestroy` gets
called twice. The right solution here is probably to prevent
`onDestroy()` from being called twice... but barring that, this change
works.

Commit Message: dynamic_modules: don't call on_http_filter_destroy_ if
filter is null
Additional Description:
Risk Level:
Testing:
Docs Changes:
Release Notes:
Platform Specific Features:
[Optional Runtime guard:]
[Optional Fixes #Issue]
[Optional Fixes commit #PR or SHA]
[Optional Deprecated:]
[Optional [API
Considerations](https://github.com/envoyproxy/envoy/blob/main/api/review_checklist.md):]

---------

Signed-off-by: Ben Plotnick <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Commit Message: support backoff between retries in udp_proxy
Additional Description: This feature also fixes a bug in the retry
mechanism in udp proxy in the following situation:
1. We are tunneling UDP over HTTP.
2. A new stream is created on existing upstream connection
(multiplexing) and waiting for response headers.
3. The upstream connection is closed.
4. During the close process, all streams created on this connection will
be reset.
5. The udp_proxy receives a callback on the stream reset.
6. They retry to connect.
7. The closed connection is picked as it is still in the connection pool
(we are still in the process of the close).
8. The new stream that is created on the second attempt will be reset
immediately.
9. The same process will happen (step 5 - step 8) until we reach the
max_connect_attepts.
10. This means that we are doing only one real attempt in this
situation.

Risk Level: medium
Testing: unit tests, integration tests
Docs Changes: added
Release Notes: added

---------

Signed-off-by: Issa Abu Kalbein <[email protected]>
Co-authored-by: Issa Abu Kalbein <[email protected]>
…ision (envoyproxy#37794)

Commit Message: ext_proc: change default sampling decision to false
Fixes: envoyproxy#37783
Additional Description: Default to using the parent span's sampled
status instead of true (default in
[StreamOptions](https://github.com/envoyproxy/envoy/blob/df5260957fc9446b57114a74ffbdc5d9b5831bfd/envoy/http/async_client.h#L315-L316)),
so that we don't emit orphan spans for all ext_proc streams where the
parent span is not sampled. That's the same issue seen in Lua httpCall
(envoyproxy#33200) and ext_authz
(envoyproxy#19343).

Risk Level: Low
Testing: changed the tracing integration test behavior and validated in
production.

---------

Signed-off-by: Fernando Cainelli <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…mobile-android config (envoyproxy#37936)

This moves `--noincompatible_enable_android_toolchain_resolution` to
`--config=mobile-android` so that it works on both local and remote
builds.

Risk Level: low
Testing: CI
Docs Changes: n/a
Release Notes: n/a
Platform Specific Features: n/a

Signed-off-by: Fredy Wijaya <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: dependency-envoy[bot] <148525496+dependency-envoy[bot]@users.noreply.github.com>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Commit Message: Add `envoy_dynamic_module_callback_http_send_response`
to the dynamic modules ABI and rust SDK
Additional Description: This adds the ability to send a local response
back. I've also fixed up `abi_impl_test.cc`. Before, it was not
asserting on the correct headers. Now i've moved to parameterized tests
so we can assert on the correct headers and also split out the tests
into smaller units.
Risk Level: low
Testing: unit tests
Docs Changes:
Release Notes:
Platform Specific Features:

---------

Signed-off-by: Ben Plotnick <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…T_STREAM (envoyproxy#37784)

We have found this patch useful in our testing.

The delta between the first and second commit demonstrates test coverage
for the new code.

The new behavior can be disabled with the runtime guard
`envoy.reloadable_features.http2_propagate_reset_events`.

Commit Message: propagates reset events to CodecEventCallbacks when
sending RST_STREAM
Additional Description:
Risk Level: low
Testing: ran unit and integration tests locally
Docs Changes:
Release Notes:
Platform Specific Features:
Runtime guard: `envoy.reloadable_features.http2_propagate_reset_events`

---------

Signed-off-by: Biren Roy <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Ryan Northey <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…envoyproxy#37627)

After the recreatStream() call, both the
decode and encode filter chains will abort. Here, by checking if the
filter chain has aborted, it prevents the subsequent filters in the
filter chain from continuing.

Risk Level: Low
Testing: Add tests to verify that when recreateStream is executed
during the decodeHeader/encodeHeader phase and returns StopIteration,
executing continueDecoding/continueEncoding does not proceed with the
processing of subsequent filters

Runtime guard: envoy.reloadable_features.filter_chain_aborted_can_not_continue
Fixes envoyproxy#37625

---------

Signed-off-by: zty98751 <[email protected]>
Signed-off-by: 澄潭 <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Commit Message: upstream lb: optimize the upstream load balancer
Additional Description:

1. Splitted the legacy LB configuration processing out to independent
method. The loadConfig() will always take the lb's extension proto now.
It's more clear and more match to common sense.
2. reduce exceptions when loading subset lb configuration.

Risk Level: low.
Testing: n/a.
Docs Changes: n/a.
Release Notes: n/a.
Platform Specific Features: n/a.

---------

Signed-off-by: wangbaiping/wbpcode <[email protected]>
Signed-off-by: wangbaiping(wbpcode) <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Fixes some notes in the AWS Credentials docs and reword some text to make it clear.

Risk Level: N/A
Testing: N/A
Docs Changes: yes
Release Notes: N/A
Signed-off-by: Rohit Agrawal <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Risk Level: low
Testing: updated tests
Docs Changes: n/a
Release Notes: n/a
envoyproxy/envoy-mobile#176

Signed-off-by: Alyssa Wilk <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
…en-Wang/envoy into yueren-wang-samesite-cookie-support
@Yueren-Wang
Copy link
Author

closed due to messed up rebased commits. please refer to #37952

@Yueren-Wang Yueren-Wang closed this Jan 9, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.