From 58237d0e7dece5ded997ee9360099b2dd0d508a3 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Thu, 3 Dec 2020 15:34:58 -0500 Subject: [PATCH] WIP: start to wire signing key into token handler This commit includes a failing test (amongst other compiler failures) for the dynamic signing key fetcher that we will inject into fosite. We are checking it in so that we can pass the WIP off. Signed-off-by: Margo Crawford --- .../supervisorconfig/jwks_observer.go | 32 +++- .../supervisorconfig/jwks_observer_test.go | 74 ++++++++- .../dynamic_open_id_connect_ecdsa_strategy.go | 42 +++++ ...mic_open_id_connect_ecdsa_strategy_test.go | 136 ++++++++++++++++ internal/oidc/jwks/dynamic_jwks_provider.go | 25 ++- internal/oidc/jwks/jwks_handler.go | 2 +- internal/oidc/jwks/jwks_handler_test.go | 8 +- internal/oidc/oidc.go | 7 +- internal/oidc/provider/manager/manager.go | 5 + .../oidc/provider/manager/manager_test.go | 149 +++++++++++++++--- internal/oidc/token/token_handler_test.go | 2 + 11 files changed, 432 insertions(+), 50 deletions(-) create mode 100644 internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go create mode 100644 internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index 0d6df5b6..5c01c5b4 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -24,7 +24,10 @@ type jwksObserverController struct { } type IssuerToJWKSMapSetter interface { - SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) + SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, + ) } // Returns a controller which watches all of the OIDCProviders and their corresponding Secrets @@ -70,6 +73,7 @@ func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { // Rebuild the whole map on any change to any Secret or OIDCProvider, because either can have changes that // can cause the map to need to be updated. issuerToJWKSMap := map[string]*jose.JSONWebKeySet{} + issuerToActiveJWKMap := map[string]*jose.JSONWebKey{} for _, provider := range allProviders { secretRef := provider.Status.JWKSSecret @@ -78,17 +82,33 @@ func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { plog.Debug("jwksObserverController Sync could not find JWKS secret", "namespace", ns, "secretName", secretRef.Name) continue } - jwkFromSecret := jose.JSONWebKeySet{} - err = json.Unmarshal(jwksSecret.Data[jwksKey], &jwkFromSecret) + + jwksFromSecret := jose.JSONWebKeySet{} + err = json.Unmarshal(jwksSecret.Data[jwksKey], &jwksFromSecret) if err != nil { plog.Debug("jwksObserverController Sync found a JWKS secret with Data in an unexpected format", "namespace", ns, "secretName", secretRef.Name) continue } - issuerToJWKSMap[provider.Spec.Issuer] = &jwkFromSecret + + activeJWKFromSecret := jose.JSONWebKey{} + err = json.Unmarshal(jwksSecret.Data[activeJWKKey], &activeJWKFromSecret) + if err != nil { + plog.Debug("jwksObserverController Sync found an active JWK secret with Data in an unexpected format", "namespace", ns, "secretName", secretRef.Name) + continue + } + + issuerToJWKSMap[provider.Spec.Issuer] = &jwksFromSecret + issuerToActiveJWKMap[provider.Spec.Issuer] = &activeJWKFromSecret } - plog.Debug("jwksObserverController Sync updated the JWKS cache", "issuerCount", len(issuerToJWKSMap)) - c.issuerToJWKSSetter.SetIssuerToJWKSMap(issuerToJWKSMap) + plog.Debug( + "jwksObserverController Sync updated the JWKS cache", + "issuerJWKSCount", + len(issuerToJWKSMap), + "issuerActiveJWKCount", + len(issuerToActiveJWKMap), + ) + c.issuerToJWKSSetter.SetIssuerToJWKSMap(issuerToJWKSMap, issuerToActiveJWKMap) return nil } diff --git a/internal/controller/supervisorconfig/jwks_observer_test.go b/internal/controller/supervisorconfig/jwks_observer_test.go index 8bf920ef..ad7323b8 100644 --- a/internal/controller/supervisorconfig/jwks_observer_test.go +++ b/internal/controller/supervisorconfig/jwks_observer_test.go @@ -96,13 +96,18 @@ func TestJWKSObserverControllerInformerFilters(t *testing.T) { } type fakeIssuerToJWKSMapSetter struct { - setIssuerToJWKSMapWasCalled bool - issuerToJWKSMapReceived map[string]*jose.JSONWebKeySet + setIssuerToJWKSMapWasCalled bool + issuerToJWKSMapReceived map[string]*jose.JSONWebKeySet + issuerToActiveJWKMapReceived map[string]*jose.JSONWebKey } -func (f *fakeIssuerToJWKSMapSetter) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { +func (f *fakeIssuerToJWKSMapSetter) SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, +) { f.setIssuerToJWKSMapWasCalled = true f.issuerToJWKSMapReceived = issuerToJWKSMap + f.issuerToActiveJWKMapReceived = issuerToActiveJWKMap } func TestJWKSObserverControllerSync(t *testing.T) { @@ -181,6 +186,7 @@ func TestJWKSObserverControllerSync(t *testing.T) { r.True(issuerToJWKSSetter.setIssuerToJWKSMapWasCalled) r.Empty(issuerToJWKSSetter.issuerToJWKSMapReceived) + r.Empty(issuerToJWKSSetter.issuerToActiveJWKMapReceived) }) }) @@ -212,10 +218,30 @@ func TestJWKSObserverControllerSync(t *testing.T) { Namespace: installedInNamespace, }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-secret-issuer.com"}, + Status: v1alpha1.OIDCProviderStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "bad-secret-name"}, + }, + } + oidcProviderWithBadJWKSSecret := &v1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-jwks-secret-oidcprovider", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-jwks-secret-issuer.com"}, Status: v1alpha1.OIDCProviderStatus{ JWKSSecret: corev1.LocalObjectReference{Name: "bad-jwks-secret-name"}, }, } + oidcProviderWithBadActiveJWKSecret := &v1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-active-jwk-secret-oidcprovider", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-active-jwk-secret-issuer.com"}, + Status: v1alpha1.OIDCProviderStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "bad-active-jwk-secret-name"}, + }, + } oidcProviderWithGoodSecret1 := &v1alpha1.OIDCProvider{ ObjectMeta: metav1.ObjectMeta{ Name: "good-secret-oidcprovider1", @@ -260,24 +286,48 @@ func TestJWKSObserverControllerSync(t *testing.T) { "jwks": []byte(`{"keys": [` + expectedJWK2 + `]}`), }, } + badSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-name", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{"junk": nil}, + } badJWKSSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: "bad-jwks-secret-name", Namespace: installedInNamespace, }, - Data: map[string][]byte{"junk": nil}, + Data: map[string][]byte{ + "activeJWK": []byte(expectedJWK2), + "jwks": []byte("bad"), + }, + } + badActiveJWKSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-active-jwk-secret-name", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{ + "activeJWK": []byte("bad"), + "jwks": []byte(`{"keys": [` + expectedJWK2 + `]}`), + }, } r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithoutSecret1)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithoutSecret2)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadJWKSSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithBadActiveJWKSecret)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithGoodSecret1)) r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderWithGoodSecret2)) r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret1)) r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(badSecret)) r.NoError(kubeInformerClient.Tracker().Add(badJWKSSecret)) + r.NoError(kubeInformerClient.Tracker().Add(badActiveJWKSecret)) }) - requireJWKJSON := func(expectedJWKJSON string, actualJWKS *jose.JSONWebKeySet) { + requireJWKSJSON := func(expectedJWKJSON string, actualJWKS *jose.JSONWebKeySet) { r.NotNil(actualJWKS) r.Len(actualJWKS.Keys, 1) actualJWK := actualJWKS.Keys[0] @@ -286,16 +336,26 @@ func TestJWKSObserverControllerSync(t *testing.T) { r.JSONEq(expectedJWKJSON, string(actualJWKJSON)) } + requireJWKJSON := func(expectedJWKJSON string, actualJWK *jose.JSONWebKey) { + r.NotNil(actualJWK) + actualJWKJSON, err := json.Marshal(actualJWK) + r.NoError(err) + r.JSONEq(expectedJWKJSON, string(actualJWKJSON)) + } + it("updates the issuerToJWKSSetter's map to include only the issuers that had valid JWKS", func() { startInformersAndController() r.NoError(controllerlib.TestSync(t, subject, *syncContext)) r.True(issuerToJWKSSetter.setIssuerToJWKSMapWasCalled) r.Len(issuerToJWKSSetter.issuerToJWKSMapReceived, 2) + r.Len(issuerToJWKSSetter.issuerToActiveJWKMapReceived, 2) // the actual JWK should match the one from the test fixture that was put into the secret - requireJWKJSON(expectedJWK1, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret1.com"]) - requireJWKJSON(expectedJWK2, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret2.com"]) + requireJWKSJSON(expectedJWK1, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret1.com"]) + requireJWKJSON(expectedJWK1, issuerToJWKSSetter.issuerToActiveJWKMapReceived["https://issuer-with-good-secret1.com"]) + requireJWKSJSON(expectedJWK2, issuerToJWKSSetter.issuerToJWKSMapReceived["https://issuer-with-good-secret2.com"]) + requireJWKJSON(expectedJWK2, issuerToJWKSSetter.issuerToActiveJWKMapReceived["https://issuer-with-good-secret2.com"]) }) }) }, spec.Parallel(), spec.Report(report.Terminal{})) diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go new file mode 100644 index 00000000..6d7d837f --- /dev/null +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy.go @@ -0,0 +1,42 @@ +// 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/openid" + + "go.pinniped.dev/internal/oidc/jwks" +) + +// TODO: doc me. +type dynamicOpenIDConnectECDSAStrategy struct { + issuer string + fositeConfig *compose.Config + jwksProvider jwks.DynamicJWKSProvider +} + +var _ openid.OpenIDConnectTokenStrategy = &dynamicOpenIDConnectECDSAStrategy{} + +func newDynamicOpenIDConnectECDSAStrategy( + issuer string, + fositeConfig *compose.Config, + jwksProvider jwks.DynamicJWKSProvider, +) *dynamicOpenIDConnectECDSAStrategy { + return &dynamicOpenIDConnectECDSAStrategy{ + issuer: issuer, + fositeConfig: fositeConfig, + jwksProvider: jwksProvider, + } +} + +func (s *dynamicOpenIDConnectECDSAStrategy) GenerateIDToken( + ctx context.Context, + requester fosite.Requester, +) (string, error) { + return "", nil +} diff --git a/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go new file mode 100644 index 00000000..93ba46cc --- /dev/null +++ b/internal/oidc/dynamic_open_id_connect_ecdsa_strategy_test.go @@ -0,0 +1,136 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + "crypto" + "crypto/ecdsa" + "crypto/elliptic" + "crypto/rand" + "crypto/rsa" + "fmt" + "testing" + + coreosoidc "github.com/coreos/go-oidc" + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/stretchr/testify/require" + "go.pinniped.dev/internal/oidc/jwks" + "gopkg.in/square/go-jose.v2" +) + +func TestDynamicOpenIDConnectECDSAStrategy(t *testing.T) { + const ( + goodIssuer = "https://some-good-issuer.com" + clientID = "some-client-id" + ) + + ecPrivateKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) + require.NoError(t, err) + + rsaPrivateKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + tests := []struct { + name string + issuer string + jwksProvider func(jwks.DynamicJWKSProvider) + wantError string + wantSigningJWK *jose.JSONWebKey + }{ + { + name: "jwks provider does contain signing key for issuer", + issuer: goodIssuer, + jwksProvider: func(provider jwks.DynamicJWKSProvider) { + provider.SetIssuerToJWKSMap( + nil, + map[string]*jose.JSONWebKey{ + goodIssuer: { + Key: ecPrivateKey, + }, + }, + ) + }, + wantSigningJWK: &jose.JSONWebKey{ + Key: ecPrivateKey, + }, + }, + { + name: "jwks provider does not contain signing key for issuer", + issuer: goodIssuer, + wantError: "some unkonwn key error", + }, + { + name: "jwks provider contains signing key of wrong type for issuer", + issuer: goodIssuer, + jwksProvider: func(provider jwks.DynamicJWKSProvider) { + provider.SetIssuerToJWKSMap( + nil, + map[string]*jose.JSONWebKey{ + goodIssuer: { + Key: rsaPrivateKey, + }, + }, + ) + }, + wantError: "some invalid key type error", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + jwksProvider := jwks.NewDynamicJWKSProvider() + if test.jwksProvider != nil { + test.jwksProvider(jwksProvider) + } + s := newDynamicOpenIDConnectECDSAStrategy(test.issuer, &compose.Config{}, jwksProvider) + + requester := &fosite.Request{ + Client: &fosite.DefaultClient{ + ID: clientID, + }, + // Session: fositeopenid.DefaultSession{}, + } + idToken, err := s.GenerateIDToken(context.Background(), requester) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + + // TODO: common-ize this code with token endpoint test. + // TODO: make more assertions about ID token + + privateKey, ok := test.wantSigningJWK.Key.(*ecdsa.PrivateKey) + require.True(t, ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", test.wantSigningJWK) + + keySet := newStaticKeySet(privateKey.Public()) + verifyConfig := coreosoidc.Config{ + ClientID: clientID, + SupportedSigningAlgs: []string{coreosoidc.ES256}, + } + verifier := coreosoidc.NewVerifier(test.issuer, keySet, &verifyConfig) + _, err := verifier.Verify(context.Background(), idToken) + require.NoError(t, err) + } + }) + } +} + +// TODO: de-dep me. +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) + } + return jws.Verify(s.publicKey) +} diff --git a/internal/oidc/jwks/dynamic_jwks_provider.go b/internal/oidc/jwks/dynamic_jwks_provider.go index 8672fd2f..fa156e3c 100644 --- a/internal/oidc/jwks/dynamic_jwks_provider.go +++ b/internal/oidc/jwks/dynamic_jwks_provider.go @@ -10,29 +10,38 @@ import ( ) type DynamicJWKSProvider interface { - SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) - GetJWKS(issuerName string) *jose.JSONWebKeySet + SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, + ) + GetJWKS(issuerName string) (jwks *jose.JSONWebKeySet, activeJWK *jose.JSONWebKey) } type dynamicJWKSProvider struct { - issuerToJWKSMap map[string]*jose.JSONWebKeySet - mutex sync.RWMutex + issuerToJWKSMap map[string]*jose.JSONWebKeySet + issuerToActiveJWKMap map[string]*jose.JSONWebKey + mutex sync.RWMutex } func NewDynamicJWKSProvider() DynamicJWKSProvider { return &dynamicJWKSProvider{ - issuerToJWKSMap: map[string]*jose.JSONWebKeySet{}, + issuerToJWKSMap: map[string]*jose.JSONWebKeySet{}, + issuerToActiveJWKMap: map[string]*jose.JSONWebKey{}, } } -func (p *dynamicJWKSProvider) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { +func (p *dynamicJWKSProvider) SetIssuerToJWKSMap( + issuerToJWKSMap map[string]*jose.JSONWebKeySet, + issuerToActiveJWKMap map[string]*jose.JSONWebKey, +) { p.mutex.Lock() // acquire a write lock defer p.mutex.Unlock() p.issuerToJWKSMap = issuerToJWKSMap + p.issuerToActiveJWKMap = issuerToActiveJWKMap } -func (p *dynamicJWKSProvider) GetJWKS(issuerName string) *jose.JSONWebKeySet { +func (p *dynamicJWKSProvider) GetJWKS(issuerName string) (*jose.JSONWebKeySet, *jose.JSONWebKey) { p.mutex.RLock() // acquire a read lock defer p.mutex.RUnlock() - return p.issuerToJWKSMap[issuerName] + return p.issuerToJWKSMap[issuerName], p.issuerToActiveJWKMap[issuerName] } diff --git a/internal/oidc/jwks/jwks_handler.go b/internal/oidc/jwks/jwks_handler.go index bdb036e6..2c975e95 100644 --- a/internal/oidc/jwks/jwks_handler.go +++ b/internal/oidc/jwks/jwks_handler.go @@ -19,7 +19,7 @@ func NewHandler(issuerName string, provider DynamicJWKSProvider) http.Handler { return } - jwks := provider.GetJWKS(issuerName) + jwks, _ := provider.GetJWKS(issuerName) if jwks == nil { http.Error(w, "JWKS not found for requested issuer", http.StatusNotFound) diff --git a/internal/oidc/jwks/jwks_handler_test.go b/internal/oidc/jwks/jwks_handler_test.go index d80e66d5..37f53e8c 100644 --- a/internal/oidc/jwks/jwks_handler_test.go +++ b/internal/oidc/jwks/jwks_handler_test.go @@ -105,8 +105,12 @@ func newDynamicJWKSProvider(t *testing.T, issuer string, jwksJSON string) Dynami var keySet jose.JSONWebKeySet err := json.Unmarshal([]byte(jwksJSON), &keySet) require.NoError(t, err) - jwksProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ + issuerToJWKSMap := map[string]*jose.JSONWebKeySet{ issuer: &keySet, - }) + } + issuerToActiveJWKMap := map[string]*jose.JSONWebKey{ + issuer: &keySet.Keys[0], + } + jwksProvider.SetIssuerToJWKSMap(issuerToJWKSMap, issuerToActiveJWKMap) return jwksProvider } diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 14f9a725..c5e6abf0 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -5,13 +5,13 @@ package oidc import ( - "crypto/ecdsa" "time" "github.com/ory/fosite" "github.com/ory/fosite/compose" "go.pinniped.dev/internal/oidc/csrftoken" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -94,7 +94,7 @@ func FositeOauth2Helper( oauthStore interface{}, issuer string, hmacSecretOfLengthAtLeast32 []byte, - jwtSigningKey *ecdsa.PrivateKey, + jwksProvider jwks.DynamicJWKSProvider, ) fosite.OAuth2Provider { oauthConfig := &compose.Config{ AuthorizeCodeLifespan: 3 * time.Minute, // seems more than long enough to exchange a code @@ -122,7 +122,8 @@ func FositeOauth2Helper( &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), - OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), + OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(issuer, oauthConfig, jwksProvider), + // OpenIDConnectTokenStrategy: compose.NewOpenIDConnectECDSAStrategy(oauthConfig, jwtSigningKey), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. compose.OAuth2AuthorizeExplicitFactory, diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 1d80a0fc..b1d9d6b5 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -18,6 +18,7 @@ import ( "go.pinniped.dev/internal/oidc/discovery" "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" + "go.pinniped.dev/internal/oidc/token" "go.pinniped.dev/internal/plog" "go.pinniped.dev/pkg/oidcclient/nonce" "go.pinniped.dev/pkg/oidcclient/pkce" @@ -115,6 +116,10 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { issuer+oidc.CallbackEndpointPath, ) + m.providerHandlers[(issuerHostWithPath + oidc.TokenEndpointPath)] = token.NewHandler( + oauthHelperWithKubeStorage, + ) + plog.Debug("oidc provider manager added or updated issuer", "issuer", issuer) } } diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index a3f8090d..d3cf1cc9 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -5,7 +5,12 @@ package manager import ( "context" + "crypto" + "crypto/ecdsa" + "crypto/sha256" + "encoding/base64" "encoding/json" + "fmt" "io/ioutil" "net/http" "net/http/httptest" @@ -13,6 +18,7 @@ import ( "strings" "testing" + coreosoidc "github.com/coreos/go-oidc" "github.com/sclevine/spec" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" @@ -48,13 +54,22 @@ func TestManager(t *testing.T) { issuer2DifferentCaseHostname = "https://exAmPlE.Com/some/path/more/deeply/nested/path" issuer2KeyID = "issuer2-key" upstreamIDPAuthorizationURL = "https://test-upstream.com/auth" + downstreamClientID = "pinniped-cli" downstreamRedirectURL = "http://127.0.0.1:12345/callback" + + downstreamPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" ) newGetRequest := func(url string) *http.Request { return httptest.NewRequest(http.MethodGet, url, nil) } + newPostRequest := func(url, body string) *http.Request { + req := httptest.NewRequest(http.MethodPost, url, strings.NewReader(body)) + req.Header.Set("Content-Type", "application/x-www-form-urlencoded") + return req + } + requireDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIssuerInResponse string) { recorder := httptest.NewRecorder() @@ -106,7 +121,7 @@ func TestManager(t *testing.T) { return csrfCookieValue, redirectStateParam } - requireCallbackRequestToBeHandled := func(requestIssuer, requestURLSuffix, csrfCookieValue string) { + requireCallbackRequestToBeHandled := func(requestIssuer, requestURLSuffix, csrfCookieValue string) string { recorder := httptest.NewRecorder() numberOfKubeActionsBeforeThisRequest := len(kubeClient.Actions()) @@ -139,9 +154,56 @@ func TestManager(t *testing.T) { // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+3, "did not perform any kube actions during the callback request, but should have") + + // Return the important parts of the response so we can use them in our next request to the token endpoint. + return actualLocationQueryParams.Get("code") } - requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) { + requireTokenRequestToBeHandled := func(requestIssuer, authCode string, jwks *jose.JSONWebKeySet) { + recorder := httptest.NewRecorder() + + numberOfKubeActionsBeforeThisRequest := len(kubeClient.Actions()) + + tokenRequestBody := url.Values{ + "code": []string{authCode}, + "client_id": []string{downstreamClientID}, + "redirect_uri": []string{downstreamRedirectURL}, + "code_verifier": []string{downstreamPKCECodeVerifier}, + "grant_type": []string{"authorization_code"}, + }.Encode() + subject.ServeHTTP(recorder, newPostRequest(requestIssuer+oidc.TokenEndpointPath, tokenRequestBody)) + + r.False(fallbackHandlerWasCalled) + + // Minimal check to ensure that the right endpoint was called + var body map[string]interface{} + r.Equal(http.StatusOK, recorder.Code) + r.NoError(json.Unmarshal(recorder.Body.Bytes(), &body)) + r.Contains(body, "id_token") + r.Contains(body, "access_token") + + // Validate ID token is signed by the correct JWK to make sure we wired the token endpoint + // signing key correctly. + // TODO: common-ize this code with token endpoint test. + idToken, ok := body["id_token"].(string) + r.True(ok, "wanted id_token type to be string, but was %T", body["id_token"]) + + r.GreaterOrEqual(len(jwks.Keys), 1) + privateKey, ok := jwks.Keys[0].Key.(*ecdsa.PrivateKey) + r.True(ok, "wanted private key to be *ecdsa.PrivateKey, but was %T", jwks.Keys[0].Key) + + keySet := newStaticKeySet(privateKey.Public()) + verifyConfig := coreosoidc.Config{ClientID: downstreamClientID, SupportedSigningAlgs: []string{coreosoidc.ES256}} + verifier := coreosoidc.NewVerifier(requestIssuer, keySet, &verifyConfig) + _, err := verifier.Verify(context.Background(), idToken) + r.NoError(err) + + // Make sure that we wired up the callback endpoint to use kube storage for fosite sessions. + r.Equal(len(kubeClient.Actions()), numberOfKubeActionsBeforeThisRequest+7, + "did not perform any kube actions during the callback request, but should have") + } + + requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) *jose.JSONWebKeySet { recorder := httptest.NewRecorder() subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.JWKSEndpointPath+requestURLSuffix)) @@ -156,6 +218,8 @@ func TestManager(t *testing.T) { err = json.Unmarshal(responseBody, &parsedJWKSResult) r.NoError(err) r.Equal(expectedJWKKeyID, parsedJWKSResult.Keys[0].KeyID) + + return &parsedJWKSResult } it.Before(func() { @@ -198,7 +262,7 @@ func TestManager(t *testing.T) { }) }) - newTestJWK := func(keyID string) jose.JSONWebKey { + newTestJWK := func(keyID string) *jose.JSONWebKey { testJWKSJSONString := here.Docf(` { "use": "sig", @@ -206,13 +270,14 @@ func TestManager(t *testing.T) { "kid": "%s", "crv": "P-256", "alg": "ES256", - "x": "awmmj6CIMhSoJyfsqH7sekbTeY72GGPLEy16tPWVz2U", - "y": "FcMh06uXLaq9b2MOixlLVidUkycO1u7IHOkrTi7N0aw" + "x": "9c_oMKjd_ruVIy4pA5y9quT1E-Fampx0w270OtPx5Ng", + "y": "-Y-9nfrtJdFPl-9kzXv55-Fq9Oo2AWDg5zZBs9P-Vds", + "d": "LXdNChAEtGKETBzYXiL_G0wESXceBuajE_EBQbcRuGg" } `, keyID) k := jose.JSONWebKey{} r.NoError(json.Unmarshal([]byte(testJWKSJSONString), &k)) - return k + return &k } requireRoutesMatchingRequestsToAppropriateProvider := func() { @@ -225,8 +290,8 @@ func TestManager(t *testing.T) { requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) - requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) - requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) + issuer1JWKS := requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) + issuer2JWKS := requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) // Hostnames are case-insensitive, so test that we can handle that. @@ -237,10 +302,10 @@ func TestManager(t *testing.T) { authRequestParams := "?" + url.Values{ "response_type": []string{"code"}, "scope": []string{"openid profile email"}, - "client_id": []string{"pinniped-cli"}, + "client_id": []string{downstreamClientID}, "state": []string{"some-state-value-that-is-32-byte"}, - "nonce": []string{"some-nonce-value"}, - "code_challenge": []string{"some-challenge"}, + "nonce": []string{"some-nonce-value-that-is-at-least-32-bytes"}, + "code_challenge": []string{doSHA256(downstreamPKCECodeVerifier)}, "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURL}, }.Encode() @@ -258,12 +323,19 @@ func TestManager(t *testing.T) { "state": []string{upstreamStateParam}, }.Encode() - requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) - requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) + downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) + downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) // // Hostnames are case-insensitive, so test that we can handle that. - requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) - requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + + requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS) + requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS) + + // Hostnames are case-insensitive, so test that we can handle that. + requireTokenRequestToBeHandled(issuer1DifferentCaseHostname, downstreamAuthCode3, issuer1JWKS) + requireTokenRequestToBeHandled(issuer2DifferentCaseHostname, downstreamAuthCode4, issuer2JWKS) } when("given some valid providers via SetProviders()", func() { @@ -274,10 +346,15 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p1, p2) - dynamicJWKSProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ - issuer1: {Keys: []jose.JSONWebKey{newTestJWK(issuer1KeyID)}}, - issuer2: {Keys: []jose.JSONWebKey{newTestJWK(issuer2KeyID)}}, - }) + jwks := map[string]*jose.JSONWebKeySet{ + issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, + issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, + } + activeJWK := map[string]*jose.JSONWebKey{ + issuer1: newTestJWK(issuer1KeyID), + issuer2: newTestJWK(issuer2KeyID), + } + dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) }) it("sends all non-matching host requests to the nextHandler", func() { @@ -312,10 +389,15 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p2, p1) - dynamicJWKSProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ - issuer1: {Keys: []jose.JSONWebKey{newTestJWK(issuer1KeyID)}}, - issuer2: {Keys: []jose.JSONWebKey{newTestJWK(issuer2KeyID)}}, - }) + jwks := map[string]*jose.JSONWebKeySet{ + issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, + issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, + } + activeJWK := map[string]*jose.JSONWebKey{ + issuer1: newTestJWK(issuer1KeyID), + issuer2: newTestJWK(issuer2KeyID), + } + dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) }) it("still routes matching requests to the appropriate provider", func() { @@ -324,3 +406,24 @@ func TestManager(t *testing.T) { }) }) } + +func doSHA256(s string) string { + b := sha256.Sum256([]byte(s)) + return base64.RawURLEncoding.EncodeToString(b[:]) +} + +func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { + return &staticKeySet{publicKey} +} + +type staticKeySet struct { + publicKey crypto.PublicKey +} + +func (s *staticKeySet) VerifySignature(ctx context.Context, jwt string) ([]byte, error) { + jws, err := jose.ParseSigned(jwt) + if err != nil { + return nil, fmt.Errorf("oidc: malformed jwt: %v", err) + } + return jws.Verify(s.publicKey) +} diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index fd52004e..ea64cc1b 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -591,6 +591,7 @@ func hashAccessToken(accessToken string) string { return base64.RawURLEncoding.EncodeToString(b[:len(b)/2]) } +// TODO: de-dup me (manager test). func doSHA256(s string) string { b := sha256.Sum256([]byte(s)) return base64.RawURLEncoding.EncodeToString(b[:]) @@ -856,6 +857,7 @@ func requireValidIDToken(t *testing.T, body map[string]interface{}, jwtSigningKe requireTimeInDelta(t, goodAuthTime, authTime, timeComparisonFudgeSeconds*time.Second) } +// TODO: de-dup me (manager test). func newStaticKeySet(publicKey crypto.PublicKey) coreosoidc.KeySet { return &staticKeySet{publicKey} }