envoyproxy / gateway

Manages Envoy Proxy as a Standalone or Kubernetes-based Application Gateway
https://gateway.envoyproxy.io
Apache License 2.0
1.63k stars 354 forks source link

oidc cookies not created #3638

Open zetaab opened 5 months ago

zetaab commented 5 months ago

Description:

I have securitypolicy that authenticates user by using oidc and then verifying it with jwt. However, the IdToken is not created anymore by automatically. When forwardAccessToken: true is defined the IdToken is created, but otherwise the cookie will be empty.

The behaviour changed https://github.com/envoyproxy/gateway/pull/3567 but for me it looks like this preserve_authorization_header now removes cookies as well

Repro steps:

create securitypolicy to endpoint by using: (cookie will be created only if forwardAccessToken: true is defined)

apiVersion: gateway.envoyproxy.io/v1alpha1
kind: SecurityPolicy
metadata:
  name: xx-dev-oidc
spec:
  jwt:
    providers:
    - claimToHeaders:
      - claim: cognito:groups
        header: x-user-groups
      - claim: email
        header: x-user-email
      extractFrom:
        cookies:
        - IdToken
      name: cognito
      remoteJWKS:
        uri: https://cognito-idp.eu-central-1.amazonaws.com/xxx/.well-known/jwks.json
  oidc:
    clientID: yyy
    clientSecret:
      group: ""
      kind: Secret
      name: backstage-oidc
    cookieNames:
      idToken: IdToken
    provider:
      issuer: https://cognito-idp.eu-central-1.amazonaws.com/xxx
    scopes:
    - openid
    - email
    - profile
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: backstage-dev

Environment:

envoy-gateway is compiled from main branch by using d49337b9c5d418a88bae84b2592b7c4b907f7134 commit

zetaab commented 5 months ago

cc @zhaohuabing @arkodg @denniskniep this breaks now existing functionality. The cookies should be created when using oidc plugin AND I would not like to forward Authorization: Bearer.. token to backend (which it now does, but cookie is just missing)

denniskniep commented 5 months ago

Hi @zetaab,

that is part of envoy proxy's logic. The idToken, bearerToken and refreshToken Cookie is only issued, if forwardBearerToken is set to true, see here

I already made a note for the release manager

zetaab commented 5 months ago

IMO that is now quite bad feature that oidc does not work together with jwt by default in envoy gateway. It should not be like that. All tutorials that documentations now has - is broken. And this is going to break people configurations if introduced like this.

My proposal is revert https://github.com/envoyproxy/gateway/pull/3567 and rethink

Its not perfect feature in envoy that forwardBEARERToken will disable cookies not only Authorization: Bearer.. header. These are two different things.

cc @zirain @arkodg @zhaohuabing

zhaohuabing commented 5 months ago

Looks like this was introduced in https://github.com/envoyproxy/gateway/pull/3514, not by #3567 . forwardBearerToken has been set to false by default in #3514

denniskniep commented 5 months ago

Even if we revert something on Envoy Gateway side, it would not change that mentioned Cookies are never issued, if forwardBearerToken is set to false

There is already a PR which tackles that: https://github.com/envoyproxy/envoy/pull/34156

zetaab commented 5 months ago

great, that is actually which will fix this issue. Otherwise the oidc filter in gateway is broken - it cannot work without cookies and we should maybe change the default behaviour back to true

github-actions[bot] commented 4 months ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

zetaab commented 3 months ago

not stale, waiting for upstream PR

github-actions[bot] commented 2 months ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

zetaab commented 2 months ago

not stale, waiting for upstream PR

github-actions[bot] commented 1 month ago

This issue has been automatically marked as stale because it has not had activity in the last 30 days.

arkodg commented 2 weeks ago

can this be closed @zetaab / @zhaohuabing ?

zhaohuabing commented 2 weeks ago

This should already have been fixed in https://github.com/envoyproxy/envoy/pull/35839. @zetaab