diff --git a/Dockerfile b/Dockerfile index be74db5f..fdec5b50 100644 --- a/Dockerfile +++ b/Dockerfile @@ -1,7 +1,7 @@ # Copyright 2020 the Pinniped contributors. All Rights Reserved. # SPDX-License-Identifier: Apache-2.0 -FROM golang:1.15.5 as build-env +FROM golang:1.15.6 as build-env WORKDIR /work # Get dependencies first so they can be cached as a layer @@ -25,7 +25,7 @@ RUN mkdir out \ && CGO_ENABLED=0 GOOS=linux GOARCH=amd64 go build -o out ./cmd/local-user-authenticator/... # Use a runtime image based on Debian slim -FROM debian:10.6-slim +FROM debian:10.7-slim RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* # Copy the binaries from the build-env stage diff --git a/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl b/apis/supervisor/config/v1alpha1/types_oidcprovider.go.tmpl index dee102c9..a351900f 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/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl index 9be04701..09f74c7c 100644 --- a/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl +++ b/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go.tmpl @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 0c54964d..461eea56 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/cmd/pinniped/cmd/kubeconfig.go b/cmd/pinniped/cmd/kubeconfig.go index 5bf8efe8..913e7ca4 100644 --- a/cmd/pinniped/cmd/kubeconfig.go +++ b/cmd/pinniped/cmd/kubeconfig.go @@ -102,12 +102,12 @@ func kubeconfigCommand(deps kubeconfigDeps) *cobra.Command { f.StringVar(&flags.oidc.issuer, "oidc-issuer", "", "OpenID Connect issuer URL (default: autodiscover)") f.StringVar(&flags.oidc.clientID, "oidc-client-id", "pinniped-cli", "OpenID Connect client ID (default: autodiscover)") f.Uint16Var(&flags.oidc.listenPort, "oidc-listen-port", 0, "TCP port for localhost listener (authorization code flow only)") - f.StringSliceVar(&flags.oidc.scopes, "oidc-scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "pinniped.sts.unrestricted"}, "OpenID Connect scopes to request during login") + f.StringSliceVar(&flags.oidc.scopes, "oidc-scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "pinniped:request-audience"}, "OpenID Connect scopes to request during login") f.BoolVar(&flags.oidc.skipBrowser, "oidc-skip-browser", false, "During OpenID Connect login, skip opening the browser (just print the URL)") f.StringVar(&flags.oidc.sessionCachePath, "oidc-session-cache", "", "Path to OpenID Connect session cache file") f.StringSliceVar(&flags.oidc.caBundlePaths, "oidc-ca-bundle", nil, "Path to TLS certificate authority bundle (PEM format, optional, can be repeated)") f.BoolVar(&flags.oidc.debugSessionCache, "oidc-debug-session-cache", false, "Print debug logs related to the OpenID Connect session cache") - f.StringVar(&flags.oidc.requestAudience, "oidc-request-audience", "", "Request a token with an alternate audience using RF8693 token exchange") + f.StringVar(&flags.oidc.requestAudience, "oidc-request-audience", "", "Request a token with an alternate audience using RFC8693 token exchange") f.StringVar(&flags.kubeconfigPath, "kubeconfig", os.Getenv("KUBECONFIG"), "Path to kubeconfig file") f.StringVar(&flags.kubeconfigContextOverride, "kubeconfig-context", "", "Kubeconfig context name (default: current active context)") diff --git a/cmd/pinniped/cmd/kubeconfig_test.go b/cmd/pinniped/cmd/kubeconfig_test.go index 739df951..e0fc8480 100644 --- a/cmd/pinniped/cmd/kubeconfig_test.go +++ b/cmd/pinniped/cmd/kubeconfig_test.go @@ -68,8 +68,8 @@ func TestGetKubeconfig(t *testing.T) { --oidc-client-id string OpenID Connect client ID (default: autodiscover) (default "pinniped-cli") --oidc-issuer string OpenID Connect issuer URL (default: autodiscover) --oidc-listen-port uint16 TCP port for localhost listener (authorization code flow only) - --oidc-request-audience string Request a token with an alternate audience using RF8693 token exchange - --oidc-scopes strings OpenID Connect scopes to request during login (default [offline_access,openid,pinniped.sts.unrestricted]) + --oidc-request-audience string Request a token with an alternate audience using RFC8693 token exchange + --oidc-scopes strings OpenID Connect scopes to request during login (default [offline_access,openid,pinniped:request-audience]) --oidc-session-cache string Path to OpenID Connect session cache file --oidc-skip-browser During OpenID Connect login, skip opening the browser (just print the URL) --static-token string Instead of doing an OIDC-based login, specify a static token @@ -415,7 +415,7 @@ func TestGetKubeconfig(t *testing.T) { - --concierge-ca-bundle-data=ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ== - --issuer=https://example.com/issuer - --client-id=pinniped-cli - - --scopes=offline_access,openid,pinniped.sts.unrestricted + - --scopes=offline_access,openid,pinniped:request-audience - --ca-bundle-data=%s - --request-audience=test-audience command: '.../path/to/pinniped' @@ -472,7 +472,7 @@ func TestGetKubeconfig(t *testing.T) { - --concierge-ca-bundle-data=ZmFrZS1jZXJ0aWZpY2F0ZS1hdXRob3JpdHktZGF0YS12YWx1ZQ== - --issuer=https://example.com/issuer - --client-id=pinniped-cli - - --scopes=offline_access,openid,pinniped.sts.unrestricted + - --scopes=offline_access,openid,pinniped:request-audience - --skip-browser - --listen-port=1234 - --ca-bundle-data=%s diff --git a/cmd/pinniped/cmd/login_oidc.go b/cmd/pinniped/cmd/login_oidc.go index e3078458..39718105 100644 --- a/cmd/pinniped/cmd/login_oidc.go +++ b/cmd/pinniped/cmd/login_oidc.go @@ -79,13 +79,13 @@ func oidcLoginCommand(deps oidcLoginCommandDeps) *cobra.Command { cmd.Flags().StringVar(&flags.issuer, "issuer", "", "OpenID Connect issuer URL") cmd.Flags().StringVar(&flags.clientID, "client-id", "pinniped-cli", "OpenID Connect client ID") cmd.Flags().Uint16Var(&flags.listenPort, "listen-port", 0, "TCP port for localhost listener (authorization code flow only)") - cmd.Flags().StringSliceVar(&flags.scopes, "scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "pinniped.sts.unrestricted"}, "OIDC scopes to request during login") + cmd.Flags().StringSliceVar(&flags.scopes, "scopes", []string{oidc.ScopeOfflineAccess, oidc.ScopeOpenID, "pinniped:request-audience"}, "OIDC scopes to request during login") cmd.Flags().BoolVar(&flags.skipBrowser, "skip-browser", false, "Skip opening the browser (just print the URL)") cmd.Flags().StringVar(&flags.sessionCachePath, "session-cache", filepath.Join(mustGetConfigDir(), "sessions.yaml"), "Path to session cache file") cmd.Flags().StringSliceVar(&flags.caBundlePaths, "ca-bundle", nil, "Path to TLS certificate authority bundle (PEM format, optional, can be repeated)") cmd.Flags().StringSliceVar(&flags.caBundleData, "ca-bundle-data", nil, "Base64 endcoded TLS certificate authority bundle (base64 encoded PEM format, optional, can be repeated)") cmd.Flags().BoolVar(&flags.debugSessionCache, "debug-session-cache", false, "Print debug logs related to the session cache") - cmd.Flags().StringVar(&flags.requestAudience, "request-audience", "", "Request a token with an alternate audience using RF8693 token exchange") + cmd.Flags().StringVar(&flags.requestAudience, "request-audience", "", "Request a token with an alternate audience using RFC8693 token exchange") cmd.Flags().BoolVar(&flags.conciergeEnabled, "enable-concierge", false, "Exchange the OIDC ID token with the Pinniped concierge during login") cmd.Flags().StringVar(&flags.conciergeNamespace, "concierge-namespace", "pinniped-concierge", "Namespace in which the concierge was installed") cmd.Flags().StringVar(&flags.conciergeAuthenticatorType, "concierge-authenticator-type", "", "Concierge authenticator type (e.g., 'webhook', 'jwt')") diff --git a/cmd/pinniped/cmd/login_oidc_test.go b/cmd/pinniped/cmd/login_oidc_test.go index df4f8f9d..72626418 100644 --- a/cmd/pinniped/cmd/login_oidc_test.go +++ b/cmd/pinniped/cmd/login_oidc_test.go @@ -69,8 +69,8 @@ func TestLoginOIDCCommand(t *testing.T) { -h, --help help for oidc --issuer string OpenID Connect issuer URL --listen-port uint16 TCP port for localhost listener (authorization code flow only) - --request-audience string Request a token with an alternate audience using RF8693 token exchange - --scopes strings OIDC scopes to request during login (default [offline_access,openid,pinniped.sts.unrestricted]) + --request-audience string Request a token with an alternate audience using RFC8693 token exchange + --scopes strings OIDC scopes to request during login (default [offline_access,openid,pinniped:request-audience]) --session-cache string Path to session cache file (default "` + cfgDir + `/sessions.yaml") --skip-browser Skip opening the browser (just print the URL) `), diff --git a/deploy/supervisor/config.supervisor.pinniped.dev_oidcproviders.yaml b/deploy/supervisor/config.supervisor.pinniped.dev_oidcproviders.yaml index e25a9f81..47c6f5a9 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 28b8d93f..1e0c75c0 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -132,6 +132,9 @@ spec: - path: "namespace" fieldRef: fieldPath: metadata.namespace + - path: "name" + fieldRef: + fieldPath: metadata.name #! This will help make sure our multiple pods run on different nodes, making #! our deployment "more" "HA". affinity: diff --git a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/deploy/supervisor/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/deploy/supervisor/rbac.yaml b/deploy/supervisor/rbac.yaml index 33e86585..b66c00fc 100644 --- a/deploy/supervisor/rbac.yaml +++ b/deploy/supervisor/rbac.yaml @@ -25,6 +25,14 @@ rules: - apiGroups: [idp.supervisor.pinniped.dev] resources: [upstreamoidcproviders/status] verbs: [get, patch, update] + #! We want to be able to read pods/replicasets/deployment so we can learn who our deployment is to set + #! as an owner reference. + - apiGroups: [""] + resources: [pods] + verbs: [get] + - apiGroups: [apps] + resources: [replicasets,deployments] + verbs: [get] --- kind: RoleBinding apiVersion: rbac.authorization.k8s.io/v1 diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index fa198e84..6db75766 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -323,6 +323,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 @@ -358,7 +378,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. |=== @@ -461,7 +481,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== 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 dee102c9..a351900f 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 f208d4d0..0cfc17a4 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/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.17/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } 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 e25a9f81..47c6f5a9 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.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.17/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 327bc8fa..a8c51180 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -323,6 +323,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 @@ -358,7 +378,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. |=== @@ -461,7 +481,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== 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 dee102c9..a351900f 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 f208d4d0..0cfc17a4 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/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.18/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } 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 e25a9f81..47c6f5a9 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.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.18/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index 3fa12249..c8a5461a 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -323,6 +323,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 @@ -358,7 +378,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. |=== @@ -461,7 +481,7 @@ OIDCClient contains information about an OIDC client (e.g., client ID and client [cols="25a,75a", options="header"] |=== | Field | Description -| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys "clientID" and "clientSecret". +| *`secretName`* __string__ | SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". |=== 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 dee102c9..a351900f 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 f208d4d0..0cfc17a4 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/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go index 9be04701..09f74c7c 100644 --- a/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go +++ b/generated/1.19/apis/supervisor/idp/v1alpha1/types_upstreamoidcprovider.go @@ -62,7 +62,7 @@ type OIDCClaims struct { type OIDCClient struct { // SecretName contains the name of a namespace-local Secret object that provides the clientID and // clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" with keys + // struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys // "clientID" and "clientSecret". SecretName string `json:"secretName"` } 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 e25a9f81..47c6f5a9 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/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml index 780fe6fe..bd239a6f 100644 --- a/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml +++ b/generated/1.19/crds/idp.supervisor.pinniped.dev_upstreamoidcproviders.yaml @@ -86,7 +86,7 @@ spec: description: SecretName contains the name of a namespace-local Secret object that provides the clientID and clientSecret for an OIDC client. If only the SecretName is specified in an OIDCClient - struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc" + struct, then it is expected that the Secret is of type "secrets.pinniped.dev/oidc-client" with keys "clientID" and "clientSecret". type: string required: diff --git a/hack/lib/tilt/Tiltfile b/hack/lib/tilt/Tiltfile index 6bd2e479..8233f665 100644 --- a/hack/lib/tilt/Tiltfile +++ b/hack/lib/tilt/Tiltfile @@ -148,7 +148,7 @@ k8s_yaml(local([ '--data-value namespace=concierge ' + '--data-value image_repo=image/concierge ' + '--data-value image_tag=tilt-dev ' + - '--data-value kube_cert_agent_image=debian:10.6-slim ' + + '--data-value kube_cert_agent_image=debian:10.7-slim ' + '--data-value discovery_url=$(TERM=dumb kubectl cluster-info | awk \'/master|control plane/ {print $NF}\') ' + '--data-value log_level=debug ' + '--data-value-yaml replicas=1 ' + diff --git a/hack/lib/tilt/concierge.Dockerfile b/hack/lib/tilt/concierge.Dockerfile index c2927bdf..e9daadd6 100644 --- a/hack/lib/tilt/concierge.Dockerfile +++ b/hack/lib/tilt/concierge.Dockerfile @@ -2,7 +2,7 @@ # SPDX-License-Identifier: Apache-2.0 # Use a runtime image based on Debian slim -FROM debian:10.6-slim +FROM debian:10.7-slim # Copy the binary which was built outside the container. COPY build/pinniped-concierge /usr/local/bin/pinniped-concierge diff --git a/hack/lib/tilt/local-user-authenticator.Dockerfile b/hack/lib/tilt/local-user-authenticator.Dockerfile index b63a212a..6b8430f6 100644 --- a/hack/lib/tilt/local-user-authenticator.Dockerfile +++ b/hack/lib/tilt/local-user-authenticator.Dockerfile @@ -2,7 +2,7 @@ # SPDX-License-Identifier: Apache-2.0 # Use a runtime image based on Debian slim -FROM debian:10.6-slim +FROM debian:10.7-slim # Copy the binary which was built outside the container. COPY build/local-user-authenticator /usr/local/bin/local-user-authenticator diff --git a/hack/lib/tilt/supervisor.Dockerfile b/hack/lib/tilt/supervisor.Dockerfile index 468749ad..916d009a 100644 --- a/hack/lib/tilt/supervisor.Dockerfile +++ b/hack/lib/tilt/supervisor.Dockerfile @@ -2,7 +2,7 @@ # SPDX-License-Identifier: Apache-2.0 # Use a runtime image based on Debian slim -FROM debian:10.6-slim +FROM debian:10.7-slim RUN apt-get update && apt-get install -y ca-certificates && rm -rf /var/lib/apt/lists/* diff --git a/internal/controller/supervisorconfig/generator/generator.go b/internal/controller/supervisorconfig/generator/generator.go new file mode 100644 index 00000000..fe711698 --- /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 00000000..b188cc18 --- /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 00000000..e888c7c2 --- /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 00000000..3a3362a6 --- /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 00000000..bbaeb4e1 --- /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 00000000..9f5ca1c4 --- /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 00000000..2522fe7d --- /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 5c01c5b4..efbdfae9 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 ad7323b8..fa2ebea1 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 4e6a51a1..2a33b77f 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 9afdc486..615a1f3d 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 ee4d65b7..75119dc4 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 9bc954d5..adc15b40 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 00000000..2fdecf1e --- /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 00000000..e5880cda --- /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 00000000..f2605f23 --- /dev/null +++ b/internal/downward/testdata/validwithoutname/namespace @@ -0,0 +1 @@ +test-namespace diff --git a/internal/httputil/securityheader/securityheader.go b/internal/httputil/securityheader/securityheader.go index 42cf1e2f..2bb3af12 100644 --- a/internal/httputil/securityheader/securityheader.go +++ b/internal/httputil/securityheader/securityheader.go @@ -9,7 +9,6 @@ import "net/http" // Wrap the provided http.Handler so it sets appropriate security-related response headers. func Wrap(wrapped http.Handler) http.Handler { return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { - wrapped.ServeHTTP(w, r) h := w.Header() h.Set("Content-Security-Policy", "default-src 'none'; frame-ancestors 'none'") h.Set("X-Frame-Options", "DENY") @@ -17,14 +16,9 @@ func Wrap(wrapped http.Handler) http.Handler { h.Set("X-Content-Type-Options", "nosniff") h.Set("Referrer-Policy", "no-referrer") h.Set("X-DNS-Prefetch-Control", "off") - - // first overwrite existing Cache-Control header with Set, then append more headers with Add - h.Set("Cache-Control", "no-cache") - h.Add("Cache-Control", "no-store") - h.Add("Cache-Control", "max-age=0") - h.Add("Cache-Control", "must-revalidate") - + h.Set("Cache-Control", "no-cache,no-store,max-age=0,must-revalidate") h.Set("Pragma", "no-cache") h.Set("Expires", "0") + wrapped.ServeHTTP(w, r) }) } diff --git a/internal/httputil/securityheader/securityheader_test.go b/internal/httputil/securityheader/securityheader_test.go index 715e7cd5..a0688c1a 100644 --- a/internal/httputil/securityheader/securityheader_test.go +++ b/internal/httputil/securityheader/securityheader_test.go @@ -4,22 +4,40 @@ package securityheader import ( + "context" + "io/ioutil" "net/http" "net/http/httptest" "testing" + "time" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) func TestWrap(t *testing.T) { - handler := http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + testServer := httptest.NewServer(Wrap(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.Header().Set("X-Test-Header", "test value") _, _ = w.Write([]byte("hello world")) - }) - rec := httptest.NewRecorder() - Wrap(handler).ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/", nil)) - require.Equal(t, http.StatusOK, rec.Code) - require.Equal(t, "hello world", rec.Body.String()) - require.EqualValues(t, http.Header{ + }))) + t.Cleanup(testServer.Close) + + ctx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + + req, err := http.NewRequestWithContext(ctx, http.MethodGet, testServer.URL, nil) + require.NoError(t, err) + resp, err := http.DefaultClient.Do(req) + require.NoError(t, err) + defer resp.Body.Close() + require.Equal(t, http.StatusOK, resp.StatusCode) + + respBody, err := ioutil.ReadAll(resp.Body) + require.NoError(t, err) + require.Equal(t, "hello world", string(respBody)) + + expected := http.Header{ + "X-Test-Header": []string{"test value"}, "Content-Security-Policy": []string{"default-src 'none'; frame-ancestors 'none'"}, "Content-Type": []string{"text/plain; charset=utf-8"}, "Referrer-Policy": []string{"no-referrer"}, @@ -27,8 +45,11 @@ func TestWrap(t *testing.T) { "X-Frame-Options": []string{"DENY"}, "X-Xss-Protection": []string{"1; mode=block"}, "X-Dns-Prefetch-Control": []string{"off"}, - "Cache-Control": []string{"no-cache", "no-store", "max-age=0", "must-revalidate"}, + "Cache-Control": []string{"no-cache,no-store,max-age=0,must-revalidate"}, "Pragma": []string{"no-cache"}, "Expires": []string{"0"}, - }, rec.Header()) + } + for key, values := range expected { + assert.Equalf(t, values, resp.Header.Values(key), "unexpected values for header %s", key) + } } diff --git a/internal/mocks/mocksecrethelper/generate.go b/internal/mocks/mocksecrethelper/generate.go new file mode 100644 index 00000000..31d52cb4 --- /dev/null +++ b/internal/mocks/mocksecrethelper/generate.go @@ -0,0 +1,6 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package mocksecrethelper + +//go:generate go run -v github.com/golang/mock/mockgen -destination=mocksecrethelper.go -package=mocksecrethelper -copyright_file=../../../hack/header.txt go.pinniped.dev/internal/controller/supervisorconfig/generator SecretHelper diff --git a/internal/mocks/mocksecrethelper/mocksecrethelper.go b/internal/mocks/mocksecrethelper/mocksecrethelper.go new file mode 100644 index 00000000..68b6cfbd --- /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.go b/internal/oidc/auth/auth_handler.go index 9375d6d6..f417f0c1 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -64,8 +64,8 @@ func NewHandler( // at this time, however we will temporarily grant the scope just in case that changes in a future release of fosite. oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) - // Grant the Pinniped STS scope if requested. - oidc.GrantScopeIfRequested(authorizeRequester, "pinniped.sts.unrestricted") + // Grant the pinniped:request-audience scope if requested. + oidc.GrantScopeIfRequested(authorizeRequester, "pinniped:request-audience") now := time.Now() _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{ diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index e0f7eaea..253a112c 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.go b/internal/oidc/callback/callback_handler.go index 3b4fea6c..66e7f3b1 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -56,10 +56,10 @@ func NewHandler( return httperr.New(http.StatusBadRequest, "error using state downstream auth params") } - // Automatically grant the openid, offline_access, and Pinniped STS scopes, but only if they were requested. + // Automatically grant the openid, offline_access, and pinniped:request-audience scopes, but only if they were requested. oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOpenID) oidc.GrantScopeIfRequested(authorizeRequester, coreosoidc.ScopeOfflineAccess) - oidc.GrantScopeIfRequested(authorizeRequester, "pinniped.sts.unrestricted") + oidc.GrantScopeIfRequested(authorizeRequester, "pinniped:request-audience") token, err := upstreamIDPConfig.ExchangeAuthcodeAndValidateTokens( r.Context(), diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index ab4fc9d9..cce60f17 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -486,10 +486,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 00000000..8d67dc2e --- /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 00000000..5168b2b7 --- /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 00000000..bcff2d03 --- /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/nullstorage_test.go b/internal/oidc/nullstorage_test.go index 523aafb5..74f1f8a1 100644 --- a/internal/oidc/nullstorage_test.go +++ b/internal/oidc/nullstorage_test.go @@ -28,7 +28,7 @@ func TestNullStorage_GetClient(t *testing.T) { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:token-exchange"}, - Scopes: []string{"openid", "offline_access", "profile", "email", "pinniped.sts.unrestricted"}, + Scopes: []string{"openid", "offline_access", "profile", "email", "pinniped:request-audience"}, }, TokenEndpointAuthMethod: "none", }, diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index d53ee663..d07a47ac 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -59,6 +59,11 @@ const ( // DownstreamGroupsClaim is what we will use to encode the groups in the downstream OIDC ID token // information. DownstreamGroupsClaim = "groups" + + // 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. @@ -100,7 +105,7 @@ func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { RedirectURIs: []string{"http://127.0.0.1/callback"}, ResponseTypes: []string{"code"}, GrantTypes: []string{"authorization_code", "refresh_token", "urn:ietf:params:oauth:grant-type:token-exchange"}, - Scopes: []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, "profile", "email", "pinniped.sts.unrestricted"}, + Scopes: []string{coreosoidc.ScopeOpenID, coreosoidc.ScopeOfflineAccess, "profile", "email", "pinniped:request-audience"}, }, TokenEndpointAuthMethod: "none", } @@ -202,7 +207,7 @@ func DefaultOIDCTimeoutsConfiguration() TimeoutsConfiguration { func FositeOauth2Helper( oauthStore interface{}, issuer string, - hmacSecretOfLengthAtLeast32 []byte, + hmacSecretOfLengthAtLeast32Func func() []byte, jwksProvider jwks.DynamicJWKSProvider, timeoutsConfiguration TimeoutsConfiguration, ) fosite.OAuth2Provider { @@ -235,7 +240,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 907bbd73..e27d6b9d 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 9776c6fa..c64f4e29 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 0f31e1dd..8d0cdbc9 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -70,6 +70,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(` { @@ -634,13 +638,13 @@ func TestTokenExchange(t *testing.T) { successfulAuthCodeExchange := tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "pinniped.sts.unrestricted"}, - wantGrantedScopes: []string{"openid", "pinniped.sts.unrestricted"}, + wantRequestedScopes: []string{"openid", "pinniped:request-audience"}, + wantGrantedScopes: []string{"openid", "pinniped:request-audience"}, } doValidAuthCodeExchange := authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + authRequest.Form.Set("scope", "openid pinniped:request-audience") }, want: successfulAuthCodeExchange, } @@ -731,7 +735,7 @@ func TestTokenExchange(t *testing.T) { wantResponseBodyContains: `invalid subject_token`, }, { - name: "access token missing pinniped.sts.unrestricted scope", + name: "access token missing pinniped:request-audience scope", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { authRequest.Form.Set("scope", "openid") @@ -745,19 +749,19 @@ func TestTokenExchange(t *testing.T) { }, requestedAudience: "some-workload-cluster", wantStatus: http.StatusForbidden, - wantResponseBodyContains: `missing the \"pinniped.sts.unrestricted\" scope`, + wantResponseBodyContains: `missing the \"pinniped:request-audience\" scope`, }, { name: "access token missing openid scope", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "pinniped.sts.unrestricted") + authRequest.Form.Set("scope", "pinniped:request-audience") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"pinniped.sts.unrestricted"}, - wantGrantedScopes: []string{"pinniped.sts.unrestricted"}, + wantRequestedScopes: []string{"pinniped:request-audience"}, + wantGrantedScopes: []string{"pinniped:request-audience"}, }, }, requestedAudience: "some-workload-cluster", @@ -768,7 +772,7 @@ func TestTokenExchange(t *testing.T) { name: "token minting failure", authcodeExchange: authcodeExchangeInputs{ modifyAuthRequest: func(authRequest *http.Request) { - authRequest.Form.Set("scope", "openid pinniped.sts.unrestricted") + authRequest.Form.Set("scope", "openid pinniped:request-audience") }, // Fail to fetch a JWK signing key after the authcode exchange has happened. makeOathHelper: makeOauthHelperWithJWTKeyThatWorksOnlyOnce, @@ -956,23 +960,23 @@ func TestRefreshGrant(t *testing.T) { { name: "when the refresh request removes a scope which was originally granted from the list of requested scopes then it is granted anyway", authcodeExchange: authcodeExchangeInputs{ - modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped.sts.unrestricted") }, + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access pinniped:request-audience") }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access", "pinniped.sts.unrestricted"}, - wantGrantedScopes: []string{"openid", "offline_access", "pinniped.sts.unrestricted"}, + wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, }, }, refreshRequest: refreshRequestInputs{ modifyTokenRequest: func(r *http.Request, refreshToken string, accessToken string) { - r.Body = happyRefreshRequestBody(refreshToken).WithScope("openid").ReadCloser() // do not ask for "pinniped.sts.unrestricted" again + r.Body = happyRefreshRequestBody(refreshToken).WithScope("openid").ReadCloser() // do not ask for "pinniped:request-audience" again }, want: tokenEndpointResponseExpectedValues{ wantStatus: http.StatusOK, wantSuccessBodyFields: []string{"id_token", "refresh_token", "access_token", "token_type", "expires_in", "scope"}, - wantRequestedScopes: []string{"openid", "offline_access", "pinniped.sts.unrestricted"}, - wantGrantedScopes: []string{"openid", "offline_access", "pinniped.sts.unrestricted"}, + wantRequestedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, + wantGrantedScopes: []string{"openid", "offline_access", "pinniped:request-audience"}, }}, }, { @@ -1361,7 +1365,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 } @@ -1393,7 +1397,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 } @@ -1412,7 +1416,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 } @@ -1442,8 +1446,8 @@ func simulateAuthEndpointHavingAlreadyRun(t *testing.T, authRequest *http.Reques if strings.Contains(authRequest.Form.Get("scope"), "offline_access") { authRequester.GrantScope("offline_access") } - if strings.Contains(authRequest.Form.Get("scope"), "pinniped.sts.unrestricted") { - authRequester.GrantScope("pinniped.sts.unrestricted") + if strings.Contains(authRequest.Form.Get("scope"), "pinniped:request-audience") { + authRequester.GrantScope("pinniped:request-audience") } authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) diff --git a/internal/oidc/token_exchange.go b/internal/oidc/token_exchange.go index 36ddfb83..8a933d37 100644 --- a/internal/oidc/token_exchange.go +++ b/internal/oidc/token_exchange.go @@ -18,7 +18,7 @@ import ( const ( tokenTypeAccessToken = "urn:ietf:params:oauth:token-type:access_token" //nolint: gosec tokenTypeJWT = "urn:ietf:params:oauth:token-type:jwt" //nolint: gosec - pinnipedTokenExchangeScope = "pinniped.sts.unrestricted" //nolint: gosec + pinnipedTokenExchangeScope = "pinniped:request-audience" //nolint: gosec ) type stsParams struct { @@ -65,7 +65,7 @@ func (t *TokenExchangeHandler) PopulateTokenEndpointResponse(ctx context.Context return errors.WithStack(err) } - // Require that the incoming access token has the STS and OpenID scopes. + // Require that the incoming access token has the pinniped:request-audience and OpenID scopes. if !originalRequester.GetGrantedScopes().Has(pinnipedTokenExchangeScope) { return errors.WithStack(fosite.ErrAccessDenied.WithHintf("missing the %q scope", pinnipedTokenExchangeScope)) } diff --git a/internal/secret/cache.go b/internal/secret/cache.go new file mode 100644 index 00000000..6c0f4063 --- /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 00000000..40fdf612 --- /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/internal/testutil/assertions.go b/internal/testutil/assertions.go index b0c3018d..54fc8563 100644 --- a/internal/testutil/assertions.go +++ b/internal/testutil/assertions.go @@ -61,7 +61,9 @@ func RequireSecurityHeaders(t *testing.T, response *httptest.ResponseRecorder) { require.Equal(t, "nosniff", response.Header().Get("X-Content-Type-Options")) require.Equal(t, "no-referrer", response.Header().Get("Referrer-Policy")) require.Equal(t, "off", response.Header().Get("X-DNS-Prefetch-Control")) - require.ElementsMatch(t, []string{"no-cache", "no-store", "max-age=0", "must-revalidate"}, response.Header().Values("Cache-Control")) require.Equal(t, "no-cache", response.Header().Get("Pragma")) require.Equal(t, "0", response.Header().Get("Expires")) + + // This check is more relaxed since Fosite can override the base header we set. + require.Contains(t, response.Header().Get("Cache-Control"), "no-store") } diff --git a/pkg/oidcclient/login.go b/pkg/oidcclient/login.go index 4d92a452..9718a297 100644 --- a/pkg/oidcclient/login.go +++ b/pkg/oidcclient/login.go @@ -37,9 +37,13 @@ const ( // API operation. minIDTokenValidity = 10 * time.Minute - // refreshTimeout is the amount of time allotted for OAuth2 refresh operations. Since these don't involve any - // user interaction, they should always be roughly as fast as network latency. - refreshTimeout = 30 * time.Second + // httpRequestTimeout is the timeout for operations that involve one (or a few) non-interactive HTTPS requests. + // Since these don't involve any user interaction, they should always be roughly as fast as network latency. + httpRequestTimeout = 60 * time.Second + + // overallTimeout is the overall time that a login is allowed to take. This includes several user interactions, so + // we set this to be relatively long. + overallTimeout = 90 * time.Minute ) type handlerState struct { @@ -155,7 +159,7 @@ func WithClient(httpClient *http.Client) Option { } } -// WithRequestAudience causes the login flow to perform an additional token exchange using the RFC8693 STS flow. +// WithRequestAudience causes the login flow to perform an additional token exchange using the RFC8693 flow. func WithRequestAudience(audience string) Option { return func(h *handlerState) error { h.requestedAudience = audience @@ -198,8 +202,13 @@ func Login(issuer string, clientID string, opts ...Option) (*oidctypes.Token, er } } + // Copy the configured HTTP client to set a request timeout (the Go default client has no timeout configured). + httpClientWithTimeout := *h.httpClient + httpClientWithTimeout.Timeout = httpRequestTimeout + h.httpClient = &httpClientWithTimeout + // Always set a long, but non-infinite timeout for this operation. - ctx, cancel := context.WithTimeout(h.ctx, 10*time.Minute) + ctx, cancel := context.WithTimeout(h.ctx, overallTimeout) defer cancel() ctx = oidc.ClientContext(ctx, h.httpClient) h.ctx = ctx @@ -404,8 +413,6 @@ func (h *handlerState) tokenExchangeRFC8693(baseToken *oidctypes.Token) (*oidcty } func (h *handlerState) handleRefresh(ctx context.Context, refreshToken *oidctypes.RefreshToken) (*oidctypes.Token, error) { - ctx, cancel := context.WithTimeout(ctx, refreshTimeout) - defer cancel() refreshSource := h.oauth2Config.TokenSource(ctx, &oauth2.Token{RefreshToken: refreshToken.Token}) refreshed, err := refreshSource.Token() @@ -473,7 +480,7 @@ func (h *handlerState) serve(listener net.Listener) func() { return func() { // Gracefully shut down the server, allowing up to 5 seconds for // clients to receive any in-flight responses. - shutdownCtx, cancel := context.WithTimeout(h.ctx, 1*time.Second) + shutdownCtx, cancel := context.WithTimeout(h.ctx, 5*time.Second) _ = srv.Shutdown(shutdownCtx) cancel() } diff --git a/test/deploy/dex/proxy.yaml b/test/deploy/dex/proxy.yaml index be4d4878..84b4307c 100644 --- a/test/deploy/dex/proxy.yaml +++ b/test/deploy/dex/proxy.yaml @@ -48,7 +48,7 @@ spec: periodSeconds: 5 failureThreshold: 2 - name: accesslogs - image: debian:10.6-slim + image: debian:10.7-slim command: - "/bin/sh" - "-c" diff --git a/test/integration/e2e_test.go b/test/integration/e2e_test.go index 1bf58c3f..e77f1a6e 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 5e12fc12..fa378245 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 d59c713e..00000000 --- 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 0e9839e2..00b2317d 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -57,19 +57,25 @@ func TestSupervisorLogin(t *testing.T) { require.NoError(t, err) // Create an HTTP client that can reach the downstream discovery endpoint using the CA certs. - httpClient := &http.Client{Transport: &http.Transport{ - TLSClientConfig: &tls.Config{RootCAs: ca.Pool()}, - Proxy: func(req *http.Request) (*url.URL, error) { - if env.Proxy == "" { - t.Logf("passing request for %s with no proxy", req.URL) - return nil, nil - } - proxyURL, err := url.Parse(env.Proxy) - require.NoError(t, err) - t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) - return proxyURL, nil + httpClient := &http.Client{ + Transport: &http.Transport{ + TLSClientConfig: &tls.Config{RootCAs: ca.Pool()}, + Proxy: func(req *http.Request) (*url.URL, error) { + if env.Proxy == "" { + t.Logf("passing request for %s with no proxy", req.URL) + return nil, nil + } + proxyURL, err := url.Parse(env.Proxy) + require.NoError(t, err) + t.Logf("passing request for %s through proxy %s", req.URL, proxyURL.String()) + return proxyURL, nil + }, }, - }} + // Don't follow redirects automatically. + CheckRedirect: func(req *http.Request, via []*http.Request) error { + return http.ErrUseLastResponse + }, + } oidcHTTPClientContext := coreosoidc.ClientContext(ctx, httpClient) // Use the CA to issue a TLS server cert. @@ -127,7 +133,7 @@ func TestSupervisorLogin(t *testing.T) { ClientID: "pinniped-cli", Endpoint: discovery.Endpoint(), RedirectURL: localCallbackServer.URL, - Scopes: []string{"openid", "pinniped.sts.unrestricted", "offline_access"}, + Scopes: []string{"openid", "pinniped:request-audience", "offline_access"}, } // Build a valid downstream authorize URL for the supervisor. @@ -144,6 +150,14 @@ func TestSupervisorLogin(t *testing.T) { pkceParam.Method(), ) + // Make the authorize request one "manually" so we can check its response headers. + authorizeRequest, err := http.NewRequestWithContext(ctx, http.MethodGet, downstreamAuthorizeURL, nil) + require.NoError(t, err) + authorizeResp, err := httpClient.Do(authorizeRequest) + require.NoError(t, err) + require.NoError(t, authorizeResp.Body.Close()) + expectSecurityHeaders(t, authorizeResp) + // Open the web browser and navigate to the downstream authorize URL. page := browsertest.Open(t) t.Logf("opening browser to downstream authorize URL %s", library.MaskTokens(downstreamAuthorizeURL)) @@ -161,16 +175,12 @@ func TestSupervisorLogin(t *testing.T) { callback := localCallbackServer.waitForCallback(10 * time.Second) t.Logf("got callback request: %s", library.MaskTokens(callback.URL.String())) require.Equal(t, stateParam.String(), callback.URL.Query().Get("state")) - require.ElementsMatch(t, []string{"openid", "pinniped.sts.unrestricted", "offline_access"}, strings.Split(callback.URL.Query().Get("scope"), " ")) + require.ElementsMatch(t, []string{"openid", "pinniped:request-audience", "offline_access"}, strings.Split(callback.URL.Query().Get("scope"), " ")) 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", "username"} @@ -310,3 +320,16 @@ func doTokenExchange(t *testing.T, config *oauth2.Config, tokenResponse *oauth2. require.NoError(t, err) t.Logf("exchanged token claims:\n%s", string(indentedClaims)) } + +func expectSecurityHeaders(t *testing.T, response *http.Response) { + h := response.Header + assert.Equal(t, "default-src 'none'; frame-ancestors 'none'", h.Get("Content-Security-Policy")) + assert.Equal(t, "DENY", h.Get("X-Frame-Options")) + assert.Equal(t, "1; mode=block", h.Get("X-XSS-Protection")) + assert.Equal(t, "nosniff", h.Get("X-Content-Type-Options")) + assert.Equal(t, "no-referrer", h.Get("Referrer-Policy")) + assert.Equal(t, "off", h.Get("X-DNS-Prefetch-Control")) + assert.Equal(t, "no-cache,no-store,max-age=0,must-revalidate", h.Get("Cache-Control")) + assert.Equal(t, "no-cache", h.Get("Pragma")) + assert.Equal(t, "0", h.Get("Expires")) +} diff --git a/test/integration/supervisor_secrets_test.go b/test/integration/supervisor_secrets_test.go new file mode 100644 index 00000000..038b6fac --- /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 4b108662..9f6baa57 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -288,6 +288,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 }