Merge pull request #275 from vmware-tanzu/fosite-storage-gc-prefactor

Fosite storage garbage collection prefactor

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"

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.

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
+		AuthorizeCodeLifespan: timeoutsConfiguration.AuthorizeCodeLifespan,
-		AudienceMatchingStrategy: nil,                       // I believe the default is fine
+		IDTokenLifespan:       timeoutsConfiguration.IDTokenLifespan,
-		EnforcePKCE:              true,                      // follow current set of best practices and always require PKCE
+		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?
-		MinParameterEntropy: 32, // 256 bits seems about right
 	}
 
 	return compose.Compose(

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

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
+	authCodeExpirationSeconds    = 10 * 60 // Current, we set our auth code expiration to 10 minutes
-	accessTokenExpirationSeconds = 5 * 60 // Currently, we set our access token expiration to 5 minutes
+	accessTokenExpirationSeconds = 15 * 60 // Currently, we set our access token expiration to 15 minutes
-	idTokenExpirationSeconds     = 5 * 60 // Currently, we set our ID token expiration to 5 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
 }

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)