From e17bc31b29de28d232895863030759d6cf887759 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 11 Dec 2020 11:11:49 -0500 Subject: [PATCH] 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 --- cmd/pinniped-supervisor/main.go | 71 ++++++++++++-- deploy/supervisor/deployment.yaml | 3 + deploy/supervisor/rbac.yaml | 8 ++ .../secretgenerator/secretgenerator.go | 65 ++++++++----- .../secretgenerator/secretgenerator_test.go | 93 ++++++++++++++----- internal/oidc/provider/manager/manager.go | 4 +- .../oidc/provider/manager/manager_test.go | 2 +- 7 files changed, 190 insertions(+), 56 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 8e61cc38..e5b5a6b4 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/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{} - - cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) // TODO fetch from `Secret` + secretCache := secret.Cache{} // 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) } } diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index 28b8d93f..1e0c75c0 100644 --- a/deploy/supervisor/deployment.yaml +++ b/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: diff --git a/deploy/supervisor/rbac.yaml b/deploy/supervisor/rbac.yaml index 33e86585..b66c00fc 100644 --- a/deploy/supervisor/rbac.yaml +++ b/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 diff --git a/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go b/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go index 89df14e1..5a767ce0 100644 --- a/internal/controller/supervisorconfig/secretgenerator/secretgenerator.go +++ b/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, - Namespace: namespace, + 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 { - secrets := c.client.CoreV1().Secrets(newSecret.Namespace) +func (c *controller) updateSecret(ctx context.Context, newSecret **corev1.Secret, secretName string) error { + 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.Data = newSecret.Data + currentSecret.Type = (*newSecret).Type + 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 -} diff --git a/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go b/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go index 884e2049..f753fcf8 100644 --- a/internal/controller/supervisorconfig/secretgenerator/secretgenerator_test.go +++ b/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" @@ -25,9 +26,9 @@ import ( func TestController(t *testing.T) { const ( - generatedSecretNamespace = "some-namespace" - generatedSecretNamePrefix = "some-name-" - generatedSecretName = "some-name-abc123" + generatedSecretNamespace = "some-namespace" + generatedSecretName = "some-name-abc123" + otherGeneratedSecretName = "some-other-name-abc123" ) var ( @@ -37,32 +38,60 @@ func TestController(t *testing.T) { Resource: "secrets", } - generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") + 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, - Namespace: generatedSecretNamespace, + 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() - generatedSecretWithName.Name = generatedSecretName + otherGeneratedSecret = &corev1.Secret{ + 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{} tests := []struct { - name string - storedSecret func(**corev1.Secret) - generateKey func() ([]byte, error) - apiClient func(*testing.T, *kubernetesfake.Clientset) - wantError string - wantActions []kubetesting.Action + name string + storedSecret func(**corev1.Secret) + generateKey func() ([]byte, error) + 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", + 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) }) } } diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index ad33b408..6043ba7c 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/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{ diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 997f5f61..0a59abb0 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/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() {