feature(translator): expose OIDC oauth cookie samesite configuration

[vibe]

Envoy added support for SameSite configuration on all oidc cookies. Currently in order to override cookies, you need an elaborate EnvoyPatchPolicy, for a better experience we should provide and map dev-friendly values.

Which issue(s) this PR fixes:
Fixes #5229

Release Notes: Yes

[zhaohuabing]

Hi @vibe thanks for working on this!

Would it make sense to set default to strict and avoid exposing this setting to the end users?

These cookies are just trivial implementation details of the Envoy OAuth2 filter, so I’d prefer not to expose them through the API.

        BearerToken  *OIDCCookieConfig `json:"bearerToken,omitempty"`
	OauthHmac    *OIDCCookieConfig `json:"oauthHmac,omitempty"`
	OauthExpires *OIDCCookieConfig `json:"oauthExpires,omitempty"`
	IdToken      *OIDCCookieConfig `json:"idToken,omitempty"`
	RefreshToken *OIDCCookieConfig `json:"RefreshToken,omitempty"`
	OauthNonce   *OIDCCookieConfig `json:"oauthNonce,omitempty"`
	CodeVerifier *OIDCCookieConfig `json:"codeVerifier,omitempty"`

Some related discussions and issues:

• api(oidc): Allow override of all OIDC CookieNames #5655 (comment)
• Option to overwrite every OAuth2Filter Cookie via OIDCCookieNames #5152
• OAuth2: remove internal cookies before forwarding request to backend envoy#39196

[vibe]

Hi @vibe thanks for working on this!

Would it make sense to set default to strict and avoid exposing this setting to the end users?

These cookies are just trivial implementation details of the Envoy OAuth2 filter, so I’d prefer not to expose them through the API.

        BearerToken  *OIDCCookieConfig `json:"bearerToken,omitempty"`
	OauthHmac    *OIDCCookieConfig `json:"oauthHmac,omitempty"`
	OauthExpires *OIDCCookieConfig `json:"oauthExpires,omitempty"`
	IdToken      *OIDCCookieConfig `json:"idToken,omitempty"`
	RefreshToken *OIDCCookieConfig `json:"RefreshToken,omitempty"`
	OauthNonce   *OIDCCookieConfig `json:"oauthNonce,omitempty"`
	CodeVerifier *OIDCCookieConfig `json:"codeVerifier,omitempty"`

Some related discussions and issues:

• api(oidc): Allow override of all OIDC CookieNames #5655 (comment)
• Option to overwrite every OAuth2Filter Cookie via OIDCCookieNames #5152
• OAuth2: remove internal cookies before forwarding request to backend envoy#39196

Hey @zhaohuabing , totally understand wanting to keep the API small, that being said in practice SameSite often needs to vary per-deployment (and sometimes per-cookie) to keep OIDC flows working while still giving users/developers the option to harden where they can. This PR is a result of a production need in our organization, where we currently use an EnvoyPatchPolicy to set cookie same site config to None.

In theory we should definitely default to strict and leave it at that, but it looks like the underlying default is Disabled which is why I chose disabled as the default, as to not change that behavior. I definitely see other teams in an organization requiring this policy to be mutable and would be painful for everyone to maintain a patch policy for something that could be as simple as a configuration value.

Open to any thoughts! Thanks for reviewing

[vibe]

Just read through the links you provided, would it be reasonable to have a middle ground of keeping bearer token, id token, and refresh token as configurable? Though I haven't thought through the implications of not leaving the rest as configurable, especially since it's low hanging fruit.

[zhaohuabing]

@vibe I talked with @arkodg offline - the SameSite setting is actually useful for iframe use cases.
Instead of configuring each cookie individually, could we just have one single konb to control SameSite for all cookies? What's your use case for this? Do you need different SameSite setting for different cookie?

[arkodg]

Also worth discussing here is should the defaults be changed from Disabled to Strict ?

[vibe]

@zhaohuabing I personally do not have a use case for different SameSite configs across different cookies, so I am open to just a single config across all oauth cookies. I think it personally makes sense to configure them together from one setting, unless we want ultimate flexibility for everyone else.

