diff --git a/internal/federationdomain/endpoints/auth/auth_handler.go b/internal/federationdomain/endpoints/auth/auth_handler.go index ec211c51f..158138985 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler.go +++ b/internal/federationdomain/endpoints/auth/auth_handler.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package auth provides a handler for the OIDC authorization endpoint. @@ -510,12 +510,32 @@ func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken } http.SetCookie(w, &http.Cookie{ - Name: oidc.CSRFCookieName, - Value: encodedCSRFValue, + // Because of the other settings below, this value can only be known by the end user's browser, not by other sites. + Value: encodedCSRFValue, + // Name starting with "__Host-" makes the cookie domain-locked (subdomains cannot set this cookie). + Name: oidc.CSRFCookieName, + // This cookie can't be accessed by JavaScript. HttpOnly: true, - SameSite: http.SameSiteLaxMode, - Secure: true, - Path: "/", + // Okay for requests from other sites to cause the user's browser to send this cookie back to this site, + // for allowing response_mode=form_post, in which an upstream IDP needs to host a web form which POSTs back + // to the Supervisor's callback endpoint. Note that this allows a malicious 3rd party site to cause the user's + // browser to include this cookie on a request to the Supervisor. However, there is no way for 3rd party sites + // to create the corresponding state param to include on a callback request to the Supervisor to cause that + // callback request be allowed by the Supervisor's callback endpoint. That state param must include this cookie's + // value. A 3rd party site cannot receive this cookie (and therefore cannot know its value), and even if it somehow + // did learn its value, it could not sign the state param (cannot know the signing key for state params, which never + // leaves the Supervisor server). So although a 3rd party site could cause the user's cookie to be sent, that + // request will never be considered acceptable by the Supervisor. + // Note that SameSite=None was created in a 2019 draft standard, so it requires modern browsers to work. + // See https://datatracker.ietf.org/doc/html/draft-west-cookie-incrementalism-00. + SameSite: http.SameSiteNoneMode, + // This cookie may only be sent via HTTPS (required for domain-locked cookies). + Secure: true, + // Sending this cookie to any path of this server is acceptable (required for domain-locked cookies). + Path: "/", + // Note that we do not set "Domain", so this cookie should not be sent to any subdomains (required for domain-locked cookies). + // Also note that we do not set "Expires" or "MaxAge", so the client may keep the cookie as long as it likes, + // which prevents the cookie from expiring during login flows. }) return nil diff --git a/internal/federationdomain/endpoints/auth/auth_handler_test.go b/internal/federationdomain/endpoints/auth/auth_handler_test.go index 5e911063f..d3518b41d 100644 --- a/internal/federationdomain/endpoints/auth/auth_handler_test.go +++ b/internal/federationdomain/endpoints/auth/auth_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package auth @@ -1551,7 +1551,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: happyGetRequestPathForOIDCUpstream, - csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + " ", + csrfCookie: "__Host-pinniped-csrf-v2=" + encodedIncomingCookieCSRFValue + " ", wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, incomingCookieCSRFValue, oidcUpstreamName, "oidc"), nil), @@ -1568,7 +1568,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: happyGetRequestPathForLDAPUpstream, - csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + " ", + csrfCookie: "__Host-pinniped-csrf-v2=" + encodedIncomingCookieCSRFValue + " ", wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(nil, incomingCookieCSRFValue, ldapUpstreamName, "ldap")}), @@ -1585,7 +1585,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: happyGetRequestPathForADUpstream, - csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + " ", + csrfCookie: "__Host-pinniped-csrf-v2=" + encodedIncomingCookieCSRFValue + " ", wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantLocationHeader: urlWithQuery(downstreamIssuer+"/login", map[string]string{"state": expectedUpstreamStateParam(nil, incomingCookieCSRFValue, activeDirectoryUpstreamName, "activedirectory")}), @@ -1931,7 +1931,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: happyGetRequestPathForOIDCUpstream, - csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + csrfCookie: "__Host-pinniped-csrf-v2=this-value-was-not-signed-by-pinniped", wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, // Generated a new CSRF cookie and set it in the response. @@ -4217,7 +4217,7 @@ func TestAuthorizationEndpoint(t *testing.T) { //nolint:gocyclo if test.wantCSRFValueInCookieHeader != "" { require.Len(t, rsp.Header().Values("Set-Cookie"), 1) actualCookie := rsp.Header().Get("Set-Cookie") - regex := regexp.MustCompile("__Host-pinniped-csrf=([^;]+); Path=/; HttpOnly; Secure; SameSite=Lax") + regex := regexp.MustCompile("__Host-pinniped-csrf-v2=([^;]+); Path=/; HttpOnly; Secure; SameSite=None") submatches := regex.FindStringSubmatch(actualCookie) require.Len(t, submatches, 2) captured := submatches[1] diff --git a/internal/federationdomain/endpoints/callback/callback_handler.go b/internal/federationdomain/endpoints/callback/callback_handler.go index fec046dcc..4acce5a41 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler.go +++ b/internal/federationdomain/endpoints/callback/callback_handler.go @@ -1,10 +1,11 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 // Package callback provides a handler for the OIDC callback endpoint. package callback import ( + "mime" "net/http" "net/url" "strings" @@ -136,8 +137,36 @@ func authcode(r *http.Request) string { } func validateRequest(r *http.Request, stateDecoder, cookieDecoder oidc.Decoder, auditLogger plog.AuditLogger) (*oidc.UpstreamStateParamData, error) { - if r.Method != http.MethodGet { - return nil, httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET)", r.Method) + // An upstream OIDC provider will typically return a redirect with the authcode, + // which the user's browser will follow and therefore perform a GET to this endpoint. + // See https://openid.net/specs/openid-connect-core-1_0.html#AuthResponse. + // + // If the upstream provider supports using response_mode=form_post, + // and if the admin configured OIDCIdentityProvider.spec.authorizationConfig.additionalAuthorizeParameters + // to include response_mode=form_post, then the user's browser will POST the authcode to this endpoint. + // See https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html. + if r.Method != http.MethodGet && r.Method != http.MethodPost { + return nil, httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) + } + + if r.Method == http.MethodPost { + // https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html says + // "the result parameters being encoded in the body using the application/x-www-form-urlencoded format", + // so only accept that format. Since "multipart/form-data" is intended for binary data, there's no need to + // support it here. + contentType := r.Header.Get("Content-Type") + if contentType == "" { + return nil, httperr.New(http.StatusUnsupportedMediaType, "no Content-Type header (try Content-Type: application/x-www-form-urlencoded)") + } + parsedContentType, _, err := mime.ParseMediaType(contentType) + if err != nil { + // Note that it should not be possible to reach this line since AuditRequestParams() is already + // parsing the form and handling unparseable form types before we get here. This is just defensive coding. + return nil, httperr.Newf(http.StatusUnsupportedMediaType, "cannot parse Content-Type: %s (try Content-Type: application/x-www-form-urlencoded)", contentType) + } + if parsedContentType != "application/x-www-form-urlencoded" { + return nil, httperr.Newf(http.StatusUnsupportedMediaType, "%s (try application/x-www-form-urlencoded)", contentType) + } } encodedState, decodedState, err := oidc.ReadStateParamAndValidateCSRFCookie(r, cookieDecoder, stateDecoder) diff --git a/internal/federationdomain/endpoints/callback/callback_handler_test.go b/internal/federationdomain/endpoints/callback/callback_handler_test.go index aded94d84..ddab57944 100644 --- a/internal/federationdomain/endpoints/callback/callback_handler_test.go +++ b/internal/federationdomain/endpoints/callback/callback_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package callback @@ -7,6 +7,7 @@ import ( "context" "errors" "fmt" + "io" "net/http" "net/http/httptest" "net/url" @@ -194,7 +195,7 @@ func TestCallbackEndpoint(t *testing.T) { encodedIncomingCookieCSRFValue, err := happyCookieCodec.Encode("csrf", happyDownstreamCSRF) require.NoError(t, err) - happyCSRFCookie := "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + happyCSRFCookie := "__Host-pinniped-csrf-v2=" + encodedIncomingCookieCSRFValue happyOIDCUpstreamExchangeAuthcodeAndValidateTokenArgs := &oidctestutil.ExchangeAuthcodeAndValidateTokenArgs{ Authcode: happyUpstreamAuthcode, @@ -229,6 +230,8 @@ func TestCallbackEndpoint(t *testing.T) { kubeResources func(t *testing.T, supervisorClient *supervisorfake.Clientset, kubeClient *fake.Clientset) method string path string + body string + headers map[string]string csrfCookie string wantStatus int @@ -318,6 +321,78 @@ func TestCallbackEndpoint(t *testing.T) { } }, }, + { + name: "OIDC: POST (like when upstream is using response_mode=form_post) with good state and cookie and successful upstream token exchange with response_mode=form_post returns 200 with HTML+JS form", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: url.Values{ + "code": []string{happyUpstreamAuthcode}, + "state": []string{ + happyOIDCUpstreamStateParam().WithAuthorizeRequestParams( + shallowCopyAndModifyQuery( + happyDownstreamRequestParamsQuery, + map[string]string{"response_mode": "form_post"}, + ).Encode(), + ).Build(t, happyStateCodec).String(), + }, + }.Encode(), + path: (&requestPath{}).String(), + headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded; charset=utf-8"}, + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusOK, + wantContentType: "text/html;charset=UTF-8", + wantBodyFormResponseRegexp: `(.+)`, + wantDownstreamIDTokenSubject: oidcUpstreamIssuer + "?idpName=" + happyOIDCUpstreamIDPName + "&sub=" + oidcUpstreamSubjectQueryEscaped, + wantDownstreamIDTokenUsername: oidcUpstreamUsername, + wantDownstreamIDTokenGroups: oidcUpstreamGroupMembership, + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamClientID: downstreamPinnipedClientID, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantDownstreamCustomSessionData: happyDownstreamCustomSessionDataForOIDCUpstream, + wantOIDCAuthcodeExchangeCall: &expectedOIDCAuthcodeExchange{ + performedByUpstreamName: happyOIDCUpstreamIDPName, + args: happyOIDCUpstreamExchangeAuthcodeAndValidateTokenArgs, + }, + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{"code": "redacted", "state": "redacted"}, + }), + testutil.WantAuditLog("AuthorizeID From Parameters", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), + }), + testutil.WantAuditLog("Using Upstream IDP", map[string]any{ + "displayName": "upstream-oidc-idp-name", + "resourceName": "upstream-oidc-idp-name", + "resourceUID": "upstream-oidc-resource-uid", + "type": "oidc", + }), + testutil.WantAuditLog("Identity From Upstream IDP", map[string]any{ + "upstreamIDPDisplayName": "upstream-oidc-idp-name", + "upstreamIDPType": "oidc", + "upstreamIDPResourceName": "upstream-oidc-idp-name", + "upstreamIDPResourceUID": "upstream-oidc-resource-uid", + "personalInfo": map[string]any{ + "upstreamUsername": "test-pinniped-username", + "upstreamGroups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, + }, + }), + testutil.WantAuditLog("Session Started", map[string]any{ + "sessionID": sessionID, + "warnings": []any{}, // json: [] + "personalInfo": map[string]any{ + "username": "test-pinniped-username", + "groups": []any{"test-pinniped-group-0", "test-pinniped-group-1"}, + "subject": "https://my-upstream-issuer.com?idpName=upstream-oidc-idp-name&sub=abc123-some+guid", + "additionalClaims": map[string]any{}, // json: {} + }, + }), + } + }, + }, { name: "GitHub: GET with good state and cookie and successful upstream token exchange with response_mode=form_post returns 200 with HTML+JS form", idps: testidplister.NewUpstreamIDPListerBuilder().WithGitHub(happyGitHubUpstream().Build()), @@ -1131,7 +1206,7 @@ func TestCallbackEndpoint(t *testing.T) { path: newRequestPath().String(), wantStatus: http.StatusMethodNotAllowed, wantContentType: htmlContentType, - wantBody: "Method Not Allowed: PUT (try GET)\n", + wantBody: "Method Not Allowed: PUT (try GET or POST)\n", wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { return []testutil.WantedAuditLog{ testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ @@ -1140,15 +1215,6 @@ func TestCallbackEndpoint(t *testing.T) { } }, }, - { - name: "POST method is invalid", - idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), - method: http.MethodPost, - path: newRequestPath().String(), - wantStatus: http.StatusMethodNotAllowed, - wantContentType: htmlContentType, - wantBody: "Method Not Allowed: POST (try GET)\n", - }, { name: "PATCH method is invalid", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), @@ -1156,7 +1222,7 @@ func TestCallbackEndpoint(t *testing.T) { path: newRequestPath().String(), wantStatus: http.StatusMethodNotAllowed, wantContentType: htmlContentType, - wantBody: "Method Not Allowed: PATCH (try GET)\n", + wantBody: "Method Not Allowed: PATCH (try GET or POST)\n", }, { name: "DELETE method is invalid", @@ -1165,7 +1231,7 @@ func TestCallbackEndpoint(t *testing.T) { path: newRequestPath().String(), wantStatus: http.StatusMethodNotAllowed, wantContentType: htmlContentType, - wantBody: "Method Not Allowed: DELETE (try GET)\n", + wantBody: "Method Not Allowed: DELETE (try GET or POST)\n", }, { name: "params cannot be parsed", @@ -1267,6 +1333,42 @@ func TestCallbackEndpoint(t *testing.T) { } }, }, + { + name: "code param was not included on request and there is no error param when the request was a POST with body", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: url.Values{ + "state": []string{ + happyOIDCUpstreamStateParam().WithAuthorizeRequestParams( + shallowCopyAndModifyQuery( + happyDownstreamRequestParamsQuery, + map[string]string{"response_mode": "form_post"}, + ).Encode(), + ).Build(t, happyStateCodec).String(), + }, + }.Encode(), + path: (&requestPath{}).String(), + headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"}, + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: here.Doc(`Bad Request: code param not found + + Something went wrong with your authentication attempt at your external identity provider. + + Pinniped AuditID: fake-audit-id + `), + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{"state": "redacted"}, + }), + testutil.WantAuditLog("AuthorizeID From Parameters", map[string]any{ + "authorizeID": encodedStateParam.AuthorizeID(), + }), + } + }, + }, { name: "state param was not included on request", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), @@ -1284,6 +1386,77 @@ func TestCallbackEndpoint(t *testing.T) { } }, }, + { + name: "state param was not included on request when the request was a POST with body", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: url.Values{"code": []string{happyUpstreamAuthcode}}.Encode(), + path: (&requestPath{}).String(), + headers: map[string]string{"Content-Type": "application/x-www-form-urlencoded"}, + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: "Bad Request: state param not found\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{"code": "redacted"}, + }), + } + }, + }, + { + name: "wrong Content-Type header included on request when the request was a POST with body", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: "code=" + happyUpstreamAuthcode, + path: (&requestPath{}).String(), + headers: map[string]string{"Content-Type": "application/text"}, + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnsupportedMediaType, + wantContentType: htmlContentType, + wantBody: "Unsupported Media Type: application/text (try application/x-www-form-urlencoded)\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{}, + }), + } + }, + }, + { + name: "unparseable Content-Type header included on request when the request was a POST with body", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: "code=" + happyUpstreamAuthcode, + path: (&requestPath{}).String(), + headers: map[string]string{"Content-Type": "bogus ;========="}, // this is unparseable garbage + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusBadRequest, + wantContentType: htmlContentType, + wantBody: "Bad Request: error parsing request params\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{} // couldn't parse params for auditing of params + }, + }, + { + name: "no Content-Type header included on request when the request was a POST with body", + idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), + method: http.MethodPost, + body: "code=" + happyUpstreamAuthcode, + path: (&requestPath{}).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusUnsupportedMediaType, + wantContentType: htmlContentType, + wantBody: "Unsupported Media Type: no Content-Type header (try Content-Type: application/x-www-form-urlencoded)\n", + wantAuditLogs: func(encodedStateParam stateparam.Encoded, sessionID string) []testutil.WantedAuditLog { + return []testutil.WantedAuditLog{ + testutil.WantAuditLog("HTTP Request Parameters", map[string]any{ + "params": map[string]any{}, + }), + } + }, + }, { name: "state param was not signed correctly, has expired, or otherwise cannot be decoded for any reason", idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), @@ -1624,7 +1797,7 @@ func TestCallbackEndpoint(t *testing.T) { idps: testidplister.NewUpstreamIDPListerBuilder().WithOIDC(happyOIDCUpstream().Build()), method: http.MethodGet, path: newRequestPath().WithState(happyOIDCState).String(), - csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + csrfCookie: "__Host-pinniped-csrf-v2=this-value-was-not-signed-by-pinniped", wantStatus: http.StatusForbidden, wantContentType: htmlContentType, wantBody: "Forbidden: error reading CSRF cookie\n", @@ -2024,10 +2197,19 @@ func TestCallbackEndpoint(t *testing.T) { ) reqContext := context.WithValue(context.Background(), struct{ name string }{name: "test"}, "request-context") - req := httptest.NewRequest(test.method, test.path, nil).WithContext(reqContext) + var bodyReader io.Reader + if test.body != "" { + bodyReader = strings.NewReader(test.body) + } + req := httptest.NewRequest(test.method, test.path, bodyReader).WithContext(reqContext) if test.csrfCookie != "" { req.Header.Set("Cookie", test.csrfCookie) } + if test.headers != nil { + for k, v := range test.headers { + req.Header.Set(k, v) + } + } req, _ = auditid.NewRequestWithAuditID(req, func() string { return "fake-audit-id" }) rsp := httptest.NewRecorder() subject.ServeHTTP(rsp, req) @@ -2116,7 +2298,12 @@ func TestCallbackEndpoint(t *testing.T) { } if test.wantAuditLogs != nil { - wantAuditLogs := test.wantAuditLogs(testutil.GetStateParam(t, test.path), sessionID) + encodedStateParam := testutil.GetStateParamFromRequestURL(t, test.path) + if test.body != "" { + // Assume that the state param was sent in a form post body. + encodedStateParam = testutil.GetStateParamFromRequestBody(t, test.body) + } + wantAuditLogs := test.wantAuditLogs(encodedStateParam, sessionID) testutil.WantAuditIDOnEveryAuditLog(wantAuditLogs, "fake-audit-id") testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) } diff --git a/internal/federationdomain/endpoints/login/login_handler_test.go b/internal/federationdomain/endpoints/login/login_handler_test.go index 243f5da14..5d9560c73 100644 --- a/internal/federationdomain/endpoints/login/login_handler_test.go +++ b/internal/federationdomain/endpoints/login/login_handler_test.go @@ -1,4 +1,4 @@ -// Copyright 2022-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2022-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package login @@ -109,7 +109,7 @@ func TestLoginEndpoint(t *testing.T) { encodedIncomingCookieCSRFValue, err := happyCookieCodec.Encode("csrf", happyDownstreamCSRF) require.NoError(t, err) - happyCSRFCookie := "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + happyCSRFCookie := "__Host-pinniped-csrf-v2=" + encodedIncomingCookieCSRFValue tests := []struct { name string @@ -261,7 +261,7 @@ func TestLoginEndpoint(t *testing.T) { name: "the CSRF cookie was not signed correctly, has expired, or otherwise cannot be decoded for any reason on GET request", method: http.MethodGet, path: happyPathWithState, - csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + csrfCookie: "__Host-pinniped-csrf-v2=this-value-was-not-signed-by-pinniped", wantStatus: http.StatusForbidden, wantContentType: htmlContentType, wantBody: "Forbidden: error reading CSRF cookie\n", @@ -270,7 +270,7 @@ func TestLoginEndpoint(t *testing.T) { name: "the CSRF cookie was not signed correctly, has expired, or otherwise cannot be decoded for any reason on POST request", method: http.MethodPost, path: happyPathWithState, - csrfCookie: "__Host-pinniped-csrf=this-value-was-not-signed-by-pinniped", + csrfCookie: "__Host-pinniped-csrf-v2=this-value-was-not-signed-by-pinniped", wantStatus: http.StatusForbidden, wantContentType: htmlContentType, wantBody: "Forbidden: error reading CSRF cookie\n", @@ -519,7 +519,7 @@ func TestLoginEndpoint(t *testing.T) { require.Equal(t, test.wantBody, rsp.Body.String()) if test.wantAuditLogs != nil { - wantAuditLogs := test.wantAuditLogs(testutil.GetStateParam(t, test.path)) + wantAuditLogs := test.wantAuditLogs(testutil.GetStateParamFromRequestURL(t, test.path)) testutil.WantAuditIDOnEveryAuditLog(wantAuditLogs, "fake-audit-id") testutil.CompareAuditLogs(t, wantAuditLogs, actualAuditLog.String()) } diff --git a/internal/federationdomain/endpointsmanager/manager_test.go b/internal/federationdomain/endpointsmanager/manager_test.go index 960755e40..aa19708b6 100644 --- a/internal/federationdomain/endpointsmanager/manager_test.go +++ b/internal/federationdomain/endpointsmanager/manager_test.go @@ -187,7 +187,7 @@ func TestManager(t *testing.T) { cookies := recorder.Result().Cookies() r.Len(cookies, 1) csrfCookie := cookies[0] - r.Equal("__Host-pinniped-csrf", csrfCookie.Name) + r.Equal("__Host-pinniped-csrf-v2", csrfCookie.Name) r.NotEmpty(csrfCookie.Value) // Return the important parts of the response so we can use them in our next request to the callback endpoint @@ -201,7 +201,7 @@ func TestManager(t *testing.T) { getRequest := newGetRequest(requestIssuer + oidc.CallbackEndpointPath + requestURLSuffix) getRequest.AddCookie(&http.Cookie{ - Name: "__Host-pinniped-csrf", + Name: "__Host-pinniped-csrf-v2", Value: csrfCookieValue, }) subject.HandlerChain().ServeHTTP(recorder, getRequest) diff --git a/internal/federationdomain/oidc/oidc.go b/internal/federationdomain/oidc/oidc.go index 7e99e1eb0..7ebf8c920 100644 --- a/internal/federationdomain/oidc/oidc.go +++ b/internal/federationdomain/oidc/oidc.go @@ -61,9 +61,11 @@ const ( UpstreamStateParamEncodingName = "s" // CSRFCookieName is the name of the browser cookie which shall hold our CSRF value. + // The "-v2" suffix was added when the SameSite value was changed from Lax to None, + // to force the creation and use of a new cookie upon upgrade. // The `__Host` prefix has a special meaning. See: // https://developer.mozilla.org/en-US/docs/Web/HTTP/Cookies#Cookie_prefixes. - CSRFCookieName = "__Host-pinniped-csrf" + CSRFCookieName = "__Host-pinniped-csrf-v2" // CSRFCookieEncodingName is the `name` passed to the encoder for encoding and decoding the CSRF // cookie contents. diff --git a/internal/testutil/log_lines.go b/internal/testutil/log_lines.go index 73b28cca4..8977b61df 100644 --- a/internal/testutil/log_lines.go +++ b/internal/testutil/log_lines.go @@ -1,4 +1,4 @@ -// Copyright 2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package testutil @@ -44,7 +44,7 @@ func WantAuditIDOnEveryAuditLog(wantedAuditLogs []WantedAuditLog, wantAuditID st } } -func GetStateParam(t *testing.T, fullURL string) stateparam.Encoded { +func GetStateParamFromRequestURL(t *testing.T, fullURL string) stateparam.Encoded { if fullURL == "" { var empty stateparam.Encoded return empty @@ -55,6 +55,12 @@ func GetStateParam(t *testing.T, fullURL string) stateparam.Encoded { return stateparam.Encoded(path.Query().Get("state")) } +func GetStateParamFromRequestBody(t *testing.T, body string) stateparam.Encoded { + values, err := url.ParseQuery(body) + require.NoError(t, err) + return stateparam.Encoded(values.Get("state")) +} + func CompareAuditLogs(t *testing.T, wantAuditLogs []WantedAuditLog, actualAuditLogsOneLiner string) { t.Helper() diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index a4f5b1713..3b6a0933c 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1,4 +1,4 @@ -// Copyright 2020-2024 the Pinniped contributors. All Rights Reserved. +// Copyright 2020-2025 the Pinniped contributors. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 package integration @@ -362,6 +362,27 @@ func TestSupervisorLogin_Browser(t *testing.T) { // the ID token Username should include the upstream user ID after the upstream issuer name wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, }, + { + name: "oidc with upstream response_mode=form_post", + maybeSkip: skipNever, + createIDP: func(t *testing.T) string { + providerSpec := basicOIDCIdentityProviderSpec() + providerSpec.AuthorizationConfig.AdditionalAuthorizeParameters = []idpv1alpha1.Parameter{{ + // Note that at the time of writing this comment, Dex completely ignores this param + // and just treats it as the default response_mode by returning a redirect. + // However, when the external OIDC provider is Okta, this should actually test + // using this capability. Okta will cause the user's browser to POST the authcode + // to the Supervisor's callback endpoint. + Name: "response_mode", + Value: "form_post", + }} + return testlib.CreateTestOIDCIdentityProvider(t, providerSpec, idpv1alpha1.PhaseReady).Name + }, + requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlowOIDC, + wantDownstreamIDTokenSubjectToMatch: expectedIDTokenSubjectRegexForUpstreamOIDC, + // the ID token Username should include the upstream user ID after the upstream issuer name + wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+" }, + }, { name: "oidc IDP using secrets of type opaque to source ca bundle with default username and groups claim settings", maybeSkip: skipExternalCABundleOIDCTestsWhenCABundleIsEmpty,