diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index dcd2f9d6..16694676 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -54,50 +54,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\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": "some-state-value", + "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", } 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\nThis client must include a code_challenge when performing the authorize code flow, but it is missing.", - "error_hint": "This client must include a code_challenge when performing the authorize code flow, but it is missing.", - "state": "some-state-value", + "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", } 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", + "state": "some-state-value-that-is-32-byte", } 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", + "state": "some-state-value-that-is-32-byte", } 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", + "state": "some-state-value-that-is-32-byte", } 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", + "state": "some-state-value-that-is-32-byte", } 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 8 characters long to ensure sufficient entropy.", - "error_hint": `Request parameter "state" must be at least be 8 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 32 characters long to ensure sufficient entropy.", + "error_hint": `Request parameter "state" must be at least be 32 characters long to ensure sufficient entropy.`, "state": "short", } @@ -105,7 +105,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", + "state": "some-state-value-that-is-32-byte", } ) @@ -125,11 +125,11 @@ func TestAuthorizationEndpoint(t *testing.T) { oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") - oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) + oauthHelper := oidc.FositeOauth2Helper(issuer, oauthStore, hmacSecret) happyCSRF := "test-csrf" happyPKCE := "test-pkce" - happyNonce := "test-nonce" + happyNonce := "test-nonce-that-is-32-bytes-long" 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 } @@ -175,7 +175,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "response_type": "code", "scope": "openid profile email", "client_id": "pinniped-cli", - "state": "some-state-value", + "state": "some-state-value-that-is-32-byte", "nonce": "some-nonce-value", "code_challenge": "some-challenge", "code_challenge_method": "S256", diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 8d319c1d..0e193046 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -5,6 +5,8 @@ package oidc import ( + "time" + "github.com/ory/fosite" "github.com/ory/fosite/compose" ) @@ -29,9 +31,25 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { } } -func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { +func FositeOauth2Helper(issuerURL string, oauthStore fosite.Storage, hmacSecretOfLengthAtLeast32 []byte) fosite.OAuth2Provider { oauthConfig := &compose.Config{ - EnforcePKCEForPublicClients: true, + AuthorizeCodeLifespan: 3 * time.Minute, // seems more than long enough to exchange a code + + IDTokenLifespan: 5 * time.Minute, // match clientCertificateTTL since it has similar properties to this token + AccessTokenLifespan: 5 * time.Minute, // match clientCertificateTTL since it has similar properties to this token + + RefreshTokenLifespan: 16 * time.Hour, // long enough for a single workday + + IDTokenIssuer: issuerURL, + TokenURL: "", // TODO set once we have this endpoint written + + ScopeStrategy: fosite.ExactScopeStrategy, // be careful and only support exact string matching for scopes + AudienceMatchingStrategy: nil, // I believe the default is fine + EnforcePKCE: true, // follow current set of best practices and always require PKCE + AllowedPromptValues: []string{"none"}, // TODO unclear what we should set here + + RefreshTokenScopes: nil, // TODO decide what makes sense when we add refresh token support + MinParameterEntropy: 32, // 256 bits seems about right } return compose.Compose( diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 723cfe67..6c2e12db 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -70,7 +70,7 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. - oauthHelper := oidc.FositeOauth2Helper(oidc.NullStorage{}, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret + oauthHelper := oidc.FositeOauth2Helper(incomingProvider.Issuer(), oidc.NullStorage{}, []byte("some secret - must have at least 32 bytes")) // TODO replace this secret // TODO use different codecs for the state and the cookie, because: // 1. we would like to state to have an embedded expiration date while the cookie does not need that diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index e2d55f32..bd1a5d4f 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -169,7 +169,7 @@ func TestManager(t *testing.T) { "response_type": []string{"code"}, "scope": []string{"openid profile email"}, "client_id": []string{"pinniped-cli"}, - "state": []string{"some-state-value"}, + "state": []string{"some-state-value-that-is-32-byte"}, "nonce": []string{"some-nonce-value"}, "code_challenge": []string{"some-challenge"}, "code_challenge_method": []string{"S256"},