feat: Optionally override Oauth Cookie Names #3274

sam-burrell · 2024-04-25T16:19:45Z

What type of PR is this?
Added ability to optionally specify a cookieSuffix in the OIDC spec.

What this PR does / why we need it:
This is necessary to link up with the JWT element of the SecurityPolicy to ultimately make passing claimHeaders from the IdToken upstream for authentication in upstream apps clear and easy.

For example it would be used like this

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: grafana-public-test
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: grafana-public-test
  oidc:
    provider:
      issuer: "FOOO"
    clientID: "BAR"
    clientSecret:
      kind: Secret
      name: internal-app-client-secret
    logoutPath: "/logout"
    redirectURL: https://grafana.com/oauth2/callback
    cookieSuffix: "grafana"
    scopes:
      - openid
      - email
  jwt:
    providers:
      - name: "FOO"
        extractFrom:
          cookies:
           - IdToken-grafana
        claimToHeaders:
          - claim: email
            header: X-Email
          - claim: group
            header: X-Group
        remoteJWKS:
          uri: "https://PROVIDER/.well-known/jwks.json"

arkodg · 2024-04-25T19:05:25Z

this makes a lot of sense
cc @mt-inside @zhaohuabing @missBerg

2 open questions

do we need to revisit the prefix naming here ?
should this be a prefix, suffix or the ability to rewrite entire cookie name ?
e.g. this is the config from the istio auth service
https://github.com/istio-ecosystem/authservice/blob/f2ef0e41de8a2d664b35a51bd43748e0ca4d10e8/config/v1/oidc/config.proto#L176

hoping we rely on work done in the past, to influence naming here

zhaohuabing · 2024-04-25T21:15:16Z

I'd vote to keep the current naming, but allow users to specify a custom suffix. We should also provide guidance in the documentation on how to avoid session name conflicts if users opt for this customization.

A preferable approach would be sending the ID token through an 'x-envoy-gateway-id-token' header, thus preventing users from tampering with cookies. This method requires some work(trival) in Envoy upstream."

sam-burrell · 2024-04-26T08:31:40Z

I am happy to do anything else required to move this forward.

Side note - great work with this we are using envoy gateway successfully as our main ingress in our 300 plus service ecs to eks migration.

zhaohuabing · 2024-05-03T17:16:30Z

should this be a prefix, suffix or the ability to rewrite entire cookie name ?

I'm inclined to add a suffix or rewrite the entire cookie name. The default cookie name has a digest of the policy UID, it's kind of strange to put a random string as a prefix.

arkodg · 2024-05-03T17:18:53Z

should this be a prefix, suffix or the ability to rewrite entire cookie name ?

I'm inclined to add a suffix because the default one is a digest of the policy UID, it's kind of strange to put a random string as a prefix.

should we just allow the user to set the entire name ?

zhaohuabing · 2024-05-03T17:19:22Z

should this be a prefix, suffix or the ability to rewrite entire cookie name ?

I'm inclined to add a suffix because the default one is a digest of the policy UID, it's kind of strange to put a random string as a prefix.

should we just allow the user to set the entire name ?

Agree, overwriting the entire name is better, users can easily know the cookie name. @sam-burrell

arkodg · 2024-05-07T19:45:20Z

api/v1alpha1/oidc_types.go

+	// The optional cookie suffix to be added to Bearer and IdToken cookies in the
+	// [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
+	// If not specified, uses a randomly generated suffix
+	CookieSuffix *string `json:"cookieSuffix,omitempty"`


Suggested change

CookieSuffix *string `json:"cookieSuffix,omitempty"`

IdTokenCookieName *string `json:"idTokenCookieName,omitempty"`

arkodg · 2024-05-07T19:45:31Z

api/v1alpha1/oidc_types.go

+	// [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
+	// If not specified, uses a randomly generated suffix
+	CookieSuffix *string `json:"cookieSuffix,omitempty"`
+


or even idTokenName

zhaohuabing · 2024-05-07T22:45:32Z

@sam-burrell Could you please address @arkodg 's comment? So we can move on this.

zetaab · 2024-05-16T06:43:57Z

