From 18df8f9971b1b97ca7f9160fd23dcf8e12deb3f5 Mon Sep 17 00:00:00 2001 From: "Huabing (Robin) Zhao" Date: Sun, 2 Aug 2026 21:29:48 -0700 Subject: [PATCH 1/2] fix: keep orphaned OIDC PKCE cookies off every request Envoy mints a nonce (CSRF) and a PKCE code verifier cookie for every authorization flow it starts, keyed by a random per-flow id so that several logins can be in flight at once. On a successful callback it deletes only the pair belonging to the flow that completed, which is deliberate - sweeping every flow would strip the cookies of the other tabs still mid-login. A flow that is started and abandoned therefore leaves its pair behind until it expires, and at the default path "/" those orphans are sent on every request. Combined with a provider that issues large id and access tokens they overflow the inbound request header limit, at which point the callback itself fails and the browser loops back through the flow, minting yet another pair. Envoy only needs the value of these two cookies when it validates the callback, so scope them to the OIDC redirect path. Orphans then stay off ordinary application requests instead of counting against the header limit on each one. This bounds the problem rather than eliminating it - they are still sent to the callback endpoint, and they still expire on their own within csrfTokenTTL. Also name the code verifier cookie CodeVerifier-. It was the only one of the seven OAuth2 cookies left at Envoy's default name, so SecurityPolicies sharing a cookie domain all wrote one shared cookie, and logging out of one policy deleted the in-flight flow cookies of the others. The redirect path is only applied when it satisfies the pattern Envoy enforces on a cookie path, which is stricter than a URL path - "," and ";" for example are legal RFC 3986 sub-delims but are rejected. An unrepresentable path falls back to leaving the cookie path unset, which Envoy defaults to "/", rather than failing xDS validation and dropping the route. Fixes #9632 Signed-off-by: Huabing (Robin) Zhao --- internal/ir/xds.go | 2 +- internal/xds/translator/oidc.go | 59 +++++++--- internal/xds/translator/oidc_test.go | 107 ++++++++++-------- ...me-port-with-different-filters.routes.yaml | 6 + .../oidc-and-jwt-with-passthrough.routes.yaml | 6 + .../oidc-backend-cluster-provider.routes.yaml | 6 + ...oidc-provider-traffic-features.routes.yaml | 6 + .../testdata/out/xds-ir/oidc.routes.yaml | 12 ++ ...ritypolicy-with-oidc-jwt-authz.routes.yaml | 6 + .../9644-oidc-orphaned-pkce-cookies.md | 18 +++ 10 files changed, 163 insertions(+), 65 deletions(-) create mode 100644 release-notes/current/bug_fixes/9644-oidc-orphaned-pkce-cookies.md diff --git a/internal/ir/xds.go b/internal/ir/xds.go index 0cf1778775..584c62db83 100644 --- a/internal/ir/xds.go +++ b/internal/ir/xds.go @@ -1463,7 +1463,7 @@ type OIDC struct { // CookieSuffix will be added to the name of the cookies set by the oauth filter. // Adding a suffix avoids multiple oauth filters from overwriting each other's cookies. // These cookies are set by the oauth filter, including: AccessToken, - // OauthHMAC, OauthExpires, IdToken, and RefreshToken. + // OauthHMAC, OauthExpires, IdToken, RefreshToken, OauthNonce and CodeVerifier. CookieSuffix string `json:"cookieSuffix,omitempty"` // CookieNameOverrides can optionally override the generated name of the cookies set by the oauth filter. diff --git a/internal/xds/translator/oidc.go b/internal/xds/translator/oidc.go index fba5f7da53..9b6246a271 100644 --- a/internal/xds/translator/oidc.go +++ b/internal/xds/translator/oidc.go @@ -8,6 +8,7 @@ package translator import ( "errors" "fmt" + "regexp" "strings" corev3 "github.com/envoyproxy/go-control-plane/envoy/config/core/v3" @@ -26,6 +27,11 @@ import ( "github.com/envoyproxy/gateway/internal/xds/types" ) +// cookiePathPattern mirrors the constraint the Envoy oauth2 proto puts on +// CookieConfig.path. It is stricter than what a URL path allows - "," and ";" +// for example are legal RFC 3986 sub-delims but are rejected here. +var cookiePathPattern = regexp.MustCompile(`^$|^/[^\x00-\x1f\x7f ",;<>\\]*$`) + func init() { registerHTTPFilter(&oidc{}) } @@ -170,6 +176,7 @@ func oauth2Config(securityFeatures *ir.SecurityFeatures) (*oauth2v3.OAuth2PerRou IdToken: fmt.Sprintf("IdToken-%s", oidc.CookieSuffix), RefreshToken: fmt.Sprintf("RefreshToken-%s", oidc.CookieSuffix), OauthNonce: fmt.Sprintf("OauthNonce-%s", oidc.CookieSuffix), + CodeVerifier: fmt.Sprintf("CodeVerifier-%s", oidc.CookieSuffix), }, }, // every OIDC provider supports basic auth @@ -260,24 +267,42 @@ func buildSameSite(config *egv1a1.OIDCCookieConfig) oauth2v3.CookieConfig_SameSi } } -// buildCookieConfigs translates the OIDC configuration from the US +// buildCookieConfigs builds the attributes Envoy sets on the OAuth2 cookies. func buildCookieConfigs(oidc *ir.OIDC) *oauth2v3.CookieConfigs { - // If the user did not specify any custom cookie configurations at all, return the defaults. - if oidc.CookieConfig == nil || oidc.CookieConfig.SameSite == nil { - return nil - } - - // Apply the user-defined SameSite policy for each cookie if it has been configured. - sameSite := buildSameSite(oidc.CookieConfig) - return &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, - } + // The nonce (CSRF) and PKCE code verifier cookies only carry state for an + // in-flight authorization flow, and Envoy only reads them back when it + // validates the callback from the authorization server. Scoping them to the + // redirect path keeps them off every other request: a flow that is started + // but never completed - a parallel request from a logged out browser, a user + // navigating away from the provider's login page - leaves its cookies behind + // until they expire, and at the default path "/" those orphans are sent on + // every request until then. + // A path Envoy refuses to accept on a cookie would fail xDS validation and take + // the whole route down with it, so fall back to leaving the path unset - Envoy + // then defaults it to "/", which is the behavior we had before scoping. + redirectPath := oidc.RedirectPath + if !cookiePathPattern.MatchString(redirectPath) { + redirectPath = "" + } + + cookieConfigs := &oauth2v3.CookieConfigs{ + OauthNonceCookieConfig: &oauth2v3.CookieConfig{Path: redirectPath}, + CodeVerifierCookieConfig: &oauth2v3.CookieConfig{Path: redirectPath}, + } + + // Apply the user-defined SameSite policy to each cookie if it has been configured. + if oidc.CookieConfig != nil && oidc.CookieConfig.SameSite != nil { + sameSite := buildSameSite(oidc.CookieConfig) + cookieConfigs.BearerTokenCookieConfig = &oauth2v3.CookieConfig{SameSite: sameSite} + cookieConfigs.OauthHmacCookieConfig = &oauth2v3.CookieConfig{SameSite: sameSite} + cookieConfigs.OauthExpiresCookieConfig = &oauth2v3.CookieConfig{SameSite: sameSite} + cookieConfigs.IdTokenCookieConfig = &oauth2v3.CookieConfig{SameSite: sameSite} + cookieConfigs.RefreshTokenCookieConfig = &oauth2v3.CookieConfig{SameSite: sameSite} + cookieConfigs.OauthNonceCookieConfig.SameSite = sameSite + cookieConfigs.CodeVerifierCookieConfig.SameSite = sameSite + } + + return cookieConfigs } func buildDenyRedirectMatcher(oidc *ir.OIDC) []*routev3.HeaderMatcher { diff --git a/internal/xds/translator/oidc_test.go b/internal/xds/translator/oidc_test.go index f2d3d27abc..cbe51e8ccc 100644 --- a/internal/xds/translator/oidc_test.go +++ b/internal/xds/translator/oidc_test.go @@ -17,6 +17,31 @@ import ( "github.com/envoyproxy/gateway/internal/ir" ) +// expectedCookieConfigs returns the cookie configs EG emits for the given SameSite +// policy: the nonce and code verifier cookies are always scoped to the redirect path, +// every cookie carries the SameSite policy. +func expectedCookieConfigs(sameSite oauth2v3.CookieConfig_SameSite, redirectPath string) *oauth2v3.CookieConfigs { + return &oauth2v3.CookieConfigs{ + BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, + OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, + OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, + IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, + RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite}, + OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite, Path: redirectPath}, + CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: sameSite, Path: redirectPath}, + } +} + +// expectedRedirectPathOnlyCookieConfigs returns the cookie configs EG emits when the +// user did not configure SameSite: only the two flow cookies are configured, so the +// session cookies keep Envoy's defaults. +func expectedRedirectPathOnlyCookieConfigs(redirectPath string) *oauth2v3.CookieConfigs { + return &oauth2v3.CookieConfigs{ + OauthNonceCookieConfig: &oauth2v3.CookieConfig{Path: redirectPath}, + CodeVerifierCookieConfig: &oauth2v3.CookieConfig{Path: redirectPath}, + } +} + func TestOIDCCookieConfigSameSite(t *testing.T) { tests := []struct { name string @@ -24,94 +49,82 @@ func TestOIDCCookieConfigSameSite(t *testing.T) { expect *oauth2v3.CookieConfigs }{ { - name: "defaults all cookie to unset/niul", - input: ir.OIDC{}, - expect: nil, + name: "SameSite unset still scopes the flow cookies to the redirect path", + input: ir.OIDC{RedirectPath: "/oauth2/callback"}, + expect: expectedRedirectPathOnlyCookieConfigs("/oauth2/callback"), }, { name: "all cookie configs set to None", input: ir.OIDC{ + RedirectPath: "/oauth2/callback", CookieConfig: &egv1a1.OIDCCookieConfig{ SameSite: new("None"), }, }, - expect: &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_NONE}, - }, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_NONE, "/oauth2/callback"), }, { name: "all cookie configs set to Lax", input: ir.OIDC{ + RedirectPath: "/oauth2/callback", CookieConfig: &egv1a1.OIDCCookieConfig{ SameSite: new("Lax"), }, }, - expect: &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_LAX}, - }, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_LAX, "/oauth2/callback"), }, { name: "all cookie configs set to Strict", input: ir.OIDC{ + RedirectPath: "/oauth2/callback", CookieConfig: &egv1a1.OIDCCookieConfig{ SameSite: new("Strict"), }, }, - expect: &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_STRICT}, - }, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_STRICT, "/oauth2/callback"), }, { name: "all cookie configs set to Disabled", input: ir.OIDC{ + RedirectPath: "/oauth2/callback", CookieConfig: &egv1a1.OIDCCookieConfig{ SameSite: new("Disabled"), }, }, - expect: &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - }, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_DISABLED, "/oauth2/callback"), }, { name: "cookie config received invalid SameSite value will default to Disabled", input: ir.OIDC{ + RedirectPath: "/oauth2/callback", CookieConfig: &egv1a1.OIDCCookieConfig{ SameSite: new("InvalidValue"), }, }, - expect: &oauth2v3.CookieConfigs{ - BearerTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthHmacCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthExpiresCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - IdTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - RefreshTokenCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - OauthNonceCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, - CodeVerifierCookieConfig: &oauth2v3.CookieConfig{SameSite: oauth2v3.CookieConfig_DISABLED}, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_DISABLED, "/oauth2/callback"), + }, + { + name: "a custom redirect path scopes the flow cookies to that path", + input: ir.OIDC{ + RedirectPath: "/auth/callback", + CookieConfig: &egv1a1.OIDCCookieConfig{ + SameSite: new("Lax"), + }, }, + expect: expectedCookieConfigs(oauth2v3.CookieConfig_LAX, "/auth/callback"), + }, + { + // Envoy defaults an empty cookie path to "/". + name: "an empty redirect path leaves the cookie path unset", + input: ir.OIDC{}, + expect: expectedRedirectPathOnlyCookieConfigs(""), + }, + { + // Envoy rejects ";" in a cookie path, and an invalid path would fail + // xDS validation and drop the route, so the path is left unset. + name: "a redirect path Envoy rejects on a cookie leaves the cookie path unset", + input: ir.OIDC{RedirectPath: "/oauth2;v2/callback"}, + expect: expectedRedirectPathOnlyCookieConfigs(""), }, } diff --git a/internal/xds/translator/testdata/out/xds-ir/multiple-listeners-same-port-with-different-filters.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/multiple-listeners-same-port-with-different-filters.routes.yaml index 1bd780a3ba..79813f034e 100644 --- a/internal/xds/translator/testdata/out/xds-ir/multiple-listeners-same-port-with-different-filters.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/multiple-listeners-same-port-with-different-filters.routes.yaml @@ -49,10 +49,16 @@ - profile authType: BASIC_AUTH authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /foo/oauth2/callback + oauthNonceCookieConfig: + path: /foo/oauth2/callback credentials: clientId: client.oauth.foo.com cookieNames: bearerToken: AccessToken-5F93C2E4 + codeVerifier: CodeVerifier-5F93C2E4 idToken: IdToken-5F93C2E4 oauthExpires: OauthExpires-5F93C2E4 oauthHmac: OauthHMAC-5F93C2E4 diff --git a/internal/xds/translator/testdata/out/xds-ir/oidc-and-jwt-with-passthrough.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/oidc-and-jwt-with-passthrough.routes.yaml index c102a99135..4317e92263 100644 --- a/internal/xds/translator/testdata/out/xds-ir/oidc-and-jwt-with-passthrough.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/oidc-and-jwt-with-passthrough.routes.yaml @@ -23,10 +23,16 @@ - openid authType: BASIC_AUTH authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /oauth2/callback + oauthNonceCookieConfig: + path: /oauth2/callback credentials: clientId: client.oauth.foo.com cookieNames: bearerToken: AccessToken-b0a1b740 + codeVerifier: CodeVerifier-b0a1b740 idToken: IdToken-b0a1b740 oauthExpires: OauthExpires-b0a1b740 oauthHmac: OauthHMAC-b0a1b740 diff --git a/internal/xds/translator/testdata/out/xds-ir/oidc-backend-cluster-provider.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/oidc-backend-cluster-provider.routes.yaml index c7b0d7289d..3f23fab9c0 100644 --- a/internal/xds/translator/testdata/out/xds-ir/oidc-backend-cluster-provider.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/oidc-backend-cluster-provider.routes.yaml @@ -20,10 +20,16 @@ - openid authType: BASIC_AUTH authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /bar/oauth2/callback + oauthNonceCookieConfig: + path: /bar/oauth2/callback credentials: clientId: client1.apps.googleusercontent.com cookieNames: bearerToken: AccessToken-b0a1b740 + codeVerifier: CodeVerifier-b0a1b740 idToken: IdToken-b0a1b740 oauthExpires: OauthExpires-b0a1b740 oauthHmac: OauthHMAC-b0a1b740 diff --git a/internal/xds/translator/testdata/out/xds-ir/oidc-provider-traffic-features.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/oidc-provider-traffic-features.routes.yaml index 813b531b39..69c64d9452 100644 --- a/internal/xds/translator/testdata/out/xds-ir/oidc-provider-traffic-features.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/oidc-provider-traffic-features.routes.yaml @@ -35,10 +35,16 @@ - openid authType: BASIC_AUTH authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /bar/oauth2/callback + oauthNonceCookieConfig: + path: /bar/oauth2/callback credentials: clientId: client1.apps.googleusercontent.com cookieNames: bearerToken: AccessToken-b0a1b740 + codeVerifier: CodeVerifier-b0a1b740 idToken: IdToken-b0a1b740 oauthExpires: OauthExpires-b0a1b740 oauthHmac: OauthHMAC-b0a1b740 diff --git a/internal/xds/translator/testdata/out/xds-ir/oidc.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/oidc.routes.yaml index c5900c53f7..add01b2496 100644 --- a/internal/xds/translator/testdata/out/xds-ir/oidc.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/oidc.routes.yaml @@ -22,10 +22,16 @@ - profile authType: BASIC_AUTH authorizationEndpoint: https://oauth.foo.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /foo/oauth2/callback + oauthNonceCookieConfig: + path: /foo/oauth2/callback credentials: clientId: client.oauth.foo.com cookieNames: bearerToken: AccessToken-5F93C2E4 + codeVerifier: CodeVerifier-5F93C2E4 idToken: IdToken-5F93C2E4 oauthExpires: OauthExpires-5F93C2E4 oauthHmac: OauthHMAC-5F93C2E4 @@ -81,11 +87,17 @@ - profile authType: BASIC_AUTH authorizationEndpoint: https://oauth.bar.com/oauth2/v2/auth + cookieConfigs: + codeVerifierCookieConfig: + path: /bar/oauth2/callback + oauthNonceCookieConfig: + path: /bar/oauth2/callback credentials: clientId: client.oauth.bar.com cookieDomain: example.com cookieNames: bearerToken: CustomAccessTokenOverride + codeVerifier: CodeVerifier-5f93c2e4 idToken: CustomIdTokenOverride oauthExpires: OauthExpires-5f93c2e4 oauthHmac: OauthHMAC-5f93c2e4 diff --git a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml index 51e1ca3fa7..d72a2f824a 100644 --- a/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml +++ b/internal/xds/translator/testdata/out/xds-ir/securitypolicy-with-oidc-jwt-authz.routes.yaml @@ -40,10 +40,16 @@ - profile authType: BASIC_AUTH authorizationEndpoint: https://oidc.example.com/authorize + cookieConfigs: + codeVerifierCookieConfig: + path: /oauth2/callback + oauthNonceCookieConfig: + path: /oauth2/callback credentials: clientId: prometheus cookieNames: bearerToken: AccessToken-5f93c2e4 + codeVerifier: CodeVerifier-5f93c2e4 idToken: IdToken oauthExpires: OauthExpires-5f93c2e4 oauthHmac: OauthHMAC-5f93c2e4 diff --git a/release-notes/current/bug_fixes/9644-oidc-orphaned-pkce-cookies.md b/release-notes/current/bug_fixes/9644-oidc-orphaned-pkce-cookies.md new file mode 100644 index 0000000000..29e292a714 --- /dev/null +++ b/release-notes/current/bug_fixes/9644-oidc-orphaned-pkce-cookies.md @@ -0,0 +1,18 @@ +Fixed OIDC flow-state cookies accumulating in the browser and overflowing the request header size limit. +Envoy mints a nonce (CSRF) and a PKCE code verifier cookie for every authorization flow it starts, but +only deletes the pair belonging to the flow that completes the callback, so flows that are abandoned - +parallel requests from a logged out browser, a user navigating away from the provider's login page - +leave their cookies behind until they expire. Envoy Gateway now scopes both cookies to the OIDC redirect +path, the only path where Envoy needs their value, so any orphans are no longer sent on every request. +Note this bounds the damage rather than eliminating it: orphans are still sent to the callback endpoint +itself until they expire, and logout can no longer purge them early because the browser no longer sends +them to the signout path, so also consider lowering `csrfTokenTTL`. +The PKCE code verifier cookie is also now named `CodeVerifier-`, carrying the same per-policy +suffix as the other OAuth2 cookies instead of Envoy's shared default, so SecurityPolicies on the same +cookie domain no longer delete each other's in-flight flow cookies on logout. +On upgrade, a browser already holding flow cookies keeps them at the old `path=/`, since the new +deletion headers are scoped to the redirect path. They expire on the lifetime they were originally +issued with - 10 minutes by default, and unaffected by any `csrfTokenTTL` you configure during the +upgrade. +The old code verifier cookie name is also no longer read, so a login that was in progress across the +rollout may need to be retried once. From 81b460b75d97c38870a132ea76c2f9fa76d2b54c Mon Sep 17 00:00:00 2001 From: "Huabing (Robin) Zhao" Date: Mon, 3 Aug 2026 00:32:55 -0700 Subject: [PATCH 2/2] test: make the OIDC e2e client path-aware The e2e cookie tracker replayed every stored cookie on every request, keyed by name alone. That made it blind to the cookie path, so the scoping this change relies on could regress without any test noticing, and cookies with the same name at two different paths overwrote each other. Track cookies by name and path, honour the RFC 6265 path-match rules when deciding what to send, and drop cookies the server expires. It is deliberately still not a net/http/cookiejar: the OAuth2 filter marks every cookie "secure" and these tests run over plain HTTP, so a spec-compliant jar would store the cookies and then never send them back. With that in place, assert that an authorization flow which has been started but not completed - the state an abandoned flow leaves behind - keeps its nonce and code verifier cookies off ordinary application requests. Signed-off-by: Huabing (Robin) Zhao --- test/e2e/tests/oidc.go | 21 ++++++++ test/e2e/tests/oidc_testclient.go | 85 ++++++++++++++++++++++++++++--- 2 files changed, 100 insertions(+), 6 deletions(-) diff --git a/test/e2e/tests/oidc.go b/test/e2e/tests/oidc.go index 6668e55648..f041af969a 100644 --- a/test/e2e/tests/oidc.go +++ b/test/e2e/tests/oidc.go @@ -12,6 +12,7 @@ import ( "io" "net" "net/http" + "net/url" "regexp" "strings" "testing" @@ -243,6 +244,26 @@ func testOIDC(t *testing.T, suite *suite.ConformanceTestSuite, tc *oidcRouteTest t.Errorf("failed to parse login form: %v", err) } + // At this point an authorization flow is in flight but not yet completed, which is + // the state an abandoned flow leaves behind. Its nonce and code verifier cookies must + // be scoped to the callback path: at "/" they would be replayed on every application + // request, and enough of them overflow the request header limit. + flowCookie := regexp.MustCompile(`^(OauthNonce|CodeVerifier)-`) + var inFlightFlowCookies int + for _, cookie := range oidcClient.Cookies() { + if flowCookie.MatchString(cookie.Name) { + inFlightFlowCookies++ + } + } + require.NotZero(t, inFlightFlowCookies, "Expected the in-flight OIDC flow to set nonce and code verifier cookies") + + appPath, err := url.Parse(tc.testURL) + require.NoError(t, err) + for _, cookie := range oidcClient.CookiesForPath(appPath.Path) { + require.False(t, flowCookie.MatchString(cookie.Name), + "OIDC flow cookie %q must not be sent to the application path %q", cookie.Name, appPath.Path) + } + // Submit the login form to the IdP. // This will authenticate and redirect back to the application res, err := oidcClient.Login(map[string]string{"username": username, "password": password, "credentialId": ""}) diff --git a/test/e2e/tests/oidc_testclient.go b/test/e2e/tests/oidc_testclient.go index 1c4912b57c..a81d02a3c2 100644 --- a/test/e2e/tests/oidc_testclient.go +++ b/test/e2e/tests/oidc_testclient.go @@ -31,6 +31,7 @@ import ( "net/http/httputil" "net/url" "strings" + "time" "golang.org/x/net/html" ) @@ -60,16 +61,31 @@ func (l LoggingRoundTripper) RoundTrip(req *http.Request) (*http.Response, error return res, err } +// cookieKey identifies a stored cookie. Name alone is not enough: the OAuth2 +// filter scopes the OIDC flow cookies to the redirect path while the session +// cookies stay at "/", so the same name can legitimately exist at two paths. +type cookieKey struct { + name string + path string +} + // CookieTracker is a http.RoundTripper that tracks cookies received from the server. +// +// It is deliberately not a full net/http/cookiejar: the OAuth2 filter marks every +// cookie "secure" and these tests run over plain HTTP, so a spec-compliant jar +// would store the cookies and then never send them back. It does honour Path and +// cookie deletion, which is what the OIDC flow depends on. type CookieTracker struct { Delegate http.RoundTripper - Cookies map[string]*http.Cookie + Cookies map[cookieKey]*http.Cookie } // RoundTrip tracks the cookies received from the server. func (c *CookieTracker) RoundTrip(req *http.Request) (*http.Response, error) { - for _, ck := range c.Cookies { - req.AddCookie(ck) + for key, ck := range c.Cookies { + if pathMatches(req.URL.Path, key.path) { + req.AddCookie(ck) + } } res, err := c.Delegate.RoundTrip(req) @@ -77,16 +93,59 @@ func (c *CookieTracker) RoundTrip(req *http.Request) (*http.Response, error) { if err == nil { // Track the cookies received from the server for _, ck := range res.Cookies() { - c.Cookies[ck.Name] = ck + key := cookieKey{name: ck.Name, path: cookiePath(ck, req.URL.Path)} + if ck.MaxAge < 0 || (!ck.Expires.IsZero() && !ck.Expires.After(time.Now())) { + delete(c.Cookies, key) + continue + } + c.Cookies[key] = ck } } return res, err } +// CookiesForPath returns the cookies the client would send to the given request path. +func (c *CookieTracker) CookiesForPath(path string) []*http.Cookie { + var cookies []*http.Cookie + for key, ck := range c.Cookies { + if pathMatches(path, key.path) { + cookies = append(cookies, ck) + } + } + return cookies +} + +// cookiePath returns the path a Set-Cookie applies to, defaulting to the +// directory of the request path as described in RFC 6265 section 5.1.4. +func cookiePath(ck *http.Cookie, requestPath string) string { + if ck.Path != "" { + return ck.Path + } + if i := strings.LastIndex(requestPath, "/"); i > 0 { + return requestPath[:i] + } + return "/" +} + +// pathMatches implements the RFC 6265 section 5.1.4 path-match algorithm. +func pathMatches(requestPath, cookiePath string) bool { + if requestPath == "" { + requestPath = "/" + } + if cookiePath == requestPath { + return true + } + if !strings.HasPrefix(requestPath, cookiePath) { + return false + } + return strings.HasSuffix(cookiePath, "/") || requestPath[len(cookiePath)] == '/' +} + // OIDCTestClient encapsulates a http.Client and keeps track of the state of the OIDC login process. type OIDCTestClient struct { http *http.Client // Delegate HTTP client + cookies *CookieTracker // Cookies received from the server loginURL string // URL of the IdP where users need to authenticate loginMethod string // Method (GET/POST) to use when posting the credentials to the IdP mappings *AddressMappings // Custom address mappings @@ -94,6 +153,20 @@ type OIDCTestClient struct { logBody bool // Whether to log the request and response bodies } +// Cookies returns every cookie the client is currently holding, regardless of path. +func (o *OIDCTestClient) Cookies() []*http.Cookie { + cookies := make([]*http.Cookie, 0, len(o.cookies.Cookies)) + for _, ck := range o.cookies.Cookies { + cookies = append(cookies, ck) + } + return cookies +} + +// CookiesForPath returns the cookies the client would send to the given request path. +func (o *OIDCTestClient) CookiesForPath(path string) []*http.Cookie { + return o.cookies.CookiesForPath(path) +} + // Option is a functional option for configuring the OIDCTestClient. type Option func(*OIDCTestClient) error @@ -135,8 +208,8 @@ func NewOIDCTestClient(opts ...Option) (*OIDCTestClient, error) { var ( defaultTransport = http.DefaultTransport.(*http.Transport).Clone() logging = &LoggingRoundTripper{Delegate: defaultTransport} - cookieTracker = &CookieTracker{Cookies: make(map[string]*http.Cookie), Delegate: logging} - client = &OIDCTestClient{http: &http.Client{Transport: cookieTracker}} + cookieTracker = &CookieTracker{Cookies: make(map[cookieKey]*http.Cookie), Delegate: logging} + client = &OIDCTestClient{http: &http.Client{Transport: cookieTracker}, cookies: cookieTracker} ) for _, opt := range opts {