diff --git a/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl b/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl index dee102c9a..a351900f7 100644 --- a/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl +++ b/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl @@ -59,6 +59,30 @@ type OIDCProviderSpec struct { TLS *OIDCProviderTLSSpec `json:"tls,omitempty"` } +// OIDCProviderSecrets holds information about this OIDC Provider's secrets. +type OIDCProviderSecrets struct { + // JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are + // stored. If it is empty, then the signing/verification keys are either unknown or they don't + // exist. + // +optional + JWKS corev1.LocalObjectReference `json:"jwks,omitempty"` + + // TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing tokens is stored. + // +optional + TokenSigningKey corev1.LocalObjectReference `json:"tokenSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing state parameters is stored. + // +optional + StateSigningKey corev1.LocalObjectReference `json:"stateSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // encrypting state parameters is stored. + // +optional + StateEncryptionKey corev1.LocalObjectReference `json:"stateEncryptionKey,omitempty"` +} + // OIDCProviderStatus is a struct that describes the actual state of an OIDC Provider. type OIDCProviderStatus struct { // Status holds an enum that describes the state of this OIDC Provider. Note that this Status can @@ -76,11 +100,9 @@ type OIDCProviderStatus struct { // +optional LastUpdateTime *metav1.Time `json:"lastUpdateTime,omitempty"` - // JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys - // are stored. If it is empty, then the signing/verification keys are either unknown or they don't - // exist. + // Secrets contains information about this OIDC Provider's secrets. // +optional - JWKSSecret corev1.LocalObjectReference `json:"jwksSecret,omitempty"` + Secrets OIDCProviderSecrets `json:"secrets,omitempty"` } // OIDCProvider describes the configuration of an OIDC provider. diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 0c54964d2..461eea56e 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" @@ -14,6 +15,10 @@ import ( "strings" "time" + "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" @@ -28,6 +33,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/generator" "go.pinniped.dev/internal/controller/supervisorconfig/upstreamwatcher" "go.pinniped.dev/internal/controller/supervisorstorage" "go.pinniped.dev/internal/controllerlib" @@ -70,6 +76,7 @@ func waitForSignal() os.Signal { return <-signalCh } +//nolint:funlen func startControllers( ctx context.Context, cfg *supervisor.Config, @@ -77,11 +84,16 @@ 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, pinnipedInformers pinnipedinformers.SharedInformerFactory, ) { + opInformer := pinnipedInformers.Config().V1alpha1().OIDCProviders() + secretInformer := kubeInformers.Core().V1().Secrets() + // Create controller manager. controllerManager := controllerlib. NewManager(). @@ -99,7 +111,7 @@ func startControllers( issuerManager, clock.RealClock{}, pinnipedClient, - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -109,8 +121,8 @@ func startControllers( cfg.Labels, kubeClient, pinnipedClient, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -118,8 +130,8 @@ func startControllers( WithController( supervisorconfig.NewJWKSObserverController( dynamicJWKSProvider, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -128,8 +140,83 @@ func startControllers( supervisorconfig.NewTLSCertObserverController( dynamicTLSCertProvider, cfg.NamesConfig.DefaultTLSCertificateSecret, - kubeInformers.Core().V1().Secrets(), - pinnipedInformers.Config().V1alpha1().OIDCProviders(), + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). + WithController( + generator.NewSupervisorSecretsController( + supervisorDeployment, + cfg.Labels, + kubeClient, + secretInformer, + func(secret []byte) { + plog.Debug("setting csrf cookie secret") + secretCache.SetCSRFCookieEncoderHashKey(secret) + }, + controllerlib.WithInformer, + controllerlib.WithInitialEvent, + ), + singletonWorker, + ). + WithController( + generator.NewOIDCProviderSecretsController( + generator.NewSymmetricSecretHelper( + "pinniped-oidc-provider-hmac-key-", + cfg.Labels, + rand.Reader, + generator.SecretUsageTokenSigningKey, + func(oidcProviderIssuer string, symmetricKey []byte) { + plog.Debug("setting hmac secret", "issuer", oidcProviderIssuer) + secretCache.SetTokenHMACKey(oidcProviderIssuer, symmetricKey) + }, + ), + kubeClient, + pinnipedClient, + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). + WithController( + generator.NewOIDCProviderSecretsController( + generator.NewSymmetricSecretHelper( + "pinniped-oidc-provider-upstream-state-signature-key-", + cfg.Labels, + rand.Reader, + generator.SecretUsageStateSigningKey, + func(oidcProviderIssuer string, symmetricKey []byte) { + plog.Debug("setting state signature key", "issuer", oidcProviderIssuer) + secretCache.SetStateEncoderHashKey(oidcProviderIssuer, symmetricKey) + }, + ), + kubeClient, + pinnipedClient, + secretInformer, + opInformer, + controllerlib.WithInformer, + ), + singletonWorker, + ). + WithController( + generator.NewOIDCProviderSecretsController( + generator.NewSymmetricSecretHelper( + "pinniped-oidc-provider-upstream-state-encryption-key-", + cfg.Labels, + rand.Reader, + generator.SecretUsageStateEncryptionKey, + func(oidcProviderIssuer string, symmetricKey []byte) { + plog.Debug("setting state encryption key", "issuer", oidcProviderIssuer) + secretCache.SetStateEncoderBlockKey(oidcProviderIssuer, symmetricKey) + }, + ), + kubeClient, + pinnipedClient, + secretInformer, + opInformer, controllerlib.WithInformer, ), singletonWorker, @@ -153,6 +240,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 { @@ -174,7 +296,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() @@ -204,15 +328,22 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider() dynamicUpstreamIDPProvider := provider.NewDynamicUpstreamIDPProvider() + 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, + &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, @@ -220,6 +351,8 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { dynamicJWKSProvider, dynamicTLSCertProvider, dynamicUpstreamIDPProvider, + &secretCache, + supervisorDeployment, kubeClient, pinnipedClient, kubeInformers, @@ -288,7 +421,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/config.supervisor.pinniped.dev_oidcproviders.yaml b/deploy/supervisor/config.supervisor.pinniped.dev_oidcproviders.yaml index e25a9f817..47c6f5a95 100644 --- a/deploy/supervisor/config.supervisor.pinniped.dev_oidcproviders.yaml +++ b/deploy/supervisor/config.supervisor.pinniped.dev_oidcproviders.yaml @@ -81,17 +81,6 @@ spec: status: description: Status of the OIDC provider. properties: - jwksSecret: - description: JWKSSecret holds the name of the secret in which this - OIDC Provider's signing/verification keys are stored. If it is empty, - then the signing/verification keys are either unknown or they don't - exist. - properties: - name: - description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Add other useful fields. apiVersion, kind, uid?' - type: string - type: object lastUpdateTime: description: LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior @@ -101,6 +90,51 @@ spec: message: description: Message provides human-readable details about the Status. type: string + secrets: + description: Secrets contains information about this OIDC Provider's + secrets. + properties: + jwks: + description: JWKS holds the name of the corev1.Secret in which + this OIDC Provider's signing/verification keys are stored. If + it is empty, then the signing/verification keys are either unknown + or they don't exist. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateEncryptionKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for encrypting state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateSigningKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + tokenSigningKey: + description: TokenSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing tokens is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + type: object status: description: Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index 28b8d93f4..1e0c75c0a 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 33e86585e..b66c00fca 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/generated/1.17/README.adoc b/generated/1.17/README.adoc index 1303b2a42..75d13c3ec 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -304,6 +304,26 @@ OIDCProvider describes the configuration of an OIDC provider. +[id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-config-v1alpha1-oidcprovidersecrets"] +==== OIDCProviderSecrets + +OIDCProviderSecrets holds information about this OIDC Provider's secrets. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-config-v1alpha1-oidcproviderstatus[$$OIDCProviderStatus$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`jwks`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`tokenSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing tokens is stored. +| *`stateSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing state parameters is stored. +| *`stateEncryptionKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for encrypting state parameters is stored. +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-config-v1alpha1-oidcproviderspec"] ==== OIDCProviderSpec @@ -339,7 +359,7 @@ OIDCProviderStatus is a struct that describes the actual state of an OIDC Provid | *`status`* __OIDCProviderStatusCondition__ | Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. | *`message`* __string__ | Message provides human-readable details about the Status. | *`lastUpdateTime`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#time-v1-meta[$$Time$$]__ | LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior with respect to the empty metav1.Time value (see https://github.com/kubernetes/kubernetes/issues/86811). -| *`jwksSecret`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.17/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`secrets`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-supervisor-config-v1alpha1-oidcprovidersecrets[$$OIDCProviderSecrets$$]__ | Secrets contains information about this OIDC Provider's secrets. |=== diff --git a/generated/1.17/apis/supervisor/config/v1alpha1/types_oidcprovider.go b/generated/1.17/apis/supervisor/config/v1alpha1/types_oidcprovider.go index dee102c9a..a351900f7 100644 --- a/generated/1.17/apis/supervisor/config/v1alpha1/types_oidcprovider.go +++ b/generated/1.17/apis/supervisor/config/v1alpha1/types_oidcprovider.go @@ -59,6 +59,30 @@ type OIDCProviderSpec struct { TLS *OIDCProviderTLSSpec `json:"tls,omitempty"` } +// OIDCProviderSecrets holds information about this OIDC Provider's secrets. +type OIDCProviderSecrets struct { + // JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are + // stored. If it is empty, then the signing/verification keys are either unknown or they don't + // exist. + // +optional + JWKS corev1.LocalObjectReference `json:"jwks,omitempty"` + + // TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing tokens is stored. + // +optional + TokenSigningKey corev1.LocalObjectReference `json:"tokenSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing state parameters is stored. + // +optional + StateSigningKey corev1.LocalObjectReference `json:"stateSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // encrypting state parameters is stored. + // +optional + StateEncryptionKey corev1.LocalObjectReference `json:"stateEncryptionKey,omitempty"` +} + // OIDCProviderStatus is a struct that describes the actual state of an OIDC Provider. type OIDCProviderStatus struct { // Status holds an enum that describes the state of this OIDC Provider. Note that this Status can @@ -76,11 +100,9 @@ type OIDCProviderStatus struct { // +optional LastUpdateTime *metav1.Time `json:"lastUpdateTime,omitempty"` - // JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys - // are stored. If it is empty, then the signing/verification keys are either unknown or they don't - // exist. + // Secrets contains information about this OIDC Provider's secrets. // +optional - JWKSSecret corev1.LocalObjectReference `json:"jwksSecret,omitempty"` + Secrets OIDCProviderSecrets `json:"secrets,omitempty"` } // OIDCProvider describes the configuration of an OIDC provider. diff --git a/generated/1.17/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go b/generated/1.17/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go index f208d4d05..0cfc17a42 100644 --- a/generated/1.17/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.17/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go @@ -72,6 +72,26 @@ func (in *OIDCProviderList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OIDCProviderSecrets) DeepCopyInto(out *OIDCProviderSecrets) { + *out = *in + out.JWKS = in.JWKS + out.TokenSigningKey = in.TokenSigningKey + out.StateSigningKey = in.StateSigningKey + out.StateEncryptionKey = in.StateEncryptionKey + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OIDCProviderSecrets. +func (in *OIDCProviderSecrets) DeepCopy() *OIDCProviderSecrets { + if in == nil { + return nil + } + out := new(OIDCProviderSecrets) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OIDCProviderSpec) DeepCopyInto(out *OIDCProviderSpec) { *out = *in @@ -100,7 +120,7 @@ func (in *OIDCProviderStatus) DeepCopyInto(out *OIDCProviderStatus) { in, out := &in.LastUpdateTime, &out.LastUpdateTime *out = (*in).DeepCopy() } - out.JWKSSecret = in.JWKSSecret + out.Secrets = in.Secrets return } diff --git a/generated/1.17/crds/config.supervisor.pinniped.dev_oidcproviders.yaml b/generated/1.17/crds/config.supervisor.pinniped.dev_oidcproviders.yaml index e25a9f817..47c6f5a95 100644 --- a/generated/1.17/crds/config.supervisor.pinniped.dev_oidcproviders.yaml +++ b/generated/1.17/crds/config.supervisor.pinniped.dev_oidcproviders.yaml @@ -81,17 +81,6 @@ spec: status: description: Status of the OIDC provider. properties: - jwksSecret: - description: JWKSSecret holds the name of the secret in which this - OIDC Provider's signing/verification keys are stored. If it is empty, - then the signing/verification keys are either unknown or they don't - exist. - properties: - name: - description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Add other useful fields. apiVersion, kind, uid?' - type: string - type: object lastUpdateTime: description: LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior @@ -101,6 +90,51 @@ spec: message: description: Message provides human-readable details about the Status. type: string + secrets: + description: Secrets contains information about this OIDC Provider's + secrets. + properties: + jwks: + description: JWKS holds the name of the corev1.Secret in which + this OIDC Provider's signing/verification keys are stored. If + it is empty, then the signing/verification keys are either unknown + or they don't exist. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateEncryptionKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for encrypting state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateSigningKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + tokenSigningKey: + description: TokenSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing tokens is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + type: object status: description: Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 7c3dda8f0..01a0c7182 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -304,6 +304,26 @@ OIDCProvider describes the configuration of an OIDC provider. +[id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-config-v1alpha1-oidcprovidersecrets"] +==== OIDCProviderSecrets + +OIDCProviderSecrets holds information about this OIDC Provider's secrets. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-config-v1alpha1-oidcproviderstatus[$$OIDCProviderStatus$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`jwks`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`tokenSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing tokens is stored. +| *`stateSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing state parameters is stored. +| *`stateEncryptionKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for encrypting state parameters is stored. +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-config-v1alpha1-oidcproviderspec"] ==== OIDCProviderSpec @@ -339,7 +359,7 @@ OIDCProviderStatus is a struct that describes the actual state of an OIDC Provid | *`status`* __OIDCProviderStatusCondition__ | Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. | *`message`* __string__ | Message provides human-readable details about the Status. | *`lastUpdateTime`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#time-v1-meta[$$Time$$]__ | LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior with respect to the empty metav1.Time value (see https://github.com/kubernetes/kubernetes/issues/86811). -| *`jwksSecret`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.18/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`secrets`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-supervisor-config-v1alpha1-oidcprovidersecrets[$$OIDCProviderSecrets$$]__ | Secrets contains information about this OIDC Provider's secrets. |=== diff --git a/generated/1.18/apis/supervisor/config/v1alpha1/types_oidcprovider.go b/generated/1.18/apis/supervisor/config/v1alpha1/types_oidcprovider.go index dee102c9a..a351900f7 100644 --- a/generated/1.18/apis/supervisor/config/v1alpha1/types_oidcprovider.go +++ b/generated/1.18/apis/supervisor/config/v1alpha1/types_oidcprovider.go @@ -59,6 +59,30 @@ type OIDCProviderSpec struct { TLS *OIDCProviderTLSSpec `json:"tls,omitempty"` } +// OIDCProviderSecrets holds information about this OIDC Provider's secrets. +type OIDCProviderSecrets struct { + // JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are + // stored. If it is empty, then the signing/verification keys are either unknown or they don't + // exist. + // +optional + JWKS corev1.LocalObjectReference `json:"jwks,omitempty"` + + // TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing tokens is stored. + // +optional + TokenSigningKey corev1.LocalObjectReference `json:"tokenSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing state parameters is stored. + // +optional + StateSigningKey corev1.LocalObjectReference `json:"stateSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // encrypting state parameters is stored. + // +optional + StateEncryptionKey corev1.LocalObjectReference `json:"stateEncryptionKey,omitempty"` +} + // OIDCProviderStatus is a struct that describes the actual state of an OIDC Provider. type OIDCProviderStatus struct { // Status holds an enum that describes the state of this OIDC Provider. Note that this Status can @@ -76,11 +100,9 @@ type OIDCProviderStatus struct { // +optional LastUpdateTime *metav1.Time `json:"lastUpdateTime,omitempty"` - // JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys - // are stored. If it is empty, then the signing/verification keys are either unknown or they don't - // exist. + // Secrets contains information about this OIDC Provider's secrets. // +optional - JWKSSecret corev1.LocalObjectReference `json:"jwksSecret,omitempty"` + Secrets OIDCProviderSecrets `json:"secrets,omitempty"` } // OIDCProvider describes the configuration of an OIDC provider. diff --git a/generated/1.18/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go b/generated/1.18/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go index f208d4d05..0cfc17a42 100644 --- a/generated/1.18/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.18/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go @@ -72,6 +72,26 @@ func (in *OIDCProviderList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OIDCProviderSecrets) DeepCopyInto(out *OIDCProviderSecrets) { + *out = *in + out.JWKS = in.JWKS + out.TokenSigningKey = in.TokenSigningKey + out.StateSigningKey = in.StateSigningKey + out.StateEncryptionKey = in.StateEncryptionKey + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OIDCProviderSecrets. +func (in *OIDCProviderSecrets) DeepCopy() *OIDCProviderSecrets { + if in == nil { + return nil + } + out := new(OIDCProviderSecrets) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OIDCProviderSpec) DeepCopyInto(out *OIDCProviderSpec) { *out = *in @@ -100,7 +120,7 @@ func (in *OIDCProviderStatus) DeepCopyInto(out *OIDCProviderStatus) { in, out := &in.LastUpdateTime, &out.LastUpdateTime *out = (*in).DeepCopy() } - out.JWKSSecret = in.JWKSSecret + out.Secrets = in.Secrets return } diff --git a/generated/1.18/crds/config.supervisor.pinniped.dev_oidcproviders.yaml b/generated/1.18/crds/config.supervisor.pinniped.dev_oidcproviders.yaml index e25a9f817..47c6f5a95 100644 --- a/generated/1.18/crds/config.supervisor.pinniped.dev_oidcproviders.yaml +++ b/generated/1.18/crds/config.supervisor.pinniped.dev_oidcproviders.yaml @@ -81,17 +81,6 @@ spec: status: description: Status of the OIDC provider. properties: - jwksSecret: - description: JWKSSecret holds the name of the secret in which this - OIDC Provider's signing/verification keys are stored. If it is empty, - then the signing/verification keys are either unknown or they don't - exist. - properties: - name: - description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Add other useful fields. apiVersion, kind, uid?' - type: string - type: object lastUpdateTime: description: LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior @@ -101,6 +90,51 @@ spec: message: description: Message provides human-readable details about the Status. type: string + secrets: + description: Secrets contains information about this OIDC Provider's + secrets. + properties: + jwks: + description: JWKS holds the name of the corev1.Secret in which + this OIDC Provider's signing/verification keys are stored. If + it is empty, then the signing/verification keys are either unknown + or they don't exist. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateEncryptionKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for encrypting state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateSigningKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + tokenSigningKey: + description: TokenSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing tokens is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + type: object status: description: Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index d4d0c8b1a..a61e10738 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -304,6 +304,26 @@ OIDCProvider describes the configuration of an OIDC provider. +[id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-config-v1alpha1-oidcprovidersecrets"] +==== OIDCProviderSecrets + +OIDCProviderSecrets holds information about this OIDC Provider's secrets. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-config-v1alpha1-oidcproviderstatus[$$OIDCProviderStatus$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`jwks`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`tokenSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing tokens is stored. +| *`stateSigningKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for signing state parameters is stored. +| *`stateEncryptionKey`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for encrypting state parameters is stored. +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-config-v1alpha1-oidcproviderspec"] ==== OIDCProviderSpec @@ -339,7 +359,7 @@ OIDCProviderStatus is a struct that describes the actual state of an OIDC Provid | *`status`* __OIDCProviderStatusCondition__ | Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. | *`message`* __string__ | Message provides human-readable details about the Status. | *`lastUpdateTime`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#time-v1-meta[$$Time$$]__ | LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior with respect to the empty metav1.Time value (see https://github.com/kubernetes/kubernetes/issues/86811). -| *`jwksSecret`* __link:https://kubernetes.io/docs/reference/generated/kubernetes-api/v1.19/#localobjectreference-v1-core[$$LocalObjectReference$$]__ | JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys are stored. If it is empty, then the signing/verification keys are either unknown or they don't exist. +| *`secrets`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-supervisor-config-v1alpha1-oidcprovidersecrets[$$OIDCProviderSecrets$$]__ | Secrets contains information about this OIDC Provider's secrets. |=== diff --git a/generated/1.19/apis/supervisor/config/v1alpha1/types_oidcprovider.go b/generated/1.19/apis/supervisor/config/v1alpha1/types_oidcprovider.go index dee102c9a..a351900f7 100644 --- a/generated/1.19/apis/supervisor/config/v1alpha1/types_oidcprovider.go +++ b/generated/1.19/apis/supervisor/config/v1alpha1/types_oidcprovider.go @@ -59,6 +59,30 @@ type OIDCProviderSpec struct { TLS *OIDCProviderTLSSpec `json:"tls,omitempty"` } +// OIDCProviderSecrets holds information about this OIDC Provider's secrets. +type OIDCProviderSecrets struct { + // JWKS holds the name of the corev1.Secret in which this OIDC Provider's signing/verification keys are + // stored. If it is empty, then the signing/verification keys are either unknown or they don't + // exist. + // +optional + JWKS corev1.LocalObjectReference `json:"jwks,omitempty"` + + // TokenSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing tokens is stored. + // +optional + TokenSigningKey corev1.LocalObjectReference `json:"tokenSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // signing state parameters is stored. + // +optional + StateSigningKey corev1.LocalObjectReference `json:"stateSigningKey,omitempty"` + + // StateSigningKey holds the name of the corev1.Secret in which this OIDC Provider's key for + // encrypting state parameters is stored. + // +optional + StateEncryptionKey corev1.LocalObjectReference `json:"stateEncryptionKey,omitempty"` +} + // OIDCProviderStatus is a struct that describes the actual state of an OIDC Provider. type OIDCProviderStatus struct { // Status holds an enum that describes the state of this OIDC Provider. Note that this Status can @@ -76,11 +100,9 @@ type OIDCProviderStatus struct { // +optional LastUpdateTime *metav1.Time `json:"lastUpdateTime,omitempty"` - // JWKSSecret holds the name of the secret in which this OIDC Provider's signing/verification keys - // are stored. If it is empty, then the signing/verification keys are either unknown or they don't - // exist. + // Secrets contains information about this OIDC Provider's secrets. // +optional - JWKSSecret corev1.LocalObjectReference `json:"jwksSecret,omitempty"` + Secrets OIDCProviderSecrets `json:"secrets,omitempty"` } // OIDCProvider describes the configuration of an OIDC provider. diff --git a/generated/1.19/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go b/generated/1.19/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go index f208d4d05..0cfc17a42 100644 --- a/generated/1.19/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.19/apis/supervisor/config/v1alpha1/zz_generated.deepcopy.go @@ -72,6 +72,26 @@ func (in *OIDCProviderList) DeepCopyObject() runtime.Object { return nil } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *OIDCProviderSecrets) DeepCopyInto(out *OIDCProviderSecrets) { + *out = *in + out.JWKS = in.JWKS + out.TokenSigningKey = in.TokenSigningKey + out.StateSigningKey = in.StateSigningKey + out.StateEncryptionKey = in.StateEncryptionKey + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new OIDCProviderSecrets. +func (in *OIDCProviderSecrets) DeepCopy() *OIDCProviderSecrets { + if in == nil { + return nil + } + out := new(OIDCProviderSecrets) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *OIDCProviderSpec) DeepCopyInto(out *OIDCProviderSpec) { *out = *in @@ -100,7 +120,7 @@ func (in *OIDCProviderStatus) DeepCopyInto(out *OIDCProviderStatus) { in, out := &in.LastUpdateTime, &out.LastUpdateTime *out = (*in).DeepCopy() } - out.JWKSSecret = in.JWKSSecret + out.Secrets = in.Secrets return } diff --git a/generated/1.19/crds/config.supervisor.pinniped.dev_oidcproviders.yaml b/generated/1.19/crds/config.supervisor.pinniped.dev_oidcproviders.yaml index e25a9f817..47c6f5a95 100644 --- a/generated/1.19/crds/config.supervisor.pinniped.dev_oidcproviders.yaml +++ b/generated/1.19/crds/config.supervisor.pinniped.dev_oidcproviders.yaml @@ -81,17 +81,6 @@ spec: status: description: Status of the OIDC provider. properties: - jwksSecret: - description: JWKSSecret holds the name of the secret in which this - OIDC Provider's signing/verification keys are stored. If it is empty, - then the signing/verification keys are either unknown or they don't - exist. - properties: - name: - description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names - TODO: Add other useful fields. apiVersion, kind, uid?' - type: string - type: object lastUpdateTime: description: LastUpdateTime holds the time at which the Status was last updated. It is a pointer to get around some undesirable behavior @@ -101,6 +90,51 @@ spec: message: description: Message provides human-readable details about the Status. type: string + secrets: + description: Secrets contains information about this OIDC Provider's + secrets. + properties: + jwks: + description: JWKS holds the name of the corev1.Secret in which + this OIDC Provider's signing/verification keys are stored. If + it is empty, then the signing/verification keys are either unknown + or they don't exist. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateEncryptionKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for encrypting state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + stateSigningKey: + description: StateSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing state parameters + is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + tokenSigningKey: + description: TokenSigningKey holds the name of the corev1.Secret + in which this OIDC Provider's key for signing tokens is stored. + properties: + name: + description: 'Name of the referent. More info: https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#names + TODO: Add other useful fields. apiVersion, kind, uid?' + type: string + type: object + type: object status: description: Status holds an enum that describes the state of this OIDC Provider. Note that this Status can represent success or failure. diff --git a/internal/controller/supervisorconfig/generator/generator.go b/internal/controller/supervisorconfig/generator/generator.go new file mode 100644 index 000000000..fe711698e --- /dev/null +++ b/internal/controller/supervisorconfig/generator/generator.go @@ -0,0 +1,104 @@ +// 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 ( + opKind = "OIDCProvider" +) + +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 +} + +// isOPCControlle returns whether the provided obj is controlled by an OPC. +func isOPControllee(obj metav1.Object) bool { + controller := metav1.GetControllerOf(obj) + return controller != nil && + controller.APIVersion == configv1alpha1.SchemeGroupVersion.String() && + controller.Kind == opKind +} diff --git a/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go new file mode 100644 index 000000000..b188cc189 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets.go @@ -0,0 +1,232 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package generator + +import ( + "context" + "fmt" + "reflect" + + corev1 "k8s.io/api/core/v1" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" + "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" + "go.pinniped.dev/internal/plog" +) + +type oidcProviderSecretsController struct { + secretHelper SecretHelper + kubeClient kubernetes.Interface + pinnipedClient pinnipedclientset.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( + 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: fmt.Sprintf("%s%s", secretHelper.NamePrefix(), "controller"), + Syncer: &oidcProviderSecretsController{ + 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. + withInformer( + secretInformer, + pinnipedcontroller.SimpleFilter(isOPControllee, func(obj metav1.Object) controllerlib.Key { + if isOPControllee(obj) { + controller := metav1.GetControllerOf(obj) + return controllerlib.Key{ + Name: controller.Name, + Namespace: obj.GetNamespace(), + } + } + return controllerlib.Key{} + }), + controllerlib.InformerOption{}, + ), + // We want to be notified when anything happens to an OPC. + withInformer( + opcInformer, + pinnipedcontroller.MatchAnythingFilter(nil), // nil parent func is fine because each event is distinct + controllerlib.InformerOption{}, + ), + ) +} + +func (c *oidcProviderSecretsController) Sync(ctx controllerlib.Context) error { + op, err := c.opcInformer.Lister().OIDCProviders(ctx.Key.Namespace).Get(ctx.Key.Name) + notFound := k8serrors.IsNotFound(err) + if err != nil && !notFound { + return fmt.Errorf( + "failed to get %s/%s OIDCProvider: %w", + ctx.Key.Namespace, + ctx.Key.Name, + err, + ) + } + + if notFound { + // 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", + klog.KRef(ctx.Key.Namespace, ctx.Key.Name), + ) + return nil + } + + newSecret, err := c.secretHelper.Generate(op) + if err != nil { + 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.KObj(op), + "secret", + klog.KObj(existingSecret), + ) + + op = c.secretHelper.ObserveActiveSecretAndUpdateParentOIDCProvider(op, existingSecret) + if err := c.updateOIDCProvider(ctx.Context, op); err != nil { + return fmt.Errorf("failed to update oidcprovider: %w", err) + } + plog.Debug("updated oidcprovider", "oidcprovider", klog.KObj(op), "secret", klog.KObj(newSecret)) + + return nil + } + + // 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)) + + op = c.secretHelper.ObserveActiveSecretAndUpdateParentOIDCProvider(op, newSecret) + if err := c.updateOIDCProvider(ctx.Context, op); err != nil { + return fmt.Errorf("failed to update oidcprovider: %w", err) + } + plog.Debug("updated oidcprovider", "oidcprovider", klog.KObj(op), "secret", klog.KObj(newSecret)) + + return nil +} + +// 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(op.Namespace).Get(secretName) + notFound := k8serrors.IsNotFound(err) + if err != nil && !notFound { + 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, nil + } + + if !c.secretHelper.IsValid(op, secret) { + // If this secret is invalid, we need to generate a new one. + return true, secret, nil + } + + return false, secret, nil +} + +func (c *oidcProviderSecretsController) createOrUpdateSecret( + ctx context.Context, + op *configv1alpha1.OIDCProvider, + newSecret **corev1.Secret, +) error { + secretClient := c.kubeClient.CoreV1().Secrets((*newSecret).Namespace) + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + oldSecret, err := secretClient.Get(ctx, (*newSecret).Name, metav1.GetOptions{}) + notFound := k8serrors.IsNotFound(err) + if err != nil && !notFound { + 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{}) + if err != nil { + 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 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.Labels = (*newSecret).Labels + oldSecret.Type = (*newSecret).Type + oldSecret.Data = (*newSecret).Data + *newSecret = oldSecret + _, err = secretClient.Update(ctx, oldSecret, metav1.UpdateOptions{}) + return err + }) +} + +func (c *oidcProviderSecretsController) updateOIDCProvider( + ctx context.Context, + newOP *configv1alpha1.OIDCProvider, +) error { + opcClient := c.pinnipedClient.ConfigV1alpha1().OIDCProviders(newOP.Namespace) + return retry.RetryOnConflict(retry.DefaultRetry, func() error { + oldOP, err := opcClient.Get(ctx, newOP.Name, metav1.GetOptions{}) + if err != nil { + return fmt.Errorf("failed to get oidcprovider %s/%s: %w", newOP.Namespace, newOP.Name, err) + } + + if reflect.DeepEqual(newOP.Status.Secrets, oldOP.Status.Secrets) { + return nil + } + + oldOP.Status.Secrets = newOP.Status.Secrets + _, err = opcClient.Update(ctx, oldOP, 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 new file mode 100644 index 000000000..e888c7c23 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/oidc_provider_secrets_test.go @@ -0,0 +1,625 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package generator + +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" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" + 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" +) + +func TestOIDCProviderControllerFilterSecret(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + secret corev1.Secret + wantAdd bool + wantUpdate bool + wantDelete bool + wantParent controllerlib.Key + }{ + { + name: "no owner reference", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{}, + }, + }, + { + name: "owner reference without correct APIVersion", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "OIDCProvider", + Name: "some-name", + Controller: boolPtr(true), + }, + }, + }, + }, + }, + { + name: "owner reference without correct Kind", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Name: "some-name", + Controller: boolPtr(true), + }, + }, + }, + }, + }, + { + name: "owner reference without controller set to true", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "OIDCProvider", + Name: "some-name", + }, + }, + }, + }, + }, + { + name: "correct owner reference", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "OIDCProvider", + Name: "some-name", + Controller: boolPtr(true), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"}, + }, + { + name: "multiple owner references", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "UnrelatedKind", + }, + { + APIVersion: configv1alpha1.SchemeGroupVersion.String(), + Kind: "OIDCProvider", + Name: "some-name", + Controller: boolPtr(true), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + wantParent: controllerlib.Key{Namespace: "some-namespace", Name: "some-name"}, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name") + + secretInformer := kubeinformers.NewSharedInformerFactory( + kubernetesfake.NewSimpleClientset(), + 0, + ).Core().V1().Secrets() + opcInformer := pinnipedinformers.NewSharedInformerFactory( + pinnipedfake.NewSimpleClientset(), + 0, + ).Config().V1alpha1().OIDCProviders() + withInformer := testutil.NewObservableWithInformerOption() + _ = NewOIDCProviderSecretsController( + secretHelper, + nil, // kubeClient, not needed + nil, // pinnipedClient, not needed + secretInformer, + opcInformer, + withInformer.WithInformer, + ) + + unrelated := corev1.Secret{} + filter := withInformer.GetFilterForInformer(secretInformer) + require.Equal(t, test.wantAdd, filter.Add(&test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated)) + require.Equal(t, test.wantDelete, filter.Delete(&test.secret)) + require.Equal(t, test.wantParent, filter.Parent(&test.secret)) + }) + } +} + +func TestNewOIDCProviderSecretsControllerFilterOPC(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + opc configv1alpha1.OIDCProvider + wantAdd bool + wantUpdate bool + wantDelete bool + wantParent controllerlib.Key + }{ + { + name: "anything goes", + opc: configv1alpha1.OIDCProvider{}, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + wantParent: controllerlib.Key{}, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name") + + secretInformer := kubeinformers.NewSharedInformerFactory( + kubernetesfake.NewSimpleClientset(), + 0, + ).Core().V1().Secrets() + opcInformer := pinnipedinformers.NewSharedInformerFactory( + pinnipedfake.NewSimpleClientset(), + 0, + ).Config().V1alpha1().OIDCProviders() + withInformer := testutil.NewObservableWithInformerOption() + _ = NewOIDCProviderSecretsController( + secretHelper, + nil, // kubeClient, not needed + nil, // pinnipedClient, not needed + secretInformer, + opcInformer, + withInformer.WithInformer, + ) + + unrelated := configv1alpha1.OIDCProvider{} + filter := withInformer.GetFilterForInformer(opcInformer) + require.Equal(t, test.wantAdd, filter.Add(&test.opc)) + require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.opc)) + require.Equal(t, test.wantUpdate, filter.Update(&test.opc, &unrelated)) + require.Equal(t, test.wantDelete, filter.Delete(&test.opc)) + require.Equal(t, test.wantParent, filter.Parent(&test.opc)) + }) + } +} + +func TestOIDCProviderSecretsControllerSync(t *testing.T) { + t.Parallel() + + const ( + namespace = "some-namespace" + + 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", + } + + secretGVR := schema.GroupVersionResource{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Resource: "secrets", + } + + goodOP := &configv1alpha1.OIDCProvider{ + ObjectMeta: metav1.ObjectMeta{ + Name: opName, + Namespace: namespace, + UID: opUID, + }, + } + + 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"), + }, + } + + goodOPWithStatus := goodOP.DeepCopy() + goodOPWithStatus.Status.Secrets.TokenSigningKey.Name = goodSecret.Name + + 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), + }, + }, + }, + } + + tests := []struct { + name string + storage func(**configv1alpha1.OIDCProvider, **corev1.Secret) + client func(*pinnipedfake.Clientset, *kubernetesfake.Clientset) + secretHelper func(*mocksecrethelper.MockSecretHelper) + wantOPActions []kubetesting.Action + wantSecretActions []kubetesting.Action + wantError string + }{ + { + name: "OIDCProvider does not exist and secret does not exist", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *op = nil + *s = nil + }, + }, + { + name: "OIDCProvider does not exist 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewCreateAction(secretGVR, namespace, goodSecret), + }, + }, + { + name: "OIDCProvider exists and invalid secret exists", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() + }, + 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + }, + }, + { + 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")) + }, + 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + }, + }, + { + name: "OIDCProvider exists and invalid secret exists and getting secret fails", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(1).Return(false) + }, + client: func(_ *pinnipedfake.Clientset, c *kubernetesfake.Clientset) { + c.PrependReactor("get", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("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: "OIDCProvider exists and secret does not exist and creating secret fails", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = nil + }, + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + }, + client: func(_ *pinnipedfake.Clientset, c *kubernetesfake.Clientset) { + c.PrependReactor("create", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("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: "OIDCProvider exists and invalid secret exists and updating secret fails", + secretHelper: func(secretHelper *mocksecrethelper.MockSecretHelper) { + secretHelper.EXPECT().Generate(goodOP).Times(1).Return(goodSecret, nil) + secretHelper.EXPECT().IsValid(goodOP, goodSecret).Times(2).Return(false) + }, + client: func(_ *pinnipedfake.Clientset, c *kubernetesfake.Clientset) { + c.PrependReactor("update", "secrets", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("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: "OIDCProvider exists and invalid secret exists and updating secret fails due to conflict", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() + }, + 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + client: func(_ *pinnipedfake.Clientset, c *kubernetesfake.Clientset) { + once := sync.Once{} + 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 + }) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + }, + 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), + }, + }, + { + name: "OIDCProvider exists and invalid secret exists and getting OIDCProvider fails", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() + }, + 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { + c.PrependReactor("get", "oidcproviders", func(_ kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + }, + wantError: fmt.Sprintf("failed to update oidcprovider: failed to get oidcprovider %s/%s: some get error", goodOPWithStatus.Namespace, goodOPWithStatus.Name), + }, + { + name: "OIDCProvider exists and invalid secret exists and updating OIDCProvider fails due to conflict", + storage: func(op **configv1alpha1.OIDCProvider, s **corev1.Secret) { + *s = invalidSecret.DeepCopy() + }, + 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().ObserveActiveSecretAndUpdateParentOIDCProvider(goodOP, goodSecret).Times(1).Return(goodOPWithStatus) + }, + client: func(c *pinnipedfake.Clientset, _ *kubernetesfake.Clientset) { + once := sync.Once{} + c.PrependReactor("update", "oidcproviders", 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 + }) + }, + wantOPActions: []kubetesting.Action{ + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + kubetesting.NewGetAction(opGVR, namespace, goodOP.Name), + kubetesting.NewUpdateAction(opGVR, namespace, goodOPWithStatus), + }, + wantSecretActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretGVR, namespace, goodSecret.Name), + kubetesting.NewUpdateAction(secretGVR, namespace, goodSecret), + }, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) + defer cancel() + + pinnipedAPIClient := pinnipedfake.NewSimpleClientset() + pinnipedInformerClient := pinnipedfake.NewSimpleClientset() + + kubeAPIClient := kubernetesfake.NewSimpleClientset() + kubeInformerClient := kubernetesfake.NewSimpleClientset() + + op := goodOP.DeepCopy() + secret := goodSecret.DeepCopy() + if test.storage != nil { + test.storage(&op, &secret) + } + if op != nil { + require.NoError(t, pinnipedAPIClient.Tracker().Add(op)) + 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.client != nil { + test.client(pinnipedAPIClient, kubeAPIClient) + } + + kubeInformers := kubeinformers.NewSharedInformerFactory( + kubeInformerClient, + 0, + ) + pinnipedInformers := pinnipedinformers.NewSharedInformerFactory( + pinnipedInformerClient, + 0, + ) + + ctrl := gomock.NewController(t) + t.Cleanup(ctrl.Finish) + secretHelper := mocksecrethelper.NewMockSecretHelper(ctrl) + secretHelper.EXPECT().NamePrefix().Times(1).Return("some-name") + if test.secretHelper != nil { + test.secretHelper(secretHelper) + } + + c := NewOIDCProviderSecretsController( + secretHelper, + kubeAPIClient, + pinnipedAPIClient, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviders(), + controllerlib.WithInformer, + ) + + // Must start informers before calling TestRunSynchronously(). + kubeInformers.Start(ctx.Done()) + pinnipedInformers.Start(ctx.Done()) + controllerlib.TestRunSynchronously(t, c) + + err := controllerlib.TestSync(t, c, controllerlib.Context{ + Context: ctx, + Key: controllerlib.Key{ + Namespace: namespace, + Name: opName, + }, + }) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + return + } + require.NoError(t, err) + + if test.wantOPActions == nil { + test.wantOPActions = []kubetesting.Action{} + } + require.Equal(t, test.wantOPActions, pinnipedAPIClient.Actions()) + if test.wantSecretActions == nil { + test.wantSecretActions = []kubetesting.Action{} + } + require.Equal(t, test.wantSecretActions, kubeAPIClient.Actions()) + }) + } +} + +func boolPtr(b bool) *bool { return &b } diff --git a/internal/controller/supervisorconfig/generator/secret_helper.go b/internal/controller/supervisorconfig/generator/secret_helper.go new file mode 100644 index 000000000..3a3362a66 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/secret_helper.go @@ -0,0 +1,151 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package generator + +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/plog" +) + +// 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 NamePrefix() that can be used to identify it from other SecretHelper instances. +type SecretHelper interface { + NamePrefix() string + Generate(*configv1alpha1.OIDCProvider) (*corev1.Secret, error) + IsValid(*configv1alpha1.OIDCProvider, *corev1.Secret) bool + ObserveActiveSecretAndUpdateParentOIDCProvider(*configv1alpha1.OIDCProvider, *corev1.Secret) *configv1alpha1.OIDCProvider +} + +const ( + // symmetricSecretType is corev1.Secret.Type of all corev1.Secret's generated by this helper. + symmetricSecretType = "secrets.pinniped.dev/symmetric" + // symmetricSecretDataKey is the corev1.Secret.Data key for the symmetric key value generated by this helper. + symmetricSecretDataKey = "key" + + // symmetricKeySize 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). + symmetricKeySize = 32 +) + +// SecretUsage describes how a cryptographic secret is going to be used. It is currently used to +// indicate to a SecretHelper which status field to set on the parent OIDCProvider for a Secret. +type SecretUsage int + +const ( + SecretUsageTokenSigningKey SecretUsage = iota + SecretUsageStateSigningKey + SecretUsageStateEncryptionKey +) + +// New returns a SecretHelper that has been parameterized with common symmetric secret generation +// knobs. +func NewSymmetricSecretHelper( + namePrefix string, + labels map[string]string, + rand io.Reader, + secretUsage SecretUsage, + updateCacheFunc func(cacheKey string, cacheValue []byte), +) SecretHelper { + return &symmetricSecretHelper{ + namePrefix: namePrefix, + labels: labels, + rand: rand, + secretUsage: secretUsage, + updateCacheFunc: updateCacheFunc, + } +} + +type symmetricSecretHelper struct { + namePrefix string + labels map[string]string + rand io.Reader + secretUsage SecretUsage + updateCacheFunc func(cacheKey string, cacheValue []byte) +} + +func (s *symmetricSecretHelper) NamePrefix() string { return s.namePrefix } + +// Generate implements SecretHelper.Generate(). +func (s *symmetricSecretHelper) Generate(parent *configv1alpha1.OIDCProvider) (*corev1.Secret, error) { + key := make([]byte, symmetricKeySize) + 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: symmetricSecretType, + Data: map[string][]byte{ + symmetricSecretDataKey: key, + }, + }, nil +} + +// IsValid implements SecretHelper.IsValid(). +func (s *symmetricSecretHelper) IsValid(parent *configv1alpha1.OIDCProvider, secret *corev1.Secret) bool { + if !metav1.IsControlledBy(secret, parent) { + return false + } + + if secret.Type != symmetricSecretType { + return false + } + + key, ok := secret.Data[symmetricSecretDataKey] + if !ok { + return false + } + if len(key) != symmetricKeySize { + return false + } + + return true +} + +// ObserveActiveSecretAndUpdateParentOIDCProvider implements SecretHelper.ObserveActiveSecretAndUpdateParentOIDCProvider(). +func (s *symmetricSecretHelper) ObserveActiveSecretAndUpdateParentOIDCProvider( + op *configv1alpha1.OIDCProvider, + secret *corev1.Secret, +) *configv1alpha1.OIDCProvider { + var cacheKey string + if op != nil { + cacheKey = op.Spec.Issuer + } + + s.updateCacheFunc(cacheKey, secret.Data[symmetricSecretDataKey]) + + switch s.secretUsage { + case SecretUsageTokenSigningKey: + op.Status.Secrets.TokenSigningKey.Name = secret.Name + case SecretUsageStateSigningKey: + op.Status.Secrets.StateSigningKey.Name = secret.Name + case SecretUsageStateEncryptionKey: + op.Status.Secrets.StateEncryptionKey.Name = secret.Name + default: + plog.Warning("unknown secret usage enum value: %d", s.secretUsage) + } + + return op +} diff --git a/internal/controller/supervisorconfig/generator/secret_helper_test.go b/internal/controller/supervisorconfig/generator/secret_helper_test.go new file mode 100644 index 000000000..bbaeb4e15 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/secret_helper_test.go @@ -0,0 +1,197 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package generator + +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 TestSymmetricSecretHelper(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + secretUsage SecretUsage + wantSetOIDCProviderField func(*configv1alpha1.OIDCProvider) string + }{ + { + name: "token signing key", + secretUsage: SecretUsageTokenSigningKey, + wantSetOIDCProviderField: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.TokenSigningKey.Name + }, + }, + { + name: "state signing key", + secretUsage: SecretUsageStateSigningKey, + wantSetOIDCProviderField: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.StateSigningKey.Name + }, + }, + { + name: "state encryption key", + secretUsage: SecretUsageStateEncryptionKey, + wantSetOIDCProviderField: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.StateEncryptionKey.Name + }, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + 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 oidcProviderIssuerValue string + var symmetricKeyValue []byte + h := NewSymmetricSecretHelper( + "some-name-prefix-", + labels, + randSource, + test.secretUsage, + func(oidcProviderIssuer string, symmetricKey []byte) { + require.True(t, oidcProviderIssuer == "" && symmetricKeyValue == nil, "expected notify func not to have been called yet") + oidcProviderIssuerValue = oidcProviderIssuer + symmetricKeyValue = symmetricKey + }, + ) + + 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.ObserveActiveSecretAndUpdateParentOIDCProvider(parent, child) + require.Equal(t, parent.Spec.Issuer, oidcProviderIssuerValue) + require.Equal(t, child.Name, test.wantSetOIDCProviderField(parent)) + require.Equal(t, child.Data["key"], symmetricKeyValue) + }) + } +} + +func TestSymmetricSecretHelperIsValid(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 := NewSymmetricSecretHelper("none of these args matter", nil, nil, SecretUsageTokenSigningKey, 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/controller/supervisorconfig/generator/supervisor_secrets.go b/internal/controller/supervisorconfig/generator/supervisor_secrets.go new file mode 100644 index 000000000..9f5ca1c46 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets.go @@ -0,0 +1,145 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package secretgenerator provides a supervisorSecretsController that can ensure existence of a generated secret. +package generator + +import ( + "context" + "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" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/client-go/kubernetes" + "k8s.io/client-go/util/retry" + "k8s.io/klog/v2" + + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/plog" +) + +// generateKey is stubbed out for the purpose of testing. The default behavior is to generate a symmetric key. +//nolint:gochecknoglobals +var generateKey = generateSymmetricKey + +type supervisorSecretsController struct { + owner *appsv1.Deployment + labels map[string]string + kubeClient kubernetes.Interface + secretInformer corev1informers.SecretInformer + setCacheFunc func(secret []byte) +} + +// NewSupervisorSecretsController instantiates a new controllerlib.Controller which will ensure existence of a generated secret. +func NewSupervisorSecretsController( + owner *appsv1.Deployment, + labels map[string]string, + kubeClient kubernetes.Interface, + secretInformer corev1informers.SecretInformer, + setCacheFunc func(secret []byte), + withInformer pinnipedcontroller.WithInformerOptionFunc, + initialEventFunc pinnipedcontroller.WithInitialEventOptionFunc, +) controllerlib.Controller { + c := supervisorSecretsController{ + owner: owner, + labels: labels, + kubeClient: kubeClient, + secretInformer: secretInformer, + setCacheFunc: setCacheFunc, + } + return controllerlib.New( + controllerlib.Config{Name: owner.Name + "-secret-generator", Syncer: &c}, + withInformer( + secretInformer, + pinnipedcontroller.SimpleFilter(func(obj metav1.Object) bool { + ownerReferences := obj.GetOwnerReferences() + for i := range obj.GetOwnerReferences() { + if ownerReferences[i].UID == owner.GetUID() { + return true + } + } + return false + }, nil), + controllerlib.InformerOption{}, + ), + initialEventFunc(controllerlib.Key{ + Namespace: owner.Namespace, + Name: owner.Name + "-key", + }), + ) +} + +// Sync implements controllerlib.Syncer.Sync(). +func (c *supervisorSecretsController) Sync(ctx controllerlib.Context) error { + secret, err := c.secretInformer.Lister().Secrets(ctx.Key.Namespace).Get(ctx.Key.Name) + isNotFound := k8serrors.IsNotFound(err) + if !isNotFound && err != nil { + return fmt.Errorf("failed to list secret %s/%s: %w", ctx.Key.Namespace, ctx.Key.Name, err) + } + + secretNeedsUpdate := isNotFound || !isValid(secret, c.labels) + if !secretNeedsUpdate { + plog.Debug("secret is up to date", "secret", klog.KObj(secret)) + c.setCacheFunc(secret.Data[symmetricSecretDataKey]) + return nil + } + + newSecret, err := generateSecret(ctx.Key.Namespace, ctx.Key.Name, c.labels, secretDataFunc, c.owner) + if err != nil { + return fmt.Errorf("failed to generate secret: %w", err) + } + + if isNotFound { + err = c.createSecret(ctx.Context, newSecret) + } else { + err = c.updateSecret(ctx.Context, &newSecret, ctx.Key.Name) + } + if err != nil { + return fmt.Errorf("failed to create/update secret %s/%s: %w", newSecret.Namespace, newSecret.Name, err) + } + + c.setCacheFunc(newSecret.Data[symmetricSecretDataKey]) + + return nil +} + +func (c *supervisorSecretsController) createSecret(ctx context.Context, newSecret *corev1.Secret) error { + _, err := c.kubeClient.CoreV1().Secrets(newSecret.Namespace).Create(ctx, newSecret, metav1.CreateOptions{}) + return err +} + +func (c *supervisorSecretsController) updateSecret(ctx context.Context, newSecret **corev1.Secret, secretName string) error { + secrets := c.kubeClient.CoreV1().Secrets((*newSecret).Namespace) + return retry.RetryOnConflict(retry.DefaultBackoff, func() error { + currentSecret, err := secrets.Get(ctx, secretName, metav1.GetOptions{}) + isNotFound := k8serrors.IsNotFound(err) + if !isNotFound && err != nil { + return fmt.Errorf("failed to get secret: %w", err) + } + + if isNotFound { + if err := c.createSecret(ctx, *newSecret); err != nil { + return fmt.Errorf("failed to create secret: %w", err) + } + return nil + } + + if isValid(currentSecret, c.labels) { + *newSecret = currentSecret + return nil + } + + currentSecret.Type = (*newSecret).Type + currentSecret.Data = (*newSecret).Data + for key, value := range c.labels { + currentSecret.Labels[key] = value + } + + _, err = secrets.Update(ctx, currentSecret, metav1.UpdateOptions{}) + return err + }) +} diff --git a/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go new file mode 100644 index 000000000..2522fe7d1 --- /dev/null +++ b/internal/controller/supervisorconfig/generator/supervisor_secrets_test.go @@ -0,0 +1,578 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package generator + +import ( + "context" + "errors" + "sync" + "testing" + "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" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + kubetesting "k8s.io/client-go/testing" + + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" +) + +var ( + owner = &appsv1.Deployment{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some-owner-name", + Namespace: "some-namespace", + UID: "some-owner-uid", + }, + } + + ownerGVK = schema.GroupVersionKind{ + Group: appsv1.SchemeGroupVersion.Group, + Version: appsv1.SchemeGroupVersion.Version, + Kind: "Deployment", + } + + labels = map[string]string{ + "some-label-key-1": "some-label-value-1", + "some-label-key-2": "some-label-value-2", + } +) + +func TestSupervisorSecretsControllerFilterSecret(t *testing.T) { + t.Parallel() + + tests := []struct { + name string + secret corev1.Secret + wantAdd bool + wantUpdate bool + wantDelete bool + }{ + { + name: "owner reference is missing", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + }, + }, + }, + { + name: "owner reference with incorrect `APIVersion`", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Name: owner.GetName(), + Kind: ownerGVK.Kind, + UID: owner.GetUID(), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "owner reference with incorrect `Kind`", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ownerGVK.String(), + Name: owner.GetName(), + Kind: "IncorrectKind", + UID: owner.GetUID(), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "owner reference with `Controller`: true", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + *metav1.NewControllerRef(owner, ownerGVK), + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "expected owner reference with incorrect `UID`", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ownerGVK.String(), + Name: owner.GetName(), + Kind: ownerGVK.Kind, + UID: "DOES_NOT_MATCH", + }, + }, + }, + }, + }, + { + name: "expected owner reference - where `Controller`: false", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ownerGVK.String(), + Name: owner.GetName(), + Kind: ownerGVK.Kind, + UID: owner.GetUID(), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + { + name: "multiple owner references (expected owner reference, and one more)", + secret: corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "some-namespace", + OwnerReferences: []metav1.OwnerReference{ + { + Kind: "UnrelatedKind", + }, + { + APIVersion: ownerGVK.String(), + Name: owner.GetName(), + Kind: ownerGVK.Kind, + UID: owner.GetUID(), + }, + }, + }, + }, + wantAdd: true, + wantUpdate: true, + wantDelete: true, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + secretInformer := kubeinformers.NewSharedInformerFactory( + kubernetesfake.NewSimpleClientset(), + 0, + ).Core().V1().Secrets() + withInformer := testutil.NewObservableWithInformerOption() + _ = NewSupervisorSecretsController( + owner, + labels, + nil, // kubeClient, not needed + secretInformer, + nil, // setCache, not needed + withInformer.WithInformer, + testutil.NewObservableWithInitialEventOption().WithInitialEvent, + ) + + unrelated := corev1.Secret{} + filter := withInformer.GetFilterForInformer(secretInformer) + require.Equal(t, test.wantAdd, filter.Add(&test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&unrelated, &test.secret)) + require.Equal(t, test.wantUpdate, filter.Update(&test.secret, &unrelated)) + require.Equal(t, test.wantDelete, filter.Delete(&test.secret)) + }) + } +} + +func TestSupervisorSecretsControllerInitialEvent(t *testing.T) { + initialEventOption := testutil.NewObservableWithInitialEventOption() + secretInformer := kubeinformers.NewSharedInformerFactory( + kubernetesfake.NewSimpleClientset(), + 0, + ).Core().V1().Secrets() + _ = NewSupervisorSecretsController( + owner, + nil, + nil, // kubeClient, not needed + secretInformer, + nil, // setCache, not needed + testutil.NewObservableWithInformerOption().WithInformer, + initialEventOption.WithInitialEvent, + ) + require.Equal(t, &controllerlib.Key{ + Namespace: owner.Namespace, + Name: owner.Name + "-key", + }, initialEventOption.GetInitialEventKey()) +} + +func TestSupervisorSecretsControllerSync(t *testing.T) { + const ( + generatedSecretNamespace = "some-namespace" + generatedSecretName = "some-name-abc123" + ) + + var ( + secretsGVR = schema.GroupVersionResource{ + Group: corev1.SchemeGroupVersion.Group, + Version: corev1.SchemeGroupVersion.Version, + Resource: "secrets", + } + + generatedSymmetricKey = []byte("some-neato-32-byte-generated-key") + otherGeneratedSymmetricKey = []byte("some-funio-32-byte-generated-key") + + blockOwnerDeletion = true + isController = false + generatedSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: generatedSecretName, + Namespace: generatedSecretNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ownerGVK.GroupVersion().String(), + Kind: ownerGVK.Kind, + Name: owner.GetName(), + UID: owner.GetUID(), + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, + }, + }, + Labels: labels, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": generatedSymmetricKey, + }, + } + + otherGeneratedSecret = &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: generatedSecretName, + Namespace: generatedSecretNamespace, + OwnerReferences: []metav1.OwnerReference{ + { + APIVersion: ownerGVK.GroupVersion().String(), + Kind: ownerGVK.Kind, + Name: owner.GetName(), + UID: owner.GetUID(), + BlockOwnerDeletion: &blockOwnerDeletion, + Controller: &isController, + }, + }, + Labels: labels, + }, + Type: "secrets.pinniped.dev/symmetric", + Data: map[string][]byte{ + "key": otherGeneratedSymmetricKey, + }, + } + ) + + // Add an extra label to make sure we don't overwrite existing labels on a Secret. + generatedSecret.Labels["extra-label-key"] = "extra-label-value" + + 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 + wantCallbackSecret []byte + }{ + { + name: "when the secrets does not exist, it gets generated", + storedSecret: func(secret **corev1.Secret) { + *secret = nil + }, + 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", + storedSecret: func(secret **corev1.Secret) { + (*secret).Type = "wrong" + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "secret gets updated when the key data does not exist", + storedSecret: func(secret **corev1.Secret) { + delete((*secret).Data, "key") + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "secret gets updated when the key data is too short", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "an error is returned when creating fails", + storedSecret: func(secret **corev1.Secret) { + *secret = nil + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: some create error", + }, + { + name: "an error is returned when updating fails", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("update", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some update error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: some update error", + }, + { + name: "an error is returned when getting fails", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: failed to get secret: some get error", + }, + { + name: "the update is retried when it fails due to a conflict", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("update", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + var err error + once.Do(func() { + err = k8serrors.NewConflict(secretsGVR.GroupResource(), generatedSecretName, errors.New("some error")) + }) + return true, nil, err + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "upon updating we discover that a valid secret exists", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, otherGeneratedSecret, nil + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + }, + wantCallbackSecret: otherGeneratedSymmetricKey, + }, + { + name: "upon updating we discover that a secret with missing labels exists", + storedSecret: func(secret **corev1.Secret) { + delete((*secret).Labels, "some-label-key-1") + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "upon updating we discover that a secret with incorrect labels exists", + storedSecret: func(secret **corev1.Secret) { + (*secret).Labels["some-label-key-1"] = "incorrect" + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewUpdateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantCallbackSecret: generatedSymmetricKey, + }, + { + name: "upon updating we discover that the secret has been deleted", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, k8serrors.NewNotFound(secretsGVR.GroupResource(), generatedSecretName) + }) + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, nil + }) + }, + wantActions: []kubetesting.Action{ + 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", + storedSecret: func(secret **corev1.Secret) { + (*secret).Data["key"] = []byte("too short") // force updating + }, + apiClient: func(t *testing.T, client *kubernetesfake.Clientset) { + client.PrependReactor("get", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, k8serrors.NewNotFound(secretsGVR.GroupResource(), generatedSecretName) + }) + client.PrependReactor("create", "secrets", func(action kubetesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some create error") + }) + }, + wantActions: []kubetesting.Action{ + kubetesting.NewGetAction(secretsGVR, generatedSecretNamespace, generatedSecretName), + kubetesting.NewCreateAction(secretsGVR, generatedSecretNamespace, generatedSecret), + }, + wantError: "failed to create/update secret some-namespace/some-name-abc123: failed to create secret: some create error", + }, + { + name: "when generating the secret fails, we return an error", + storedSecret: func(secret **corev1.Secret) { + *secret = nil + }, + generateKey: func() ([]byte, error) { + return nil, errors.New("some generate error") + }, + wantError: "failed to generate secret: some generate error", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + // We cannot currently run this test in parallel since it uses the global generateKey function. + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) + defer cancel() + + if test.generateKey != nil { + generateKey = test.generateKey + } else { + generateKey = func() ([]byte, error) { + return generatedSymmetricKey, nil + } + } + + apiClient := kubernetesfake.NewSimpleClientset() + if test.apiClient != nil { + test.apiClient(t, apiClient) + } + informerClient := kubernetesfake.NewSimpleClientset() + + storedSecret := generatedSecret.DeepCopy() + if test.storedSecret != nil { + test.storedSecret(&storedSecret) + } + if storedSecret != nil { + require.NoError(t, apiClient.Tracker().Add(storedSecret)) + require.NoError(t, informerClient.Tracker().Add(storedSecret)) + } + + informers := kubeinformers.NewSharedInformerFactory(informerClient, 0) + secrets := informers.Core().V1().Secrets() + + var callbackSecret []byte + c := NewSupervisorSecretsController( + owner, + labels, + apiClient, + secrets, + func(secret []byte) { + require.Nil(t, callbackSecret, "callback was called twice") + callbackSecret = secret + }, + testutil.NewObservableWithInformerOption().WithInformer, + testutil.NewObservableWithInitialEventOption().WithInitialEvent, + ) + + // Must start informers before calling TestRunSynchronously(). + informers.Start(ctx.Done()) + controllerlib.TestRunSynchronously(t, c) + + err := controllerlib.TestSync(t, c, controllerlib.Context{ + Context: ctx, + Key: controllerlib.Key{ + Namespace: generatedSecretNamespace, + Name: generatedSecretName, + }, + }) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + } + + if test.wantActions == nil { + test.wantActions = []kubetesting.Action{} + } + require.Equal(t, test.wantActions, apiClient.Actions()) + + require.Equal(t, test.wantCallbackSecret, callbackSecret) + }) + } +} diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index 5c01c5b46..efbdfae92 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -76,7 +76,7 @@ func (c *jwksObserverController) Sync(ctx controllerlib.Context) error { issuerToActiveJWKMap := map[string]*jose.JSONWebKey{} for _, provider := range allProviders { - secretRef := provider.Status.JWKSSecret + secretRef := provider.Status.Secrets.JWKS jwksSecret, err := c.secretInformer.Lister().Secrets(ns).Get(secretRef.Name) if err != nil { plog.Debug("jwksObserverController Sync could not find JWKS secret", "namespace", ns, "secretName", secretRef.Name) diff --git a/internal/controller/supervisorconfig/jwks_observer_test.go b/internal/controller/supervisorconfig/jwks_observer_test.go index ad7323b80..fa2ebea1b 100644 --- a/internal/controller/supervisorconfig/jwks_observer_test.go +++ b/internal/controller/supervisorconfig/jwks_observer_test.go @@ -202,7 +202,7 @@ func TestJWKSObserverControllerSync(t *testing.T) { Namespace: installedInNamespace, }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://no-secret-issuer1.com"}, - Status: v1alpha1.OIDCProviderStatus{}, // no JWKSSecret field + Status: v1alpha1.OIDCProviderStatus{}, // no Secrets.JWKS field } oidcProviderWithoutSecret2 := &v1alpha1.OIDCProvider{ ObjectMeta: metav1.ObjectMeta{ @@ -219,7 +219,9 @@ func TestJWKSObserverControllerSync(t *testing.T) { }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-secret-issuer.com"}, Status: v1alpha1.OIDCProviderStatus{ - JWKSSecret: corev1.LocalObjectReference{Name: "bad-secret-name"}, + Secrets: v1alpha1.OIDCProviderSecrets{ + JWKS: corev1.LocalObjectReference{Name: "bad-secret-name"}, + }, }, } oidcProviderWithBadJWKSSecret := &v1alpha1.OIDCProvider{ @@ -229,7 +231,9 @@ func TestJWKSObserverControllerSync(t *testing.T) { }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-jwks-secret-issuer.com"}, Status: v1alpha1.OIDCProviderStatus{ - JWKSSecret: corev1.LocalObjectReference{Name: "bad-jwks-secret-name"}, + Secrets: v1alpha1.OIDCProviderSecrets{ + JWKS: corev1.LocalObjectReference{Name: "bad-jwks-secret-name"}, + }, }, } oidcProviderWithBadActiveJWKSecret := &v1alpha1.OIDCProvider{ @@ -239,7 +243,9 @@ func TestJWKSObserverControllerSync(t *testing.T) { }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://bad-active-jwk-secret-issuer.com"}, Status: v1alpha1.OIDCProviderStatus{ - JWKSSecret: corev1.LocalObjectReference{Name: "bad-active-jwk-secret-name"}, + Secrets: v1alpha1.OIDCProviderSecrets{ + JWKS: corev1.LocalObjectReference{Name: "bad-active-jwk-secret-name"}, + }, }, } oidcProviderWithGoodSecret1 := &v1alpha1.OIDCProvider{ @@ -249,7 +255,9 @@ func TestJWKSObserverControllerSync(t *testing.T) { }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://issuer-with-good-secret1.com"}, Status: v1alpha1.OIDCProviderStatus{ - JWKSSecret: corev1.LocalObjectReference{Name: "good-jwks-secret-name1"}, + Secrets: v1alpha1.OIDCProviderSecrets{ + JWKS: corev1.LocalObjectReference{Name: "good-jwks-secret-name1"}, + }, }, } oidcProviderWithGoodSecret2 := &v1alpha1.OIDCProvider{ @@ -259,7 +267,9 @@ func TestJWKSObserverControllerSync(t *testing.T) { }, Spec: v1alpha1.OIDCProviderSpec{Issuer: "https://issuer-with-good-secret2.com"}, Status: v1alpha1.OIDCProviderStatus{ - JWKSSecret: corev1.LocalObjectReference{Name: "good-jwks-secret-name2"}, + Secrets: v1alpha1.OIDCProviderSecrets{ + JWKS: corev1.LocalObjectReference{Name: "good-jwks-secret-name2"}, + }, }, } expectedJWK1 = string(readJWKJSON(t, "testdata/public-jwk.json")) diff --git a/internal/controller/supervisorconfig/jwks_writer.go b/internal/controller/supervisorconfig/jwks_writer.go index 4e6a51a13..2a33b77f6 100644 --- a/internal/controller/supervisorconfig/jwks_writer.go +++ b/internal/controller/supervisorconfig/jwks_writer.go @@ -169,7 +169,7 @@ func (c *jwksWriterController) Sync(ctx controllerlib.Context) error { // Ensure that the OPC points to the secret. newOPC := opc.DeepCopy() - newOPC.Status.JWKSSecret.Name = secret.Name + newOPC.Status.Secrets.JWKS.Name = secret.Name if err := c.updateOPC(ctx.Context, newOPC); err != nil { return fmt.Errorf("cannot update opc: %w", err) } @@ -179,13 +179,13 @@ func (c *jwksWriterController) Sync(ctx controllerlib.Context) error { } func (c *jwksWriterController) secretNeedsUpdate(opc *configv1alpha1.OIDCProvider) (bool, error) { - if opc.Status.JWKSSecret.Name == "" { + if opc.Status.Secrets.JWKS.Name == "" { // If the OPC says it doesn't have a secret associated with it, then let's create one. return true, nil } // 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(opc.Status.JWKSSecret.Name) + secret, err := c.secretInformer.Lister().Secrets(opc.Namespace).Get(opc.Status.Secrets.JWKS.Name) notFound := k8serrors.IsNotFound(err) if err != nil && !notFound { return false, fmt.Errorf("cannot get secret: %w", err) @@ -301,12 +301,12 @@ func (c *jwksWriterController) updateOPC( return fmt.Errorf("cannot get opc: %w", err) } - if newOPC.Status.JWKSSecret.Name == oldOPC.Status.JWKSSecret.Name { + if newOPC.Status.Secrets.JWKS.Name == oldOPC.Status.Secrets.JWKS.Name { // If the existing OPC is up to date, we don't need to update it. return nil } - oldOPC.Status.JWKSSecret.Name = newOPC.Status.JWKSSecret.Name + oldOPC.Status.Secrets.JWKS.Name = newOPC.Status.Secrets.JWKS.Name _, err = opcClient.Update(ctx, oldOPC, metav1.UpdateOptions{}) return err }) diff --git a/internal/controller/supervisorconfig/jwks_writer_test.go b/internal/controller/supervisorconfig/jwks_writer_test.go index 9afdc4861..615a1f3d7 100644 --- a/internal/controller/supervisorconfig/jwks_writer_test.go +++ b/internal/controller/supervisorconfig/jwks_writer_test.go @@ -253,7 +253,7 @@ func TestJWKSWriterControllerSync(t *testing.T) { }, } goodOPCWithStatus := goodOPC.DeepCopy() - goodOPCWithStatus.Status.JWKSSecret.Name = goodOPCWithStatus.Name + "-jwks" + goodOPCWithStatus.Status.Secrets.JWKS.Name = goodOPCWithStatus.Name + "-jwks" secretGVR := schema.GroupVersionResource{ Group: corev1.SchemeGroupVersion.Group, @@ -264,7 +264,7 @@ func TestJWKSWriterControllerSync(t *testing.T) { newSecret := func(activeJWKPath, jwksPath string) *corev1.Secret { s := corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ - Name: goodOPCWithStatus.Status.JWKSSecret.Name, + Name: goodOPCWithStatus.Status.Secrets.JWKS.Name, Namespace: namespace, Labels: map[string]string{ "myLabelKey1": "myLabelValue1", diff --git a/internal/downward/downward.go b/internal/downward/downward.go index ee4d65b75..75119dc4c 100644 --- a/internal/downward/downward.go +++ b/internal/downward/downward.go @@ -13,6 +13,8 @@ import ( "path/filepath" "strconv" "strings" + + "go.pinniped.dev/internal/plog" ) // PodInfo contains pod metadata about the current pod. @@ -20,6 +22,9 @@ type PodInfo struct { // Namespace where the current pod is running. Namespace string + // Name of the current pod. + Name string + // Labels of the current pod. Labels map[string]string } @@ -33,6 +38,13 @@ func Load(directory string) (*PodInfo, error) { } result.Namespace = strings.TrimSpace(string(ns)) + name, err := ioutil.ReadFile(filepath.Join(directory, "name")) + if err != nil { + plog.Warning("could not read 'name' downward API file") + } else { + result.Name = strings.TrimSpace(string(name)) + } + labels, err := ioutil.ReadFile(filepath.Join(directory, "labels")) if err != nil { return nil, fmt.Errorf("could not load labels: %w", err) diff --git a/internal/downward/downward_test.go b/internal/downward/downward_test.go index 9bc954d56..adc15b407 100644 --- a/internal/downward/downward_test.go +++ b/internal/downward/downward_test.go @@ -35,6 +35,15 @@ func TestLoad(t *testing.T) { { name: "valid", inputDir: "./testdata/valid", + want: &PodInfo{ + Namespace: "test-namespace", + Name: "test-name", + Labels: map[string]string{"foo": "bar", "bat": "baz"}, + }, + }, + { + name: "valid without name", + inputDir: "./testdata/validwithoutname", want: &PodInfo{ Namespace: "test-namespace", Labels: map[string]string{"foo": "bar", "bat": "baz"}, diff --git a/internal/downward/testdata/valid/name b/internal/downward/testdata/valid/name new file mode 100644 index 000000000..2fdecf1e0 --- /dev/null +++ b/internal/downward/testdata/valid/name @@ -0,0 +1 @@ +test-name diff --git a/internal/downward/testdata/validwithoutname/labels b/internal/downward/testdata/validwithoutname/labels new file mode 100644 index 000000000..e5880cda4 --- /dev/null +++ b/internal/downward/testdata/validwithoutname/labels @@ -0,0 +1,2 @@ +foo="bar" +bat="baz" diff --git a/internal/downward/testdata/validwithoutname/namespace b/internal/downward/testdata/validwithoutname/namespace new file mode 100644 index 000000000..f2605f230 --- /dev/null +++ b/internal/downward/testdata/validwithoutname/namespace @@ -0,0 +1 @@ +test-namespace diff --git a/internal/mocks/mocksecrethelper/generate.go b/internal/mocks/mocksecrethelper/generate.go new file mode 100644 index 000000000..31d52cb41 --- /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 000000000..68b6cfbdc --- /dev/null +++ b/internal/mocks/mocksecrethelper/mocksecrethelper.go @@ -0,0 +1,96 @@ +// 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) +} + +// NamePrefix mocks base method +func (m *MockSecretHelper) NamePrefix() string { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "NamePrefix") + ret0, _ := ret[0].(string) + return ret0 +} + +// NamePrefix indicates an expected call of NamePrefix +func (mr *MockSecretHelperMockRecorder) NamePrefix() *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "NamePrefix", reflect.TypeOf((*MockSecretHelper)(nil).NamePrefix)) +} + +// ObserveActiveSecretAndUpdateParentOIDCProvider mocks base method +func (m *MockSecretHelper) ObserveActiveSecretAndUpdateParentOIDCProvider(arg0 *v1alpha1.OIDCProvider, arg1 *v1.Secret) *v1alpha1.OIDCProvider { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ObserveActiveSecretAndUpdateParentOIDCProvider", arg0, arg1) + ret0, _ := ret[0].(*v1alpha1.OIDCProvider) + return ret0 +} + +// ObserveActiveSecretAndUpdateParentOIDCProvider indicates an expected call of ObserveActiveSecretAndUpdateParentOIDCProvider +func (mr *MockSecretHelperMockRecorder) ObserveActiveSecretAndUpdateParentOIDCProvider(arg0, arg1 interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ObserveActiveSecretAndUpdateParentOIDCProvider", reflect.TypeOf((*MockSecretHelper)(nil).ObserveActiveSecretAndUpdateParentOIDCProvider), arg0, arg1) +} diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index e0f7eaeac..253a112cb 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -127,10 +127,10 @@ func TestAuthorizationEndpoint(t *testing.T) { // Configure fosite the same way that the production code would, using NullStorage to turn off storage. oauthStore := oidc.NullStorage{} - hmacSecret := []byte("some secret - must have at least 32 bytes") - require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") + hmacSecretFunc := func() []byte { return []byte("some secret - must have at least 32 bytes") } + require.GreaterOrEqual(t, len(hmacSecretFunc()), 32, "fosite requires that hmac secrets have at least 32 bytes") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, oidc.DefaultOIDCTimeoutsConfiguration()) happyCSRF := "test-csrf" happyPKCE := "test-pkce" diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 752e07d26..8641212d1 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -461,10 +461,10 @@ func TestCallbackEndpoint(t *testing.T) { // Inject this into our test subject at the last second so we get a fresh storage for every test. timeoutsConfiguration := oidc.DefaultOIDCTimeoutsConfiguration() oauthStore := oidc.NewKubeStorage(secrets, timeoutsConfiguration) - hmacSecret := []byte("some secret - must have at least 32 bytes") - require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") + hmacSecretFunc := func() []byte { return []byte("some secret - must have at least 32 bytes") } + require.GreaterOrEqual(t, len(hmacSecretFunc()), 32, "fosite requires that hmac secrets have at least 32 bytes") jwksProviderIsUnused := jwks.NewDynamicJWKSProvider() - oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecret, jwksProviderIsUnused, timeoutsConfiguration) + oauthHelper := oidc.FositeOauth2Helper(oauthStore, downstreamIssuer, hmacSecretFunc, jwksProviderIsUnused, timeoutsConfiguration) idpListGetter := oidctestutil.NewIDPListGetter(&test.idp) subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec, happyUpstreamRedirectURI) diff --git a/internal/oidc/dynamic_oauth2_hmac_strategy.go b/internal/oidc/dynamic_oauth2_hmac_strategy.go new file mode 100644 index 000000000..8d67dc2ee --- /dev/null +++ b/internal/oidc/dynamic_oauth2_hmac_strategy.go @@ -0,0 +1,98 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package oidc + +import ( + "context" + + "github.com/ory/fosite" + "github.com/ory/fosite/compose" + "github.com/ory/fosite/handler/oauth2" +) + +// dynamicOauth2HMACStrategy is an oauth2.CoreStrategy that can dynamically load an HMAC key to sign +// stuff (access tokens, refresh tokens, and auth codes). We want this dynamic capability since our +// controllers for loading OIDCProvider's and signing keys run in parallel, and thus the signing key +// might not be ready when an OIDCProvider is otherwise ready. +// +// If we ever update OIDCProvider's to hold their signing key, we might not need this type, since we +// could have an invariant that routes to an OIDCProvider's endpoints are only wired up if an +// OIDCProvider has a valid signing key. +type dynamicOauth2HMACStrategy struct { + fositeConfig *compose.Config + keyFunc func() []byte +} + +var _ oauth2.CoreStrategy = &dynamicOauth2HMACStrategy{} + +func newDynamicOauth2HMACStrategy( + fositeConfig *compose.Config, + keyFunc func() []byte, +) *dynamicOauth2HMACStrategy { + return &dynamicOauth2HMACStrategy{ + fositeConfig: fositeConfig, + keyFunc: keyFunc, + } +} + +func (s *dynamicOauth2HMACStrategy) AccessTokenSignature(token string) string { + return s.delegate().AccessTokenSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateAccessToken( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateAccessToken(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateAccessToken( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateAccessToken(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) RefreshTokenSignature(token string) string { + return s.delegate().RefreshTokenSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateRefreshToken( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateRefreshToken(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateRefreshToken( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateRefreshToken(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) AuthorizeCodeSignature(token string) string { + return s.delegate().AuthorizeCodeSignature(token) +} + +func (s *dynamicOauth2HMACStrategy) GenerateAuthorizeCode( + ctx context.Context, + requester fosite.Requester, +) (token string, signature string, err error) { + return s.delegate().GenerateAuthorizeCode(ctx, requester) +} + +func (s *dynamicOauth2HMACStrategy) ValidateAuthorizeCode( + ctx context.Context, + requester fosite.Requester, + token string, +) (err error) { + return s.delegate().ValidateAuthorizeCode(ctx, requester, token) +} + +func (s *dynamicOauth2HMACStrategy) delegate() *oauth2.HMACSHAStrategy { + return compose.NewOAuth2HMACStrategy(s.fositeConfig, s.keyFunc(), nil) +} diff --git a/internal/oidc/dynamiccodec/codec.go b/internal/oidc/dynamiccodec/codec.go new file mode 100644 index 000000000..5168b2b77 --- /dev/null +++ b/internal/oidc/dynamiccodec/codec.go @@ -0,0 +1,58 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +// Package dynamiccodec provides a type that can encode information using a just-in-time signing and +// (optionally) encryption secret. +package dynamiccodec + +import ( + "time" + + "github.com/gorilla/securecookie" + + "go.pinniped.dev/internal/oidc" +) + +var _ oidc.Codec = &Codec{} + +// KeyFunc returns a single key: a symmetric key. +type KeyFunc func() []byte + +// Codec can dynamically encode and decode information by using a KeyFunc to get its keys +// just-in-time. +type Codec struct { + lifespan time.Duration + signingKeyFunc KeyFunc + encryptionKeyFunc KeyFunc +} + +// New creates a new Codec that will use the provided keyFuncs for its key source, and +// use the securecookie.JSONEncoder. The securecookie.JSONEncoder is used because the default +// securecookie.GobEncoder is less compact and more difficult to make forward compatible. +// +// The returned Codec will make ensure that the encoded values will only be valid for the provided +// lifespan. +func New(lifespan time.Duration, signingKeyFunc, encryptionKeyFunc KeyFunc) *Codec { + return &Codec{ + lifespan: lifespan, + signingKeyFunc: signingKeyFunc, + encryptionKeyFunc: encryptionKeyFunc, + } +} + +// Encode implements oidc.Encode(). +func (c *Codec) Encode(name string, value interface{}) (string, error) { + return c.delegate().Encode(name, value) +} + +// Decode implements oidc.Decode(). +func (c *Codec) Decode(name string, value string, into interface{}) error { + return c.delegate().Decode(name, value, into) +} + +func (c *Codec) delegate() *securecookie.SecureCookie { + codec := securecookie.New(c.signingKeyFunc(), c.encryptionKeyFunc()) + codec.MaxAge(int(c.lifespan.Seconds())) + codec.SetSerializer(securecookie.JSONEncoder{}) + return codec +} diff --git a/internal/oidc/dynamiccodec/codec_test.go b/internal/oidc/dynamiccodec/codec_test.go new file mode 100644 index 000000000..bcff2d038 --- /dev/null +++ b/internal/oidc/dynamiccodec/codec_test.go @@ -0,0 +1,127 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package dynamiccodec + +import ( + "strings" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestCodec(t *testing.T) { + tests := []struct { + name string + lifespan time.Duration + keys func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) + wantEncoderErrorPrefix string + wantDecoderError string + }{ + { + name: "good signing and encryption keys", + }, + { + name: "good signing keys and no encryption key", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *encoderEncryptionKey = nil + *decoderEncryptionKey = nil + }, + }, + { + name: "good signing keys and bad encoding encryption key", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *encoderEncryptionKey = []byte("this-secret-is-not-16-bytes") + }, + wantEncoderErrorPrefix: "securecookie: error - caused by: crypto/aes: invalid key size 27", + }, + { + name: "good signing keys and bad decoding encryption key", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *decoderEncryptionKey = []byte("this-secret-is-not-16-bytes") + }, + wantDecoderError: "securecookie: error - caused by: crypto/aes: invalid key size 27", + }, + { + name: "aaa encoder times stuff out", + lifespan: time.Second, + wantDecoderError: "securecookie: expired timestamp", + }, + { + name: "bad encoder signing key", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *encoderSigningKey = nil + }, + wantEncoderErrorPrefix: "securecookie: hash key is not set", + }, + { + name: "bad decoder signing key", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *decoderSigningKey = nil + }, + wantDecoderError: "securecookie: hash key is not set", + }, + { + name: "signing key mismatch", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *encoderSigningKey = []byte("this key does not match the decoder key") + }, + wantDecoderError: "securecookie: the value is not valid", + }, + { + name: "encryption key mismatch", + keys: func(encoderSigningKey, encoderEncryptionKey, decoderSigningKey, decoderEncryptionKey *[]byte) { + *encoderEncryptionKey = []byte("16-byte-no-match") + }, + wantDecoderError: "securecookie: error - caused by: securecookie: error - caused by: ", + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + var ( + encoderSigningKey = []byte("some-signing-key") + encoderEncryptionKey = []byte("16-byte-encr-key") + decoderSigningKey = []byte("some-signing-key") + decoderEncryptionKey = []byte("16-byte-encr-key") + ) + if test.keys != nil { + test.keys(&encoderSigningKey, &encoderEncryptionKey, &decoderSigningKey, &decoderEncryptionKey) + } + + lifespan := test.lifespan + if lifespan == 0 { + lifespan = time.Hour + } + + encoder := New(lifespan, func() []byte { return encoderSigningKey }, + func() []byte { return encoderEncryptionKey }) + + encoded, err := encoder.Encode("some-name", "some-message") + if test.wantEncoderErrorPrefix != "" { + require.EqualError(t, err, test.wantEncoderErrorPrefix) + return + } + require.NoError(t, err) + + if test.lifespan != 0 { + time.Sleep(test.lifespan + time.Second) + } + + decoder := New(lifespan, func() []byte { return decoderSigningKey }, + func() []byte { return decoderEncryptionKey }) + + var decoded string + err = decoder.Decode("some-name", encoded, &decoded) + if test.wantDecoderError != "" { + require.Error(t, err) + require.True(t, strings.HasPrefix(err.Error(), test.wantDecoderError), "expected %q to start with %q", err.Error(), test.wantDecoderError) + return + } + require.NoError(t, err) + + require.Equal(t, "some-message", decoded) + }) + } +} diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index 152509aa7..94a697b08 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -44,6 +44,11 @@ const ( // CSRFCookieEncodingName is the `name` passed to the encoder for encoding and decoding the CSRF // cookie contents. CSRFCookieEncodingName = "csrf" + + // CSRFCookieLifespan is the length of time that the CSRF cookie is valid. After this time, the + // Supervisor's authorization endpoint should give the browser a new CSRF cookie. We set it to + // a week so that it is unlikely to expire during a login. + CSRFCookieLifespan = time.Hour * 24 * 7 ) // Encoder is the encoding side of the securecookie.Codec interface. @@ -187,7 +192,7 @@ func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration { func FositeOauth2Helper( oauthStore interface{}, issuer string, - hmacSecretOfLengthAtLeast32 []byte, + hmacSecretOfLengthAtLeast32Func func() []byte, jwksProvider jwks.DynamicJWKSProvider, timeoutsConfiguration TimeoutsConfiguration, ) fosite.OAuth2Provider { @@ -220,7 +225,7 @@ func FositeOauth2Helper( oauthStore, &compose.CommonStrategy{ // Note that Fosite requires the HMAC secret to be at least 32 bytes. - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32, nil), + CoreStrategy: newDynamicOauth2HMACStrategy(oauthConfig, hmacSecretOfLengthAtLeast32Func), OpenIDConnectTokenStrategy: newDynamicOpenIDConnectECDSAStrategy(oauthConfig, jwksProvider), }, nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 907bbd73f..e27d6b9d4 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -8,7 +8,10 @@ import ( "strings" "sync" - "github.com/gorilla/securecookie" + "go.pinniped.dev/internal/secret" + + "go.pinniped.dev/internal/oidc/dynamiccodec" + corev1client "k8s.io/client-go/kubernetes/typed/core/v1" "go.pinniped.dev/internal/oidc" @@ -34,6 +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 + secretCache *secret.Cache // in-memory cache of cryptographic material secretsClient corev1client.SecretInterface } @@ -45,6 +49,7 @@ func NewManager( nextHandler http.Handler, dynamicJWKSProvider jwks.DynamicJWKSProvider, idpListGetter oidc.IDPListGetter, + secretCache *secret.Cache, secretsClient corev1client.SecretInterface, ) *Manager { return &Manager{ @@ -52,6 +57,7 @@ func NewManager( nextHandler: nextHandler, dynamicJWKSProvider: dynamicJWKSProvider, idpListGetter: idpListGetter, + secretCache: secretCache, secretsClient: secretsClient, } } @@ -71,33 +77,32 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { m.providers = oidcProviders m.providerHandlers = make(map[string]http.Handler) + var csrfCookieEncoder = dynamiccodec.New( + oidc.CSRFCookieLifespan, + m.secretCache.GetCSRFCookieEncoderHashKey, + func() []byte { return nil }, + ) + for _, incomingProvider := range oidcProviders { issuer := incomingProvider.Issuer() issuerHostWithPath := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() - fositeHMACSecretForThisProvider := []byte("some secret - must have at least 32 bytes") // TODO replace this secret + tokenHMACKeyGetter := wrapGetter(incomingProvider.Issuer(), m.secretCache.GetTokenHMACKey) timeoutsConfiguration := oidc.DefaultOIDCTimeoutsConfiguration() // Use NullStorage for the authorize endpoint because we do not actually want to store anything until // the upstream callback endpoint is called later. - oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, fositeHMACSecretForThisProvider, nil, timeoutsConfiguration) + oauthHelperWithNullStorage := oidc.FositeOauth2Helper(oidc.NullStorage{}, issuer, tokenHMACKeyGetter, nil, timeoutsConfiguration) // For all the other endpoints, make another oauth helper with exactly the same settings except use real storage. - oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient, timeoutsConfiguration), issuer, fositeHMACSecretForThisProvider, m.dynamicJWKSProvider, timeoutsConfiguration) + oauthHelperWithKubeStorage := oidc.FositeOauth2Helper(oidc.NewKubeStorage(m.secretsClient, timeoutsConfiguration), issuer, tokenHMACKeyGetter, m.dynamicJWKSProvider, timeoutsConfiguration) - // TODO use different codecs for the state and the cookie, because: - // 1. we would like to state to have an embedded expiration date while the cookie does not need that - // 2. we would like each downstream provider to use different secrets for signing/encrypting the upstream state, not share secrets - // 3. we would like *all* downstream providers to use the *same* signing key for the CSRF cookie (which doesn't need to be encrypted) because cookies are sent per-domain and our issuers can share a domain name (but have different paths) - var upstreamStateEncoderHashKey = []byte("fake-state-hash-secret") // TODO replace this secret - var upstreamStateEncoderBlockKey = []byte("16-bytes-STATE01") // TODO replace this secret - var upstreamStateEncoder = securecookie.New(upstreamStateEncoderHashKey, upstreamStateEncoderBlockKey) - upstreamStateEncoder.SetSerializer(securecookie.JSONEncoder{}) - - var csrfCookieEncoderHashKey = []byte("fake-csrf-hash-secret") // TODO replace this secret - var csrfCookieEncoder = securecookie.New(csrfCookieEncoderHashKey, nil) - csrfCookieEncoder.SetSerializer(securecookie.JSONEncoder{}) + var upstreamStateEncoder = dynamiccodec.New( + timeoutsConfiguration.UpstreamStateParamLifespan, + wrapGetter(incomingProvider.Issuer(), m.secretCache.GetStateEncoderHashKey), + wrapGetter(incomingProvider.Issuer(), m.secretCache.GetStateEncoderBlockKey), + ) m.providerHandlers[(issuerHostWithPath + oidc.WellKnownEndpointPath)] = discovery.NewHandler(issuer) @@ -154,3 +159,9 @@ func (m *Manager) findHandler(req *http.Request) http.Handler { return m.providerHandlers[strings.ToLower(req.Host)+"/"+req.URL.Path] } + +func wrapGetter(issuer string, getter func(string) []byte) func() []byte { + return func() []byte { + return getter(issuer) + } +} diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index 9776c6fa1..c64f4e291 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -14,6 +14,8 @@ import ( "strings" "testing" + "go.pinniped.dev/internal/secret" + "github.com/sclevine/spec" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" @@ -241,7 +243,18 @@ func TestManager(t *testing.T) { kubeClient = fake.NewSimpleClientset() secretsClient := kubeClient.CoreV1().Secrets("some-namespace") - subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, secretsClient) + cache := secret.Cache{} + cache.SetCSRFCookieEncoderHashKey([]byte("fake-csrf-hash-secret")) + + cache.SetTokenHMACKey(issuer1, []byte("some secret 1 - must have at least 32 bytes")) + cache.SetStateEncoderHashKey(issuer1, []byte("some-state-encoder-hash-key-1")) + cache.SetStateEncoderBlockKey(issuer1, []byte("16-bytes-STATE01")) + + cache.SetTokenHMACKey(issuer2, []byte("some secret 2 - must have at least 32 bytes")) + cache.SetStateEncoderHashKey(issuer2, []byte("some-state-encoder-hash-key-2")) + cache.SetStateEncoderBlockKey(issuer2, []byte("16-bytes-STATE02")) + + subject = NewManager(nextHandler, dynamicJWKSProvider, idpListGetter, &cache, secretsClient) }) when("given no providers via SetProviders()", func() { @@ -304,21 +317,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) @@ -336,7 +354,7 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p1, p2) - jwks := map[string]*jose.JSONWebKeySet{ + jwksMap := map[string]*jose.JSONWebKeySet{ issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, } @@ -344,7 +362,7 @@ func TestManager(t *testing.T) { issuer1: newTestJWK(issuer1KeyID), issuer2: newTestJWK(issuer2KeyID), } - dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) + dynamicJWKSProvider.SetIssuerToJWKSMap(jwksMap, activeJWK) }) it("sends all non-matching host requests to the nextHandler", func() { @@ -379,7 +397,7 @@ func TestManager(t *testing.T) { r.NoError(err) subject.SetProviders(p2, p1) - jwks := map[string]*jose.JSONWebKeySet{ + jwksMap := map[string]*jose.JSONWebKeySet{ issuer1: {Keys: []jose.JSONWebKey{*newTestJWK(issuer1KeyID)}}, issuer2: {Keys: []jose.JSONWebKey{*newTestJWK(issuer2KeyID)}}, } @@ -387,7 +405,7 @@ func TestManager(t *testing.T) { issuer1: newTestJWK(issuer1KeyID), issuer2: newTestJWK(issuer2KeyID), } - dynamicJWKSProvider.SetIssuerToJWKSMap(jwks, activeJWK) + dynamicJWKSProvider.SetIssuerToJWKSMap(jwksMap, activeJWK) }) it("still routes matching requests to the appropriate provider", func() { diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index cddbb9290..002bcc9ad 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -69,6 +69,10 @@ var ( goodAuthTime = time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC) goodRequestedAtTime = time.Date(7, 6, 5, 4, 3, 2, 1, time.UTC) + hmacSecretFunc = func() []byte { + return []byte(hmacSecret) + } + fositeInvalidMethodErrorBody = func(actual string) string { return here.Docf(` { @@ -1323,7 +1327,7 @@ func makeHappyOauthHelper( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1355,7 +1359,7 @@ func makeOauthHelperWithJWTKeyThatWorksOnlyOnce( t.Helper() jwtSigningKey, jwkProvider := generateJWTSigningKeyAndJWKSProvider(t, goodIssuer) - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, &singleUseJWKProvider{DynamicJWKSProvider: jwkProvider}, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), jwtSigningKey } @@ -1374,7 +1378,7 @@ func makeOauthHelperWithNilPrivateJWTSigningKey( t.Helper() jwkProvider := jwks.NewDynamicJWKSProvider() // empty provider which contains no signing key for this issuer - oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, []byte(hmacSecret), jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) + oauthHelper := oidc.FositeOauth2Helper(store, goodIssuer, hmacSecretFunc, jwkProvider, oidc.DefaultOIDCTimeoutsConfiguration()) authResponder := simulateAuthEndpointHavingAlreadyRun(t, authRequest, oauthHelper) return oauthHelper, authResponder.GetCode(), nil } diff --git a/internal/secret/cache.go b/internal/secret/cache.go new file mode 100644 index 000000000..6c0f4063e --- /dev/null +++ b/internal/secret/cache.go @@ -0,0 +1,71 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package secret + +import ( + "sync" + "sync/atomic" +) + +type Cache struct { + csrfCookieEncoderHashKey atomic.Value + oidcProviderCacheMap sync.Map +} + +// New returns an empty Cache. +func New() *Cache { return &Cache{} } + +type oidcProviderCache struct { + tokenHMACKey atomic.Value + stateEncoderHashKey atomic.Value + stateEncoderBlockKey atomic.Value +} + +func (c *Cache) GetCSRFCookieEncoderHashKey() []byte { + return bytesOrNil(c.csrfCookieEncoderHashKey.Load()) +} + +func (c *Cache) SetCSRFCookieEncoderHashKey(key []byte) { + c.csrfCookieEncoderHashKey.Store(key) +} + +func (c *Cache) GetTokenHMACKey(oidcIssuer string) []byte { + return bytesOrNil(c.getOIDCProviderCache(oidcIssuer).tokenHMACKey.Load()) +} + +func (c *Cache) SetTokenHMACKey(oidcIssuer string, key []byte) { + c.getOIDCProviderCache(oidcIssuer).tokenHMACKey.Store(key) +} + +func (c *Cache) GetStateEncoderHashKey(oidcIssuer string) []byte { + return bytesOrNil(c.getOIDCProviderCache(oidcIssuer).stateEncoderHashKey.Load()) +} + +func (c *Cache) SetStateEncoderHashKey(oidcIssuer string, key []byte) { + c.getOIDCProviderCache(oidcIssuer).stateEncoderHashKey.Store(key) +} + +func (c *Cache) GetStateEncoderBlockKey(oidcIssuer string) []byte { + return bytesOrNil(c.getOIDCProviderCache(oidcIssuer).stateEncoderBlockKey.Load()) +} + +func (c *Cache) SetStateEncoderBlockKey(oidcIssuer string, key []byte) { + c.getOIDCProviderCache(oidcIssuer).stateEncoderBlockKey.Store(key) +} + +func (c *Cache) getOIDCProviderCache(oidcIssuer string) *oidcProviderCache { + value, ok := c.oidcProviderCacheMap.Load(oidcIssuer) + if !ok { + value = &oidcProviderCache{} + c.oidcProviderCacheMap.Store(oidcIssuer, value) + } + return value.(*oidcProviderCache) +} + +func bytesOrNil(b interface{}) []byte { + if b == nil { + return nil + } + return b.([]byte) +} diff --git a/internal/secret/cache_test.go b/internal/secret/cache_test.go new file mode 100644 index 000000000..40fdf6122 --- /dev/null +++ b/internal/secret/cache_test.go @@ -0,0 +1,106 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package secret + +import ( + "context" + "testing" + "time" + + "github.com/stretchr/testify/require" + "golang.org/x/sync/errgroup" +) + +const ( + issuer = "some-issuer" + otherIssuer = "other-issuer" +) + +var ( + csrfCookieEncoderHashKey = []byte("csrf-cookie-encoder-hash-key") + tokenHMACKey = []byte("token-hmac-key") + stateEncoderHashKey = []byte("state-encoder-hash-key") + otherStateEncoderHashKey = []byte("other-state-encoder-hash-key") + stateEncoderBlockKey = []byte("state-encoder-block-key") +) + +func TestCache(t *testing.T) { + c := New() + + // Validate we get a nil return value when stuff does not exist. + require.Nil(t, c.GetCSRFCookieEncoderHashKey()) + require.Nil(t, c.GetTokenHMACKey(issuer)) + require.Nil(t, c.GetStateEncoderHashKey(issuer)) + require.Nil(t, c.GetStateEncoderBlockKey(issuer)) + + // Validate we get some nil and non-nil values when some stuff exists. + c.SetCSRFCookieEncoderHashKey(csrfCookieEncoderHashKey) + require.Equal(t, csrfCookieEncoderHashKey, c.GetCSRFCookieEncoderHashKey()) + require.Nil(t, c.GetTokenHMACKey(issuer)) + c.SetStateEncoderHashKey(issuer, stateEncoderHashKey) + require.Equal(t, stateEncoderHashKey, c.GetStateEncoderHashKey(issuer)) + require.Nil(t, c.GetStateEncoderBlockKey(issuer)) + + // Validate we get non-nil values when all stuff exists. + c.SetCSRFCookieEncoderHashKey(csrfCookieEncoderHashKey) + c.SetTokenHMACKey(issuer, tokenHMACKey) + c.SetStateEncoderHashKey(issuer, otherStateEncoderHashKey) + c.SetStateEncoderBlockKey(issuer, stateEncoderBlockKey) + require.Equal(t, csrfCookieEncoderHashKey, c.GetCSRFCookieEncoderHashKey()) + require.Equal(t, tokenHMACKey, c.GetTokenHMACKey(issuer)) + require.Equal(t, otherStateEncoderHashKey, c.GetStateEncoderHashKey(issuer)) + require.Equal(t, stateEncoderBlockKey, c.GetStateEncoderBlockKey(issuer)) + + // Validate that stuff is still nil for an unknown issuer. + require.Nil(t, c.GetTokenHMACKey(otherIssuer)) + require.Nil(t, c.GetStateEncoderHashKey(otherIssuer)) + require.Nil(t, c.GetStateEncoderBlockKey(otherIssuer)) +} + +// TestCacheSynchronized should mimic the behavior of an OIDCProvider: multiple goroutines +// read the same fields, sequentially, from the cache. +func TestCacheSynchronized(t *testing.T) { + c := New() + + c.SetCSRFCookieEncoderHashKey(csrfCookieEncoderHashKey) + c.SetTokenHMACKey(issuer, tokenHMACKey) + c.SetStateEncoderHashKey(issuer, stateEncoderHashKey) + c.SetStateEncoderBlockKey(issuer, stateEncoderBlockKey) + + ctx, cancel := context.WithTimeout(context.Background(), time.Second*3) + defer cancel() + + eg, _ := errgroup.WithContext(ctx) + + eg.Go(func() error { + for i := 0; i < 100; i++ { + require.Equal(t, csrfCookieEncoderHashKey, c.GetCSRFCookieEncoderHashKey()) + require.Equal(t, tokenHMACKey, c.GetTokenHMACKey(issuer)) + require.Equal(t, stateEncoderHashKey, c.GetStateEncoderHashKey(issuer)) + require.Equal(t, stateEncoderBlockKey, c.GetStateEncoderBlockKey(issuer)) + } + return nil + }) + + eg.Go(func() error { + for i := 0; i < 100; i++ { + require.Equal(t, csrfCookieEncoderHashKey, c.GetCSRFCookieEncoderHashKey()) + require.Equal(t, tokenHMACKey, c.GetTokenHMACKey(issuer)) + require.Equal(t, stateEncoderHashKey, c.GetStateEncoderHashKey(issuer)) + require.Equal(t, stateEncoderBlockKey, c.GetStateEncoderBlockKey(issuer)) + } + return nil + }) + + eg.Go(func() error { + for i := 0; i < 100; i++ { + require.Nil(t, c.GetTokenHMACKey(otherIssuer)) + require.Nil(t, c.GetStateEncoderHashKey(otherIssuer)) + require.Nil(t, c.GetStateEncoderBlockKey(otherIssuer)) + } + return nil + }) + + require.NoError(t, eg.Wait()) +} diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 1bf58c3f7..e77f1a6e3 100644 --- a/test/integration/e2e_test.go +++ b/test/integration/e2e_test.go @@ -35,6 +35,11 @@ import ( // TestE2EFullIntegration tests a full integration scenario that combines the supervisor, concierge, and CLI. func TestE2EFullIntegration(t *testing.T) { env := library.IntegrationEnv(t).WithCapability(library.ClusterSigningKeyIsAvailable) + + // If anything in this test crashes, dump out the supervisor and proxy pod logs. + defer library.DumpLogs(t, env.SupervisorNamespace, "") + defer library.DumpLogs(t, "dex", "app=proxy") + ctx, cancelFunc := context.WithTimeout(context.Background(), 5*time.Minute) defer cancelFunc() @@ -160,7 +165,15 @@ func TestE2EFullIntegration(t *testing.T) { t.Cleanup(func() { err := kubectlCmd.Wait() t.Logf("kubectl subprocess exited with code %d", kubectlCmd.ProcessState.ExitCode()) - require.NoErrorf(t, err, "kubectl process did not exit cleanly") + stdout, stdoutErr := ioutil.ReadAll(stdoutPipe) + if stdoutErr != nil { + stdout = []byte("") + } + stderr, stderrErr := ioutil.ReadAll(stderrPipe) + if stderrErr != nil { + stderr = []byte("") + } + require.NoErrorf(t, err, "kubectl process did not exit cleanly, stdout/stderr: %q/%q", string(stdout), string(stderr)) }) // Start a background goroutine to read stderr from the CLI and parse out the login URL. @@ -244,7 +257,7 @@ func TestE2EFullIntegration(t *testing.T) { require.Fail(t, "timed out waiting for kubectl output") case kubectlOutput = <-kubectlOutputChan: } - require.Greaterf(t, len(strings.Split(kubectlOutput, "\n")), 2, "expected some namespaces to be returned") + require.Greaterf(t, len(strings.Split(kubectlOutput, "\n")), 2, "expected some namespaces to be returned, got %q", kubectlOutput) t.Logf("first kubectl command took %s", time.Since(start).String()) // Run kubectl again, which should work with no browser interaction. diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 5e12fc120..fa3782450 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -592,13 +592,16 @@ func requireDelete(t *testing.T, client pinnipedclientset.Interface, ns, name st func requireStatus(t *testing.T, client pinnipedclientset.Interface, ns, name string, status v1alpha1.OIDCProviderStatusCondition) { t.Helper() - ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + ctx, cancel := context.WithTimeout(context.Background(), 15*time.Second) defer cancel() var opc *v1alpha1.OIDCProvider var err error assert.Eventually(t, func() bool { opc, err = client.ConfigV1alpha1().OIDCProviders(ns).Get(ctx, name, metav1.GetOptions{}) + if err != nil { + t.Logf("error trying to get OIDCProvider: %s", err.Error()) + } return err == nil && opc.Status.Status == status }, 10*time.Second, 200*time.Millisecond) require.NoError(t, err) diff --git a/test/integration/supervisor_keys_test.go b/test/integration/supervisor_keys_test.go deleted file mode 100644 index d59c713e1..000000000 --- a/test/integration/supervisor_keys_test.go +++ /dev/null @@ -1,101 +0,0 @@ -// Copyright 2020 the Pinniped contributors. All Rights Reserved. -// SPDX-License-Identifier: Apache-2.0 - -package integration - -import ( - "context" - "encoding/json" - "testing" - "time" - - "github.com/stretchr/testify/assert" - "github.com/stretchr/testify/require" - "gopkg.in/square/go-jose.v2" - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - - configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" - "go.pinniped.dev/test/library" -) - -func TestSupervisorOIDCKeys(t *testing.T) { - env := library.IntegrationEnv(t) - kubeClient := library.NewClientset(t) - supervisorClient := library.NewSupervisorClientset(t) - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - // Create our OPC under test. - opc := library.CreateTestOIDCProvider(ctx, t, "", "", "") - - // Ensure a secret is created with the OPC's JWKS. - var updatedOPC *configv1alpha1.OIDCProvider - var err error - assert.Eventually(t, func() bool { - updatedOPC, err = supervisorClient. - ConfigV1alpha1(). - OIDCProviders(env.SupervisorNamespace). - Get(ctx, opc.Name, metav1.GetOptions{}) - return err == nil && updatedOPC.Status.JWKSSecret.Name != "" - }, time.Second*10, time.Millisecond*500) - require.NoError(t, err) - require.NotEmpty(t, updatedOPC.Status.JWKSSecret.Name) - - // Ensure the secret actually exists. - secret, err := kubeClient. - CoreV1(). - Secrets(env.SupervisorNamespace). - Get(ctx, updatedOPC.Status.JWKSSecret.Name, metav1.GetOptions{}) - require.NoError(t, err) - - // Ensure that the secret was labelled. - for k, v := range env.SupervisorCustomLabels { - require.Equalf(t, v, secret.Labels[k], "expected secret to have label `%s: %s`", k, v) - } - require.Equal(t, env.SupervisorAppName, secret.Labels["app"]) - - // Ensure the secret has an active key. - jwkData, ok := secret.Data["activeJWK"] - require.True(t, ok, "secret is missing active jwk") - - // Ensure the secret's active key is valid. - var activeJWK jose.JSONWebKey - require.NoError(t, json.Unmarshal(jwkData, &activeJWK)) - require.True(t, activeJWK.Valid(), "active jwk is invalid") - require.False(t, activeJWK.IsPublic(), "active jwk is public") - - // Ensure the secret has a JWKS. - jwksData, ok := secret.Data["jwks"] - require.True(t, ok, "secret is missing jwks") - - // Ensure the secret's JWKS is valid, public, and contains the singing key. - var jwks jose.JSONWebKeySet - require.NoError(t, json.Unmarshal(jwksData, &jwks)) - foundActiveJWK := false - for _, jwk := range jwks.Keys { - require.Truef(t, jwk.Valid(), "jwk %s is invalid", jwk.KeyID) - require.Truef(t, jwk.IsPublic(), "jwk %s is not public", jwk.KeyID) - if jwk.KeyID == activeJWK.KeyID { - foundActiveJWK = true - } - } - require.True(t, foundActiveJWK, "could not find active JWK in JWKS: %s", jwks) - - // Ensure upon deleting the secret, it is eventually brought back. - err = kubeClient. - CoreV1(). - Secrets(env.SupervisorNamespace). - Delete(ctx, updatedOPC.Status.JWKSSecret.Name, metav1.DeleteOptions{}) - require.NoError(t, err) - assert.Eventually(t, func() bool { - secret, err = kubeClient. - CoreV1(). - Secrets(env.SupervisorNamespace). - Get(ctx, updatedOPC.Status.JWKSSecret.Name, metav1.GetOptions{}) - return err == nil - }, time.Second*10, time.Millisecond*500) - require.NoError(t, err) - - // Upon deleting the OPC, the secret is deleted (we test this behavior in our uninstall tests). -} diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index eb2e28cd9..3d1914f1b 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -179,12 +179,8 @@ func TestSupervisorLogin(t *testing.T) { authcode := callback.URL.Query().Get("code") require.NotEmpty(t, authcode) - // Call the token endpoint to get tokens. Give the Supervisor a couple of seconds to wire up its signing key. - var tokenResponse *oauth2.Token - assert.Eventually(t, func() bool { - tokenResponse, err = downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) - return err == nil - }, time.Second*5, time.Second*1) + // Call the token endpoint to get tokens. + tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) require.NoError(t, err) expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat"} diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go new file mode 100644 index 000000000..038b6fac9 --- /dev/null +++ b/test/integration/supervisor_secrets_test.go @@ -0,0 +1,166 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package integration + +import ( + "context" + "encoding/json" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "gopkg.in/square/go-jose.v2" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + + configv1alpha1 "go.pinniped.dev/generated/1.19/apis/supervisor/config/v1alpha1" + "go.pinniped.dev/test/library" +) + +func TestSupervisorSecrets(t *testing.T) { + env := library.IntegrationEnv(t) + kubeClient := library.NewClientset(t) + supervisorClient := library.NewSupervisorClientset(t) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + // Create our OP under test. + op := library.CreateTestOIDCProvider(ctx, t, "", "", "") + + tests := []struct { + name string + secretName func(op *configv1alpha1.OIDCProvider) string + ensureValid func(t *testing.T, secret *corev1.Secret) + }{ + { + name: "csrf cookie signing key", + secretName: func(op *configv1alpha1.OIDCProvider) string { + return env.SupervisorAppName + "-key" + }, + ensureValid: ensureValidSymmetricKey, + }, + { + name: "jwks", + secretName: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.JWKS.Name + }, + ensureValid: ensureValidJWKS, + }, + { + name: "hmac signing secret", + secretName: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.TokenSigningKey.Name + }, + ensureValid: ensureValidSymmetricKey, + }, + { + name: "state signature secret", + secretName: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.StateSigningKey.Name + }, + ensureValid: ensureValidSymmetricKey, + }, + { + name: "state encryption secret", + secretName: func(op *configv1alpha1.OIDCProvider) string { + return op.Status.Secrets.StateEncryptionKey.Name + }, + ensureValid: ensureValidSymmetricKey, + }, + } + for _, test := range tests { + test := test + t.Run(test.name, func(t *testing.T) { + // Ensure a secret is created with the OP's JWKS. + var updatedOP *configv1alpha1.OIDCProvider + var err error + assert.Eventually(t, func() bool { + updatedOP, err = supervisorClient. + ConfigV1alpha1(). + OIDCProviders(env.SupervisorNamespace). + Get(ctx, op.Name, metav1.GetOptions{}) + return err == nil && test.secretName(updatedOP) != "" + }, time.Second*10, time.Millisecond*500) + require.NoError(t, err) + require.NotEmpty(t, test.secretName(updatedOP)) + + // Ensure the secret actually exists. + secret, err := kubeClient. + CoreV1(). + Secrets(env.SupervisorNamespace). + Get(ctx, test.secretName(updatedOP), metav1.GetOptions{}) + require.NoError(t, err) + + // Ensure that the secret was labelled. + for k, v := range env.SupervisorCustomLabels { + require.Equalf(t, v, secret.Labels[k], "expected secret to have label `%s: %s`", k, v) + } + require.Equal(t, env.SupervisorAppName, secret.Labels["app"]) + + // Ensure that the secret is valid. + test.ensureValid(t, secret) + + // Ensure upon deleting the secret, it is eventually brought back. + err = kubeClient. + CoreV1(). + Secrets(env.SupervisorNamespace). + Delete(ctx, test.secretName(updatedOP), metav1.DeleteOptions{}) + require.NoError(t, err) + assert.Eventually(t, func() bool { + secret, err = kubeClient. + CoreV1(). + Secrets(env.SupervisorNamespace). + Get(ctx, test.secretName(updatedOP), metav1.GetOptions{}) + return err == nil + }, time.Second*10, time.Millisecond*500) + require.NoError(t, err) + + // Ensure that the new secret is valid. + test.ensureValid(t, secret) + }) + } + + // Upon deleting the OP, the secret is deleted (we test this behavior in our uninstall tests). +} + +func ensureValidJWKS(t *testing.T, secret *corev1.Secret) { + t.Helper() + + // Ensure the secret has an active key. + jwkData, ok := secret.Data["activeJWK"] + require.True(t, ok, "secret is missing active jwk") + + // Ensure the secret's active key is valid. + var activeJWK jose.JSONWebKey + require.NoError(t, json.Unmarshal(jwkData, &activeJWK)) + require.True(t, activeJWK.Valid(), "active jwk is invalid") + require.False(t, activeJWK.IsPublic(), "active jwk is public") + + // Ensure the secret has a JWKS. + jwksData, ok := secret.Data["jwks"] + require.True(t, ok, "secret is missing jwks") + + // Ensure the secret's JWKS is valid, public, and contains the singing key. + var jwks jose.JSONWebKeySet + require.NoError(t, json.Unmarshal(jwksData, &jwks)) + foundActiveJWK := false + for _, jwk := range jwks.Keys { + require.Truef(t, jwk.Valid(), "jwk %s is invalid", jwk.KeyID) + require.Truef(t, jwk.IsPublic(), "jwk %s is not public", jwk.KeyID) + if jwk.KeyID == activeJWK.KeyID { + foundActiveJWK = true + } + } + 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)) +} diff --git a/test/library/client.go b/test/library/client.go index 0f622b44e..a6e5fa577 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -285,6 +285,22 @@ func CreateTestOIDCProvider(ctx context.Context, t *testing.T, issuer string, ce }, 60*time.Second, 1*time.Second, "expected the OIDCProvider to have status %q", expectStatus) require.Equal(t, expectStatus, result.Status.Status) + // If the OIDCProvider was successfully created, ensure all secrets are present before continuing + if result.Status.Status == configv1alpha1.SuccessOIDCProviderStatusCondition { + assert.Eventually(t, func() bool { + var err error + result, err = opcs.Get(ctx, opc.Name, metav1.GetOptions{}) + require.NoError(t, err) + return result.Status.Secrets.JWKS.Name != "" && + result.Status.Secrets.TokenSigningKey.Name != "" && + result.Status.Secrets.StateSigningKey.Name != "" && + result.Status.Secrets.StateEncryptionKey.Name != "" + }, 60*time.Second, 1*time.Second, "expected the OIDCProvider to have secrets populated") + require.NotEmpty(t, result.Status.Secrets.JWKS.Name) + require.NotEmpty(t, result.Status.Secrets.TokenSigningKey.Name) + require.NotEmpty(t, result.Status.Secrets.StateSigningKey.Name) + require.NotEmpty(t, result.Status.Secrets.StateEncryptionKey.Name) + } return opc }