ping @sam-burrell could you check the review comments

denniskniep · 2024-05-19T21:36:53Z

Hi,

this feature is very welcome 👍

I propose additionally to @arkodg´s review comment to make all cookie names configurable (and not only idToken cookie):

gateway/internal/xds/translator/oidc.go

Lines 161 to 167 in 6dcf78a

    
           CookieNames: &oauth2v3.OAuth2Credentials_CookieNames{ 
        
           	BearerToken:  fmt.Sprintf("BearerToken-%s", oidc.CookieSuffix), 
        
           	OauthHmac:    fmt.Sprintf("OauthHMAC-%s", oidc.CookieSuffix), 
        
           	OauthExpires: fmt.Sprintf("OauthExpires-%s", oidc.CookieSuffix), 
        
           	IdToken:      fmt.Sprintf("IdToken-%s", oidc.CookieSuffix), 
        
           	RefreshToken: fmt.Sprintf("RefreshToken-%s", oidc.CookieSuffix), 
        
           },

bearerTokenCookieName
oauthHmacCookieName
oauthExpiresCookieName
idTokenCookieName
refreshTokenCookieName

arkodg · 2024-05-20T16:44:29Z

Hi,

this feature is very welcome 👍

I propose additionally to @arkodg´s review comment to make all cookie names configurable (and not only idToken cookie):

gateway/internal/xds/translator/oidc.go

Lines 161 to 167 in 6dcf78a

CookieNames: &oauth2v3.OAuth2Credentials_CookieNames{

BearerToken: fmt.Sprintf("BearerToken-%s", oidc.CookieSuffix),

OauthHmac: fmt.Sprintf("OauthHMAC-%s", oidc.CookieSuffix),

OauthExpires: fmt.Sprintf("OauthExpires-%s", oidc.CookieSuffix),

IdToken: fmt.Sprintf("IdToken-%s", oidc.CookieSuffix),

RefreshToken: fmt.Sprintf("RefreshToken-%s", oidc.CookieSuffix),

},

bearerTokenCookieName

oauthHmacCookieName

oauthExpiresCookieName

idTokenCookieName

refreshTokenCookieName

if we need to have predictable names for all the cookies then I'd suggest making the struct a little more tiered

cookieName:
  idToken:
  ........

zhaohuabing · 2024-05-21T02:10:56Z

Hi,

this feature is very welcome 👍

I propose additionally to @arkodg´s review comment to make all cookie names configurable (and not only idToken cookie):

gateway/internal/xds/translator/oidc.go

Lines 161 to 167 in 6dcf78a

CookieNames: &oauth2v3.OAuth2Credentials_CookieNames{

BearerToken: fmt.Sprintf("BearerToken-%s", oidc.CookieSuffix),

OauthHmac: fmt.Sprintf("OauthHMAC-%s", oidc.CookieSuffix),

OauthExpires: fmt.Sprintf("OauthExpires-%s", oidc.CookieSuffix),

IdToken: fmt.Sprintf("IdToken-%s", oidc.CookieSuffix),

RefreshToken: fmt.Sprintf("RefreshToken-%s", oidc.CookieSuffix),

},

bearerTokenCookieName

oauthHmacCookieName

oauthExpiresCookieName

idTokenCookieName

refreshTokenCookieName

Exposing BearerTokenCookieName and idTokenCookieName to API makes sense to me, but others should not be in the API as they are trivial details of the Envoy OAuth2 filter implementation.

@denniskniep do you need these cookies somewhere?

Signed-off-by: sam-burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

Co-authored-by: Sam Burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

sam-burrell · 2024-05-22T13:08:10Z

@zhaohuabing @arkodg

We have refactored this and now made this the ability to optionally overide oauth cookie names.
Is there anything else we can help with?

codecov · 2024-05-22T14:42:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 67.19%. Comparing base (101aa48) to head (728ac57).
Report is 12 commits behind head on main.

❗ Current head 728ac57 differs from pull request most recent head 14e216c

Please upload reports for the commit 14e216c to get more accurate results.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3274      +/-   ##
==========================================
- Coverage   67.35%   67.19%   -0.16%     
==========================================
  Files         166      166              
  Lines       19355    19470     +115     
