OAuth2: Add samesite attribute support for all OAuth2 supported cookie types#37952
Merged
mattklein123 merged 14 commits intoenvoyproxy:mainfrom Jan 17, 2025
Merged
Conversation
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
|
CC @envoyproxy/coverage-shephards: FYI only for changes made to |
Contributor
|
/assign @mattklein123 |
Contributor
Author
|
/retest |
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Contributor
Author
|
/retest |
abeyad
reviewed
Jan 10, 2025
mattklein123
requested changes
Jan 11, 2025
Member
mattklein123
left a comment
There was a problem hiding this comment.
LGTM at a high level modulo outstanding comments.
/wait
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
b00cb60 to
6e5ef61
Compare
Contributor
Author
|
/ptal @abeyad re-approve required, thx! |
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
Contributor
Author
|
/retest |
Contributor
Author
|
/ptal @mattklein123 |
abeyad
previously approved these changes
Jan 14, 2025
Contributor
abeyad
left a comment
There was a problem hiding this comment.
/lgtm api
you will also need @mattklein123 approval for the entirety of the PR
Signed-off-by: Yueren Wang <yuerenwang@lyft.com>
mattklein123
previously approved these changes
Jan 15, 2025
Member
|
Sorry needs main merge, thanks. /wait |
Signed-off-by: Yueren Wang <yuerenwang@tm4wwrxcwd.tailbaa43.ts.net>
Contributor
Author
just merged main. all CI passed |
Signed-off-by: Yueren Wang <yuerenwang@tm4wwrxcwd.tailbaa43.ts.net>
Contributor
Author
|
/ptal @mattklein123 Hi matt, friendly ping again. really wanted to get this merged in to unblock ourselves. |
Contributor
Author
|
/retest |
mattklein123
approved these changes
Jan 17, 2025
bazmurphy
pushed a commit
to bazmurphy/envoy
that referenced
this pull request
Jan 29, 2025
…e types (envoyproxy#37952) 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 <yuerenwang@lyft.com> Signed-off-by: Yueren Wang <yuerenwang@tm4wwrxcwd.tailbaa43.ts.net> Co-authored-by: Yueren Wang <yuerenwang@tm4wwrxcwd.tailbaa43.ts.net>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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