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 #37952

Merged

Conversation

Yueren-Wang
Copy link
Contributor

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:

bearer_token_cookie
hmac_cookie
expires_cookie
id_token_cookie
refresh_token_cookie
nonce_cookie
The samesite attribute value allowed are:

Strict
Lax
None
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 added

Signed-off-by: Yueren Wang <[email protected]>
Copy link

CC @envoyproxy/coverage-shephards: FYI only for changes made to (test/per_file_coverage.sh).
envoyproxy/coverage-shephards assignee is @RyanTheOptimist
CC @envoyproxy/api-shepherds: Your approval is needed for changes made to (api/envoy/|docs/root/api-docs/).
envoyproxy/api-shepherds assignee is @abeyad
CC @envoyproxy/api-watchers: FYI only for changes made to (api/envoy/|docs/root/api-docs/).

🐱

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

see: more, trace.

Signed-off-by: Yueren Wang <[email protected]>
@RyanTheOptimist
Copy link
Contributor

/assign @mattklein123
Since it's oauth related

@Yueren-Wang
Copy link
Contributor Author

/retest

@Yueren-Wang
Copy link
Contributor Author

/retest

api/envoy/extensions/filters/http/oauth2/v3/oauth.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/oauth2/v3/oauth.proto Outdated Show resolved Hide resolved
api/envoy/extensions/filters/http/oauth2/v3/oauth.proto Outdated Show resolved Hide resolved
changelogs/current.yaml Outdated Show resolved Hide resolved
changelogs/current.yaml Outdated Show resolved Hide resolved
changelogs/current.yaml Outdated Show resolved Hide resolved
Copy link
Member

@mattklein123 mattklein123 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM at a high level modulo outstanding comments.

/wait

source/extensions/filters/http/oauth2/filter.cc Outdated Show resolved Hide resolved
test/per_file_coverage.sh Outdated Show resolved Hide resolved
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
abeyad
abeyad previously approved these changes Jan 13, 2025
Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

@Yueren-Wang Yueren-Wang force-pushed the yueren-wang-samesite-cookie-support-3 branch from b00cb60 to 6e5ef61 Compare January 14, 2025 00:35
@Yueren-Wang
Copy link
Contributor Author

/ptal @abeyad re-approve required, thx!

Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
Signed-off-by: Yueren Wang <[email protected]>
@Yueren-Wang
Copy link
Contributor Author

/retest

@Yueren-Wang
Copy link
Contributor Author

/ptal @mattklein123

abeyad
abeyad previously approved these changes Jan 14, 2025
Copy link
Contributor

@abeyad abeyad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm api

you will also need @mattklein123 approval for the entirety of the PR

Signed-off-by: Yueren Wang <[email protected]>
mattklein123
mattklein123 previously approved these changes Jan 15, 2025
@mattklein123
Copy link
Member

Sorry needs main merge, thanks.

/wait

Signed-off-by: Yueren Wang <[email protected]>
@Yueren-Wang
Copy link
Contributor Author

Sorry needs main merge, thanks.

/wait

just merged main. all CI passed

Signed-off-by: Yueren Wang <[email protected]>
@Yueren-Wang
Copy link
Contributor Author

/ptal @mattklein123 Hi matt, friendly ping again. really wanted to get this merged in to unblock ourselves.

@Yueren-Wang
Copy link
Contributor Author

/retest

@mattklein123 mattklein123 merged commit d76115f into envoyproxy:main Jan 17, 2025
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants