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. 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 {