From d9d76726c2730e22c89dd946de2fd4db21a497a9 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 16 Oct 2020 17:51:40 -0700 Subject: [PATCH 1/2] Implement per-issuer OIDC JWKS endpoint --- cmd/pinniped-supervisor/main.go | 22 +- .../webhookcachecleaner.go | 4 +- .../webhookcachefiller/webhookcachefiller.go | 2 +- .../supervisorconfig/jwks_observer.go | 94 ++++++ .../supervisorconfig/jwks_observer_test.go | 302 ++++++++++++++++++ .../{jwks.go => jwks_writer.go} | 20 +- .../{jwks_test.go => jwks_writer_test.go} | 18 +- .../oidcproviderconfig_watcher.go | 2 +- .../testdata/public-jwk2.json | 9 + internal/controller/utils.go | 4 +- .../{discovery.go => discovery_handler.go} | 6 +- ...very_test.go => discovery_handler_test.go} | 34 +- internal/oidc/jwks/dynamic_jwks_provider.go | 38 +++ internal/oidc/jwks/jwks_handler.go | 33 ++ internal/oidc/jwks/jwks_handler_test.go | 112 +++++++ internal/oidc/provider/manager/manager.go | 26 +- .../oidc/provider/manager/manager_test.go | 90 +++++- 17 files changed, 745 insertions(+), 71 deletions(-) create mode 100644 internal/controller/supervisorconfig/jwks_observer.go create mode 100644 internal/controller/supervisorconfig/jwks_observer_test.go rename internal/controller/supervisorconfig/{jwks.go => jwks_writer.go} (94%) rename internal/controller/supervisorconfig/{jwks_test.go => jwks_writer_test.go} (98%) create mode 100644 internal/controller/supervisorconfig/testdata/public-jwk2.json rename internal/oidc/discovery/{discovery.go => discovery_handler.go} (92%) rename internal/oidc/discovery/{discovery_test.go => discovery_handler_test.go} (72%) create mode 100644 internal/oidc/jwks/dynamic_jwks_provider.go create mode 100644 internal/oidc/jwks/jwks_handler.go create mode 100644 internal/oidc/jwks/jwks_handler_test.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 6a3d5c4f..9d231f2b 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -27,6 +27,7 @@ import ( "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider/manager" ) @@ -65,7 +66,8 @@ func waitForSignal() os.Signal { func startControllers( ctx context.Context, cfg *supervisor.Config, - issuerProvider *manager.Manager, + issuerManager *manager.Manager, + dynamicJWKSProvider jwks.DynamicJWKSProvider, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, kubeInformers kubeinformers.SharedInformerFactory, @@ -76,7 +78,7 @@ func startControllers( NewManager(). WithController( supervisorconfig.NewOIDCProviderConfigWatcherController( - issuerProvider, + issuerManager, clock.RealClock{}, pinnipedClient, pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), @@ -85,7 +87,7 @@ func startControllers( singletonWorker, ). WithController( - supervisorconfig.NewJWKSController( + supervisorconfig.NewJWKSWriterController( cfg.Labels, kubeClient, pinnipedClient, @@ -94,6 +96,15 @@ func startControllers( controllerlib.WithInformer, ), singletonWorker, + ). + WithController( + supervisorconfig.NewJWKSObserverController( + dynamicJWKSProvider, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ), + singletonWorker, ) kubeInformers.Start(ctx.Done()) @@ -144,8 +155,9 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { pinnipedinformers.WithNamespace(serverInstallationNamespace), ) - oidProvidersManager := manager.NewManager(http.NotFoundHandler()) - startControllers(ctx, cfg, oidProvidersManager, kubeClient, pinnipedClient, kubeInformers, pinnipedInformers) + dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() + oidProvidersManager := manager.NewManager(http.NotFoundHandler(), dynamicJWKSProvider) + startControllers(ctx, cfg, oidProvidersManager, dynamicJWKSProvider, kubeClient, pinnipedClient, kubeInformers, pinnipedInformers) //nolint: gosec // Intentionally binding to all network interfaces. l, err := net.Listen("tcp", ":80") diff --git a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go index c383a18a..ba8944ab 100644 --- a/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go +++ b/internal/controller/identityprovider/webhookcachecleaner/webhookcachecleaner.go @@ -31,7 +31,7 @@ func New(cache *idpcache.Cache, webhookIDPs idpinformers.WebhookIdentityProvider }, controllerlib.WithInformer( webhookIDPs, - pinnipedcontroller.NoOpFilter(), + pinnipedcontroller.MatchAnythingFilter(), controllerlib.InformerOption{}, ), ) @@ -44,7 +44,7 @@ type controller struct { } // Sync implements controllerlib.Syncer. -func (c *controller) Sync(ctx controllerlib.Context) error { +func (c *controller) Sync(_ controllerlib.Context) error { webhooks, err := c.webhookIDPs.Lister().List(labels.Everything()) if err != nil { return fmt.Errorf("failed to list WebhookIdentityProviders: %w", err) diff --git a/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go b/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go index e741a8b3..c2c642a5 100644 --- a/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go +++ b/internal/controller/identityprovider/webhookcachefiller/webhookcachefiller.go @@ -40,7 +40,7 @@ func New(cache *idpcache.Cache, webhookIDPs idpinformers.WebhookIdentityProvider }, controllerlib.WithInformer( webhookIDPs, - pinnipedcontroller.NoOpFilter(), + pinnipedcontroller.MatchAnythingFilter(), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go new file mode 100644 index 00000000..c9911b13 --- /dev/null +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -0,0 +1,94 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "encoding/json" + "fmt" + + "gopkg.in/square/go-jose.v2" + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/klog/v2" + + "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1" + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" +) + +type jwksObserverController struct { + issuerToJWKSSetter IssuerToJWKSMapSetter + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer +} + +type IssuerToJWKSMapSetter interface { + SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) +} + +// Returns a controller which watches all of the OIDCProviderConfigs and their corresponding Secrets +// and fills an in-memory cache of the JWKS info for each currently configured issuer. +// This controller assumes that the informers passed to it are already scoped down to the +// appropriate namespace. It also assumes that the IssuerToJWKSMapSetter passed to it has an +// underlying implementation which is thread-safe. +func NewJWKSObserverController( + issuerToJWKSSetter IssuerToJWKSMapSetter, + secretInformer corev1informers.SecretInformer, + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "certs-observer-controller", + Syncer: &jwksObserverController{ + issuerToJWKSSetter: issuerToJWKSSetter, + oidcProviderConfigInformer: oidcProviderConfigInformer, + secretInformer: secretInformer, + }, + }, + withInformer( + secretInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + withInformer( + oidcProviderConfigInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + ) +} + +func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { + ns := ctx.Key.Namespace + allProviders, err := c.oidcProviderConfigInformer.Lister().OIDCProviderConfigs(ns).List(labels.Everything()) + if err != nil { + return fmt.Errorf("failed to list OIDCProviderConfigs: %w", err) + } + + // 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{} + + for _, provider := range allProviders { + secretRef := provider.Status.JWKSSecret + jwksSecret, err := c.secretInformer.Lister().Secrets(ns).Get(secretRef.Name) + if err != nil { + klog.InfoS("jwksObserverController Sync could not find JWKS secret", "namespace", ns, "secretName", secretRef.Name) + continue + } + jwkFromSecret := jose.JSONWebKeySet{} + err = json.Unmarshal(jwksSecret.Data[jwksKey], &jwkFromSecret) + if err != nil { + klog.InfoS("jwksObserverController Sync found a JWKS secret with Data in an unexpected format", "namespace", ns, "secretName", secretRef.Name) + continue + } + issuerToJWKSMap[provider.Spec.Issuer] = &jwkFromSecret + } + + klog.InfoS("jwksObserverController Sync updated the JWKS cache", "issuerCount", len(issuerToJWKSMap)) + c.issuerToJWKSSetter.SetIssuerToJWKSMap(issuerToJWKSMap) + + return nil +} diff --git a/internal/controller/supervisorconfig/jwks_observer_test.go b/internal/controller/supervisorconfig/jwks_observer_test.go new file mode 100644 index 00000000..9a72473a --- /dev/null +++ b/internal/controller/supervisorconfig/jwks_observer_test.go @@ -0,0 +1,302 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + + "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" + pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" +) + +func TestJWKSObserverControllerInformerFilters(t *testing.T) { + spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) { + var ( + r *require.Assertions + observableWithInformerOption *testutil.ObservableWithInformerOption + secretsInformerFilter controllerlib.Filter + oidcProviderConfigInformerFilter controllerlib.Filter + ) + + it.Before(func() { + r = require.New(t) + observableWithInformerOption = testutil.NewObservableWithInformerOption() + secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() + oidcProviderConfigInformer := pinnipedinformers.NewSharedInformerFactory(nil, 0).Config().V1alpha1().OIDCProviderConfigs() + _ = NewJWKSObserverController( + nil, + secretsInformer, + oidcProviderConfigInformer, + observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters + ) + secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) + oidcProviderConfigInformerFilter = observableWithInformerOption.GetFilterForInformer(oidcProviderConfigInformer) + }) + + when("watching Secret objects", func() { + var ( + subject controllerlib.Filter + secret, otherSecret *corev1.Secret + ) + + it.Before(func() { + subject = secretsInformerFilter + secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any Secret changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(secret)) + r.True(subject.Update(secret, otherSecret)) + r.True(subject.Update(otherSecret, secret)) + r.True(subject.Delete(secret)) + }) + }) + }) + + when("watching OIDCProviderConfig objects", func() { + var ( + subject controllerlib.Filter + provider, otherProvider *v1alpha1.OIDCProviderConfig + ) + + it.Before(func() { + subject = oidcProviderConfigInformerFilter + provider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherProvider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any OIDCProviderConfig changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(provider)) + r.True(subject.Update(provider, otherProvider)) + r.True(subject.Update(otherProvider, provider)) + r.True(subject.Delete(provider)) + }) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} + +type fakeIssuerToJWKSMapSetter struct { + setIssuerToJWKSMapWasCalled bool + issuerToJWKSMapReceived map[string]*jose.JSONWebKeySet +} + +func (f *fakeIssuerToJWKSMapSetter) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { + f.setIssuerToJWKSMapWasCalled = true + f.issuerToJWKSMapReceived = issuerToJWKSMap +} + +func TestJWKSObserverControllerSync(t *testing.T) { + spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { + const installedInNamespace = "some-namespace" + + var ( + r *require.Assertions + subject controllerlib.Controller + pinnipedInformerClient *pinnipedfake.Clientset + kubeInformerClient *kubernetesfake.Clientset + pinnipedInformers pinnipedinformers.SharedInformerFactory + kubeInformers kubeinformers.SharedInformerFactory + timeoutContext context.Context + timeoutContextCancel context.CancelFunc + syncContext *controllerlib.Context + issuerToJWKSSetter *fakeIssuerToJWKSMapSetter + ) + + // Defer starting the informers until the last possible moment so that the + // nested Before's can keep adding things to the informer caches. + var startInformersAndController = func() { + // Set this at the last second to allow for injection of server override. + subject = NewJWKSObserverController( + issuerToJWKSSetter, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ) + + // Set this at the last second to support calling subject.Name(). + syncContext = &controllerlib.Context{ + Context: timeoutContext, + Name: subject.Name(), + Key: controllerlib.Key{ + Namespace: installedInNamespace, + Name: "any-name", + }, + } + + // Must start informers before calling TestRunSynchronously() + kubeInformers.Start(timeoutContext.Done()) + pinnipedInformers.Start(timeoutContext.Done()) + controllerlib.TestRunSynchronously(t, subject) + } + + it.Before(func() { + r = require.New(t) + + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) + + kubeInformerClient = kubernetesfake.NewSimpleClientset() + kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) + pinnipedInformerClient = pinnipedfake.NewSimpleClientset() + pinnipedInformers = pinnipedinformers.NewSharedInformerFactory(pinnipedInformerClient, 0) + issuerToJWKSSetter = &fakeIssuerToJWKSMapSetter{} + + unrelatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some other unrelated secret", + Namespace: installedInNamespace, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(unrelatedSecret)) + }) + + it.After(func() { + timeoutContextCancel() + }) + + when("there are no OIDCProviderConfigs and no JWKS Secrets yet", func() { + it("sets the issuerToJWKSSetter's map to be empty", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + r.True(issuerToJWKSSetter.setIssuerToJWKSMapWasCalled) + r.Empty(issuerToJWKSSetter.issuerToJWKSMapReceived) + }) + }) + + when("there are OIDCProviderConfigs where some have corresponding JWKS Secrets and some don't", func() { + var ( + expectedJWK1, expectedJWK2 string + ) + + it.Before(func() { + oidcProviderConfigWithoutSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer1.com"}, + Status: v1alpha1.OIDCProviderConfigStatus{}, // no JWKSSecret field + } + oidcProviderConfigWithoutSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer2.com"}, + // no Status field + } + oidcProviderConfigWithBadSecret := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://bad-secret-issuer.com"}, + Status: v1alpha1.OIDCProviderConfigStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "bad-jwks-secret-name"}, + }, + } + oidcProviderConfigWithGoodSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-with-good-secret1.com"}, + Status: v1alpha1.OIDCProviderConfigStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "good-jwks-secret-name1"}, + }, + } + oidcProviderConfigWithGoodSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-with-good-secret2.com"}, + Status: v1alpha1.OIDCProviderConfigStatus{ + JWKSSecret: corev1.LocalObjectReference{Name: "good-jwks-secret-name2"}, + }, + } + expectedJWK1 = string(readJWKJSON(t, "testdata/public-jwk.json")) + r.NotEmpty(expectedJWK1) + expectedJWK2 = string(readJWKJSON(t, "testdata/public-jwk2.json")) + r.NotEmpty(expectedJWK2) + goodJWKSSecret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-jwks-secret-name1", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{ + "activeJWK": []byte(expectedJWK1), + "jwks": []byte(`{"keys": [` + expectedJWK1 + `]}`), + }, + } + goodJWKSSecret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-jwks-secret-name2", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{ + "activeJWK": []byte(expectedJWK2), + "jwks": []byte(`{"keys": [` + expectedJWK2 + `]}`), + }, + } + badJWKSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-jwks-secret-name", + Namespace: installedInNamespace, + }, + Data: map[string][]byte{"junk": nil}, + } + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret2)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithBadSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret1)) + r.NoError(kubeInformerClient.Tracker().Add(goodJWKSSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(badJWKSSecret)) + }) + + requireJWKJSON := func(expectedJWKJSON string, actualJWKS *jose.JSONWebKeySet) { + r.NotNil(actualJWKS) + r.Len(actualJWKS.Keys, 1) + actualJWK := actualJWKS.Keys[0] + 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) + + // 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"]) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} diff --git a/internal/controller/supervisorconfig/jwks.go b/internal/controller/supervisorconfig/jwks_writer.go similarity index 94% rename from internal/controller/supervisorconfig/jwks.go rename to internal/controller/supervisorconfig/jwks_writer.go index 563df927..5314816a 100644 --- a/internal/controller/supervisorconfig/jwks.go +++ b/internal/controller/supervisorconfig/jwks_writer.go @@ -55,7 +55,7 @@ func generateECKey(r io.Reader) (interface{}, error) { // jwkController holds the fields necessary for the JWKS controller to communicate with OPC's and // secrets, both via a cache and via the API. -type jwksController struct { +type jwksWriterController struct { jwksSecretLabels map[string]string pinnipedClient pinnipedclientset.Interface kubeClient kubernetes.Interface @@ -63,9 +63,9 @@ type jwksController struct { secretInformer corev1informers.SecretInformer } -// NewJWKSController returns a controllerlib.Controller that ensures an OPC has a corresponding +// NewJWKSWriterController returns a controllerlib.Controller that ensures an OPC has a corresponding // Secret that contains a valid active JWK and JWKS. -func NewJWKSController( +func NewJWKSWriterController( jwksSecretLabels map[string]string, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, @@ -76,7 +76,7 @@ func NewJWKSController( return controllerlib.New( controllerlib.Config{ Name: "JWKSController", - Syncer: &jwksController{ + Syncer: &jwksWriterController{ jwksSecretLabels: jwksSecretLabels, kubeClient: kubeClient, pinnipedClient: pinnipedClient, @@ -110,14 +110,14 @@ func NewJWKSController( // We want to be notified when anything happens to an OPC. withInformer( opcInformer, - pinnipedcontroller.NoOpFilter(), + pinnipedcontroller.MatchAnythingFilter(), controllerlib.InformerOption{}, ), ) } // Sync implements controllerlib.Syncer. -func (c *jwksController) Sync(ctx controllerlib.Context) error { +func (c *jwksWriterController) Sync(ctx controllerlib.Context) error { opc, err := c.opcInformer.Lister().OIDCProviderConfigs(ctx.Key.Namespace).Get(ctx.Key.Name) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { @@ -177,7 +177,7 @@ func (c *jwksController) Sync(ctx controllerlib.Context) error { return nil } -func (c *jwksController) secretNeedsUpdate(opc *configv1alpha1.OIDCProviderConfig) (bool, error) { +func (c *jwksWriterController) secretNeedsUpdate(opc *configv1alpha1.OIDCProviderConfig) (bool, error) { if opc.Status.JWKSSecret.Name == "" { // If the OPC says it doesn't have a secret associated with it, then let's create one. return true, nil @@ -202,7 +202,7 @@ func (c *jwksController) secretNeedsUpdate(opc *configv1alpha1.OIDCProviderConfi return false, nil } -func (c *jwksController) generateSecret(opc *configv1alpha1.OIDCProviderConfig) (*corev1.Secret, error) { +func (c *jwksWriterController) generateSecret(opc *configv1alpha1.OIDCProviderConfig) (*corev1.Secret, error) { // Note! This is where we could potentially add more handling of OPC spec fields which tell us how // this OIDC provider should sign and verify ID tokens (e.g., hardcoded token secret, gRPC // connection to KMS, etc). @@ -255,7 +255,7 @@ func (c *jwksController) generateSecret(opc *configv1alpha1.OIDCProviderConfig) return &s, nil } -func (c *jwksController) createOrUpdateSecret( +func (c *jwksWriterController) createOrUpdateSecret( ctx context.Context, newSecret *corev1.Secret, ) error { @@ -289,7 +289,7 @@ func (c *jwksController) createOrUpdateSecret( }) } -func (c *jwksController) updateOPC( +func (c *jwksWriterController) updateOPC( ctx context.Context, newOPC *configv1alpha1.OIDCProviderConfig, ) error { diff --git a/internal/controller/supervisorconfig/jwks_test.go b/internal/controller/supervisorconfig/jwks_writer_test.go similarity index 98% rename from internal/controller/supervisorconfig/jwks_test.go rename to internal/controller/supervisorconfig/jwks_writer_test.go index 00e2a15d..39006054 100644 --- a/internal/controller/supervisorconfig/jwks_test.go +++ b/internal/controller/supervisorconfig/jwks_writer_test.go @@ -30,7 +30,7 @@ import ( "go.pinniped.dev/internal/testutil" ) -func TestJWKSControllerFilterSecret(t *testing.T) { +func TestJWKSWriterControllerFilterSecret(t *testing.T) { t.Parallel() tests := []struct { @@ -150,7 +150,7 @@ func TestJWKSControllerFilterSecret(t *testing.T) { 0, ).Config().V1alpha1().OIDCProviderConfigs() withInformer := testutil.NewObservableWithInformerOption() - _ = NewJWKSController( + _ = NewJWKSWriterController( nil, // labels, not needed nil, // kubeClient, not needed nil, // pinnipedClient, not needed @@ -170,7 +170,7 @@ func TestJWKSControllerFilterSecret(t *testing.T) { } } -func TestJWKSControllerFilterOPC(t *testing.T) { +func TestJWKSWriterControllerFilterOPC(t *testing.T) { t.Parallel() tests := []struct { @@ -204,7 +204,7 @@ func TestJWKSControllerFilterOPC(t *testing.T) { 0, ).Config().V1alpha1().OIDCProviderConfigs() withInformer := testutil.NewObservableWithInformerOption() - _ = NewJWKSController( + _ = NewJWKSWriterController( nil, // labels, not needed nil, // kubeClient, not needed nil, // pinnipedClient, not needed @@ -224,7 +224,7 @@ func TestJWKSControllerFilterOPC(t *testing.T) { } } -func TestJWKSControllerSync(t *testing.T) { +func TestJWKSWriterControllerSync(t *testing.T) { // We shouldn't run this test in parallel since it messes with a global function (generateKey). const namespace = "tuna-namespace" @@ -284,10 +284,10 @@ func TestJWKSControllerSync(t *testing.T) { } s.Data = make(map[string][]byte) if activeJWKPath != "" { - s.Data["activeJWK"] = read(t, activeJWKPath) + s.Data["activeJWK"] = readJWKJSON(t, activeJWKPath) } if jwksPath != "" { - s.Data["jwks"] = read(t, jwksPath) + s.Data["jwks"] = readJWKJSON(t, jwksPath) } return &s } @@ -653,7 +653,7 @@ func TestJWKSControllerSync(t *testing.T) { 0, ) - c := NewJWKSController( + c := NewJWKSWriterController( map[string]string{ "myLabelKey1": "myLabelValue1", "myLabelKey2": "myLabelValue2", @@ -692,7 +692,7 @@ func TestJWKSControllerSync(t *testing.T) { } } -func read(t *testing.T, path string) []byte { +func readJWKJSON(t *testing.T, path string) []byte { t.Helper() data, err := ioutil.ReadFile(path) diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index d6420f4c..77acd401 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -58,7 +58,7 @@ func NewOIDCProviderConfigWatcherController( }, withInformer( opcInformer, - pinnipedcontroller.NoOpFilter(), + pinnipedcontroller.MatchAnythingFilter(), controllerlib.InformerOption{}, ), ) diff --git a/internal/controller/supervisorconfig/testdata/public-jwk2.json b/internal/controller/supervisorconfig/testdata/public-jwk2.json new file mode 100644 index 00000000..3782f21b --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/public-jwk2.json @@ -0,0 +1,9 @@ +{ + "use": "sig", + "kty": "EC", + "kid": "pinniped-supervisor-key2", + "crv": "P-256", + "alg": "ES256", + "x" : "SVqB4JcUD6lsfvqMr-OKUNUphdNn64Eay60978ZlL74", + "y" : "lf0u0pMj4lGAzZix5u4Cm5CMQIgMNpkwy163wtKYVKI" +} diff --git a/internal/controller/utils.go b/internal/controller/utils.go index a0eecb9b..8a6cd4fb 100644 --- a/internal/controller/utils.go +++ b/internal/controller/utils.go @@ -15,8 +15,8 @@ func NameAndNamespaceExactMatchFilterFactory(name, namespace string) controllerl }) } -// NoOpFilter returns a controllerlib.Filter that allows all objects. -func NoOpFilter() controllerlib.Filter { +// MatchAnythingFilter returns a controllerlib.Filter that allows all objects. +func MatchAnythingFilter() controllerlib.Filter { return SimpleFilter(func(object metav1.Object) bool { return true }) } diff --git a/internal/oidc/discovery/discovery.go b/internal/oidc/discovery/discovery_handler.go similarity index 92% rename from internal/oidc/discovery/discovery.go rename to internal/oidc/discovery/discovery_handler.go index c919e150..04039c5b 100644 --- a/internal/oidc/discovery/discovery.go +++ b/internal/oidc/discovery/discovery_handler.go @@ -39,13 +39,13 @@ type Metadata struct { // ^^^ Optional ^^^ } -// New returns an http.Handler that serves an OIDC discovery endpoint. -func New(issuerURL string) http.Handler { +// NewHandler returns an http.Handler that serves an OIDC discovery endpoint. +func NewHandler(issuerURL string) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Header().Set("Content-Type", "application/json") if r.Method != http.MethodGet { - http.Error(w, `{"error": "Method not allowed (try GET)"}`, http.StatusMethodNotAllowed) + http.Error(w, `Method not allowed (try GET)`, http.StatusMethodNotAllowed) return } diff --git a/internal/oidc/discovery/discovery_test.go b/internal/oidc/discovery/discovery_handler_test.go similarity index 72% rename from internal/oidc/discovery/discovery_test.go rename to internal/oidc/discovery/discovery_handler_test.go index e21b3c4a..c8fef948 100644 --- a/internal/oidc/discovery/discovery_test.go +++ b/internal/oidc/discovery/discovery_handler_test.go @@ -24,7 +24,8 @@ func TestDiscovery(t *testing.T) { wantStatus int wantContentType string - wantBody interface{} + wantBodyJSON interface{} + wantBodyString string }{ { name: "happy path", @@ -33,7 +34,7 @@ func TestDiscovery(t *testing.T) { path: "/some/path" + oidc.WellKnownEndpointPath, wantStatus: http.StatusOK, wantContentType: "application/json", - wantBody: &Metadata{ + wantBodyJSON: &Metadata{ Issuer: "https://some-issuer.com/some/path", AuthorizationEndpoint: "https://some-issuer.com/some/path/oauth2/authorize", TokenEndpoint: "https://some-issuer.com/some/path/oauth2/token", @@ -48,35 +49,36 @@ func TestDiscovery(t *testing.T) { }, }, { - name: "bad method", - issuer: "https://some-issuer.com", - method: http.MethodPost, - path: oidc.WellKnownEndpointPath, - wantStatus: http.StatusMethodNotAllowed, - wantBody: map[string]string{ - "error": "Method not allowed (try GET)", - }, + name: "bad method", + issuer: "https://some-issuer.com", + method: http.MethodPost, + path: oidc.WellKnownEndpointPath, + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method not allowed (try GET)\n", }, } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - handler := New(test.issuer) + handler := NewHandler(test.issuer) req := httptest.NewRequest(test.method, test.path, nil) rsp := httptest.NewRecorder() handler.ServeHTTP(rsp, req) require.Equal(t, test.wantStatus, rsp.Code) - if test.wantContentType != "" { - require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) - } + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) - if test.wantBody != nil { - wantJSON, err := json.Marshal(test.wantBody) + if test.wantBodyJSON != nil { + wantJSON, err := json.Marshal(test.wantBodyJSON) require.NoError(t, err) require.JSONEq(t, string(wantJSON), rsp.Body.String()) } + + if test.wantBodyString != "" { + require.Equal(t, test.wantBodyString, rsp.Body.String()) + } }) } } diff --git a/internal/oidc/jwks/dynamic_jwks_provider.go b/internal/oidc/jwks/dynamic_jwks_provider.go new file mode 100644 index 00000000..8672fd2f --- /dev/null +++ b/internal/oidc/jwks/dynamic_jwks_provider.go @@ -0,0 +1,38 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import ( + "sync" + + "gopkg.in/square/go-jose.v2" +) + +type DynamicJWKSProvider interface { + SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) + GetJWKS(issuerName string) *jose.JSONWebKeySet +} + +type dynamicJWKSProvider struct { + issuerToJWKSMap map[string]*jose.JSONWebKeySet + mutex sync.RWMutex +} + +func NewDynamicJWKSProvider() DynamicJWKSProvider { + return &dynamicJWKSProvider{ + issuerToJWKSMap: map[string]*jose.JSONWebKeySet{}, + } +} + +func (p *dynamicJWKSProvider) SetIssuerToJWKSMap(issuerToJWKSMap map[string]*jose.JSONWebKeySet) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.issuerToJWKSMap = issuerToJWKSMap +} + +func (p *dynamicJWKSProvider) GetJWKS(issuerName string) *jose.JSONWebKeySet { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.issuerToJWKSMap[issuerName] +} diff --git a/internal/oidc/jwks/jwks_handler.go b/internal/oidc/jwks/jwks_handler.go new file mode 100644 index 00000000..bdb036e6 --- /dev/null +++ b/internal/oidc/jwks/jwks_handler.go @@ -0,0 +1,33 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package discovery provides a handler for the OIDC discovery endpoint. +package jwks + +import ( + "encoding/json" + "net/http" +) + +// NewHandler returns an http.Handler that serves an OIDC JWKS endpoint for a specific issuer. +func NewHandler(issuerName string, provider DynamicJWKSProvider) http.Handler { + return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("Content-Type", "application/json") + + if r.Method != http.MethodGet { + http.Error(w, `Method not allowed (try GET)`, http.StatusMethodNotAllowed) + return + } + + jwks := provider.GetJWKS(issuerName) + + if jwks == nil { + http.Error(w, "JWKS not found for requested issuer", http.StatusNotFound) + return + } + + if err := json.NewEncoder(w).Encode(&jwks); err != nil { + http.Error(w, err.Error(), http.StatusInternalServerError) + } + }) +} diff --git a/internal/oidc/jwks/jwks_handler_test.go b/internal/oidc/jwks/jwks_handler_test.go new file mode 100644 index 00000000..d80e66d5 --- /dev/null +++ b/internal/oidc/jwks/jwks_handler_test.go @@ -0,0 +1,112 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package jwks + +import ( + "encoding/json" + "net/http" + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" + + "go.pinniped.dev/internal/here" +) + +func TestJWKSEndpoint(t *testing.T) { + testJWKSJSONString := here.Doc(` + { + "keys": [ + { + "use": "sig", + "kty": "EC", + "kid": "pinniped-supervisor-key", + "crv": "P-256", + "alg": "ES256", + "x": "awmmj6CIMhSoJyfsqH7sekbTeY72GGPLEy16tPWVz2U", + "y": "FcMh06uXLaq9b2MOixlLVidUkycO1u7IHOkrTi7N0aw" + } + ] + } + `) + + tests := []struct { + name string + + issuer string + provider DynamicJWKSProvider + method string + path string + + wantStatus int + wantContentType string + wantBodyJSONString string + wantBodyString string + }{ + { + name: "happy path", + issuer: "https://some-issuer.com/some/path", + provider: newDynamicJWKSProvider(t, "https://some-issuer.com/some/path", testJWKSJSONString), + method: http.MethodGet, + path: "/some/path", + wantStatus: http.StatusOK, + wantContentType: "application/json", + wantBodyJSONString: testJWKSJSONString, + }, + { + name: "bad method", + issuer: "https://some-issuer.com", + provider: newDynamicJWKSProvider(t, "https://some-issuer.com", testJWKSJSONString), + method: http.MethodPost, + path: "/some/path", + wantStatus: http.StatusMethodNotAllowed, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "Method not allowed (try GET)\n", + }, + { + name: "no JWKS found in provider's cache for this issuer", + issuer: "https://some-issuer.com", + provider: newDynamicJWKSProvider(t, "https://some-other-unrelated-issuer.com", testJWKSJSONString), + method: http.MethodGet, + path: "/some/path", + wantStatus: http.StatusNotFound, + wantContentType: "text/plain; charset=utf-8", + wantBodyString: "JWKS not found for requested issuer\n", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + handler := NewHandler(test.issuer, test.provider) + req := httptest.NewRequest(test.method, test.path, nil) + rsp := httptest.NewRecorder() + handler.ServeHTTP(rsp, req) + + require.Equal(t, test.wantStatus, rsp.Code) + + require.Equal(t, test.wantContentType, rsp.Header().Get("Content-Type")) + + if test.wantBodyJSONString != "" { + require.JSONEq(t, test.wantBodyJSONString, rsp.Body.String()) + } + + if test.wantBodyString != "" { + require.Equal(t, test.wantBodyString, rsp.Body.String()) + } + }) + } +} + +func newDynamicJWKSProvider(t *testing.T, issuer string, jwksJSON string) DynamicJWKSProvider { + t.Helper() + jwksProvider := NewDynamicJWKSProvider() + var keySet jose.JSONWebKeySet + err := json.Unmarshal([]byte(jwksJSON), &keySet) + require.NoError(t, err) + jwksProvider.SetIssuerToJWKSMap(map[string]*jose.JSONWebKeySet{ + issuer: &keySet, + }) + return jwksProvider +} diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 0626c48c..0737c3f5 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -11,6 +11,7 @@ import ( "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/discovery" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" ) @@ -18,16 +19,22 @@ import ( // // It is thread-safe. type Manager struct { - mu sync.RWMutex - providers []*provider.OIDCProvider - providerHandlers map[string]http.Handler // map of all routes for all providers - nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request + mu sync.RWMutex + providers []*provider.OIDCProvider + providerHandlers map[string]http.Handler // map of all routes for all providers + nextHandler http.Handler // the next handler in a chain, called when this manager didn't know how to handle a request + dynamicJWKSProvider jwks.DynamicJWKSProvider // in-memory cache of per-issuer JWKS data } // NewManager returns an empty Manager. // nextHandler will be invoked for any requests that could not be handled by this manager's providers. -func NewManager(nextHandler http.Handler) *Manager { - return &Manager{providerHandlers: make(map[string]http.Handler), nextHandler: nextHandler} +// dynamicJWKSProvider will be used as an in-memory cache for per-issuer JWKS data. +func NewManager(nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider) *Manager { + return &Manager{ + providerHandlers: make(map[string]http.Handler), + nextHandler: nextHandler, + dynamicJWKSProvider: dynamicJWKSProvider, + } } // SetProviders adds or updates all the given providerHandlers using each provider's issuer string @@ -46,7 +53,12 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { m.providerHandlers = make(map[string]http.Handler) for _, incomingProvider := range oidcProviders { - m.providerHandlers[incomingProvider.IssuerHost()+"/"+incomingProvider.IssuerPath()+oidc.WellKnownEndpointPath] = discovery.New(incomingProvider.Issuer()) + wellKnownURL := incomingProvider.IssuerHost() + "/" + incomingProvider.IssuerPath() + oidc.WellKnownEndpointPath + m.providerHandlers[wellKnownURL] = discovery.NewHandler(incomingProvider.Issuer()) + + jwksURL := incomingProvider.IssuerHost() + "/" + incomingProvider.IssuerPath() + oidc.JWKSEndpointPath + m.providerHandlers[jwksURL] = jwks.NewHandler(incomingProvider.Issuer(), m.dynamicJWKSProvider) + klog.InfoS("oidc provider manager added or updated issuer", "issuer", incomingProvider.Issuer()) } } diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index c877f3ac..d68e0782 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -13,18 +13,29 @@ import ( "github.com/sclevine/spec" "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" + "go.pinniped.dev/internal/here" "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/discovery" + "go.pinniped.dev/internal/oidc/jwks" "go.pinniped.dev/internal/oidc/provider" ) func TestManager(t *testing.T) { spec.Run(t, "ServeHTTP", func(t *testing.T, when spec.G, it spec.S) { - var r *require.Assertions - var subject *Manager - var nextHandler http.HandlerFunc - var fallbackHandlerWasCalled bool + var ( + r *require.Assertions + subject *Manager + nextHandler http.HandlerFunc + fallbackHandlerWasCalled bool + dynamicJWKSProvider jwks.DynamicJWKSProvider + ) + + issuer1 := "https://example.com/some/path" + issuer1KeyID := "issuer1-key" + issuer2 := "https://example.com/some/path/more/deeply/nested/path" // note that this is a sub-path of the other issuer url + issuer2KeyID := "issuer2-key" newGetRequest := func(url string) *http.Request { return httptest.NewRequest(http.MethodGet, url, nil) @@ -35,6 +46,8 @@ func TestManager(t *testing.T) { subject.ServeHTTP(recorder, newGetRequest(issuer+oidc.WellKnownEndpointPath+requestURLSuffix)) + r.False(fallbackHandlerWasCalled) + r.Equal(http.StatusOK, recorder.Code) responseBody, err := ioutil.ReadAll(recorder.Body) r.NoError(err) @@ -42,18 +55,38 @@ func TestManager(t *testing.T) { err = json.Unmarshal(responseBody, &parsedDiscoveryResult) r.NoError(err) + // Minimal check to ensure that the right discovery endpoint was called r.Equal(issuer, parsedDiscoveryResult.Issuer) } + requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) { + recorder := httptest.NewRecorder() + + subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.JWKSEndpointPath+requestURLSuffix)) + + r.False(fallbackHandlerWasCalled) + + r.Equal(http.StatusOK, recorder.Code) + responseBody, err := ioutil.ReadAll(recorder.Body) + r.NoError(err) + parsedJWKSResult := jose.JSONWebKeySet{} + err = json.Unmarshal(responseBody, &parsedJWKSResult) + r.NoError(err) + + // Minimal check to ensure that the right JWKS endpoint was called + r.Equal(expectedJWKKeyID, parsedJWKSResult.Keys[0].KeyID) + } + it.Before(func() { r = require.New(t) nextHandler = func(http.ResponseWriter, *http.Request) { fallbackHandlerWasCalled = true } - subject = NewManager(nextHandler) + dynamicJWKSProvider = jwks.NewDynamicJWKSProvider() + subject = NewManager(nextHandler, dynamicJWKSProvider) }) - when("given no providers", func() { + when("given no providers via SetProviders()", func() { it("sends all requests to the nextHandler", func() { r.False(fallbackHandlerWasCalled) subject.ServeHTTP(httptest.NewRecorder(), newGetRequest("/anything")) @@ -61,16 +94,35 @@ func TestManager(t *testing.T) { }) }) - when("given some valid providers", func() { - issuer1 := "https://example.com/some/path" - issuer2 := "https://example.com/some/path/more/deeply/nested/path" // note that this is a sub-path of the other issuer url + newTestJWK := func(keyID string) jose.JSONWebKey { + testJWKSJSONString := here.Docf(` + { + "use": "sig", + "kty": "EC", + "kid": "%s", + "crv": "P-256", + "alg": "ES256", + "x": "awmmj6CIMhSoJyfsqH7sekbTeY72GGPLEy16tPWVz2U", + "y": "FcMh06uXLaq9b2MOixlLVidUkycO1u7IHOkrTi7N0aw" + } + `, keyID) + k := jose.JSONWebKey{} + r.NoError(json.Unmarshal([]byte(testJWKSJSONString), &k)) + return k + } + when("given some valid providers via SetProviders()", func() { it.Before(func() { p1, err := provider.NewOIDCProvider(issuer1) r.NoError(err) p2, err := provider.NewOIDCProvider(issuer2) 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)}}, + }) }) it("sends all non-matching host requests to the nextHandler", func() { @@ -96,27 +148,35 @@ func TestManager(t *testing.T) { requireDiscoveryRequestToBeHandled(issuer1, "") requireDiscoveryRequestToBeHandled(issuer2, "") requireDiscoveryRequestToBeHandled(issuer2, "?some=query") - r.False(fallbackHandlerWasCalled) + + requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) }) }) - when("given the same valid providers in reverse order", func() { - issuer1 := "https://example.com/some/path" - issuer2 := "https://example.com/some/path/more/deeply/nested/path" - + when("given the same valid providers as arguments to SetProviders() in reverse order", func() { it.Before(func() { p1, err := provider.NewOIDCProvider(issuer1) r.NoError(err) p2, err := provider.NewOIDCProvider(issuer2) 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)}}, + }) }) it("still routes matching requests to the appropriate provider", func() { requireDiscoveryRequestToBeHandled(issuer1, "") requireDiscoveryRequestToBeHandled(issuer2, "") requireDiscoveryRequestToBeHandled(issuer2, "?some=query") - r.False(fallbackHandlerWasCalled) + + requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) }) }) }) From 4da64f38b54ccbcde3a8f3130c6af5f0ca83010b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 19 Oct 2020 12:21:18 -0700 Subject: [PATCH 2/2] Integration test for per-issuer OIDC JWKS endpoints --- test/integration/supervisor_discovery_test.go | 200 +++++++++++++----- 1 file changed, 148 insertions(+), 52 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 9b983fe2..4da92004 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -5,6 +5,7 @@ package integration import ( "context" + "encoding/json" "fmt" "io/ioutil" "net/http" @@ -51,7 +52,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { }) // Test that there is no default discovery endpoint available when there are no OIDCProviderConfigs. - requireDiscoveryEndpointIsNotFound(t, fmt.Sprintf("http://%s", env.SupervisorAddress)) + requireDiscoveryEndpointsAreNotFound(t, fmt.Sprintf("http://%s", env.SupervisorAddress)) // Define several unique issuer strings. issuer1 := fmt.Sprintf("http://%s/nested/issuer1", env.SupervisorAddress) @@ -59,56 +60,79 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { issuer3 := fmt.Sprintf("http://%s/issuer3", env.SupervisorAddress) issuer4 := fmt.Sprintf("http://%s/issuer4", env.SupervisorAddress) issuer5 := fmt.Sprintf("http://%s/issuer5", env.SupervisorAddress) + issuer6 := fmt.Sprintf("http://%s/issuer6", env.SupervisorAddress) badIssuer := fmt.Sprintf("http://%s/badIssuer?cannot-use=queries", env.SupervisorAddress) // When OIDCProviderConfig are created in sequence they each cause a discovery endpoint to appear only for as long as the OIDCProviderConfig exists. - config1 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(ctx, t, issuer1, client) + config1, jwks1 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, issuer1, client) requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config1, client, ns, issuer1) - config2 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(ctx, t, issuer2, client) + config2, jwks2 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, issuer2, client) requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config2, client, ns, issuer2) + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks1.Keys[0]["x"], jwks2.Keys[0]["x"]) + require.NotEqual(t, jwks1.Keys[0]["y"], jwks2.Keys[0]["y"]) // When multiple OIDCProviderConfigs exist at the same time they each serve a unique discovery endpoint. - config3 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(ctx, t, issuer3, client) - config4 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(ctx, t, issuer4, client) - requireWellKnownEndpointIsWorking(t, issuer3) // discovery for issuer3 is still working after issuer4 started working + config3, jwks3 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, issuer3, client) + config4, jwks4 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, issuer4, client) + requireDiscoveryEndpointsAreWorking(t, issuer3) // discovery for issuer3 is still working after issuer4 started working + // The auto-created JWK's were different from each other. + require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) + require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) + + // Editing a provider to change the issuer name updates the endpoints that are being served. + updatedConfig4 := editOIDCProviderConfigIssuerName(t, config4, client, ns, issuer5) + requireDiscoveryEndpointsAreNotFound(t, issuer4) + jwks5 := requireDiscoveryEndpointsAreWorking(t, issuer5) + // The JWK did not change when the issuer name was updated. + require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) // When they are deleted they stop serving discovery endpoints. requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config3, client, ns, issuer3) - requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config4, client, ns, issuer4) + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, updatedConfig4, client, ns, issuer5) // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. - config5Duplicate1 := requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear(ctx, t, issuer5, client) - config5Duplicate2 := library.CreateTestOIDCProvider(ctx, t, issuer5) + config5Duplicate1, _ := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, issuer6, client) + config5Duplicate2 := library.CreateTestOIDCProvider(ctx, t, issuer6) requireStatus(t, client, ns, config5Duplicate1.Name, v1alpha1.DuplicateOIDCProviderStatus) requireStatus(t, client, ns, config5Duplicate2.Name, v1alpha1.DuplicateOIDCProviderStatus) - requireDiscoveryEndpointIsNotFound(t, issuer5) + requireDiscoveryEndpointsAreNotFound(t, issuer6) // If we delete the first duplicate issuer, the second duplicate issuer starts serving. requireDelete(t, client, ns, config5Duplicate1.Name) - requireWellKnownEndpointIsWorking(t, issuer5) + requireWellKnownEndpointIsWorking(t, issuer6) requireStatus(t, client, ns, config5Duplicate2.Name, v1alpha1.SuccessOIDCProviderStatus) // When we finally delete all issuers, the endpoint should be down. - requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config5Duplicate2, client, ns, issuer5) + requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t, config5Duplicate2, client, ns, issuer6) // When we create a provider with an invalid issuer, the status is set to invalid. badConfig := library.CreateTestOIDCProvider(ctx, t, badIssuer) requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidOIDCProviderStatus) - requireDiscoveryEndpointIsNotFound(t, badIssuer) + requireDiscoveryEndpointsAreNotFound(t, badIssuer) } -func requireDiscoveryEndpointIsNotFound(t *testing.T, issuerName string) { +func jwksURLForIssuer(issuerName string) string { + return fmt.Sprintf("%s/jwks.json", issuerName) +} + +func wellKnownURLForIssuer(issuerName string) string { + return fmt.Sprintf("%s/.well-known/openid-configuration", issuerName) +} + +func requireDiscoveryEndpointsAreNotFound(t *testing.T, issuerName string) { + t.Helper() + requireEndpointNotFound(t, wellKnownURLForIssuer(issuerName)) + requireEndpointNotFound(t, jwksURLForIssuer(issuerName)) +} + +func requireEndpointNotFound(t *testing.T, url string) { t.Helper() httpClient := &http.Client{} ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() - requestNonExistentPath, err := http.NewRequestWithContext( - ctx, - http.MethodGet, - fmt.Sprintf("%s/.well-known/openid-configuration", issuerName), - nil, - ) + requestNonExistentPath, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) var response *http.Response assert.Eventually(t, func() bool { @@ -121,20 +145,36 @@ func requireDiscoveryEndpointIsNotFound(t *testing.T, issuerName string) { require.NoError(t, err) } -func requireCreatingOIDCProviderConfigCausesWellKnownEndpointToAppear( +func requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear( ctx context.Context, t *testing.T, issuerName string, client pinnipedclientset.Interface, -) *v1alpha1.OIDCProviderConfig { +) (*v1alpha1.OIDCProviderConfig, *ExpectedJWKSResponseFormat) { t.Helper() + newOIDCProviderConfig := library.CreateTestOIDCProvider(ctx, t, issuerName) - requireWellKnownEndpointIsWorking(t, issuerName) + + jwksResult := requireDiscoveryEndpointsAreWorking(t, issuerName) + requireStatus(t, client, newOIDCProviderConfig.Namespace, newOIDCProviderConfig.Name, v1alpha1.SuccessOIDCProviderStatus) - return newOIDCProviderConfig + + return newOIDCProviderConfig, jwksResult } -func requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t *testing.T, existingOIDCProviderConfig *v1alpha1.OIDCProviderConfig, client pinnipedclientset.Interface, ns string, issuerName string) { +func requireDiscoveryEndpointsAreWorking(t *testing.T, issuerName string) *ExpectedJWKSResponseFormat { + requireWellKnownEndpointIsWorking(t, issuerName) + jwksResult := requireJWKSEndpointIsWorking(t, issuerName) + return jwksResult +} + +func requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear( + t *testing.T, + existingOIDCProviderConfig *v1alpha1.OIDCProviderConfig, + client pinnipedclientset.Interface, + ns string, + issuerName string, +) { t.Helper() ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -144,37 +184,13 @@ func requireDeletingOIDCProviderConfigCausesWellKnownEndpointToDisappear(t *test require.NoError(t, err) // Fetch that same discovery endpoint as before, but now it should not exist anymore. Give it some time for the endpoint to go away. - requireDiscoveryEndpointIsNotFound(t, issuerName) + requireDiscoveryEndpointsAreNotFound(t, issuerName) } func requireWellKnownEndpointIsWorking(t *testing.T, issuerName string) { t.Helper() - httpClient := &http.Client{} - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - // Define a request to the new discovery endpoint which should have been created by an OIDCProviderConfig. - requestDiscoveryEndpoint, err := http.NewRequestWithContext( - ctx, - http.MethodGet, - fmt.Sprintf("%s/.well-known/openid-configuration", issuerName), - nil, - ) - require.NoError(t, err) - - // Fetch that discovery endpoint. Give it some time for the endpoint to come into existence. - var response *http.Response - assert.Eventually(t, func() bool { - response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose - return err == nil && response.StatusCode == http.StatusOK - }, 10*time.Second, 200*time.Millisecond) - require.NoError(t, err) - require.Equal(t, http.StatusOK, response.StatusCode) - - responseBody, err := ioutil.ReadAll(response.Body) - require.NoError(t, err) - err = response.Body.Close() - require.NoError(t, err) + response, responseBody := requireSuccessEndpointResponse(t, wellKnownURLForIssuer(issuerName)) //nolint:bodyclose // Check that the response matches our expectations. expectedResultTemplate := here.Doc(`{ @@ -193,7 +209,87 @@ func requireWellKnownEndpointIsWorking(t *testing.T, issuerName string) { expectedJSON := fmt.Sprintf(expectedResultTemplate, issuerName, issuerName, issuerName, issuerName) require.Equal(t, "application/json", response.Header.Get("content-type")) - require.JSONEq(t, expectedJSON, string(responseBody)) + require.JSONEq(t, expectedJSON, responseBody) +} + +type ExpectedJWKSResponseFormat struct { + Keys []map[string]string +} + +func requireJWKSEndpointIsWorking(t *testing.T, issuerName string) *ExpectedJWKSResponseFormat { + t.Helper() + + response, responseBody := requireSuccessEndpointResponse(t, jwksURLForIssuer(issuerName)) //nolint:bodyclose + + var result ExpectedJWKSResponseFormat + err := json.Unmarshal([]byte(responseBody), &result) + require.NoError(t, err) + + require.Len(t, result.Keys, 1) + jwk := result.Keys[0] + require.Len(t, jwk, 7) // make sure there are no extra values, i.e. does not include private key + require.NotEmpty(t, jwk["kid"]) + require.Equal(t, "sig", jwk["use"]) + require.Equal(t, "EC", jwk["kty"]) + require.Equal(t, "P-256", jwk["crv"]) + require.Equal(t, "ES256", jwk["alg"]) + require.NotEmpty(t, jwk["x"]) + require.NotEmpty(t, jwk["y"]) + + require.Equal(t, "application/json", response.Header.Get("content-type")) + + return &result +} + +func requireSuccessEndpointResponse(t *testing.T, wellKnownURL string) (*http.Response, string) { + httpClient := &http.Client{} + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // Define a request to the new discovery endpoint which should have been created by an OIDCProviderConfig. + requestDiscoveryEndpoint, err := http.NewRequestWithContext( + ctx, + http.MethodGet, + wellKnownURL, + nil, + ) + require.NoError(t, err) + + // Fetch that discovery endpoint. Give it some time for the endpoint to come into existence. + var response *http.Response + assert.Eventually(t, func() bool { + response, err = httpClient.Do(requestDiscoveryEndpoint) //nolint:bodyclose + return err == nil && response.StatusCode == http.StatusOK + }, 10*time.Second, 200*time.Millisecond) + require.NoError(t, err) + require.Equal(t, http.StatusOK, response.StatusCode) + + responseBody, err := ioutil.ReadAll(response.Body) + require.NoError(t, err) + err = response.Body.Close() + require.NoError(t, err) + return response, string(responseBody) +} + +func editOIDCProviderConfigIssuerName( + t *testing.T, + existingOIDCProviderConfig *v1alpha1.OIDCProviderConfig, + client pinnipedclientset.Interface, + ns string, + newIssuerName string, +) *v1alpha1.OIDCProviderConfig { + t.Helper() + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + mostRecentVersion, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Get(ctx, existingOIDCProviderConfig.Name, metav1.GetOptions{}) + require.NoError(t, err) + + mostRecentVersion.Spec.Issuer = newIssuerName + updated, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Update(ctx, mostRecentVersion, metav1.UpdateOptions{}) + require.NoError(t, err) + + return updated } func requireDelete(t *testing.T, client pinnipedclientset.Interface, ns, name string) {