feat: Optionally override Oauth Cookie Names

[sam-burrell]

trafficstars

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]

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]

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]

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]

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]

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]

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

[zhaohuabing]

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

[zetaab]

ping @sam-burrell could you check the review comments

[denniskniep]

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):

https://github.com/envoyproxy/gateway/blob/6dcf78aee3aa88fd7feae6fb7449ba6f3b9c57ff/internal/xds/translator/oidc.go#L161-L167

• bearerTokenCookieName
• oauthHmacCookieName
• oauthExpiresCookieName
• idTokenCookieName
• refreshTokenCookieName

[arkodg]

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):

https://github.com/envoyproxy/gateway/blob/6dcf78aee3aa88fd7feae6fb7449ba6f3b9c57ff/internal/xds/translator/oidc.go#L161-L167

• 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]

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):

https://github.com/envoyproxy/gateway/blob/6dcf78aee3aa88fd7feae6fb7449ba6f3b9c57ff/internal/xds/translator/oidc.go#L161-L167

• 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?

[sam-burrell]

@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[bot]]

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

Project coverage is 67.35%. Comparing base (78fe57a) to head (3d28ef9).

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3274      +/-   ##
==========================================
- Coverage   67.36%   67.35%   -0.01%     
==========================================
  Files         167      167              
  Lines       19925    19932       +7     
==========================================
+ Hits        13422    13426       +4     
- Misses       5538     5540       +2     
- Partials      965      966       +1     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[denniskniep]

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): https://github.com/envoyproxy/gateway/blob/6dcf78aee3aa88fd7feae6fb7449ba6f3b9c57ff/internal/xds/translator/oidc.go#L161-L167

• 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]

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

[coro]

@arkodg Added, thanks!

[coro]

@arkodg @zhaohuabing Added your changes, thanks!

[arkodg]

@coro can you run make testdata again ? looks like some test o/p changed

[coro]

@arkodg I believe that was unrelated to my change - it seems the example certificate used in that particular test with a diff (internal/gatewayapi/testdata/gateway-with-listener-with-valid-multiple-tls-configuration-with-same-algorithm-different-fqdn.out.yaml) had expired May 24th. I've rebased onto the latest commit and it has resolved that diff.