==========================================
+ Hits        13036    13083      +47     
- Misses       5378     5444      +66     
- Partials      941      943       +2

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

denniskniep · 2024-05-22T15:22:38Z

Hi,
this feature is very welcome 👍
I propose additionally to @arkodg´s review comment to make all cookie names configurable (and not only idToken cookie):

gateway/internal/xds/translator/oidc.go

Lines 161 to 167 in 6dcf78a

CookieNames: &oauth2v3.OAuth2Credentials_CookieNames{

BearerToken: fmt.Sprintf("BearerToken-%s", oidc.CookieSuffix),

OauthHmac: fmt.Sprintf("OauthHMAC-%s", oidc.CookieSuffix),

OauthExpires: fmt.Sprintf("OauthExpires-%s", oidc.CookieSuffix),

IdToken: fmt.Sprintf("IdToken-%s", oidc.CookieSuffix),

RefreshToken: fmt.Sprintf("RefreshToken-%s", oidc.CookieSuffix),

},

bearerTokenCookieName

oauthHmacCookieName

oauthExpiresCookieName

idTokenCookieName

refreshTokenCookieName

Exposing BearerTokenCookieName and idTokenCookieName to API makes sense to me, but others should not be in the API as they are trivial details of the Envoy OAuth2 filter implementation.

@denniskniep do you need these cookies somewhere?

@zhaohuabing BearerTokenCookieName and idTokenCookieName is sufficient for my current use case

arkodg · 2024-05-22T15:55:51Z

hey this diff looks good !
can we add a test in the gateway-api lib and the xds translator lib to avoid any future regressions ? the make testdata target should help generate the o/p data for you once you've written the input config

Co-authored-by: Sam Burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

coro · 2024-05-23T09:43:03Z

@arkodg Added, thanks!

arkodg · 2024-05-23T21:52:47Z

internal/gatewayapi/testdata/securitypolicy-with-oidc-custom-cookies.out.yaml

+        namespace: envoy-gateway
+      conditions:
+      - lastTransitionTime: null
+        message: HMAC secret envoy-gateway-system/envoy-oidc-hmac not found


something is off, Accepted=False, you may have forgotten to include the Secret, the security IR field is also empty

zhaohuabing · 2024-05-24T00:11:47Z

api/v1alpha1/oidc_types.go

+	// [Authentication Request](https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest).
+	// If not specified, defaults to "BearerToken-(randomly generated uid)"
+	// +optional
+	BearerToken *string `json:"bearerToken,omitempty"`


The name BearerToken is confusing in this context. Let's rename it AccessToken to keep consistent with #3423

Suggested change

BearerToken *string `json:"bearerToken,omitempty"`

AccessToken *string `json:"accessToken,omitempty"`

zhaohuabing · 2024-05-24T01:03:59Z

internal/xds/translator/oidc.go

