diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 36e42b25..5bfe7947 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -121,13 +121,22 @@ func NewHandler( } } + authCodeOptions := []oauth2.AuthCodeOption{ + oauth2.AccessTypeOffline, + nonceValue.Param(), + pkceValue.Challenge(), + pkceValue.Method(), + } + + promptParam := r.Form.Get("prompt") + if promptParam != "" && oidc.ScopeWasRequested(authorizeRequester, coreosoidc.ScopeOpenID) { + authCodeOptions = append(authCodeOptions, oauth2.SetAuthURLParam("prompt", promptParam)) + } + http.Redirect(w, r, upstreamOAuthConfig.AuthCodeURL( encodedStateParamValue, - oauth2.AccessTypeOffline, - nonceValue.Param(), - pkceValue.Challenge(), - pkceValue.Method(), + authCodeOptions..., ), 302, ) diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d9dc051c..ee07552b 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -32,8 +32,11 @@ func TestAuthorizationEndpoint(t *testing.T) { downstreamIssuer = "https://my-downstream-issuer.com/some-path" downstreamRedirectURI = "http://127.0.0.1/callback" downstreamRedirectURIWithDifferentPort = "http://127.0.0.1:42/callback" + happyState = "8b-state" ) + require.Len(t, happyState, 8, "we expect fosite to allow 8 byte state params, so we want to test that boundary case") + var ( fositeInvalidClientErrorBody = here.Doc(` { @@ -57,50 +60,50 @@ func TestAuthorizationEndpoint(t *testing.T) { fositePromptHasNoneAndOtherValueErrorQuery = map[string]string{ "error": "invalid_request", - "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nUsed unknown value \"[none login]\" for prompt parameter", - "error_hint": "Used unknown value \"[none login]\" for prompt parameter", - "state": "some-state-value-that-is-32-byte", + "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nParameter \"prompt\" was set to \"none\", but contains other values as well which is not allowed.", + "error_hint": "Parameter \"prompt\" was set to \"none\", but contains other values as well which is not allowed.", + "state": happyState, } fositeMissingCodeChallengeErrorQuery = map[string]string{ "error": "invalid_request", "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nClients must include a code_challenge when performing the authorize code flow, but it is missing.", "error_hint": "Clients must include a code_challenge when performing the authorize code flow, but it is missing.", - "state": "some-state-value-that-is-32-byte", + "state": happyState, } fositeMissingCodeChallengeMethodErrorQuery = map[string]string{ "error": "invalid_request", "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nClients must use code_challenge_method=S256, plain is not allowed.", "error_hint": "Clients must use code_challenge_method=S256, plain is not allowed.", - "state": "some-state-value-that-is-32-byte", + "state": happyState, } fositeInvalidCodeChallengeErrorQuery = map[string]string{ "error": "invalid_request", "error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThe code_challenge_method is not supported, use S256 instead.", "error_hint": "The code_challenge_method is not supported, use S256 instead.", - "state": "some-state-value-that-is-32-byte", + "state": happyState, } fositeUnsupportedResponseTypeErrorQuery = map[string]string{ "error": "unsupported_response_type", "error_description": "The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\".", "error_hint": `The client is not allowed to request response_type "unsupported".`, - "state": "some-state-value-that-is-32-byte", + "state": happyState, } fositeInvalidScopeErrorQuery = map[string]string{ "error": "invalid_scope", "error_description": "The requested scope is invalid, unknown, or malformed\n\nThe OAuth 2.0 Client is not allowed to request scope \"tuna\".", "error_hint": `The OAuth 2.0 Client is not allowed to request scope "tuna".`, - "state": "some-state-value-that-is-32-byte", + "state": happyState, } fositeInvalidStateErrorQuery = map[string]string{ "error": "invalid_state", - "error_description": "The state is missing or does not have enough characters and is therefore considered too weak\n\nRequest parameter \"state\" must be at least be 32 characters long to ensure sufficient entropy.", - "error_hint": `Request parameter "state" must be at least be 32 characters long to ensure sufficient entropy.`, + "error_description": "The state is missing or does not have enough characters and is therefore considered too weak\n\nRequest parameter \"state\" must be at least be 8 characters long to ensure sufficient entropy.", + "error_hint": `Request parameter "state" must be at least be 8 characters long to ensure sufficient entropy.`, "state": "short", } @@ -108,7 +111,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "error": "unsupported_response_type", "error_description": "The authorization server does not support obtaining a token using this method\n\nThe request is missing the \"response_type\"\" parameter.", "error_hint": `The request is missing the "response_type"" parameter.`, - "state": "some-state-value-that-is-32-byte", + "state": happyState, } ) @@ -131,7 +134,7 @@ func TestAuthorizationEndpoint(t *testing.T) { happyCSRF := "test-csrf" happyPKCE := "test-pkce" - happyNonce := "test-nonce-that-is-32-bytes-long" + happyNonce := "test-nonce" happyCSRFGenerator := func() (csrftoken.CSRFToken, error) { return csrftoken.CSRFToken(happyCSRF), nil } happyPKCEGenerator := func() (pkce.Code, error) { return pkce.Code(happyPKCE), nil } happyNonceGenerator := func() (nonce.Nonce, error) { return nonce.Nonce(happyNonce), nil } @@ -177,7 +180,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "response_type": "code", "scope": "openid profile email", "client_id": "pinniped-cli", - "state": "some-state-value-that-is-32-byte", + "state": happyState, "nonce": "some-nonce-value", "code_challenge": "some-challenge", "code_challenge_method": "S256", @@ -229,8 +232,8 @@ func TestAuthorizationEndpoint(t *testing.T) { return encoded } - expectedRedirectLocation := func(expectedUpstreamState string) string { - return urlWithQuery(upstreamAuthURL.String(), map[string]string{ + expectedRedirectLocation := func(expectedUpstreamState string, expectedPrompt string) string { + query := map[string]string{ "response_type": "code", "access_type": "offline", "scope": "scope1 scope2", @@ -240,7 +243,11 @@ func TestAuthorizationEndpoint(t *testing.T) { "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", "redirect_uri": downstreamIssuer + "/callback", - }) + } + if expectedPrompt != "" { + query["prompt"] = expectedPrompt + } + return urlWithQuery(upstreamAuthURL.String(), query) } incomingCookieCSRFValue := "csrf-value-from-cookie" @@ -288,7 +295,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantContentType: "text/html; charset=utf-8", wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", ""), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -306,7 +313,7 @@ func TestAuthorizationEndpoint(t *testing.T) { csrfCookie: "__Host-pinniped-csrf=" + encodedIncomingCookieCSRFValue + " ", wantStatus: http.StatusFound, wantContentType: "text/html; charset=utf-8", - wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, incomingCookieCSRFValue, "")), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, incomingCookieCSRFValue, ""), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -327,7 +334,27 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "", wantBodyString: "", wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", ""), ""), + wantUpstreamStateParamInLocationHeader: true, + }, + { + name: "happy path with prompt param login passed through to redirect uri", + issuer: downstreamIssuer, + idpListGetter: oidctestutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"prompt": "login"}), + contentType: "application/x-www-form-urlencoded", + body: encodeQuery(happyGetRequestQueryMap), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyStringWithLocationInHref: true, + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{"prompt": "login"}, "", ""), "login"), wantUpstreamStateParamInLocationHeader: true, }, { @@ -346,7 +373,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "text/html; charset=utf-8", // Generated a new CSRF cookie and set it in the response. wantCSRFValueInCookieHeader: happyCSRF, - wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", "")), + wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(nil, "", ""), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -368,7 +395,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantCSRFValueInCookieHeader: happyCSRF, wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ "redirect_uri": downstreamRedirectURIWithDifferentPort, // not the same port number that is registered for the client - }, "", "")), + }, "", ""), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -388,7 +415,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantCSRFValueInCookieHeader: happyCSRF, wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam(map[string]string{ "scope": "openid offline_access", - }, "", "")), + }, "", ""), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, @@ -586,7 +613,7 @@ func TestAuthorizationEndpoint(t *testing.T) { wantCSRFValueInCookieHeader: happyCSRF, wantLocationHeader: expectedRedirectLocation(expectedUpstreamStateParam( map[string]string{"prompt": "none login", "scope": "email"}, "", "", - )), + ), ""), wantUpstreamStateParamInLocationHeader: true, wantBodyStringWithLocationInHref: true, }, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index dfc153f1..38d12b29 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -48,7 +48,7 @@ const ( happyUpstreamRedirectURI = "https://example.com/callback" - happyDownstreamState = "some-downstream-state-with-at-least-32-bytes" + happyDownstreamState = "8b-state" happyDownstreamCSRF = "test-csrf" happyDownstreamPKCE = "test-pkce" happyDownstreamNonce = "test-nonce" @@ -84,6 +84,8 @@ var ( ) func TestCallbackEndpoint(t *testing.T) { + require.Len(t, happyDownstreamState, 8, "we expect fosite to allow 8 byte state params, so we want to test that boundary case") + otherUpstreamOIDCIdentityProvider := oidctestutil.TestUpstreamOIDCIdentityProvider{ Name: "other-upstream-idp-name", ClientID: "other-some-client-id", diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go index 85a63501..f0250e22 100644 --- a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -30,7 +30,7 @@ func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { clientID = "some-client-id" goodSubject = "some-subject" goodUsername = "some-username" - goodNonce = "some-nonce-that-is-at-least-32-characters-to-meet-entropy-requirements" + goodNonce = "some-nonce-value-with-enough-bytes-to-exceed-min-allowed" ) ecPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 76600470..a1cb56fe 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -199,12 +199,20 @@ func FositeOauth2Helper( AccessTokenLifespan: timeoutsConfiguration.AccessTokenLifespan, RefreshTokenLifespan: timeoutsConfiguration.RefreshTokenLifespan, - ScopeStrategy: fosite.ExactScopeStrategy, - AudienceMatchingStrategy: nil, - EnforcePKCE: true, - AllowedPromptValues: []string{"none"}, // TODO unclear what we should set here - RefreshTokenScopes: []string{coreosoidc.ScopeOfflineAccess}, // as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess - MinParameterEntropy: 32, // TODO is 256 bits too high? + ScopeStrategy: fosite.ExactScopeStrategy, + EnforcePKCE: true, + + // "offline_access" as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess + RefreshTokenScopes: []string{coreosoidc.ScopeOfflineAccess}, + + // The default is to support all prompt values from the spec. + // See https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest + // We'll make a best effort to support these by passing the value of this prompt param to the upstream IDP + // and rely on its implementation of this param. + AllowedPromptValues: nil, + + // Use the fosite default to make it more likely that off the shelf OIDC clients can work with the supervisor. + MinParameterEntropy: fosite.MinParameterEntropy, } return compose.Compose( @@ -251,9 +259,16 @@ type IDPListGetter interface { } func GrantScopeIfRequested(authorizeRequester fosite.AuthorizeRequester, scopeName string) { - for _, scope := range authorizeRequester.GetRequestedScopes() { - if scope == scopeName { - authorizeRequester.GrantScope(scope) - } + if ScopeWasRequested(authorizeRequester, scopeName) { + authorizeRequester.GrantScope(scopeName) } } + +func ScopeWasRequested(authorizeRequester fosite.AuthorizeRequester, scopeName string) bool { + for _, scope := range authorizeRequester.GetRequestedScopes() { + if scope == scopeName { + return true + } + } + return false +} diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 0720b719..9776c6fa 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -142,7 +142,7 @@ func TestManager(t *testing.T) { actualLocationQueryParams := parsedLocation.Query() r.Contains(actualLocationQueryParams, "code") r.Equal("openid", actualLocationQueryParams.Get("scope")) - r.Equal("some-state-value-that-is-32-byte", actualLocationQueryParams.Get("state")) + r.Equal("some-state-value-with-enough-bytes-to-exceed-min-allowed", actualLocationQueryParams.Get("state")) // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+3, @@ -293,8 +293,8 @@ func TestManager(t *testing.T) { "response_type": []string{"code"}, "scope": []string{"openid profile email"}, "client_id": []string{downstreamClientID}, - "state": []string{"some-state-value-that-is-32-byte"}, - "nonce": []string{"some-nonce-value-that-is-at-least-32-bytes"}, + "state": []string{"some-state-value-with-enough-bytes-to-exceed-min-allowed"}, + "nonce": []string{"some-nonce-value-with-enough-bytes-to-exceed-min-allowed"}, "code_challenge": []string{testutil.SHA256(downstreamPKCECodeVerifier)}, "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURL}, diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index fa64dc3b..f4610293 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -52,7 +52,7 @@ const ( goodClient = "pinniped-cli" goodRedirectURI = "http://127.0.0.1/callback" goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" - goodNonce = "some-nonce-that-is-at-least-32-characters-to-meet-entropy-requirements" + goodNonce = "some-nonce-value-with-enough-bytes-to-exceed-min-allowed" goodSubject = "some-subject" goodUsername = "some-username" @@ -213,7 +213,7 @@ var ( "response_type": {"code"}, "scope": {"openid profile email"}, "client_id": {goodClient}, - "state": {"some-state-value-that-is-32-byte"}, + "state": {"some-state-value-with-enough-bytes-to-exceed-min-allowed"}, "nonce": {goodNonce}, "code_challenge": {testutil.SHA256(goodPKCECodeVerifier)}, "code_challenge_method": {"S256"},