From b043dae1491142ab4227f7c3d8c15ff768ce8dbc Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Mon, 14 Dec 2020 10:36:45 -0500 Subject: [PATCH] Finish first implementation of generic secret generator controller Signed-off-by: Andrew Keesler --- cmd/pinniped-supervisor/main.go | 80 ++- .../generator/oidc_provider_secrets.go | 113 +++-- .../generator/oidc_provider_secrets_test.go | 473 ++++++++---------- .../generator/supervisor_secrets.go | 1 + .../symmetric_secret_helper.go | 110 ++++ .../symmetric_secret_helper_test.go | 154 ++++++ internal/mocks/mocksecrethelper/generate.go | 6 + .../mocksecrethelper/mocksecrethelper.go | 94 ++++ internal/oidc/provider/manager/manager.go | 8 - .../oidc/provider/manager/manager_test.go | 31 +- internal/secret/cache.go | 10 +- 11 files changed, 753 insertions(+), 327 deletions(-) create mode 100644 internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper.go create mode 100644 internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper_test.go create mode 100644 internal/mocks/mocksecrethelper/generate.go create mode 100644 internal/mocks/mocksecrethelper/mocksecrethelper.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index ccc26947..eb2fe2c4 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -5,6 +5,7 @@ package main import ( "context" + "crypto/rand" "crypto/tls" "fmt" "net" @@ -17,6 +18,7 @@ import ( "go.pinniped.dev/internal/secret" appsv1 "k8s.io/api/apps/v1" + corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/util/clock" kubeinformers "k8s.io/client-go/informers" @@ -28,11 +30,13 @@ import ( "k8s.io/klog/v2" "k8s.io/klog/v2/klogr" + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned" pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/config/supervisor" "go.pinniped.dev/internal/controller/supervisorconfig" "go.pinniped.dev/internal/controller/supervisorconfig/generator" + "go.pinniped.dev/internal/controller/supervisorconfig/generator/symmetricsecrethelper" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher" "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" @@ -88,6 +92,9 @@ func startControllers( kubeInformers kubeinformers.SharedInformerFactory, pinnipedInformers pinnipedinformers.SharedInformerFactory, ) { + opInformer := pinnipedInformers.Config().V1alpha1().OIDCProviders() + secretInformer := kubeInformers.Core().V1().Secrets() + // Create controller manager. controllerManager := controllerlib. NewManager(). @@ -96,7 +103,7 @@ func startControllers( issuerManager, clock.RealClock{}, pinnipedClient, - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -106,8 +113,8 @@ func startControllers( cfg.Labels, kubeClient, pinnipedClient, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -115,8 +122,8 @@ func startControllers( WithController( supervisorconfig.NewJWKSObserverController( dynamicJWKSProvider, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -125,8 +132,8 @@ func startControllers( supervisorconfig.NewTLSCertObserverController( dynamicTLSCertProvider, cfg.NamesConfig.DefaultTLSCertificateSecret, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -135,7 +142,7 @@ func startControllers( generator.NewSupervisorSecretsController( supervisorDeployment, kubeClient, - kubeInformers.Core().V1().Secrets(), + secretInformer, func(secret []byte) { plog.Debug("setting csrf cookie secret") secretCache.SetCSRFCookieEncoderHashKey(secret) @@ -143,6 +150,63 @@ func startControllers( ), singletonWorker, ). + WithController( + generator.NewOIDCProviderSecretsController( + symmetricsecrethelper.New( + "pinniped-oidc-provider-hmac-key-", + cfg.Labels, + rand.Reader, + func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) { + plog.Debug("setting hmac secret", "issuer", parent.Spec.Issuer) + secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer). + SetTokenHMACKey(child.Data[symmetricsecrethelper.SecretDataKey]) + }, + ), + kubeClient, + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). + WithController( + generator.NewOIDCProviderSecretsController( + symmetricsecrethelper.New( + "pinniped-oidc-provider-upstream-state-signature-key-", + cfg.Labels, + rand.Reader, + func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) { + plog.Debug("setting state signature key", "issuer", parent.Spec.Issuer) + secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer). + SetStateEncoderHashKey(child.Data[symmetricsecrethelper.SecretDataKey]) + }, + ), + kubeClient, + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). + WithController( + generator.NewOIDCProviderSecretsController( + symmetricsecrethelper.New( + "pinniped-oidc-provider-upstream-state-encryption-key-", + cfg.Labels, + rand.Reader, + func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) { + plog.Debug("setting state encryption key", "issuer", parent.Spec.Issuer) + secretCache.GetOIDCProviderCacheFor(parent.Spec.Issuer). + SetStateEncoderHashKey(child.Data[symmetricsecrethelper.SecretDataKey]) + }, + ), + kubeClient, + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). WithController( upstreamwatcher.New( dynamicUpstreamIDPProvider, diff --git a/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go index e1dbab4e..df3fad84 100644 --- a/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go @@ -16,7 +16,6 @@ import ( "k8s.io/klog/v2" configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" - pinnipedclientset "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned" configinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions/config/v1alpha1" pinnipedcontroller "go.pinniped.dev/internal/controller" "go.pinniped.dev/internal/controllerlib" @@ -28,43 +27,47 @@ const ( opcKind = "OIDCProvider" // TODO: deduplicate - internal/controller/supervisorconfig/jwks_writer.go ) -// jwkController holds the fields necessary for the JWKS controller to communicate with OPC's and -// secrets, both via a cache and via the API. +// SecretHelper describes an object that can Generate() a Secret and determine whether a Secret +// IsValid(). It can also be Notify()'d about a Secret being persisted. +// +// A SecretHelper has a Name() that can be used to identify it from other SecretHelper instances. +type SecretHelper interface { + Name() string + Generate(*configv1alpha1.OIDCProvider) (*corev1.Secret, error) + IsValid(*configv1alpha1.OIDCProvider, *corev1.Secret) bool + Notify(*configv1alpha1.OIDCProvider, *corev1.Secret) +} + type oidcProviderSecretsController struct { - secretNameFunc func(*configv1alpha1.OIDCProvider) string - secretLabels map[string]string - secretDataFunc func() (map[string][]byte, error) - pinnipedClient pinnipedclientset.Interface + secretHelper SecretHelper kubeClient kubernetes.Interface opcInformer configinformers.OIDCProviderInformer secretInformer corev1informers.SecretInformer } +// NewOIDCProviderSecretsController returns a controllerlib.Controller that ensures a child Secret +// always exists for a parent OIDCProvider. It does this using the provided secretHelper, which +// provides the parent/child mapping logic. func NewOIDCProviderSecretsController( - secretNameFunc func(*configv1alpha1.OIDCProvider) string, - secretLabels map[string]string, - secretDataFunc func() (map[string][]byte, error), + secretHelper SecretHelper, kubeClient kubernetes.Interface, - pinnipedClient pinnipedclientset.Interface, secretInformer corev1informers.SecretInformer, opcInformer configinformers.OIDCProviderInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ - Name: "JWKSController", + Name: fmt.Sprintf("%s%s", secretHelper.Name(), "controller"), Syncer: &oidcProviderSecretsController{ - secretNameFunc: secretNameFunc, - secretLabels: secretLabels, - secretDataFunc: secretDataFunc, + secretHelper: secretHelper, kubeClient: kubeClient, - pinnipedClient: pinnipedClient, secretInformer: secretInformer, opcInformer: opcInformer, }, }, // We want to be notified when a OPC's secret gets updated or deleted. When this happens, we // should get notified via the corresponding OPC key. + // TODO: de-dup me (jwks_writer.go). withInformer( secretInformer, controllerlib.FilterFuncs{ @@ -96,7 +99,7 @@ func NewOIDCProviderSecretsController( } func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error { - opc, err := c.opcInformer.Lister().OIDCProviders(ctx.Key.Namespace).Get(ctx.Key.Name) + op, err := c.opcInformer.Lister().OIDCProviders(ctx.Key.Namespace).Get(ctx.Key.Name) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { return fmt.Errorf( @@ -108,8 +111,8 @@ func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error { } if notFound { - // The corresponding secret to this OPC should have been garbage collected since it should have - // had this OPC as its owner. + // The corresponding secret to this OP should have been garbage collected since it should have + // had this OP as its owner. plog.Debug( "oidcprovider deleted", "oidcprovider", @@ -118,83 +121,99 @@ func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error { return nil } - secretNeedsUpdate, err := c.secretNeedsUpdate(opc) + newSecret, err := c.secretHelper.Generate(op) if err != nil { - return fmt.Errorf("cannot determine secret status: %w", err) + return fmt.Errorf("failed to generate secret: %w", err) + } + + secretNeedsUpdate, existingSecret, err := c.secretNeedsUpdate(op, newSecret.Name) + if err != nil { + return fmt.Errorf("failed to determine secret status: %w", err) } if !secretNeedsUpdate { // Secret is up to date - we are good to go. plog.Debug( "secret is up to date", "oidcprovider", - klog.KRef(ctx.Key.Namespace, ctx.Key.Name), + klog.KObj(op), + "secret", + klog.KObj(existingSecret), ) + c.secretHelper.Notify(op, existingSecret) return nil } - // If the OPC does not have a secret associated with it, that secret does not exist, or the secret - // is invalid, we will generate a new secret (i.e., a JWKS). - secret, err := generateSecret(opc.Namespace, c.secretNameFunc(opc), c.secretDataFunc, opc) - if err != nil { - return fmt.Errorf("cannot generate secret: %w", err) + // If the OP does not have a secret associated with it, that secret does not exist, or the secret + // is invalid, we will create a new secret. + if err := c.createOrUpdateSecret(ctx.Context, op, &newSecret); err != nil { + return fmt.Errorf("failed to create or update secret: %w", err) } + plog.Debug("created/updated secret", "oidcprovider", klog.KObj(op), "secret", klog.KObj(newSecret)) - if err := c.createOrUpdateSecret(ctx.Context, secret); err != nil { - return fmt.Errorf("cannot create or update secret: %w", err) - } - plog.Debug("created/updated secret", "secret", klog.KObj(secret)) + c.secretHelper.Notify(op, newSecret) return nil } -func (c *oidcProviderSecretsController) secretNeedsUpdate(opc *configv1alpha1.OIDCProvider) (bool, error) { +// secretNeedsUpdate returns whether or not the Secret, with name secretName, for OIDCProvider op +// needs to be updated. It returns the existing secret as its second argument. +func (c *oidcProviderSecretsController) secretNeedsUpdate( + op *configv1alpha1.OIDCProvider, + secretName string, +) (bool, *corev1.Secret, error) { // This OPC says it has a secret associated with it. Let's try to get it from the cache. - secret, err := c.secretInformer.Lister().Secrets(opc.Namespace).Get(c.secretNameFunc(opc)) + secret, err := c.secretInformer.Lister().Secrets(op.Namespace).Get(secretName) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { - return false, fmt.Errorf("cannot get secret: %w", err) + return false, nil, fmt.Errorf("cannot get secret: %w", err) } if notFound { // If we can't find the secret, let's assume we need to create it. - return true, nil + return true, nil, nil } - if !isValid(secret) { + if !c.secretHelper.IsValid(op, secret) { // If this secret is invalid, we need to generate a new one. - return true, nil + return true, secret, nil } - return false, nil + return false, secret, nil } func (c *oidcProviderSecretsController) createOrUpdateSecret( ctx context.Context, - newSecret *corev1.Secret, + op *configv1alpha1.OIDCProvider, + newSecret **corev1.Secret, ) error { - secretClient := c.kubeClient.CoreV1().Secrets(newSecret.Namespace) + secretClient := c.kubeClient.CoreV1().Secrets((*newSecret).Namespace) return retry.RetryOnConflict(retry.DefaultRetry, func() error { - oldSecret, err := secretClient.Get(ctx, newSecret.Name, metav1.GetOptions{}) + oldSecret, err := secretClient.Get(ctx, (*newSecret).Name, metav1.GetOptions{}) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { - return fmt.Errorf("cannot get secret: %w", err) + return fmt.Errorf("failed to get secret %s/%s: %w", (*newSecret).Namespace, (*newSecret).Name, err) } if notFound { // New secret doesn't exist, so create it. - _, err := secretClient.Create(ctx, newSecret, metav1.CreateOptions{}) + _, err := secretClient.Create(ctx, *newSecret, metav1.CreateOptions{}) if err != nil { - return fmt.Errorf("cannot create secret: %w", err) + return fmt.Errorf("failed to create secret %s/%s: %w", (*newSecret).Namespace, (*newSecret).Name, err) } return nil } // New secret already exists, so ensure it is up to date. - if isValid(oldSecret) { - // If the secret already has valid JWK's, then we are good to go and we don't need an update. + if c.secretHelper.IsValid(op, oldSecret) { + // If the secret already has valid a valid Secret, then we are good to go and we don't need an + // update. + *newSecret = oldSecret return nil } - oldSecret.Data = newSecret.Data + oldSecret.Labels = (*newSecret).Labels + oldSecret.Type = (*newSecret).Type + oldSecret.Data = (*newSecret).Data + *newSecret = oldSecret _, err = secretClient.Update(ctx, oldSecret, metav1.UpdateOptions{}) return err }) diff --git a/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go b/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go index 801b481e..a271aa22 100644 --- a/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go @@ -7,11 +7,14 @@ import ( "context" "errors" "fmt" + "sync" "testing" "time" + "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" @@ -23,6 +26,7 @@ import ( pinnipedfake "go.pinniped.dev/generated/1.19/client/supervisor/clientset/versioned/fake" pinnipedinformers "go.pinniped.dev/generated/1.19/client/supervisor/informers/externalversions" "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/mocks/mocksecrethelper" "go.pinniped.dev/internal/testutil" ) @@ -137,6 +141,11 @@ func TestOIDCProviderControllerFilterSecret(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().Name().Times(1).Return("some-name") + secretInformer := kubeinformers.NewSharedInformerFactory( kubernetesfake.NewSimpleClientset(), 0, @@ -147,11 +156,8 @@ func TestOIDCProviderControllerFilterSecret(t *testing.T) { ).Config().V1alpha1().OIDCProviders() withInformer := testutil.NewObservableWithInformerOption() _ = NewOIDCProviderSecretsController( - secretNameFunc, - nil, // labels, not needed - fakeSecretDataFunc, + secretHelper, nil, // kubeClient, not needed - nil, // pinnipedClient, not needed secretInformer, opcInformer, withInformer.WithInformer, @@ -193,6 +199,11 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) { t.Run(test.name, func(t *testing.T) { t.Parallel() + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().Name().Times(1).Return("some-name") + secretInformer := kubeinformers.NewSharedInformerFactory( kubernetesfake.NewSimpleClientset(), 0, @@ -203,11 +214,8 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) { ).Config().V1alpha1().OIDCProviders() withInformer := testutil.NewObservableWithInformerOption() _ = NewOIDCProviderSecretsController( - secretNameFunc, - nil, // labels, not needed - fakeSecretDataFunc, + secretHelper, nil, // kubeClient, not needed - nil, // pinnipedClient, not needed secretInformer, opcInformer, withInformer.WithInformer, @@ -224,307 +232,271 @@ func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) { } } -func TestNewOIDCProviderSecretsControllerSync(t *testing.T) { - // We shouldn't run this test in parallel since it messes with a global function (generateKey). +func TestOIDCProviderSecretsControllerSync(t *testing.T) { + t.Parallel() - const namespace = "tuna-namespace" + const ( + namespace = "some-namespace" - opcGVR := schema.GroupVersionResource{ + opName = "op-name" + opUID = "op-uid" + + secretName = "secret-name" + secretUID = "secret-uid" + ) + + opGVR := schema.GroupVersionResource{ Group: configv1alpha1.SchemeGroupVersion.Group, Version: configv1alpha1.SchemeGroupVersion.Version, Resource: "oidcproviders", } - goodOPC := &configv1alpha1.OIDCProvider{ - ObjectMeta: metav1.ObjectMeta{ - Name: "good-opc", - Namespace: namespace, - UID: "good-opc-uid", - }, - Spec: configv1alpha1.OIDCProviderSpec{ - Issuer: "https://some-issuer.com", - }, - } - - expectedSecretName := secretNameFunc(goodOPC) - secretGVR := schema.GroupVersionResource{ Group: corev1.SchemeGroupVersion.Group, Version: corev1.SchemeGroupVersion.Version, Resource: "secrets", } - newSecret := func(secretData map[string][]byte) *corev1.Secret { - s := corev1.Secret{ - Type: symmetricKeySecretType, - ObjectMeta: metav1.ObjectMeta{ - Name: expectedSecretName, - Namespace: namespace, - Labels: map[string]string{ - "myLabelKey1": "myLabelValue1", - "myLabelKey2": "myLabelValue2", - }, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: opcGVR.GroupVersion().String(), - Kind: "OIDCProvider", - Name: goodOPC.Name, - UID: goodOPC.UID, - BlockOwnerDeletion: boolPtr(true), - Controller: boolPtr(true), - }, - }, - }, - Data: secretData, - } - - return &s + goodOP := &configv1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: opName, + Namespace: namespace, + UID: opUID, + }, } - secretData, err := fakeSecretDataFunc() - require.NoError(t, err) + goodSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + UID: secretUID, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: opGVR.GroupVersion().String(), + Kind: "OIDCProvider", + Name: opName, + UID: opUID, + BlockOwnerDeletion: boolPtr(true), + Controller: boolPtr(true), + }, + }, + Labels: map[string]string{ + "some-key-0": "some-value-0", + "some-key-1": "some-value-1", + }, + }, + Type: "some-secret-type", + Data: map[string][]byte{ + "some-key": []byte("some-value"), + }, + } - goodSecret := newSecret(secretData) + invalidSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: namespace, + UID: secretUID, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: opGVR.GroupVersion().String(), + Kind: "OIDCProvider", + Name: opName, + UID: opUID, + BlockOwnerDeletion: boolPtr(true), + Controller: boolPtr(true), + }, + }, + }, + } + + once := sync.Once{} tests := []struct { - name string - key controllerlib.Key - secrets []*corev1.Secret - configKubeClient func(*kubernetesfake.Clientset) - configPinnipedClient func(*pinnipedfake.Clientset) - opcs []*configv1alpha1.OIDCProvider - generateKeyErr error - wantGenerateKeyCount int - wantSecretActions []kubetesting.Action - wantOPCActions []kubetesting.Action - wantError string + name string + storage func(**configv1alpha1.OIDCProvider, **corev1.Secret) + client func(*kubernetesfake.Clientset) + secretHelper func(*mocksecrethelper.MockSecretHelper) + wantSecretActions []kubetesting.Action + wantOPActions []kubetesting.Action + wantError string }{ { - name: "new opc with no secret", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and secret does not exist", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *op = nil + *s = nil + }, + }, + { + name: "OIDCProvider exists and secret exists", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *op = nil + }, + }, + { + name: "OIDCProvider exists and secret does not exist", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = nil + }, + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1) }, - wantGenerateKeyCount: 1, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), + }, + { + name: "OIDCProvider exists and valid secret exists", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(true) + secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1) }, }, { - name: "opc without status with existing secret", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and invalid secret exists", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() }, - secrets: []*corev1.Secret{ - goodSecret, + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(2).Return(false) + secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1) }, - wantGenerateKeyCount: 1, - wantSecretActions: []kubetesting.Action{ - kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), - }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), - }, - }, - { - name: "existing opc with no secret", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, - }, - wantGenerateKeyCount: 1, - wantSecretActions: []kubetesting.Action{ - kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), - kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), - }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), - }, - }, - { - name: "existing opc with existing secret", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, - }, - secrets: []*corev1.Secret{ - goodSecret, - }, - }, - { - name: "deleted opc", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - // Nothing to do here since Kube will garbage collect our child secret via its OwnerReference. - }, - { - name: "secret data is empty", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, - }, - secrets: []*corev1.Secret{ - newSecret(map[string][]byte{}), - }, - wantGenerateKeyCount: 1, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), - }, }, { - name: fmt.Sprintf("secret missing key %s", symmetricKeySecretDataKey), - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and generating a secret fails", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(nil, errors.New("some generate error")) }, - secrets: []*corev1.Secret{ - newSecret(map[string][]byte{"badKey": []byte("some secret - must have at least 32 bytes")}), + wantError: "failed to generate secret: some generate error", + }, + { + name: "OIDCProvider exists and invalid secret exists and upon update we learn of a valid secret", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + otherSecret := goodSecret.DeepCopy() + otherSecret.UID = "other-secret-uid" + + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(otherSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(true) + secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1) }, - wantGenerateKeyCount: 1, wantSecretActions: []kubetesting.Action{ kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), - kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), - }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), }, }, { - name: fmt.Sprintf("secret data value for key %s", symmetricKeySecretDataKey), - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and invalid secret exists and get fails", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false) }, - secrets: []*corev1.Secret{ - newSecret(map[string][]byte{symmetricKeySecretDataKey: {}}), - }, - wantGenerateKeyCount: 1, - wantSecretActions: []kubetesting.Action{ - kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), - kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), - }, - wantOPCActions: []kubetesting.Action{ - kubetesting.NewGetAction(opcGVR, namespace, goodOPC.Name), - }, - }, - { - name: "generate key fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, - }, - generateKeyErr: errors.New("some generate error"), - wantError: "cannot generate secret: cannot generate key: some generate error", - }, - { - name: "get secret fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, - }, - configKubeClient: func(client *kubernetesfake.Clientset) { - client.PrependReactor("get", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + client: func(c *kubernetesfake.Clientset) { + c.PrependReactor("get", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { return true, nil, errors.New("some get error") }) }, - wantError: "cannot create or update secret: cannot get secret: some get error", + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + }, + wantError: fmt.Sprintf("failed to create or update secret: failed to get secret %s/%s: some get error", namespace, goodSecret.Name), }, { - name: "create secret fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and secret does not exist and create fails", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = nil }, - configKubeClient: func(client *kubernetesfake.Clientset) { - client.PrependReactor("create", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + }, + client: func(c *kubernetesfake.Clientset) { + c.PrependReactor("create", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { return true, nil, errors.New("some create error") }) }, - wantError: "cannot create or update secret: cannot create secret: some create error", + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), + }, + wantError: fmt.Sprintf("failed to create or update secret: failed to create secret %s/%s: some create error", namespace, goodSecret.Name), }, { - name: "update secret fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and invalid secret exists and update fails", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(2).Return(false) }, - secrets: []*corev1.Secret{ - newSecret(map[string][]byte{}), - }, - configKubeClient: func(client *kubernetesfake.Clientset) { - client.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + client: func(c *kubernetesfake.Clientset) { + c.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { return true, nil, errors.New("some update error") }) }, - wantError: "cannot create or update secret: some update error", + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + }, + wantError: "failed to create or update secret: some update error", }, { - name: "get opc fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + name: "OIDCProvider exists and invalid secret exists and update fails due to conflict", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() }, - configPinnipedClient: func(client *pinnipedfake.Clientset) { - client.PrependReactor("get", "oidcproviders", func(_ kubetesting.Action) (bool, runtime.Object, error) { - return true, nil, errors.New("some get error") + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, invalidSecret).Times(3).Return(false) + secretHelper.EXPECT().Notify(goodOP, goodSecret).Times(1) + }, + client: func(c *kubernetesfake.Clientset) { + c.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + var err error + once.Do(func() { err = k8serrors.NewConflict(secretGVR.GroupResource(), namespace, errors.New("some error")) }) + return true, nil, err }) }, - wantError: "cannot update opc: cannot get opc: some get error", - }, - { - name: "update opc fails", - key: controllerlib.Key{Namespace: goodOPC.Namespace, Name: goodOPC.Name}, - opcs: []*configv1alpha1.OIDCProvider{ - goodOPC, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), }, - configPinnipedClient: func(client *pinnipedfake.Clientset) { - client.PrependReactor("update", "oidcproviders", func(_ kubetesting.Action) (bool, runtime.Object, error) { - return true, nil, errors.New("some update error") - }) - }, - wantError: "cannot update opc: some update error", }, } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - // We shouldn't run this test in parallel since it messes with a global function (generateKey). - generateKeyCount := 0 - generateKey := func() (map[string][]byte, error) { - generateKeyCount++ - return map[string][]byte{ - symmetricKeySecretDataKey: []byte("some secret - must have at least 32 bytes"), - }, nil - } + t.Parallel() + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) defer cancel() + pinnipedInformerClient := pinnipedfake.NewSimpleClientset() + kubeAPIClient := kubernetesfake.NewSimpleClientset() kubeInformerClient := kubernetesfake.NewSimpleClientset() - for _, secret := range test.secrets { + + op := goodOP.DeepCopy() + secret := goodSecret.DeepCopy() + if test.storage != nil { + test.storage(&op, &secret) + } + if op != nil { + require.NoError(t, pinnipedInformerClient.Tracker().Add(op)) + } + if secret != nil { require.NoError(t, kubeAPIClient.Tracker().Add(secret)) require.NoError(t, kubeInformerClient.Tracker().Add(secret)) } - if test.configKubeClient != nil { - test.configKubeClient(kubeAPIClient) - } - pinnipedAPIClient := pinnipedfake.NewSimpleClientset() - pinnipedInformerClient := pinnipedfake.NewSimpleClientset() - for _, opc := range test.opcs { - require.NoError(t, pinnipedAPIClient.Tracker().Add(opc)) - require.NoError(t, pinnipedInformerClient.Tracker().Add(opc)) - } - if test.configPinnipedClient != nil { - test.configPinnipedClient(pinnipedAPIClient) + if test.client != nil { + test.client(kubeAPIClient) } kubeInformers := kubeinformers.NewSharedInformerFactory( @@ -536,15 +508,17 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) { 0, ) + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().Name().Times(1).Return("some-name") + if test.secretHelper != nil { + test.secretHelper(secretHelper) + } + c := NewOIDCProviderSecretsController( - secretNameFunc, - map[string]string{ - "myLabelKey1": "myLabelValue1", - "myLabelKey2": "myLabelValue2", - }, - generateKey, + secretHelper, kubeAPIClient, - pinnipedAPIClient, kubeInformers.Core().V1().Secrets(), pinnipedInformers.Config().V1alpha1().OIDCProviders(), controllerlib.WithInformer, @@ -557,7 +531,10 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) { err := controllerlib.TestSync(t, c, controllerlib.Context{ Context: ctx, - Key: test.key, + Key: controllerlib.Key{ + Namespace: namespace, + Name: opName, + }, }) if test.wantError != "" { require.EqualError(t, err, test.wantError) @@ -565,26 +542,12 @@ func TestNewOIDCProviderSecretsControllerSync(t *testing.T) { } require.NoError(t, err) - require.Equal(t, test.wantGenerateKeyCount, generateKeyCount) - - if test.wantSecretActions != nil { - require.Equal(t, test.wantSecretActions, kubeAPIClient.Actions()) - } - if test.wantOPCActions != nil { - require.Equal(t, test.wantOPCActions, pinnipedAPIClient.Actions()) + if test.wantSecretActions == nil { + test.wantSecretActions = []kubetesting.Action{} } + require.Equal(t, test.wantSecretActions, kubeAPIClient.Actions()) }) } } -func secretNameFunc(opc *configv1alpha1.OIDCProvider) string { - return fmt.Sprintf("pinniped-%s-%s-test_secret", opc.Kind, opc.UID) -} - -func fakeSecretDataFunc() (map[string][]byte, error) { - return map[string][]byte{ - symmetricKeySecretDataKey: []byte("some secret - must have at least 32 bytes"), - }, nil -} - func boolPtr(b bool) *bool { return &b } diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets.go b/internal/controller/supervisorconfig/generator/supervisor_secrets.go index 24d177d7..cc3a9d33 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets.go @@ -24,6 +24,7 @@ import ( "go.pinniped.dev/internal/plog" ) +// TODO: de-dup me when we abstract these controllers. const ( symmetricKeySecretType = "secrets.pinniped.dev/symmetric" symmetricKeySecretDataKey = "key" diff --git a/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper.go b/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper.go new file mode 100644 index 00000000..f7baecf3 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper.go @@ -0,0 +1,110 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package symmetricsecrethelper provides a type that can generate and validate symmetric keys as +// Secret's. +package symmetricsecrethelper + +import ( + "fmt" + "io" + + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/internal/controller/supervisorconfig/generator" +) + +const ( + // SecretType is corev1.Secret.Type of all corev1.Secret's generated by this helper. + SecretType = "secrets.pinniped.dev/symmetric" + // SecretDataKey is the corev1.Secret.Data key for the symmetric key value generated by this helper. + SecretDataKey = "key" + + // keySize is the default length, in bytes, of generated keys. It is set to 32 since this + // seems like reasonable entropy for our keys, and a 32-byte key will allow for AES-256 + // to be used in our codecs (see dynamiccodec.Codec). + keySize = 32 +) + +type helper struct { + namePrefix string + labels map[string]string + rand io.Reader + notifyFunc func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) +} + +var _ generator.SecretHelper = &helper{} + +// New returns a SecretHelper that has been parameterized with common symmetric secret generation +// knobs. +func New( + namePrefix string, + labels map[string]string, + rand io.Reader, + notifyFunc func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret), +) generator.SecretHelper { + return &helper{ + namePrefix: namePrefix, + labels: labels, + rand: rand, + notifyFunc: notifyFunc, + } +} + +func (s *helper) Name() string { return s.namePrefix } + +// Generate implements SecretHelper.Generate(). +func (s *helper) Generate(parent *configv1alpha1.OIDCProvider) (*corev1.Secret, error) { + key := make([]byte, keySize) + if _, err := s.rand.Read(key); err != nil { + return nil, err + } + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: fmt.Sprintf("%s%s", s.namePrefix, parent.UID), + Namespace: parent.Namespace, + Labels: s.labels, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(parent, schema.GroupVersionKind{ + Group: configv1alpha1.SchemeGroupVersion.Group, + Version: configv1alpha1.SchemeGroupVersion.Version, + Kind: "OIDCProvider", + }), + }, + }, + Type: SecretType, + Data: map[string][]byte{ + SecretDataKey: key, + }, + }, nil +} + +// IsValid implements SecretHelper.IsValid(). +func (s *helper) IsValid(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) bool { + if !metav1.IsControlledBy(child, parent) { + return false + } + + if child.Type != SecretType { + return false + } + + key, ok := child.Data[SecretDataKey] + if !ok { + return false + } + if len(key) != keySize { + return false + } + + return true +} + +// Notify implements SecretHelper.Notify(). +func (s *helper) Notify(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) { + s.notifyFunc(parent, child) +} diff --git a/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper_test.go b/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper_test.go new file mode 100644 index 00000000..b0787a0d --- /dev/null +++ b/internal/controller/supervisorconfig/generator/symmetricsecrethelper/symmetric_secret_helper_test.go @@ -0,0 +1,154 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package symmetricsecrethelper + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "k8s.io/apimachinery/pkg/runtime/schema" + + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" +) + +const keyWith32Bytes = "0123456789abcdef0123456789abcdef" + +func TestHelper(t *testing.T) { + labels := map[string]string{ + "some-label-key-1": "some-label-value-1", + "some-label-key-2": "some-label-value-2", + } + randSource := strings.NewReader(keyWith32Bytes) + var notifyParent *configv1alpha1.OIDCProvider + var notifyChild *corev1.Secret + h := New("some-name-prefix-", labels, randSource, func(parent *configv1alpha1.OIDCProvider, child *corev1.Secret) { + require.True(t, notifyParent == nil && notifyChild == nil, "expected notify func not to have been called yet") + notifyParent = parent + notifyChild = child + }) + + parent := &configv1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + UID: "some-uid", + Namespace: "some-namespace", + }, + } + child, err := h.Generate(parent) + require.NoError(t, err) + require.Equal(t, child, &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-name-prefix-some-uid", + Namespace: "some-namespace", + Labels: labels, + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(parent, schema.GroupVersionKind{ + Group: configv1alpha1.SchemeGroupVersion.Group, + Version: configv1alpha1.SchemeGroupVersion.Version, + Kind: "OIDCProvider", + }), + }, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": []byte(keyWith32Bytes), + }, + }) + + require.True(t, h.IsValid(parent, child)) + + h.Notify(parent, child) + require.Equal(t, parent, notifyParent) + require.Equal(t, child, notifyChild) +} + +func TestHelperIsValid(t *testing.T) { + tests := []struct { + name string + child func(*corev1.Secret) + parent func(*configv1alpha1.OIDCProvider) + want bool + }{ + { + name: "wrong type", + child: func(s *corev1.Secret) { + s.Type = "wrong" + }, + want: false, + }, + { + name: "empty type", + child: func(s *corev1.Secret) { + s.Type = "" + }, + want: false, + }, + { + name: "data key is too short", + child: func(s *corev1.Secret) { + s.Data["key"] = []byte("short") + }, + want: false, + }, + { + name: "data key does not exist", + child: func(s *corev1.Secret) { + delete(s.Data, "key") + }, + want: false, + }, + { + name: "child not owned by parent", + parent: func(op *configv1alpha1.OIDCProvider) { + op.UID = "wrong" + }, + want: false, + }, + { + name: "happy path", + want: true, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + h := New("none of these args matter", nil, nil, nil) + + parent := &configv1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-parent-name", + Namespace: "some-namespace", + UID: "some-parent-uid", + }, + } + child := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-name-prefix-some-uid", + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(parent, schema.GroupVersionKind{ + Group: configv1alpha1.SchemeGroupVersion.Group, + Version: configv1alpha1.SchemeGroupVersion.Version, + Kind: "OIDCProvider", + }), + }, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": []byte(keyWith32Bytes), + }, + } + if test.child != nil { + test.child(child) + } + if test.parent != nil { + test.parent(parent) + } + + require.Equalf(t, test.want, h.IsValid(parent, child), "child: %#v", child) + }) + } +} diff --git a/internal/mocks/mocksecrethelper/generate.go b/internal/mocks/mocksecrethelper/generate.go new file mode 100644 index 00000000..31d52cb4 --- /dev/null +++ b/internal/mocks/mocksecrethelper/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mocksecrethelper + +//go:generate go run -v github.com/golang/mock/mockgen -destination=mocksecrethelper.go -package=mocksecrethelper -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/controller/supervisorconfig/generator SecretHelper diff --git a/internal/mocks/mocksecrethelper/mocksecrethelper.go b/internal/mocks/mocksecrethelper/mocksecrethelper.go new file mode 100644 index 00000000..68b185f3 --- /dev/null +++ b/internal/mocks/mocksecrethelper/mocksecrethelper.go @@ -0,0 +1,94 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 +// + +// Code generated by MockGen. DO NOT EDIT. +// Source: go.pinniped.dev/internal/controller/supervisorconfig/generator (interfaces: SecretHelper) + +// Package mocksecrethelper is a generated GoMock package. +package mocksecrethelper + +import ( + gomock "github.com/golang/mock/gomock" + v1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" + v1 "k8s.io/api/core/v1" + reflect "reflect" +) + +// MockSecretHelper is a mock of SecretHelper interface +type MockSecretHelper struct { + ctrl *gomock.Controller + recorder *MockSecretHelperMockRecorder +} + +// MockSecretHelperMockRecorder is the mock recorder for MockSecretHelper +type MockSecretHelperMockRecorder struct { + mock *MockSecretHelper +} + +// NewMockSecretHelper creates a new mock instance +func NewMockSecretHelper(ctrl *gomock.Controller) *MockSecretHelper { + mock := &MockSecretHelper{ctrl: ctrl} + mock.recorder = &MockSecretHelperMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use +func (m *MockSecretHelper) EXPECT() *MockSecretHelperMockRecorder { + return m.recorder +} + +// Generate mocks base method +func (m *MockSecretHelper) Generate(arg0 *v1alpha1.OIDCProvider) (*v1.Secret, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Generate", arg0) + ret0, _ := ret[0].(*v1.Secret) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// Generate indicates an expected call of Generate +func (mr *MockSecretHelperMockRecorder) Generate(arg0 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Generate", reflect.TypeOf((*MockSecretHelper)(nil).Generate), arg0) +} + +// IsValid mocks base method +func (m *MockSecretHelper) IsValid(arg0 *v1alpha1.OIDCProvider, arg1 *v1.Secret) bool { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "IsValid", arg0, arg1) + ret0, _ := ret[0].(bool) + return ret0 +} + +// IsValid indicates an expected call of IsValid +func (mr *MockSecretHelperMockRecorder) IsValid(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "IsValid", reflect.TypeOf((*MockSecretHelper)(nil).IsValid), arg0, arg1) +} + +// Name mocks base method +func (m *MockSecretHelper) Name() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "Name") + ret0, _ := ret[0].(string) + return ret0 +} + +// Name indicates an expected call of Name +func (mr *MockSecretHelperMockRecorder) Name() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Name", reflect.TypeOf((*MockSecretHelper)(nil).Name)) +} + +// Notify mocks base method +func (m *MockSecretHelper) Notify(arg0 *v1alpha1.OIDCProvider, arg1 *v1.Secret) { + m.ctrl.T.Helper() + m.ctrl.Call(m, "Notify", arg0, arg1) +} + +// Notify indicates an expected call of Notify +func (mr *MockSecretHelperMockRecorder) Notify(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "Notify", reflect.TypeOf((*MockSecretHelper)(nil).Notify), arg0, arg1) +} diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 6043ba7c..d16ed682 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -86,14 +86,6 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { for _, incomingProvider := range oidcProviders { providerCache := m.cache.GetOIDCProviderCacheFor(incomingProvider.Issuer()) - if providerCache == nil { // TODO remove when populated from `Secret` values - providerCache = &secret.OIDCProviderCache{} - providerCache.SetTokenHMACKey([]byte("some secret - must have at least 32 bytes")) // TODO fetch from `Secret` - providerCache.SetStateEncoderHashKey([]byte("fake-state-hash-secret")) // TODO fetch from `Secret` - providerCache.SetStateEncoderBlockKey([]byte("16-bytes-STATE01")) // TODO fetch from `Secret` - m.cache.SetOIDCProviderCacheFor(incomingProvider.Issuer(), providerCache) - } - issuer := incomingProvider.Issuer() issuerHostWithPath := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() oidcTimeouts := oidc.DefaultOIDCTimeoutsConfiguration() diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 0a59abb0..78b2cfd2 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -246,6 +246,16 @@ func TestManager(t *testing.T) { cache := secret.Cache{} cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) + oidcProvider1Cache := cache.GetOIDCProviderCacheFor(issuer1) + oidcProvider1Cache.SetStateEncoderHashKey([]byte("some-state-encoder-hash-key-1")) + oidcProvider1Cache.SetStateEncoderBlockKey([]byte("16-bytes-STATE01")) + oidcProvider1Cache.SetTokenHMACKey([]byte("some secret 1 - must have at least 32 bytes")) + + oidcProvider2Cache := cache.GetOIDCProviderCacheFor(issuer2) + oidcProvider2Cache.SetStateEncoderHashKey([]byte("some-state-encoder-hash-key-2")) + oidcProvider2Cache.SetStateEncoderBlockKey([]byte("16-bytes-STATE02")) + oidcProvider2Cache.SetTokenHMACKey([]byte("some secret 2 - must have at least 32 bytes")) + subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient) }) @@ -309,21 +319,26 @@ func TestManager(t *testing.T) { requireAuthorizationRequestToBeHandled(issuer2, authRequestParams, upstreamIDPAuthorizationURL) // Hostnames are case-insensitive, so test that we can handle that. - requireAuthorizationRequestToBeHandled(issuer1DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL) - csrfCookieValue, upstreamStateParam := + csrfCookieValue1, upstreamStateParam1 := + requireAuthorizationRequestToBeHandled(issuer1DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL) + csrfCookieValue2, upstreamStateParam2 := requireAuthorizationRequestToBeHandled(issuer2DifferentCaseHostname, authRequestParams, upstreamIDPAuthorizationURL) - callbackRequestParams := "?" + url.Values{ + callbackRequestParams1 := "?" + url.Values{ "code": []string{"some-fake-code"}, - "state": []string{upstreamStateParam}, + "state": []string{upstreamStateParam1}, + }.Encode() + callbackRequestParams2 := "?" + url.Values{ + "code": []string{"some-fake-code"}, + "state": []string{upstreamStateParam2}, }.Encode() - downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams, csrfCookieValue) - downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams, csrfCookieValue) + downstreamAuthCode1 := requireCallbackRequestToBeHandled(issuer1, callbackRequestParams1, csrfCookieValue1) + downstreamAuthCode2 := requireCallbackRequestToBeHandled(issuer2, callbackRequestParams2, csrfCookieValue2) // Hostnames are case-insensitive, so test that we can handle that. - downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams, csrfCookieValue) - downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams, csrfCookieValue) + downstreamAuthCode3 := requireCallbackRequestToBeHandled(issuer1DifferentCaseHostname, callbackRequestParams1, csrfCookieValue1) + downstreamAuthCode4 := requireCallbackRequestToBeHandled(issuer2DifferentCaseHostname, callbackRequestParams2, csrfCookieValue2) requireTokenRequestToBeHandled(issuer1, downstreamAuthCode1, issuer1JWKS, issuer1) requireTokenRequestToBeHandled(issuer2, downstreamAuthCode2, issuer2JWKS, issuer2) diff --git a/internal/secret/cache.go b/internal/secret/cache.go index 96ccae4a..e8b39965 100644 --- a/internal/secret/cache.go +++ b/internal/secret/cache.go @@ -3,6 +3,9 @@ package secret +// TODO: synchronize me. +// TODO: use SetIssuerXXX() functions instead of returning a struct so that we don't have to worry about reentrancy. + type Cache struct { csrfCookieEncoderHashKey []byte csrfCookieEncoderBlockKey []byte @@ -26,7 +29,12 @@ func (c *Cache) SetCSRFCookieEncoderBlockKey(key []byte) { } func (c *Cache) GetOIDCProviderCacheFor(oidcIssuer string) *OIDCProviderCache { - return c.oidcProviderCaches()[oidcIssuer] + oidcProvider, ok := c.oidcProviderCaches()[oidcIssuer] + if !ok { + oidcProvider = &OIDCProviderCache{} + c.oidcProviderCaches()[oidcIssuer] = oidcProvider + } + return oidcProvider } func (c *Cache) SetOIDCProviderCacheFor(oidcIssuer string, oidcProviderCache *OIDCProviderCache) {