+
+	if oidc.CookieNameOverrides != nil &&
+		oidc.CookieNameOverrides.BearerToken != nil {
+		oauth2.Config.Credentials.CookieNames.BearerToken = *oidc.CookieNameOverrides.BearerToken


Could you please also change the default name for "BearerTokentoAccessToken` ?

Change

BearerToken: fmt.Sprintf("BearerToken-%s", oidc.CookieSuffix),

To

BearerToken: fmt.Sprintf("AccessToken-%s", oidc.CookieSuffix)

Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

sam-burrell requested a review from a team as a code owner April 25, 2024 16:19

zhaohuabing self-requested a review May 3, 2024 15:54

arkodg mentioned this pull request May 7, 2024

Way to include id token claims as headers? #3335

Open

arkodg reviewed May 7, 2024

View reviewed changes

arkodg added this to the v1.1.0-rc1 milestone May 13, 2024

coro mentioned this pull request May 21, 2024

API: add more oidc configuration settings #3423

Open

coro force-pushed the main branch from 7a4f51a to 101aa48 Compare May 22, 2024 09:55

sam-burrell and others added 2 commits May 22, 2024 13:55

Added ability to specifiy a cookieSuffix

2bce575

Signed-off-by: sam-burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

Allow override of OAuth cookie names

1077aef

Co-authored-by: Sam Burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

coro force-pushed the main branch from dcb8b71 to 1077aef Compare May 22, 2024 12:55

Update Helm CRD docs

e58c466

Co-authored-by: Sam Burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

sam-burrell changed the title ~~feat: added ability to optionally specify a cookieSuffix~~ feat: Optionally override Oauth Cookie Names May 22, 2024

Add testdata for custom cookie name overrides

728ac57

Co-authored-by: Sam Burrell <sam.burrell@gmail.com> Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

arkodg reviewed May 23, 2024

View reviewed changes

zhaohuabing reviewed May 24, 2024

View reviewed changes

coro added 2 commits May 24, 2024 10:14

Fix regression tests to have accepted gateway

24ad396

Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

Rename BearerToken to AccessToken

14e216c

Signed-off-by: Connor Rogers <23215294+coro@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Optionally override Oauth Cookie Names #3274

feat: Optionally override Oauth Cookie Names #3274

sam-burrell commented Apr 25, 2024 •

edited

arkodg commented Apr 25, 2024 •

edited

zhaohuabing commented Apr 25, 2024 •

edited

sam-burrell commented Apr 26, 2024

zhaohuabing commented May 3, 2024 •

edited

arkodg commented May 3, 2024

zhaohuabing commented May 3, 2024 •

edited

arkodg May 7, 2024

arkodg May 7, 2024

zhaohuabing commented May 7, 2024

zetaab commented May 16, 2024

denniskniep commented May 19, 2024

arkodg commented May 20, 2024 •

edited

zhaohuabing commented May 21, 2024 •

edited

sam-burrell commented May 22, 2024

codecov bot commented May 22, 2024 •

edited

denniskniep commented May 22, 2024

arkodg commented May 22, 2024

coro commented May 23, 2024

arkodg May 23, 2024

zhaohuabing May 24, 2024

zhaohuabing May 24, 2024 •

edited

	CookieSuffix *string `json:"cookieSuffix,omitempty"`
	IdTokenCookieName *string `json:"idTokenCookieName,omitempty"`

	BearerToken *string `json:"bearerToken,omitempty"`
	AccessToken *string `json:"accessToken,omitempty"`

feat: Optionally override Oauth Cookie Names #3274

Are you sure you want to change the base?

feat: Optionally override Oauth Cookie Names #3274

Conversation

sam-burrell commented Apr 25, 2024 • edited

arkodg commented Apr 25, 2024 • edited

zhaohuabing commented Apr 25, 2024 • edited

sam-burrell commented Apr 26, 2024

zhaohuabing commented May 3, 2024 • edited

arkodg commented May 3, 2024

zhaohuabing commented May 3, 2024 • edited

arkodg May 7, 2024

Choose a reason for hiding this comment

arkodg May 7, 2024

Choose a reason for hiding this comment

zhaohuabing commented May 7, 2024

zetaab commented May 16, 2024

denniskniep commented May 19, 2024

arkodg commented May 20, 2024 • edited

zhaohuabing commented May 21, 2024 • edited

sam-burrell commented May 22, 2024

codecov bot commented May 22, 2024 • edited

Codecov Report

denniskniep commented May 22, 2024

arkodg commented May 22, 2024

coro commented May 23, 2024

arkodg May 23, 2024

Choose a reason for hiding this comment

zhaohuabing May 24, 2024

Choose a reason for hiding this comment

zhaohuabing May 24, 2024 • edited

Choose a reason for hiding this comment

sam-burrell commented Apr 25, 2024 •

edited

arkodg commented Apr 25, 2024 •

edited

zhaohuabing commented Apr 25, 2024 •

edited

zhaohuabing commented May 3, 2024 •

edited

zhaohuabing commented May 3, 2024 •

edited

arkodg commented May 20, 2024 •

edited

zhaohuabing commented May 21, 2024 •

edited

codecov bot commented May 22, 2024 •

edited

zhaohuabing May 24, 2024 •

edited