Pass CSRF cookie signing key from controller to cache

This also sets the CSRF cookie Secret's OwnerReference to the Pod's grandparent
Deployment so that when the Deployment is cleaned up, then the Secret is as
well.

Obviously this controller implementation has a lot of issues, but it will at
least get us started.

Signed-off-by: Andrew Keesler <akeesler@vmware.com>

cmd/pinniped-supervisor/main.go

@@ -16,6 +16,8 @@ import (
 
 	"go.pinniped.dev/internal/secret"
 
+	appsv1 "k8s.io/api/apps/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/util/clock"
 	kubeinformers "k8s.io/client-go/informers"
 	"k8s.io/client-go/kubernetes"
@@ -30,6 +32,7 @@ import (
 	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/secretgenerator"
 	"go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher"
 	"go.pinniped.dev/internal/controllerlib"
 	"go.pinniped.dev/internal/downward"
@@ -78,6 +81,8 @@ func startControllers(
 	dynamicJWKSProvider jwks.DynamicJWKSProvider,
 	dynamicTLSCertProvider provider.DynamicTLSCertProvider,
 	dynamicUpstreamIDPProvider provider.DynamicUpstreamIDPProvider,
+	secretCache *secret.Cache,
+	supervisorDeployment *appsv1.Deployment,
 	kubeClient kubernetes.Interface,
 	pinnipedClient pinnipedclientset.Interface,
 	kubeInformers kubeinformers.SharedInformerFactory,
@@ -126,6 +131,18 @@ func startControllers(
 			),
 			singletonWorker,
 		).
+		WithController(
+			secretgenerator.New(
+				supervisorDeployment,
+				kubeClient,
+				kubeInformers.Core().V1().Secrets(),
+				func(secret []byte) {
+					plog.Debug("setting csrf cookie secret")
+					secretCache.SetCSRFCookieEncoderHashKey(secret)
+				},
+			),
+			singletonWorker,
+		).
 		WithController(
 			upstreamwatcher.New(
 				dynamicUpstreamIDPProvider,
@@ -145,6 +162,41 @@ func startControllers(
 	go controllerManager.Start(ctx)
 }
 
+func getSupervisorDeployment(
+	ctx context.Context,
+	kubeClient kubernetes.Interface,
+	podInfo *downward.PodInfo,
+) (*appsv1.Deployment, error) {
+	ns := podInfo.Namespace
+
+	pod, err := kubeClient.CoreV1().Pods(ns).Get(ctx, podInfo.Name, metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("could not get pod: %w", err)
+	}
+
+	podOwner := metav1.GetControllerOf(pod)
+	if podOwner == nil {
+		return nil, fmt.Errorf("pod %s/%s is missing owner", ns, podInfo.Name)
+	}
+
+	rs, err := kubeClient.AppsV1().ReplicaSets(ns).Get(ctx, podOwner.Name, metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("could not get replicaset: %w", err)
+	}
+
+	rsOwner := metav1.GetControllerOf(rs)
+	if rsOwner == nil {
+		return nil, fmt.Errorf("replicaset %s/%s is missing owner", ns, podInfo.Name)
+	}
+
+	d, err := kubeClient.AppsV1().Deployments(ns).Get(ctx, rsOwner.Name, metav1.GetOptions{})
+	if err != nil {
+		return nil, fmt.Errorf("could not get deployment: %w", err)
+	}
+
+	return d, nil
+}
+
 func newClients() (kubernetes.Interface, pinnipedclientset.Interface, error) {
 	kubeConfig, err := restclient.InClusterConfig()
 	if err != nil {
@@ -166,7 +218,9 @@ func newClients() (kubernetes.Interface, pinnipedclientset.Interface, error) {
 	return kubeClient, pinnipedClient, nil
 }
 
-func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
+func run(podInfo *downward.PodInfo, cfg *supervisor.Config) error {
+	serverInstallationNamespace := podInfo.Namespace
+
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
@@ -196,19 +250,22 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
 	dynamicJWKSProvider := jwks.NewDynamicJWKSProvider()
 	dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider()
 	dynamicUpstreamIDPProvider := provider.NewDynamicUpstreamIDPProvider()
-	cache := secret.Cache{}
+	secretCache := secret.Cache{}
-
-	cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) // TODO fetch from `Secret`
 
 	// OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux.
 	oidProvidersManager := manager.NewManager(
 		healthMux,
 		dynamicJWKSProvider,
 		dynamicUpstreamIDPProvider,
-		cache,
+		&secretCache,
 		kubeClient.CoreV1().Secrets(serverInstallationNamespace),
 	)
 
+	supervisorDeployment, err := getSupervisorDeployment(ctx, kubeClient, podInfo)
+	if err != nil {
+		return fmt.Errorf("cannot get supervisor deployment: %w", err)
+	}
+
 	startControllers(
 		ctx,
 		cfg,
@@ -216,6 +273,8 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error {
 		dynamicJWKSProvider,
 		dynamicTLSCertProvider,
 		dynamicUpstreamIDPProvider,
+		&secretCache,
+		supervisorDeployment,
 		kubeClient,
 		pinnipedClient,
 		kubeInformers,
@@ -284,7 +343,7 @@ func main() {
 		klog.Fatal(fmt.Errorf("could not load config: %w", err))
 	}
 
-	if err := run(podInfo.Namespace, cfg); err != nil {
+	if err := run(podInfo, cfg); err != nil {
 		klog.Fatal(err)
 	}
 }

deploy/supervisor/deployment.yaml

@@ -132,6 +132,9 @@ spec:
               - path: "namespace"
                 fieldRef:
                   fieldPath: metadata.namespace
+              - path: "name"
+                fieldRef:
+                  fieldPath: metadata.name
       #! This will help make sure our multiple pods run on different nodes, making
       #! our deployment "more" "HA".
       affinity:

deploy/supervisor/rbac.yaml

@@ -25,6 +25,14 @@ rules:
   - apiGroups: [idp.supervisor.pinniped.dev]
     resources: [upstreamoidcproviders/status]
     verbs: [get, patch, update]
+    #! We want to be able to read pods/replicasets/deployment so we can learn who our deployment is to set
+    #! as an owner reference.
+  - apiGroups: [""]
+    resources: [pods]
+    verbs: [get]
+  - apiGroups: [apps]
+    resources: [replicasets,deployments]
+    verbs: [get]
 ---
 kind: RoleBinding
 apiVersion: rbac.authorization.k8s.io/v1

internal/controller/supervisorconfig/secretgenerator/secretgenerator.go

@@ -9,9 +9,11 @@ import (
 	"crypto/rand"
 	"fmt"
 
+	appsv1 "k8s.io/api/apps/v1"
 	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/schema"
 	corev1informers "k8s.io/client-go/informers/core/v1"
 	"k8s.io/client-go/kubernetes"
 	"k8s.io/client-go/util/retry"
@@ -42,22 +44,35 @@ func generateSymmetricKey() ([]byte, error) {
 }
 
 type controller struct {
-	secretNamePrefix string
+	owner            *appsv1.Deployment
 	client           kubernetes.Interface
 	secrets          corev1informers.SecretInformer
+	onCreateOrUpdate func(secret []byte)
 }
 
 // New instantiates a new controllerlib.Controller which will ensure existence of a generated secret.
-func New(secretNamePrefix string, client kubernetes.Interface, secrets corev1informers.SecretInformer) controllerlib.Controller {
+func New(
+	owner *appsv1.Deployment,
+	client kubernetes.Interface,
+	secrets corev1informers.SecretInformer,
+	onCreateOrUpdate func(secret []byte),
+) controllerlib.Controller {
 	c := controller{
-		secretNamePrefix: secretNamePrefix,
+		owner:            owner,
 		client:           client,
 		secrets:          secrets,
+		onCreateOrUpdate: onCreateOrUpdate,
 	}
-	filter := pinnipedcontroller.SimpleFilterWithSingletonQueue(isOwnee)
+	filter := pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool {
+		return metav1.IsControlledBy(obj, owner)
+	}, nil)
 	return controllerlib.New(
-		controllerlib.Config{Name: secretNamePrefix + "-secrets-generator", Syncer: &c},
+		controllerlib.Config{Name: owner.Name + "-secret-generator", Syncer: &c},
 		controllerlib.WithInformer(secrets, filter, controllerlib.InformerOption{}),
+		controllerlib.WithInitialEvent(controllerlib.Key{
+			Namespace: owner.Namespace,
+			Name:      owner.Name + "-keys",
+		}),
 	)
 }
 
@@ -72,10 +87,11 @@ func (c *controller) Sync(ctx controllerlib.Context) error {
 	secretNeedsUpdate := isNotFound || !c.isValid(secret)
 	if !secretNeedsUpdate {
 		plog.Debug("secret is up to date", "secret", klog.KObj(secret))
+		c.onCreateOrUpdate(secret.Data[symmetricKeySecretDataKey])
 		return nil
 	}
 
-	newSecret, err := c.generateSecret(ctx.Key.Namespace)
+	newSecret, err := c.generateSecret(ctx.Key.Namespace, ctx.Key.Name)
 	if err != nil {
 		return fmt.Errorf("failed to generate secret: %w", err)
 	}
@@ -83,12 +99,14 @@ func (c *controller) Sync(ctx controllerlib.Context) error {
 	if isNotFound {
 		err = c.createSecret(ctx.Context, newSecret)
 	} else {
-		err = c.updateSecret(ctx.Context, newSecret, ctx.Key.Name)
+		err = c.updateSecret(ctx.Context, &newSecret, ctx.Key.Name)
 	}
 	if err != nil {
-		return fmt.Errorf("failed to create/update secret %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err)
+		return fmt.Errorf("failed to create/update secret %s/%s: %w", newSecret.Namespace, newSecret.Name, err)
 	}
 
+	c.onCreateOrUpdate(newSecret.Data[symmetricKeySecretDataKey])
+
 	return nil
 }
 
@@ -108,16 +126,24 @@ func (c *controller) isValid(secret *corev1.Secret) bool {
 	return true
 }
 
-func (c *controller) generateSecret(namespace string) (*corev1.Secret, error) {
+func (c *controller) generateSecret(namespace, name string) (*corev1.Secret, error) {
 	symmetricKey, err := generateKey()
 	if err != nil {
 		return nil, err
 	}
 
+	deploymentGVK := schema.GroupVersionKind{
+		Group:   appsv1.SchemeGroupVersion.Group,
+		Version: appsv1.SchemeGroupVersion.Version,
+		Kind:    "Deployment",
+	}
 	return &corev1.Secret{
 		ObjectMeta: metav1.ObjectMeta{
-			GenerateName: c.secretNamePrefix,
+			Name:      name,
 			Namespace: namespace,
+			OwnerReferences: []metav1.OwnerReference{
+				*metav1.NewControllerRef(c.owner, deploymentGVK),
+			},
 		},
 		Type: symmetricKeySecretType,
 		Data: map[string][]byte{
@@ -131,8 +157,8 @@ func (c *controller) createSecret(ctx context.Context, newSecret *corev1.Secret)
 	return err
 }
 
-func (c *controller) updateSecret(ctx context.Context, newSecret *corev1.Secret, secretName string) error {
+func (c *controller) updateSecret(ctx context.Context, newSecret **corev1.Secret, secretName string) error {
-	secrets := c.client.CoreV1().Secrets(newSecret.Namespace)
+	secrets := c.client.CoreV1().Secrets((*newSecret).Namespace)
 	return retry.RetryOnConflict(retry.DefaultBackoff, func() error {
 		currentSecret, err := secrets.Get(ctx, secretName, metav1.GetOptions{})
 		isNotFound := k8serrors.IsNotFound(err)
@@ -141,26 +167,21 @@ func (c *controller) updateSecret(ctx context.Context, newSecret *corev1.Secret,
 		}
 
 		if isNotFound {
-			if err := c.createSecret(ctx, newSecret); err != nil {
+			if err := c.createSecret(ctx, *newSecret); err != nil {
 				return fmt.Errorf("failed to create secret: %w", err)
 			}
 			return nil
 		}
 
 		if c.isValid(currentSecret) {
+			*newSecret = currentSecret
 			return nil
 		}
 
-		currentSecret.Type = newSecret.Type
+		currentSecret.Type = (*newSecret).Type
-		currentSecret.Data = newSecret.Data
+		currentSecret.Data = (*newSecret).Data
 
 		_, err = secrets.Update(ctx, currentSecret, metav1.UpdateOptions{})
 		return err
 	})
 }
-
-// isOwnee returns whether the provided obj is owned by this controller.
-func isOwnee(obj metav1.Object) bool {
-	// TODO: how do we say we are owned by our Deployment?
-	return true
-}

internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go

@@ -11,6 +11,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	k8serrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
@@ -26,8 +27,8 @@ import (
 func TestController(t *testing.T) {
 	const (
 		generatedSecretNamespace = "some-namespace"
-		generatedSecretNamePrefix = "some-name-"
 		generatedSecretName      = "some-name-abc123"
+		otherGeneratedSecretName = "some-other-name-abc123"
 	)
 
 	var (
@@ -37,22 +38,49 @@ func TestController(t *testing.T) {
 			Resource: "secrets",
 		}
 
+		owner = &appsv1.Deployment{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "some-owner-name",
+				UID:  "some-owner-uid",
+			},
+		}
+		ownerGVK = schema.GroupVersionKind{
+			Group:   appsv1.SchemeGroupVersion.Group,
+			Version: appsv1.SchemeGroupVersion.Version,
+			Kind:    "Deployment",
+		}
+
 		generatedSymmetricKey      = []byte("some-neato-32-byte-generated-key")
+		otherGeneratedSymmetricKey = []byte("some-funio-32-byte-generated-key")
 
 		generatedSecret = &corev1.Secret{
 			ObjectMeta: metav1.ObjectMeta{
-				GenerateName: generatedSecretNamePrefix,
+				Name:      generatedSecretName,
 				Namespace: generatedSecretNamespace,
+				OwnerReferences: []metav1.OwnerReference{
+					*metav1.NewControllerRef(owner, ownerGVK),
+				},
 			},
 			Type: "secrets.pinniped.dev/symmetric",
 			Data: map[string][]byte{
 				"key": generatedSymmetricKey,
 			},
 		}
-	)
 
-	generatedSecretWithName := generatedSecret.DeepCopy()
+		otherGeneratedSecret = &corev1.Secret{
-	generatedSecretWithName.Name = generatedSecretName
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      generatedSecretName,
+				Namespace: generatedSecretNamespace,
+				OwnerReferences: []metav1.OwnerReference{
+					*metav1.NewControllerRef(owner, ownerGVK),
+				},
+			},
+			Type: "secrets.pinniped.dev/symmetric",
+			Data: map[string][]byte{
+				"key": otherGeneratedSymmetricKey,
+			},
+		}
+	)
 
 	once := sync.Once{}
 
@@ -63,6 +91,7 @@ func TestController(t *testing.T) {
 		apiClient          func(*testing.T, *kubernetesfake.Clientset)
 		wantError          string
 		wantActions        []kubetesting.Action
+		wantCallbackSecret []byte
 	}{
 		{
 			name: "when the secrets does not exist, it gets generated",
@@ -72,9 +101,11 @@ func TestController(t *testing.T) {
 			wantActions: []kubetesting.Action{
 				kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name:               "when a valid secret exists, nothing happens",
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "secret gets updated when the type is wrong",
@@ -83,8 +114,9 @@ func TestController(t *testing.T) {
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "secret gets updated when the key data does not exist",
@@ -93,8 +125,9 @@ func TestController(t *testing.T) {
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "secret gets updated when the key data is too short",
@@ -103,8 +136,9 @@ func TestController(t *testing.T) {
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "an error is returned when creating fails",
@@ -133,7 +167,7 @@ func TestController(t *testing.T) {
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
 			wantError: "failed to create/update secret some-namespace/some-name-abc123: some update error",
 		},
@@ -168,10 +202,11 @@ func TestController(t *testing.T) {
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
-				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecretWithName),
+				kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "upon updating we discover that a valid secret exists",
@@ -180,12 +215,13 @@ func TestController(t *testing.T) {
 			},
 			apiClient: func(t *testing.T, client *kubernetesfake.Clientset) {
 				client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) {
-					return true, generatedSecretWithName, nil
+					return true, otherGeneratedSecret, nil
 				})
 			},
 			wantActions: []kubetesting.Action{
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
 			},
+			wantCallbackSecret: otherGeneratedSymmetricKey,
 		},
 		{
 			name: "upon updating we discover that the secret has been deleted",
@@ -204,6 +240,7 @@ func TestController(t *testing.T) {
 				kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName),
 				kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret),
 			},
+			wantCallbackSecret: generatedSymmetricKey,
 		},
 		{
 			name: "upon updating we discover that the secret has been deleted and our create fails",
@@ -257,7 +294,7 @@ func TestController(t *testing.T) {
 			}
 			informerClient := kubernetesfake.NewSimpleClientset()
 
-			storedSecret := generatedSecretWithName.DeepCopy()
+			storedSecret := generatedSecret.DeepCopy()
 			if test.storedSecret != nil {
 				test.storedSecret(&storedSecret)
 			}
@@ -269,7 +306,11 @@ func TestController(t *testing.T) {
 			informers := kubeinformers.NewSharedInformerFactory(informerClient, 0)
 			secrets := informers.Core().V1().Secrets()
 
-			c := New(generatedSecretNamePrefix, apiClient, secrets)
+			var callbackSecret []byte
+			c := New(owner, apiClient, secrets, func(secret []byte) {
+				require.Nil(t, callbackSecret, "callback was called twice")
+				callbackSecret = secret
+			})
 
 			// Must start informers before calling TestRunSynchronously().
 			informers.Start(ctx.Done())
@@ -292,6 +333,8 @@ func TestController(t *testing.T) {
 				test.wantActions = []kubetesting.Action{}
 			}
 			require.Equal(t, test.wantActions, apiClient.Actions())
+
+			require.Equal(t, test.wantCallbackSecret, callbackSecret)
 		})
 	}
 }

internal/oidc/provider/manager/manager.go

@@ -37,7 +37,7 @@ type Manager struct {
 	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
 	idpListGetter       oidc.IDPListGetter       // in-memory cache of upstream IDPs
-	cache               secret.Cache             // in-memory cache of cryptographic material
+	cache               *secret.Cache            // in-memory cache of cryptographic material
 	secretsClient       corev1client.SecretInterface
 }
 
@@ -49,7 +49,7 @@ func NewManager(
 	nextHandler http.Handler,
 	dynamicJWKSProvider jwks.DynamicJWKSProvider,
 	idpListGetter oidc.IDPListGetter,
-	cache secret.Cache,
+	cache *secret.Cache,
 	secretsClient corev1client.SecretInterface,
 ) *Manager {
 	return &Manager{

internal/oidc/provider/manager/manager_test.go

@@ -246,7 +246,7 @@ func TestManager(t *testing.T) {
 			cache := secret.Cache{}
 			cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret"))
 
-			subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, cache, secretsClient)
+			subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient)
 		})
 
 		when("given no providers via SetProviders()", func() {