diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 0175c64c..d9dc051c 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -127,7 +127,7 @@ func TestAuthorizationEndpoint(t *testing.T) { 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") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) happyCSRF := "test-csrf" happyPKCE := "test-pkce" diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 67857f6a..df0c2779 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -61,6 +61,7 @@ const ( downstreamPKCEChallenge = "some-challenge" downstreamPKCEChallengeMethod = "S256" + authCodeExpirationSeconds = 10 * 60 // Current, we set our auth code expiration to 10 minutes timeComparisonFudgeFactor = time.Second * 15 ) @@ -460,7 +461,7 @@ func TestCallbackEndpoint(t *testing.T) { 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") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) @@ -768,7 +769,7 @@ func validateAuthcodeStorage( require.Empty(t, storedSessionFromAuthcode.Headers) // The authcode that we are issuing should be good for the length of time that we declare in the fosite config. - testutil.RequireTimeInDelta(t, time.Now().Add(time.Minute*3), storedSessionFromAuthcode.ExpiresAt[fosite.AuthorizeCode], timeComparisonFudgeFactor) + testutil.RequireTimeInDelta(t, time.Now().Add(authCodeExpirationSeconds*time.Second), storedSessionFromAuthcode.ExpiresAt[fosite.AuthorizeCode], timeComparisonFudgeFactor) require.Len(t, storedSessionFromAuthcode.ExpiresAt, 1) // Now confirm the ID token claims. diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 762fc842..76600470 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -91,30 +91,120 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { } } +type TimeoutsConfiguration struct { + // The length of time that our state param that we encrypt and pass to the upstream OIDC IDP should be considered + // valid. If a state param generated by the authorize endpoint is sent to the callback endpoint after this much + // time has passed, then the callback endpoint should reject it. This allows us to set a limit on how long + // the end user has to finish their login with the upstream IDP, including the time that it takes to fumble + // with password manager and two-factor authenticator apps, and also accounting for taking a coffee break while + // the browser is sitting at the upstream IDP's login page. + UpstreamStateParamLifespan time.Duration + + // How long an authcode issued by the callback endpoint is valid. This determines how much time the end user + // has to come back to exchange the authcode for tokens at the token endpoint. + AuthorizeCodeLifespan time.Duration + + // The lifetime of an downstream access token issued by the token endpoint. Access tokens should generally + // be fairly short-lived. + AccessTokenLifespan time.Duration + + // The lifetime of an downstream ID token issued by the token endpoint. This should generally be the same + // as the AccessTokenLifespan, or longer if it would be useful for the user's proof of identity to be valid + // for longer than their proof of authorization. + IDTokenLifespan time.Duration + + // The lifetime of an downstream refresh token issued by the token endpoint. This should generally be + // significantly longer than the access token lifetime, so it can be used to refresh the access token + // multiple times. Once the refresh token expires, the user's session is over and they will need + // to start a new authorization request, which will require them to log in again with the upstream IDP + // in their web browser. + RefreshTokenLifespan time.Duration + + // AuthorizationCodeSessionStorageLifetime is the length of time after which an authcode is allowed to be garbage + // collected from storage. Authcodes are kept in storage after they are redeemed to allow the system to mark the + // authcode as already used, so it can reject any future uses of the same authcode with special case handling which + // include revoking the access and refresh tokens associated with the session. Therefore, this should be + // significantly longer than the AuthorizeCodeLifespan, and there is probably no reason to make it longer than + // the sum of the AuthorizeCodeLifespan and the RefreshTokenLifespan. + AuthorizationCodeSessionStorageLifetime time.Duration + + // PKCESessionStorageLifetime is the length of time after which PKCE data is allowed to be garbage collected from + // storage. PKCE sessions are closely related to authorization code sessions. After the authcode is successfully + // redeemed, the PKCE session is explicitly deleted. After the authcode expires, the PKCE session is no longer needed, + // but it is not explicitly deleted. Therefore, this can be just slightly longer than the AuthorizeCodeLifespan. We'll + // avoid making it exactly the same as AuthorizeCodeLifespan to avoid any chance of the garbage collector deleting it + // while it is being used. + PKCESessionStorageLifetime time.Duration + + // OIDCSessionStorageLifetime is the length of time after which the OIDC session data related to an authcode + // is allowed to be garbage collected from storage. Due to a bug in an underlying library, these are not explicitly + // deleted. Similar to the PKCE session, they are not needed anymore after the corresponding authcode has expired. + // Therefore, this can be just slightly longer than the AuthorizeCodeLifespan. We'll avoid making it exactly the same + // as AuthorizeCodeLifespan to avoid any chance of the garbage collector deleting it while it is being used. + OIDCSessionStorageLifetime time.Duration + + // AccessTokenSessionStorageLifetime is the length of time after which an access token's session data is allowed + // to be garbage collected from storage. These must exist in storage for as long as the refresh token is valid. + // Therefore, this can be just slightly longer than the AccessTokenLifespan. Access tokens are handed back to + // the token endpoint for the token exchange use case. During a token exchange, if the access token is expired + // and still exists in storage, then the endpoint will be able to give a slightly more specific error message, + // rather than a more generic error that is returned when the token does not exist. If this is desirable, then + // the AccessTokenSessionStorageLifetime can be made to be significantly larger than AccessTokenLifespan, at the + // cost of slower cleanup. + AccessTokenSessionStorageLifetime time.Duration + + // RefreshTokenSessionStorageLifetime is the length of time after which a refresh token's session data is allowed + // to be garbage collected from storage. These must exist in storage for as long as the refresh token is valid. + // Therefore, this can be just slightly longer than the RefreshTokenLifespan. We'll avoid making it exactly the same + // as RefreshTokenLifespan to avoid any chance of the garbage collector deleting it while it is being used. + // If an expired token is still stored when the user tries to refresh it, then they will get a more specific + // error message telling them that the token is expired, rather than a more generic error that is returned + // when the token does not exist. If this is desirable, then the RefreshTokenSessionStorageLifetime can be made + // to be significantly larger than RefreshTokenLifespan, at the cost of slower cleanup. + RefreshTokenSessionStorageLifetime time.Duration +} + +// Get the defaults for the Supervisor server. +func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration { + accessTokenLifespan := 15 * time.Minute + authorizationCodeLifespan := 10 * time.Minute + refreshTokenLifespan := 9 * time.Hour + + return TimeoutsConfiguration{ + UpstreamStateParamLifespan: 90 * time.Minute, + AuthorizeCodeLifespan: authorizationCodeLifespan, + AccessTokenLifespan: accessTokenLifespan, + IDTokenLifespan: accessTokenLifespan, + RefreshTokenLifespan: refreshTokenLifespan, + AuthorizationCodeSessionStorageLifetime: authorizationCodeLifespan + refreshTokenLifespan, + PKCESessionStorageLifetime: authorizationCodeLifespan + (1 * time.Minute), + OIDCSessionStorageLifetime: authorizationCodeLifespan + (1 * time.Minute), + AccessTokenSessionStorageLifetime: accessTokenLifespan + (1 * time.Minute), + RefreshTokenSessionStorageLifetime: refreshTokenLifespan + accessTokenLifespan, + } +} + func FositeOauth2Helper( oauthStore interface{}, issuer string, hmacSecretOfLengthAtLeast32 []byte, jwksProvider jwks.DynamicJWKSProvider, + timeoutsConfiguration TimeoutsConfiguration, ) fosite.OAuth2Provider { oauthConfig := &compose.Config{ - 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: issuer, - 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 + AuthorizeCodeLifespan: timeoutsConfiguration.AuthorizeCodeLifespan, + IDTokenLifespan: timeoutsConfiguration.IDTokenLifespan, + AccessTokenLifespan: timeoutsConfiguration.AccessTokenLifespan, + RefreshTokenLifespan: timeoutsConfiguration.RefreshTokenLifespan, - RefreshTokenScopes: []string{coreosoidc.ScopeOfflineAccess}, // as per https://openid.net/specs/openid-connect-core-1_0.html#OfflineAccess - - MinParameterEntropy: 32, // 256 bits seems about right + 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? } return compose.Compose( diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 4a429696..fc4ff1eb 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -79,10 +79,10 @@ 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. - oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil) + oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil, oidc.DefaultOIDCTimeoutsConfiguration()) // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. - oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider, oidc.DefaultOIDCTimeoutsConfiguration()) // 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/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index b5e51018..d0ae5f3a 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -58,9 +58,9 @@ const ( hmacSecret = "this needs to be at least 32 characters to meet entropy requirements" - authCodeExpirationSeconds = 3 * 60 // Current, we set our auth code expiration to 3 minutes - accessTokenExpirationSeconds = 5 * 60 // Currently, we set our access token expiration to 5 minutes - idTokenExpirationSeconds = 5 * 60 // Currently, we set our ID token expiration to 5 minutes + authCodeExpirationSeconds = 10 * 60 // Current, we set our auth code expiration to 10 minutes + accessTokenExpirationSeconds = 15 * 60 // Currently, we set our access token expiration to 15 minutes + idTokenExpirationSeconds = 15 * 60 // Currently, we set our ID token expiration to 15 minutes timeComparisonFudgeSeconds = 15 ) @@ -1346,7 +1346,7 @@ func makeHappyOauthHelper( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1378,7 +1378,7 @@ func makeOauthHelperWithJWTKeyThatWorksOnlyOnce( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1397,7 +1397,7 @@ func makeOauthHelperWithNilPrivateJWTSigningKey( t.Helper() jwkProvider := jwks.NewDynamicJWKSProvider() // empty provider which contains no signing key for this issuer - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), nil } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 221978d4..ed4ca1eb 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -17,7 +17,7 @@ import ( "testing" "time" - "github.com/coreos/go-oidc" + coreosoidc "github.com/coreos/go-oidc" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" "golang.org/x/oauth2" @@ -25,6 +25,7 @@ import ( configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" idpv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/idp/v1alpha1" "go.pinniped.dev/internal/certauthority" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -69,7 +70,7 @@ func TestSupervisorLogin(t *testing.T) { return proxyURL, nil }, }} - oidcHTTPClientContext := oidc.ClientContext(ctx, httpClient) + oidcHTTPClientContext := coreosoidc.ClientContext(ctx, httpClient) // Use the CA to issue a TLS server cert. t.Logf("issuing test certificate") @@ -110,9 +111,9 @@ func TestSupervisorLogin(t *testing.T) { }, idpv1alpha1.PhaseReady) // Perform OIDC discovery for our downstream. - var discovery *oidc.Provider + var discovery *coreosoidc.Provider assert.Eventually(t, func() bool { - discovery, err = oidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer) + discovery, err = coreosoidc.NewProvider(oidcHTTPClientContext, downstream.Spec.Issuer) return err == nil }, 30*time.Second, 200*time.Millisecond) require.NoError(t, err) @@ -193,7 +194,7 @@ func TestSupervisorLogin(t *testing.T) { func verifyTokenResponse( t *testing.T, tokenResponse *oauth2.Token, - discovery *oidc.Provider, + discovery *coreosoidc.Provider, downstreamOAuth2Config oauth2.Config, upstreamIssuerName string, nonceParam nonce.Nonce, @@ -205,7 +206,7 @@ func verifyTokenResponse( // Verify the ID Token. rawIDToken, ok := tokenResponse.Extra("id_token").(string) require.True(t, ok, "expected to get an ID token but did not") - var verifier = discovery.Verifier(&oidc.Config{ClientID: downstreamOAuth2Config.ClientID}) + var verifier = discovery.Verifier(&coreosoidc.Config{ClientID: downstreamOAuth2Config.ClientID}) idToken, err := verifier.Verify(ctx, rawIDToken) require.NoError(t, err) @@ -215,7 +216,8 @@ func verifyTokenResponse( require.Greater(t, len(idToken.Subject), len(expectedSubjectPrefix), "the ID token Subject should include the upstream user ID after the upstream issuer name") require.NoError(t, nonceParam.Validate(idToken)) - testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), idToken.Expiry, time.Second*30) + expectedIDTokenLifetime := oidc.DefaultOIDCTimeoutsConfiguration().IDTokenLifespan + testutil.RequireTimeInDelta(t, time.Now().UTC().Add(expectedIDTokenLifetime), idToken.Expiry, time.Second*30) idTokenClaims := map[string]interface{}{} err = idToken.Claims(&idTokenClaims) require.NoError(t, err) @@ -229,7 +231,8 @@ func verifyTokenResponse( require.NotEmpty(t, tokenResponse.AccessToken) require.Equal(t, "bearer", tokenResponse.TokenType) require.NotZero(t, tokenResponse.Expiry) - testutil.RequireTimeInDelta(t, time.Now().UTC().Add(time.Minute*5), tokenResponse.Expiry, time.Second*30) + expectedAccessTokenLifetime := oidc.DefaultOIDCTimeoutsConfiguration().AccessTokenLifespan + testutil.RequireTimeInDelta(t, time.Now().UTC().Add(expectedAccessTokenLifetime), tokenResponse.Expiry, time.Second*30) require.NotEmpty(t, tokenResponse.RefreshToken) } @@ -262,7 +265,7 @@ func (s *localCallbackServer) waitForCallback(timeout time.Duration) *http.Reque } } -func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.Token, httpClient *http.Client, provider *oidc.Provider) { +func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2.Token, httpClient *http.Client, provider *coreosoidc.Provider) { ctx, cancel := context.WithTimeout(context.Background(), 1*time.Minute) defer cancel() @@ -289,7 +292,7 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. } require.NoError(t, json.NewDecoder(resp.Body).Decode(&respBody)) - var clusterVerifier = provider.Verifier(&oidc.Config{ClientID: "cluster-1234"}) + var clusterVerifier = provider.Verifier(&coreosoidc.Config{ClientID: "cluster-1234"}) exchangedToken, err := clusterVerifier.Verify(ctx, respBody.AccessToken) require.NoError(t, err)