From 9460b08873e6be712aa6ffabba08e66920eb0d30 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 11 Dec 2020 11:01:07 -0500 Subject: [PATCH] Use just-in-time HMAC signing key fetching in our Fosite config This pattern is similar to what we did in 58237d0e7dece5ded997ee9360099b2dd0d508a3. Signed-off-by: Andrew Keesler --- internal/oidc/auth/auth_handler_test.go | 6 +- .../oidc/callback/callback_handler_test.go | 6 +- internal/oidc/dynamic_oauth2_hmac_strategy.go | 98 +++++++++++++++++++ internal/oidc/oidc.go | 4 +- internal/oidc/provider/manager/manager.go | 5 +- internal/oidc/token/token_handler_test.go | 10 +- 6 files changed, 116 insertions(+), 13 deletions(-) create mode 100644 internal/oidc/dynamic_oauth2_hmac_strategy.go diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d9dc051c..68596000 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -124,10 +124,10 @@ func TestAuthorizationEndpoint(t *testing.T) { // Configure fosite the same way that the production code would, using NullStorage to turn off storage. 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") + hmacSecretFunc := func() []byte { return []byte("some secret - must have at least 32 bytes") } + require.GreaterOrEqual(t, len(hmacSecretFunc()), 32, "fosite requires that hmac secrets have at least 32 bytes") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, 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 df0c2779..c304af78 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -458,10 +458,10 @@ func TestCallbackEndpoint(t *testing.T) { // Configure fosite the same way that the production code would. // Inject this into our test subject at the last second so we get a fresh storage for every test. oauthStore := oidc.NewKubeStorage(secrets) - 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") + hmacSecretFunc := func() []byte { return []byte("some secret - must have at least 32 bytes") } + require.GreaterOrEqual(t, len(hmacSecretFunc()), 32, "fosite requires that hmac secrets have at least 32 bytes") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) diff --git a/internal/oidc/dynamic_oauth2_hmac_strategy.go b/internal/oidc/dynamic_oauth2_hmac_strategy.go new file mode 100644 index 00000000..8d67dc2e --- /dev/null +++ b/internal/oidc/dynamic_oauth2_hmac_strategy.go @@ -0,0 +1,98 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/ory/fosite/handler/oauth2" +) + +// dynamicOauth2HMACStrategy is an oauth2.CoreStrategy that can dynamically load an HMAC key to sign +// stuff (access tokens, refresh tokens, and auth codes). We want this dynamic capability since our +// controllers for loading OIDCProvider's and signing keys run in parallel, and thus the signing key +// might not be ready when an OIDCProvider is otherwise ready. +// +// If we ever update OIDCProvider's to hold their signing key, we might not need this type, since we +// could have an invariant that routes to an OIDCProvider's endpoints are only wired up if an +// OIDCProvider has a valid signing key. +type dynamicOauth2HMACStrategy struct { + fositeConfig *compose.Config + keyFunc func() []byte +} + +var _ oauth2.CoreStrategy = &dynamicOauth2HMACStrategy{} + +func newDynamicOauth2HMACStrategy( + fositeConfig *compose.Config, + keyFunc func() []byte, +) *dynamicOauth2HMACStrategy { + return &dynamicOauth2HMACStrategy{ + fositeConfig: fositeConfig, + keyFunc: keyFunc, + } +} + +func (s *dynamicOauth2HMACStrategy) AccessTokenSignature(token string) string { + return s.delegate().AccessTokenSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateAccessToken( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateAccessToken(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateAccessToken( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateAccessToken(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) RefreshTokenSignature(token string) string { + return s.delegate().RefreshTokenSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateRefreshToken( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateRefreshToken(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateRefreshToken( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateRefreshToken(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) AuthorizeCodeSignature(token string) string { + return s.delegate().AuthorizeCodeSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateAuthorizeCode( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateAuthorizeCode(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateAuthorizeCode( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateAuthorizeCode(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) delegate() *oauth2.HMACSHAStrategy { + return compose.NewOAuth2HMACStrategy(s.fositeConfig, s.keyFunc(), nil) +} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 76600470..233e93f1 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -187,7 +187,7 @@ func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration { func FositeOauth2Helper( oauthStore interface{}, issuer string, - hmacSecretOfLengthAtLeast32 []byte, + hmacSecretOfLengthAtLeast32Func func() []byte, jwksProvider jwks.DynamicJWKSProvider, timeoutsConfiguration TimeoutsConfiguration, ) fosite.OAuth2Provider { @@ -212,7 +212,7 @@ func FositeOauth2Helper( oauthStore, &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), + CoreStrategy: newDynamicOauth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32Func), OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(oauthConfig, jwksProvider), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 5846449a..b09448df 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -92,13 +92,14 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { issuer := incomingProvider.Issuer() issuerHostWithPath := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidcTimeouts := oidc.DefaultOIDCTimeoutsConfiguration() // 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, providerCache.GetTokenHMACKey(), nil, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, providerCache.GetTokenHMACKey, nil, oidcTimeouts) // 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, providerCache.GetTokenHMACKey(), m.dynamicJWKSProvider, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient), issuer, providerCache.GetTokenHMACKey, m.dynamicJWKSProvider, oidcTimeouts) var upstreamStateEncoder = dynamiccodec.New(providerCache.GetStateEncoderHashKey, providerCache.GetStateEncoderBlockKey) diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index d0ae5f3a..37fccf80 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -69,6 +69,10 @@ var ( goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC) goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.UTC) + hmacSecretFunc = func() []byte { + return []byte(hmacSecret) + } + fositeInvalidMethodErrorBody = func(actual string) string { return here.Docf(` { @@ -1346,7 +1350,7 @@ func makeHappyOauthHelper( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1378,7 +1382,7 @@ func makeOauthHelperWithJWTKeyThatWorksOnlyOnce( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1397,7 +1401,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, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), nil }