@arkodg - As far as I'm concerned, defaulting to Strict is better than disabled.

[zhaohuabing]

As far as I'm concerned, defaulting to Strict is better than disabled.

Strict is better, but it could be a break change for iframe use cases. We need to add this to the release note if we decide to change default to Strict.

[arkodg]

prefer if this API looked more like the existing API

Attributes map[string]string `json:"attributes,omitempty"`

https://github.com/envoyproxy/gateway/blob/28424b012665450a3ee00c3948508dfb56798ff9/api/v1alpha1/loadbalancer_types.go#L112C2-L112C60

[arkodg]

nvm its a limitation of the upstream API which only supports SameSite
https://www.envoyproxy.io/docs/envoy/latest/api-v3/extensions/filters/http/oauth2/v3/oauth.proto#envoy-v3-api-msg-extensions-filters-http-oauth2-v3-cookieconfigs

Gateway API fields call in cookieConfig https://gateway-api.sigs.k8s.io/reference/spec/#cookieconfig
lets go with CookieConfig *OIDCCookieConfig ?

[vibe]

@arkodg, I like it, updated.

[arkodg]

as discussed lets default to Strict and we can add this as a breaking change note in the relase notes

[vibe]

Updated default to Strict

[arkodg]

thanks can you also update the comment/ doc string, it says Disabled is the default

[codecov]

Codecov Report

Attention: Patch coverage is 93.18182% with 3 lines in your changes missing coverage. Please review.

Project coverage is 70.67%. Comparing base (e7f58d2) to head (c344e8b).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
internal/xds/translator/oidc.go	93.02%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #6289      +/-   ##
==========================================
- Coverage   70.67%   70.67%   -0.01%     
==========================================
  Files         220      220              
  Lines       36954    36998      +44     
==========================================
+ Hits        26116    26147      +31     
- Misses       9304     9314      +10     
- Partials     1534     1537       +3     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[zhaohuabing]

I personally do not have a use case for different SameSite configs across different cookies, so I am open to just a single config across all oauth cookies.

Hi @vibe let's just use a single config and not expose cookies details.

[vibe]

I personally do not have a use case for different SameSite configs across different cookies, so I am open to just a single config across all oauth cookies.

Hi @vibe let's just use a single config and not expose cookies details.

Thanks for clarifying, updated! @zhaohuabing

[arkodg]

thanks @vibe, overall LGTM, some minor comments

• update doc string feature(translator): expose OIDC oauth cookie samesite configuration #6289 (comment)
• add a yaml test in gateway-api layer e.g. edit https://github.com/envoyproxy/gateway/blob/v1.4.1/internal/gatewayapi/testdata/securitypolicy-with-oidc-custom-cookies.in.yaml
  and run make testdata which should generate the out file

[vibe]

@arkodg nice catch, updated!

[arkodg]

LGTM thanks !

[arkodg]

@vibe can you run make testdata again and commit those changes
seeing a gen-check error

+++ b/internal/gatewayapi/testdata/securitypolicy-with-oidc-custom-cookies-samesite.out.yaml
@@ -93,6 +93,9 @@ infraIR:
         labels:
           gateway.envoyproxy.io/owning-gateway-name: gateway-1
           gateway.envoyproxy.io/owning-gateway-namespace: envoy-gateway
+        ownerReference:
+          kind: GatewayClass
+          name: envoy-gateway-class

[vibe]

@arkodg running make testdata is yielding no changes.

For what it's worth securitypolicy-with-oidc-custom-cookies.out.yaml doesn't have ownerReference property.

I can manually add it to the out file, but please advise.

[arkodg]

@vibe it got added post rebase due to #6238, can you rebase to main and try ?

[vibe]

@arkodg love it, generated 👍

[vibe]

@arkodg Are e2e tests stable? The output log is massive, so mostly asking if there's a possibility of flakiness considering the other conformance-tests passed, before I jump in to review the logs.

[arkodg]

@vibe its flaky, you can ignore it
/retest