diff --git a/internal/controller/supervisorconfig/generator/federation_domain_secrets.go b/internal/controller/supervisorconfig/generator/federation_domain_secrets.go index e9e169d1..879e488a 100644 --- a/internal/controller/supervisorconfig/generator/federation_domain_secrets.go +++ b/internal/controller/supervisorconfig/generator/federation_domain_secrets.go @@ -24,6 +24,10 @@ import ( "go.pinniped.dev/internal/plog" ) +const ( + federationDomainKind = "FederationDomain" +) + type federationDomainSecretsController struct { secretHelper SecretHelper secretRefFunc func(domain *configv1alpha1.FederationDomain) *corev1.LocalObjectReference @@ -236,3 +240,11 @@ func (c *federationDomainSecretsController) updateFederationDomain( return err }) } + +// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain. +func isFederationDomainControllee(obj metav1.Object) bool { + controller := metav1.GetControllerOf(obj) + return controller != nil && + controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() && + controller.Kind == federationDomainKind +} diff --git a/internal/controller/supervisorconfig/generator/generator.go b/internal/controller/supervisorconfig/generator/generator.go deleted file mode 100644 index d37ec235..00000000 --- a/internal/controller/supervisorconfig/generator/generator.go +++ /dev/null @@ -1,104 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package generator - -import ( - "crypto/rand" - - appsv1 "k8s.io/api/apps/v1" - 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 ( - federationDomainKind = "FederationDomain" -) - -func generateSymmetricKey() ([]byte, error) { - b := make([]byte, symmetricKeySize) - if _, err := rand.Read(b); err != nil { - return nil, err - } - return b, nil -} - -func isValid(secret *corev1.Secret, labels map[string]string) bool { - if secret.Type != symmetricSecretType { - return false - } - - data, ok := secret.Data[symmetricSecretDataKey] - if !ok { - return false - } - if len(data) != symmetricKeySize { - return false - } - - for key, value := range labels { - if secret.Labels[key] != value { - return false - } - } - - return true -} - -func secretDataFunc() (map[string][]byte, error) { - symmetricKey, err := generateKey() - if err != nil { - return nil, err - } - - return map[string][]byte{ - symmetricSecretDataKey: symmetricKey, - }, nil -} - -func generateSecret(namespace, name string, labels map[string]string, secretDataFunc func() (map[string][]byte, error), owner metav1.Object) (*corev1.Secret, error) { - secretData, err := secretDataFunc() - if err != nil { - return nil, err - } - - deploymentGVK := schema.GroupVersionKind{ - Group: appsv1.SchemeGroupVersion.Group, - Version: appsv1.SchemeGroupVersion.Version, - Kind: "Deployment", - } - - blockOwnerDeletion := true - isController := false - - return &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{ - Name: name, - Namespace: namespace, - OwnerReferences: []metav1.OwnerReference{ - { - APIVersion: deploymentGVK.GroupVersion().String(), - Kind: deploymentGVK.Kind, - Name: owner.GetName(), - UID: owner.GetUID(), - BlockOwnerDeletion: &blockOwnerDeletion, - Controller: &isController, - }, - }, - Labels: labels, - }, - Type: symmetricSecretType, - Data: secretData, - }, nil -} - -// isFederationDomainControllee returns whether the provided obj is controlled by an FederationDomain. -func isFederationDomainControllee(obj metav1.Object) bool { - controller := metav1.GetControllerOf(obj) - return controller != nil && - controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() && - controller.Kind == federationDomainKind -} diff --git a/internal/controller/supervisorconfig/generator/secret_helper.go b/internal/controller/supervisorconfig/generator/secret_helper.go index c0e66fb7..0113a929 100644 --- a/internal/controller/supervisorconfig/generator/secret_helper.go +++ b/internal/controller/supervisorconfig/generator/secret_helper.go @@ -12,7 +12,6 @@ import ( "k8s.io/apimachinery/pkg/runtime/schema" configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" - "go.pinniped.dev/internal/plog" ) // SecretHelper describes an object that can Generate() a Secret and determine whether a Secret @@ -27,8 +26,18 @@ type SecretHelper interface { } const ( - // symmetricSecretType is corev1.Secret.Type of all corev1.Secret's generated by this helper. - symmetricSecretType = "secrets.pinniped.dev/symmetric" + // SupervisorCSRFSigningKeySecretType for the Secret storing the CSRF signing key. + SupervisorCSRFSigningKeySecretType corev1.SecretType = "secrets.pinniped.dev/supervisor-csrf-signing-key" + + // FederationDomainTokenSigningKeyType for the Secret storing the FederationDomain token signing key. + FederationDomainTokenSigningKeyType corev1.SecretType = "secrets.pinniped.dev/federation-domain-token-signing-key" + + // FederationDomainStateSigningKeyType for the Secret storing the FederationDomain state signing key. + FederationDomainStateSigningKeyType corev1.SecretType = "secrets.pinniped.dev/federation-domain-state-signing-key" + + // FederationDomainStateEncryptionKeyType for the Secret storing the FederationDomain state encryption key. + FederationDomainStateEncryptionKeyType corev1.SecretType = "secrets.pinniped.dev/federation-domain-state-encryption-key" + // symmetricSecretDataKey is the corev1.Secret.Data key for the symmetric key value generated by this helper. symmetricSecretDataKey = "key" @@ -96,7 +105,7 @@ func (s *symmetricSecretHelper) Generate(parent *configv1alpha1.FederationDomain }), }, }, - Type: symmetricSecretType, + Type: s.secretType(), Data: map[string][]byte{ symmetricSecretDataKey: key, }, @@ -109,7 +118,7 @@ func (s *symmetricSecretHelper) IsValid(parent *configv1alpha1.FederationDomain, return false } - if secret.Type != symmetricSecretType { + if secret.Type != s.secretType() { return false } @@ -129,12 +138,7 @@ func (s *symmetricSecretHelper) ObserveActiveSecretAndUpdateParentFederationDoma federationDomain *configv1alpha1.FederationDomain, secret *corev1.Secret, ) *configv1alpha1.FederationDomain { - var cacheKey string - if federationDomain != nil { - cacheKey = federationDomain.Spec.Issuer - } - - s.updateCacheFunc(cacheKey, secret.Data[symmetricSecretDataKey]) + s.updateCacheFunc(federationDomain.Spec.Issuer, secret.Data[symmetricSecretDataKey]) switch s.secretUsage { case SecretUsageTokenSigningKey: @@ -144,8 +148,21 @@ func (s *symmetricSecretHelper) ObserveActiveSecretAndUpdateParentFederationDoma case SecretUsageStateEncryptionKey: federationDomain.Status.Secrets.StateEncryptionKey.Name = secret.Name default: - plog.Warning("unknown secret usage enum value: %d", s.secretUsage) + panic(fmt.Sprintf("unknown secret usage enum value: %d", s.secretUsage)) } return federationDomain } + +func (s *symmetricSecretHelper) secretType() corev1.SecretType { + switch s.secretUsage { + case SecretUsageTokenSigningKey: + return FederationDomainTokenSigningKeyType + case SecretUsageStateSigningKey: + return FederationDomainStateSigningKeyType + case SecretUsageStateEncryptionKey: + return FederationDomainStateEncryptionKeyType + default: + panic(fmt.Sprintf("unknown secret usage enum value: %d", s.secretUsage)) + } +} diff --git a/internal/controller/supervisorconfig/generator/secret_helper_test.go b/internal/controller/supervisorconfig/generator/secret_helper_test.go index f25a515b..b83bed50 100644 --- a/internal/controller/supervisorconfig/generator/secret_helper_test.go +++ b/internal/controller/supervisorconfig/generator/secret_helper_test.go @@ -23,25 +23,29 @@ func TestSymmetricSecretHelper(t *testing.T) { tests := []struct { name string secretUsage SecretUsage + wantSecretType corev1.SecretType wantSetFederationDomainField func(*configv1alpha1.FederationDomain) string }{ { - name: "token signing key", - secretUsage: SecretUsageTokenSigningKey, + name: "token signing key", + secretUsage: SecretUsageTokenSigningKey, + wantSecretType: "secrets.pinniped.dev/federation-domain-token-signing-key", wantSetFederationDomainField: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.TokenSigningKey.Name }, }, { - name: "state signing key", - secretUsage: SecretUsageStateSigningKey, + name: "state signing key", + secretUsage: SecretUsageStateSigningKey, + wantSecretType: "secrets.pinniped.dev/federation-domain-state-signing-key", wantSetFederationDomainField: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.StateSigningKey.Name }, }, { - name: "state encryption key", - secretUsage: SecretUsageStateEncryptionKey, + name: "state encryption key", + secretUsage: SecretUsageStateEncryptionKey, + wantSecretType: "secrets.pinniped.dev/federation-domain-state-encryption-key", wantSetFederationDomainField: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.StateEncryptionKey.Name }, @@ -92,7 +96,7 @@ func TestSymmetricSecretHelper(t *testing.T) { }), }, }, - Type: "secrets.pinniped.dev/symmetric", + Type: test.wantSecretType, Data: map[string][]byte{ "key": []byte(keyWith32Bytes), }, @@ -110,55 +114,69 @@ func TestSymmetricSecretHelper(t *testing.T) { func TestSymmetricSecretHelperIsValid(t *testing.T) { tests := []struct { - name string - child func(*corev1.Secret) - parent func(*configv1alpha1.FederationDomain) - want bool + name string + secretUsage SecretUsage + child func(*corev1.Secret) + parent func(*configv1alpha1.FederationDomain) + want bool }{ { - name: "wrong type", + name: "wrong type", + secretUsage: SecretUsageTokenSigningKey, child: func(s *corev1.Secret) { s.Type = "wrong" }, want: false, }, { - name: "empty type", + name: "empty type", + secretUsage: SecretUsageTokenSigningKey, child: func(s *corev1.Secret) { s.Type = "" }, want: false, }, { - name: "data key is too short", + name: "data key is too short", + secretUsage: SecretUsageTokenSigningKey, child: func(s *corev1.Secret) { + s.Type = FederationDomainTokenSigningKeyType s.Data["key"] = []byte("short") }, want: false, }, { - name: "data key does not exist", + name: "data key does not exist", + secretUsage: SecretUsageTokenSigningKey, child: func(s *corev1.Secret) { + s.Type = FederationDomainTokenSigningKeyType delete(s.Data, "key") }, want: false, }, { - name: "child not owned by parent", + name: "child not owned by parent", + secretUsage: SecretUsageTokenSigningKey, + child: func(s *corev1.Secret) { + s.Type = FederationDomainTokenSigningKeyType + }, parent: func(federationDomain *configv1alpha1.FederationDomain) { federationDomain.UID = "wrong" }, want: false, }, { - name: "happy path", - want: true, + name: "happy path", + secretUsage: SecretUsageTokenSigningKey, + child: func(s *corev1.Secret) { + s.Type = FederationDomainTokenSigningKeyType + }, want: true, }, } for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - h := NewSymmetricSecretHelper("none of these args matter", nil, nil, SecretUsageTokenSigningKey, nil) + h := NewSymmetricSecretHelper("none of these args matter", nil, nil, test.secretUsage, nil) parent := &configv1alpha1.FederationDomain{ ObjectMeta: metav1.ObjectMeta{ @@ -179,7 +197,7 @@ func TestSymmetricSecretHelperIsValid(t *testing.T) { }), }, }, - Type: "secrets.pinniped.dev/symmetric", + Type: "invalid default", Data: map[string][]byte{ "key": []byte(keyWith32Bytes), }, diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets.go b/internal/controller/supervisorconfig/generator/supervisor_secrets.go index 9f5ca1c4..b7325648 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets.go @@ -6,12 +6,14 @@ package generator import ( "context" + "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" @@ -143,3 +145,80 @@ func (c *supervisorSecretsController) updateSecret(ctx context.Context, newSecre return err }) } + +func generateSymmetricKey() ([]byte, error) { + b := make([]byte, symmetricKeySize) + if _, err := rand.Read(b); err != nil { + return nil, err + } + return b, nil +} + +func isValid(secret *corev1.Secret, labels map[string]string) bool { + if secret.Type != SupervisorCSRFSigningKeySecretType { + return false + } + + data, ok := secret.Data[symmetricSecretDataKey] + if !ok { + return false + } + if len(data) != symmetricKeySize { + return false + } + + for key, value := range labels { + if secret.Labels[key] != value { + return false + } + } + + return true +} + +func secretDataFunc() (map[string][]byte, error) { + symmetricKey, err := generateKey() + if err != nil { + return nil, err + } + + return map[string][]byte{ + symmetricSecretDataKey: symmetricKey, + }, nil +} + +func generateSecret(namespace, name string, labels map[string]string, secretDataFunc func() (map[string][]byte, error), owner metav1.Object) (*corev1.Secret, error) { + secretData, err := secretDataFunc() + if err != nil { + return nil, err + } + + deploymentGVK := schema.GroupVersionKind{ + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "Deployment", + } + + blockOwnerDeletion := true + isController := false + + return &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + Namespace: namespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: deploymentGVK.GroupVersion().String(), + Kind: deploymentGVK.Kind, + Name: owner.GetName(), + UID: owner.GetUID(), + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, + }, + }, + Labels: labels, + }, + Type: SupervisorCSRFSigningKeySecretType, + Data: secretData, + }, nil +} diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go index 2522fe7d..f0b75f7c 100644 --- a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go @@ -258,7 +258,7 @@ func TestSupervisorSecretsControllerSync(t *testing.T) { }, Labels: labels, }, - Type: "secrets.pinniped.dev/symmetric", + Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", Data: map[string][]byte{ "key": generatedSymmetricKey, }, @@ -280,7 +280,7 @@ func TestSupervisorSecretsControllerSync(t *testing.T) { }, Labels: labels, }, - Type: "secrets.pinniped.dev/symmetric", + Type: "secrets.pinniped.dev/supervisor-csrf-signing-key", Data: map[string][]byte{ "key": otherGeneratedSymmetricKey, }, diff --git a/internal/controller/supervisorconfig/jwks_writer.go b/internal/controller/supervisorconfig/jwks_writer.go index 559b1afa..34f0b3aa 100644 --- a/internal/controller/supervisorconfig/jwks_writer.go +++ b/internal/controller/supervisorconfig/jwks_writer.go @@ -40,6 +40,8 @@ const ( // // Note! The value for this key will contain only public key material! jwksKey = "jwks" + + jwksSecretTypeValue corev1.SecretType = "secrets.pinniped.dev/federation-domain-jwks" ) const ( @@ -251,6 +253,7 @@ func (c *jwksWriterController) generateSecret(federationDomain *configv1alpha1.F activeJWKKey: jwkData, jwksKey: jwksData, }, + Type: jwksSecretTypeValue, } return &s, nil @@ -285,6 +288,7 @@ func (c *jwksWriterController) createOrUpdateSecret( } oldSecret.Data = newSecret.Data + oldSecret.Type = jwksSecretTypeValue _, err = secretClient.Update(ctx, oldSecret, metav1.UpdateOptions{}) return err }) @@ -322,6 +326,11 @@ func isFederationDomainControllee(obj metav1.Object) bool { // isValid returns whether the provided secret contains a valid active JWK and verification JWKS. func isValid(secret *corev1.Secret) bool { + if secret.Type != jwksSecretTypeValue { + plog.Debug("secret does not have the expected type", "expectedType", jwksSecretTypeValue, "actualType", secret.Type) + return false + } + jwkData, ok := secret.Data[activeJWKKey] if !ok { plog.Debug("secret does not contain active jwk") diff --git a/internal/controller/supervisorconfig/jwks_writer_test.go b/internal/controller/supervisorconfig/jwks_writer_test.go index 1c3d6565..d1947d98 100644 --- a/internal/controller/supervisorconfig/jwks_writer_test.go +++ b/internal/controller/supervisorconfig/jwks_writer_test.go @@ -281,6 +281,7 @@ func TestJWKSWriterControllerSync(t *testing.T) { }, }, }, + Type: "secrets.pinniped.dev/federation-domain-jwks", } s.Data = make(map[string][]byte) if activeJWKPath != "" { @@ -294,6 +295,9 @@ func TestJWKSWriterControllerSync(t *testing.T) { goodSecret := newSecret("testdata/good-jwk.json", "testdata/good-jwks.json") + secretWithWrongType := newSecret("testdata/good-jwk.json", "testdata/good-jwks.json") + secretWithWrongType.Type = "not-the-right-type" + tests := []struct { name string key controllerlib.Key @@ -407,6 +411,24 @@ func TestJWKSWriterControllerSync(t *testing.T) { kubetesting.NewGetAction(federationDomainGVR, namespace, goodFederationDomain.Name), }, }, + { + name: "wrong type in secret", + key: controllerlib.Key{Namespace: goodFederationDomain.Namespace, Name: goodFederationDomain.Name}, + federationDomains: []*configv1alpha1.FederationDomain{ + goodFederationDomainWithStatus, + }, + secrets: []*corev1.Secret{ + secretWithWrongType, + }, + wantGenerateKeyCount: 1, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + }, + wantFederationDomainActions: []kubetesting.Action{ + kubetesting.NewGetAction(federationDomainGVR, namespace, goodFederationDomain.Name), + }, + }, { name: "invalid jwk JSON in secret", key: controllerlib.Key{Namespace: goodFederationDomain.Namespace, Name: goodFederationDomain.Name}, diff --git a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go index 19b79fb4..df90a5ab 100644 --- a/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go +++ b/internal/controller/supervisorconfig/upstreamwatcher/upstreamwatcher.go @@ -18,6 +18,7 @@ import ( "github.com/coreos/go-oidc" "github.com/go-logr/logr" "golang.org/x/oauth2" + corev1 "k8s.io/api/core/v1" "k8s.io/apimachinery/pkg/api/equality" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/labels" @@ -39,9 +40,10 @@ const ( controllerName = "upstream-observer" // Constants related to the client credentials Secret. - oidcClientSecretType = "secrets.pinniped.dev/oidc-client" - clientIDDataKey = "clientID" - clientSecretDataKey = "clientSecret" + oidcClientSecretType corev1.SecretType = "secrets.pinniped.dev/oidc-client" + + clientIDDataKey = "clientID" + clientSecretDataKey = "clientSecret" // Constants related to the OIDC provider discovery cache. These do not affect the cache of JWKS. validatorCacheTTL = 15 * time.Minute diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go index a0387ec0..c5c21a16 100644 --- a/test/integration/supervisor_secrets_test.go +++ b/test/integration/supervisor_secrets_test.go @@ -40,7 +40,7 @@ func TestSupervisorSecrets(t *testing.T) { secretName: func(federationDomain *configv1alpha1.FederationDomain) string { return env.SupervisorAppName + "-key" }, - ensureValid: ensureValidSymmetricKey, + ensureValid: ensureValidSymmetricSecretOfTypeFunc("secrets.pinniped.dev/supervisor-csrf-signing-key"), }, { name: "jwks", @@ -54,21 +54,21 @@ func TestSupervisorSecrets(t *testing.T) { secretName: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.TokenSigningKey.Name }, - ensureValid: ensureValidSymmetricKey, + ensureValid: ensureValidSymmetricSecretOfTypeFunc("secrets.pinniped.dev/federation-domain-token-signing-key"), }, { name: "state signature secret", secretName: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.StateSigningKey.Name }, - ensureValid: ensureValidSymmetricKey, + ensureValid: ensureValidSymmetricSecretOfTypeFunc("secrets.pinniped.dev/federation-domain-state-signing-key"), }, { name: "state encryption secret", secretName: func(federationDomain *configv1alpha1.FederationDomain) string { return federationDomain.Status.Secrets.StateEncryptionKey.Name }, - ensureValid: ensureValidSymmetricKey, + ensureValid: ensureValidSymmetricSecretOfTypeFunc("secrets.pinniped.dev/federation-domain-state-encryption-key"), }, } for _, test := range tests { @@ -129,6 +129,9 @@ func TestSupervisorSecrets(t *testing.T) { func ensureValidJWKS(t *testing.T, secret *corev1.Secret) { t.Helper() + // Ensure the secret has the right type. + require.Equal(t, corev1.SecretType("secrets.pinniped.dev/federation-domain-jwks"), secret.Type) + // Ensure the secret has an active key. jwkData, ok := secret.Data["activeJWK"] require.True(t, ok, "secret is missing active jwk") @@ -157,10 +160,12 @@ func ensureValidJWKS(t *testing.T, secret *corev1.Secret) { require.True(t, foundActiveJWK, "could not find active JWK in JWKS: %s", jwks) } -func ensureValidSymmetricKey(t *testing.T, secret *corev1.Secret) { - t.Helper() - require.Equal(t, corev1.SecretType("secrets.pinniped.dev/symmetric"), secret.Type) - key, ok := secret.Data["key"] - require.Truef(t, ok, "secret data does not contain 'key': %s", secret.Data) - require.Equal(t, 32, len(key)) +func ensureValidSymmetricSecretOfTypeFunc(secretTypeValue string) func(*testing.T, *corev1.Secret) { + return func(t *testing.T, secret *corev1.Secret) { + t.Helper() + require.Equal(t, corev1.SecretType(secretTypeValue), secret.Type) + key, ok := secret.Data["key"] + require.Truef(t, ok, "secret data does not contain 'key': %s", secret.Data) + require.Equal(t, 32, len(key)) + } }