From 8b7c30cfbd678ea09c794dabdb2e09c537ba364f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 26 Oct 2020 17:03:26 -0700 Subject: [PATCH] Supervisor listens for HTTPS on port 443 with configurable TLS certs - TLS certificates can be configured on the OIDCProviderConfig using the `secretName` field. - When listening for incoming TLS connections, choose the TLS cert based on the SNI hostname of the incoming request. - Because SNI hostname information on incoming requests does not include the port number of the request, we add a validation that OIDCProviderConfigs where the issuer hostnames (not including port number) are the same must use the same `secretName`. - Note that this approach does not yet support requests made to an IP address instead of a hostname. Also note that `localhost` is considered a hostname by SNI. - Add port 443 as a container port to the pod spec. - A new controller watches for TLS secrets and caches them in memory. That same in-memory cache is used while servicing incoming connections on the TLS port. - Make it easy to configure both port 443 and/or port 80 for various Service types using our ytt templates for the supervisor. - When deploying to kind, add another nodeport and forward it to the host on another port to expose our new HTTPS supervisor port to the host. --- cmd/pinniped-supervisor/main.go | 53 ++- deploy/supervisor/deployment.yaml | 2 + deploy/supervisor/service.yaml | 52 ++- deploy/supervisor/values.yaml | 17 +- hack/lib/kind-config/single-node.yaml | 9 +- hack/lib/tilt/Tiltfile | 8 +- hack/prepare-for-integration-tests.sh | 9 +- .../supervisorconfig/jwks_observer.go | 2 +- .../oidcproviderconfig_watcher.go | 19 +- .../oidcproviderconfig_watcher_test.go | 10 +- .../supervisorconfig/testdata/test.crt | 19 ++ .../supervisorconfig/testdata/test.key | 28 ++ .../supervisorconfig/testdata/test2.crt | 18 ++ .../supervisorconfig/testdata/test2.key | 28 ++ .../supervisorconfig/tls_cert_observer.go | 101 ++++++ .../tls_cert_observer_test.go | 304 ++++++++++++++++++ .../provider/dynamic_tls_cert_provider.go | 37 +++ 17 files changed, 672 insertions(+), 44 deletions(-) create mode 100644 internal/controller/supervisorconfig/testdata/test.crt create mode 100644 internal/controller/supervisorconfig/testdata/test.key create mode 100644 internal/controller/supervisorconfig/testdata/test2.crt create mode 100644 internal/controller/supervisorconfig/testdata/test2.key create mode 100644 internal/controller/supervisorconfig/tls_cert_observer.go create mode 100644 internal/controller/supervisorconfig/tls_cert_observer_test.go create mode 100644 internal/oidc/provider/dynamic_tls_cert_provider.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 346f83ca..7b296fa9 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -5,11 +5,13 @@ package main import ( "context" + "crypto/tls" "fmt" "net" "net/http" "os" "os/signal" + "strings" "time" "k8s.io/apimachinery/pkg/util/clock" @@ -28,6 +30,7 @@ import ( "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/oidc/jwks" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider/manager" ) @@ -68,6 +71,7 @@ func startControllers( cfg *supervisor.Config, issuerManager *manager.Manager, dynamicJWKSProvider jwks.DynamicJWKSProvider, + dynamicTLSCertProvider provider.DynamicTLSCertProvider, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, kubeInformers kubeinformers.SharedInformerFactory, @@ -105,6 +109,15 @@ func startControllers( controllerlib.WithInformer, ), singletonWorker, + ). + WithController( + supervisorconfig.NewTLSCertObserverController( + dynamicTLSCertProvider, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ), + singletonWorker, ) kubeInformers.Start(ctx.Done()) @@ -166,19 +179,49 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { })) dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() + dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider() + // OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux. oidProvidersManager := manager.NewManager(healthMux, dynamicJWKSProvider) - startControllers(ctx, cfg, oidProvidersManager, dynamicJWKSProvider, kubeClient, pinnipedClient, kubeInformers, pinnipedInformers) + + startControllers( + ctx, + cfg, + oidProvidersManager, + dynamicJWKSProvider, + dynamicTLSCertProvider, + kubeClient, + pinnipedClient, + kubeInformers, + pinnipedInformers, + ) //nolint: gosec // Intentionally binding to all network interfaces. - l, err := net.Listen("tcp", ":80") + httpListener, err := net.Listen("tcp", ":80") if err != nil { return fmt.Errorf("cannot create listener: %w", err) } - defer l.Close() - start(ctx, l, oidProvidersManager) + defer httpListener.Close() + start(ctx, httpListener, oidProvidersManager) - klog.InfoS("supervisor is ready", "address", l.Addr().String()) + //nolint: gosec // Intentionally binding to all network interfaces. + httpsListener, err := tls.Listen("tcp", ":443", &tls.Config{ + MinVersion: tls.VersionTLS13, + GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { + klog.InfoS("GetCertificate called", "info.ServerName", info.ServerName) + return dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)), nil + }, + }) + if err != nil { + return fmt.Errorf("cannot create listener: %w", err) + } + defer httpsListener.Close() + start(ctx, httpsListener, oidProvidersManager) + + klog.InfoS("supervisor is ready", + "httpAddress", httpListener.Addr().String(), + "httpsAddress", httpsListener.Addr().String(), + ) gotSignal := waitForSignal() klog.InfoS("supervisor exiting", "signal", gotSignal) diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index afa4efd5..d5161d46 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -87,6 +87,8 @@ spec: ports: - containerPort: 80 protocol: TCP + - containerPort: 443 + protocol: TCP livenessProbe: httpGet: path: /healthz diff --git a/deploy/supervisor/service.yaml b/deploy/supervisor/service.yaml index 19076e5e..e2841a63 100644 --- a/deploy/supervisor/service.yaml +++ b/deploy/supervisor/service.yaml @@ -4,7 +4,7 @@ #@ load("@ytt:data", "data") #@ load("helpers.lib.yaml", "defaultLabel", "labels", "namespace", "defaultResourceName", "defaultResourceNameWithSuffix") -#@ if data.values.service_nodeport_port: +#@ if data.values.service_http_nodeport_port or data.values.service_https_nodeport_port: --- apiVersion: v1 kind: Service @@ -17,15 +17,27 @@ spec: selector: app: #@ data.values.app_name ports: - - protocol: TCP - port: #@ data.values.service_nodeport_port + #@ if data.values.service_http_nodeport_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_nodeport_port targetPort: 80 - #@ if data.values.service_nodeport_nodeport: - nodePort: #@ data.values.service_nodeport_nodeport + #@ if data.values.service_http_nodeport_nodeport: + nodePort: #@ data.values.service_http_nodeport_nodeport #@ end + #@ end + #@ if data.values.service_https_nodeport_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_nodeport_port + targetPort: 443 + #@ if data.values.service_https_nodeport_nodeport: + nodePort: #@ data.values.service_https_nodeport_nodeport + #@ end + #@ end #@ end -#@ if data.values.service_clusterip_port: +#@ if data.values.service_http_clusterip_port or data.values.service_https_clusterip_port: --- apiVersion: v1 kind: Service @@ -37,12 +49,21 @@ spec: type: ClusterIP selector: #@ defaultLabel() ports: - - protocol: TCP - port: #@ data.values.service_clusterip_port + #@ if data.values.service_http_clusterip_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_clusterip_port targetPort: 80 + #@ end + #@ if data.values.service_https_clusterip_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_clusterip_port + targetPort: 443 + #@ end #@ end -#@ if data.values.service_loadbalancer_port: +#@ if data.values.service_http_loadbalancer_port or data.values.service_https_loadbalancer_port: --- apiVersion: v1 kind: Service @@ -54,7 +75,16 @@ spec: type: LoadBalancer selector: #@ defaultLabel() ports: - - protocol: TCP - port: #@ data.values.service_loadbalancer_port + #@ if data.values.service_http_loadbalancer_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_loadbalancer_port targetPort: 80 + #@ end + #@ if data.values.service_https_loadbalancer_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_loadbalancer_port + targetPort: 443 + #@ end #@ end diff --git a/deploy/supervisor/values.yaml b/deploy/supervisor/values.yaml index ea06cf30..aea4a04e 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -35,10 +35,15 @@ image_tag: latest #! Optional. image_pull_dockerconfigjson: #! e.g. {"auths":{"https://registry.example.com":{"username":"USERNAME","password":"PASSWORD","auth":"BASE64_ENCODED_USERNAME_COLON_PASSWORD"}}} -#! Specify how to expose the Supervisor app as a Service. -#! Typically you would set a value for only one of the following. +#! Specify how to expose the Supervisor app's HTTP and/or HTTPS ports as a Service. +#! Typically you would set a value for only one of the following service types, for either HTTP or HTTPS depending on your needs. +#! An HTTP service should not be exposed outside the cluster. It would not be secure to serve OIDC endpoints to end users via HTTP. #! Setting any of these values means that a Service of that type will be created. -service_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, e.g. 31234 -service_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_nodeport_port` is specified, e.g. 31234 -service_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, e.g. 443 -service_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, e.g. 443 +service_http_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, with port 80 as its `targetPort`; e.g. 31234 +service_http_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_http_nodeport_port` is specified; e.g. 31234 +service_http_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, with port 80 as its `targetPort`; e.g. 443 +service_http_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, with port 80 as its `targetPort`; e.g. 443 +service_https_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, with port 443 as its `targetPort`; e.g. 31243 +service_https_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_http_nodeport_port` is specified; e.g. 31243 +service_https_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, with port 443 as its `targetPort`; e.g. 443 +service_https_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, with port 443 as its `targetPort`; e.g. 443 diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index 83c36a58..5247f5b5 100644 --- a/hack/lib/kind-config/single-node.yaml +++ b/hack/lib/kind-config/single-node.yaml @@ -6,7 +6,14 @@ nodes: - protocol: TCP # This same port number is hardcoded in the integration test setup # when creating a Service on a kind cluster. It is used to talk to - # the supervisor app. + # the supervisor app via HTTPS. + containerPort: 31243 + hostPort: 12344 + listenAddress: 127.0.0.1 + - protocol: TCP + # This same port number is hardcoded in the integration test setup + # when creating a Service on a kind cluster. It is used to talk to + # the supervisor app via HTTP. containerPort: 31234 hostPort: 12345 listenAddress: 127.0.0.1 diff --git a/hack/lib/tilt/Tiltfile b/hack/lib/tilt/Tiltfile index 899ea4a5..ebd6e3b8 100644 --- a/hack/lib/tilt/Tiltfile +++ b/hack/lib/tilt/Tiltfile @@ -86,7 +86,7 @@ docker_build_with_restart('image/supervisor', '.', # Render the supervisor installation manifest using ytt. # -# 31234 is the same port number hardcoded in the port forwarding of our kind configuration. +# 31234 and 31243 are the same port numbers hardcoded in the port forwarding of our kind configuration. # Don't think that you can just change this! k8s_yaml(local([ 'ytt', @@ -96,8 +96,10 @@ k8s_yaml(local([ '--data-value', 'image_repo=image/supervisor', '--data-value', 'image_tag=tilt-dev', '--data-value-yaml', 'replicas=1', - '--data-value-yaml', 'service_nodeport_port=80', - '--data-value-yaml', 'service_nodeport_nodeport=31234', + '--data-value-yaml', 'service_http_nodeport_port=80', + '--data-value-yaml', 'service_http_nodeport_nodeport=31234', + '--data-value-yaml', 'service_https_nodeport_port=443', + '--data-value-yaml', 'service_https_nodeport_nodeport=31243', '--data-value-yaml', 'custom_labels={mySupervisorCustomLabelName: mySupervisorCustomLabelValue}', ])) # Tell tilt to watch all of those files for changes. diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 19b3cac4..0e06fe1f 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -119,7 +119,7 @@ if ! tilt_mode; then log_note "Checking for running kind clusters..." if ! kind get clusters | grep -q -e '^pinniped$'; then log_note "Creating a kind cluster..." - # single-node.yaml exposes node port 31234 as 127.0.0.1:12345 and port 31235 as 127.0.0.1:12346 + # single-node.yaml exposes node port 31234 as 127.0.0.1:12345, 31243 as 127.0.0.1:12344, and 31235 as 127.0.0.1:12346 kind create cluster --config "$pinniped_path/hack/lib/kind-config/single-node.yaml" --name pinniped else if ! kubectl cluster-info | grep master | grep -q 127.0.0.1; then @@ -224,8 +224,11 @@ if ! tilt_mode; then --data-value "image_repo=$registry_repo" \ --data-value "image_tag=$tag" \ --data-value-yaml "custom_labels=$supervisor_custom_labels" \ - --data-value-yaml 'service_nodeport_port=80' \ - --data-value-yaml 'service_nodeport_nodeport=31234' >"$manifest" + --data-value-yaml 'service_http_nodeport_port=80' \ + --data-value-yaml 'service_http_nodeport_nodeport=31234' \ + --data-value-yaml 'service_https_nodeport_port=443' \ + --data-value-yaml 'service_https_nodeport_nodeport=31243' \ + >"$manifest" kapp deploy --yes --app "$supervisor_app_name" --diff-changes --file "$manifest" diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index c9911b13..bec3cef6 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -40,7 +40,7 @@ func NewJWKSObserverController( ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ - Name: "certs-observer-controller", + Name: "jwks-observer-controller", Syncer: &jwksObserverController{ issuerToJWKSSetter: issuerToJWKSSetter, oidcProviderConfigInformer: oidcProviderConfigInformer, diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index 12dc2b63..cb17bb46 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -81,14 +81,13 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er return fmt.Sprintf("%s://%s%s", issuerURL.Scheme, strings.ToLower(issuerURL.Host), issuerURL.Path) } - // Make a map of issuer addresses -> set of unique secret names. This will help us complain when - // multiple OIDCProviderConfigs have the same issuer address (host) component but specify + // Make a map of issuer hostnames -> set of unique secret names. This will help us complain when + // multiple OIDCProviderConfigs have the same issuer hostname (excluding port) but specify // different TLS serving Secrets. Doesn't make sense to have the one address use more than one - // TLS cert. Also make a helper function for forming keys into this map. + // TLS cert. Ignore ports because SNI information on the incoming requests is not going to include + // port numbers. Also make a helper function for forming keys into this map. uniqueSecretNamesPerIssuerAddress := make(map[string]map[string]bool) - issuerURLToHostKey := func(issuerURL *url.URL) string { - return strings.ToLower(issuerURL.Host) - } + issuerURLToHostnameKey := lowercaseHostWithoutPort for _, opc := range all { issuerURL, err := url.Parse(opc.Spec.Issuer) @@ -98,10 +97,10 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er issuerCounts[issuerURLToIssuerKey(issuerURL)]++ - setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] + setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] if setOfSecretNames == nil { setOfSecretNames = make(map[string]bool) - uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] = setOfSecretNames + uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] = setOfSecretNames } setOfSecretNames[opc.Spec.SecretName] = true } @@ -129,13 +128,13 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er } // Skip url parse errors because they will be validated below. - if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)]) > 1 { + if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)]) > 1 { if err := c.updateStatus( ctx.Context, opc.Namespace, opc.Name, configv1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus, - "Issuers with the same address must use the same secretName: "+issuerURLToHostKey(issuerURL), + "Issuers with the same DNS hostname (address not including port) must use the same secretName: "+issuerURLToHostnameKey(issuerURL), ); err != nil { errs.Add(fmt.Errorf("could not update status: %w", err)) } diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go index 1028b324..7efeffe4 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go @@ -807,7 +807,7 @@ func TestSync(t *testing.T) { }) }) - when("there are OIDCProviderConfigs with the same issuer address using different secretNames", func() { + when("there are OIDCProviderConfigs with the same issuer DNS hostname using different secretNames", func() { var ( oidcProviderConfigSameIssuerAddress1 *v1alpha1.OIDCProviderConfig oidcProviderConfigSameIssuerAddress2 *v1alpha1.OIDCProviderConfig @@ -828,7 +828,9 @@ func TestSync(t *testing.T) { oidcProviderConfigSameIssuerAddress2 = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "provider2", Namespace: namespace}, Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: "https://issuer-duplicate-address.com/path2", + // Validation treats these as the same DNS hostname even though they have different port numbers, + // because SNI information on the incoming requests is not going to include port numbers. + Issuer: "https://issuer-duplicate-address.com:1234/path2", SecretName: "secret2", }, } @@ -888,11 +890,11 @@ func TestSync(t *testing.T) { oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigSameIssuerAddress1.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus - oidcProviderConfigSameIssuerAddress1.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress1.Status.Message = "Issuers with the same DNS hostname (address not including port) must use the same secretName: issuer-duplicate-address.com" oidcProviderConfigSameIssuerAddress1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigSameIssuerAddress2.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus - oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same DNS hostname (address not including port) must use the same secretName: issuer-duplicate-address.com" oidcProviderConfigSameIssuerAddress2.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigWithInvalidIssuerURL.Status.Status = v1alpha1.InvalidOIDCProviderStatus diff --git a/internal/controller/supervisorconfig/testdata/test.crt b/internal/controller/supervisorconfig/testdata/test.crt new file mode 100644 index 00000000..e54883b0 --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test.crt @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDBjCCAe4CCQDDx1zebLLuzzANBgkqhkiG9w0BAQsFADBFMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCQ0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xETAPBgNVBAoM +CFBpbm5pcGVkMB4XDTIwMTAyNjE2MzcyOVoXDTIxMTAyNjE2MzcyOVowRTELMAkG +A1UEBhMCVVMxCzAJBgNVBAgMAkNBMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMREw +DwYDVQQKDAhQaW5uaXBlZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB +AJik4mXLVHEIGTK679gjNZNFsutcFGhwCg6WTPy+EAEUjGOUEI/Ca7JAnZGGSZpD +bxnWdXwSt+k7taMWzZIiCosXnvrFmlyCO4wlcajDIOTauG6DKop+S2NjydZxuwUR +G1fb2zXm6Kh3dqwbSzCM7i4pPTEhXJLI04fX6gxyETUGr+rs/p44KtELzSU9NKmP +KUyf8wtoSCz00HYu1auV1px/I1JaKdubx9c5zpr93gJDF2euVV5yaLr1BoRr3UVB +Y5Qa0UWPYCWcTvXyeAku4h4yT6B9iZP/reZfpHSmBxSAPrv4Y8oUqal+i92R77WJ +EkBRm5lVym7l/st3iTmpMlsCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAd+P3Dfkz +REzsdzja0wYb10q1vggAyMtxhvQdG6kND8esWAki/nAgVnXxIq4Eg0Jnanq9SS2Q +Ab6zpRelEB5YeDPZ7Xm6ApLBxqoEciPNqPARK2YJUPFyZJgntsLBeKKojLVE2KqY +DB8ZxKcmh7NPF4KVL3DSoWGwl4UkZt06R+VfxSSuOm/HtxPmdrz5fR6fNYjb4ss8 +sYY6wJTAILzGxpkhiWGXpE6VgdD5qh6+SevRuynHFKiTQ9L1T5aiAEC55VSizmcT +MRkiZHBMQ5pCaPppnaqahWZ757fdk853miG9ckZ58lq7ccCqU0FlaMwf5jjuMb49 +rM1zqYxgeIqwvw== +-----END CERTIFICATE----- diff --git a/internal/controller/supervisorconfig/testdata/test.key b/internal/controller/supervisorconfig/testdata/test.key new file mode 100644 index 00000000..e29ea462 --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCYpOJly1RxCBky +uu/YIzWTRbLrXBRocAoOlkz8vhABFIxjlBCPwmuyQJ2RhkmaQ28Z1nV8ErfpO7Wj +Fs2SIgqLF576xZpcgjuMJXGowyDk2rhugyqKfktjY8nWcbsFERtX29s15uiod3as +G0swjO4uKT0xIVySyNOH1+oMchE1Bq/q7P6eOCrRC80lPTSpjylMn/MLaEgs9NB2 +LtWrldacfyNSWinbm8fXOc6a/d4CQxdnrlVecmi69QaEa91FQWOUGtFFj2AlnE71 +8ngJLuIeMk+gfYmT/63mX6R0pgcUgD67+GPKFKmpfovdke+1iRJAUZuZVcpu5f7L +d4k5qTJbAgMBAAECggEBAJFM4vVjB45Q1yujJovneCgoQJgpnoOLowcfq0kq4rEk +jj57wwgVWc7kExljasydRDSkIFFqwAYUAGKuYiCopsCgS4UKdFV64pQVUIwEslsm +mEkaMnSCo+CILKkkuZGpJw4LCi/VDcLPdPd/Q6ODg3YNa2JJD4XqBPFaZkBSlG6T +5pE++Lwno0t9CWprvE1i/gxMT9CzN0XsBCFZMirqImqpunRs/T7X2uxERXxE+eCY +bi/Q9yGbYVQzuhVyR/aHfuvOfsqrTpdEybQWzgzOwbuX0jgGv88AmcDgYm/d287K +chFHUrpGEJbLIFHv1/XHeVUEB0WXJxK5GO3J34i8DoECgYEAyibL+ZD197Ugn2RB +O1n7tqSWcRR3RlOC83CGdYFkyLc93UOxs4XUGo/xnXA7/2eyNlb8E+W7Z0G1oGln +0+ZkUjZHnJr1d/5uvZILM5/V8JhI2Tz11+gVbnbdKBj+1tdcxqZCuCZX0jkVTdOZ +/i4lDD53oJitWuvM3DkjiRFeV2UCgYEAwU4NrOSuNYaLsSuXbJGQpLDK86FxRRx3 +zAcMKUIKZItRzGab6iwyl+w3xTkouu2+tp3MbLrBoVortWHw8NgR9zibjM6r7L0P +n7uexa1RAT0O5XuGJtjjlJZ4CFbsrBVTAHlGKr8lhXIBJTt5UINBsCQivp+fQdbg +fXq+1kHJJr8CgYB11SGGimnlhq3KWwzvBKeFsfCDX5Oa6ajmL8wgiFjv6mfkJsZZ +R4P4K7mBtN80JASsSg3Lp1iSeqndJDPCP4Rwq3UYova8iBGS7KMc52k0QgAMqM0A +miaL6jtFWTSKlKReoqE3aBo+zslNQS99CvbLaUof0X8TBWm3YJMHHZmpRQKBgQCr +0H+xO/VoF/XT/QXzdxLUf1t03vs5zYrhayYxCcUJBxgmkNFme/BgPpJ3l02PkL+h +u3I29mwiyW3uI2av+61ESylfJ1eC7ayUcoQ1+c31RtsVuAxOPRtTN8bqyrBEaBPF +aQWn+wwTp3hDKrCykmfxcrz7KA+6yo3wmghDkmeDKwKBgCrWwlhVKtigrBKlkraZ +iD+Inq/KBpjgdimJMQDrRFusX6Rp35k1nvRsFVt+JETiDQij/hKqYecbkkWu9wTd +h7F6q0i2Y4yZaiiagK7SSDBd1quVd0+yJ5u3TgTcMVqB17iTCN4YlEEnaCi/S0dM +LlmHgtyFDi6BxB1lheKDewSo +-----END PRIVATE KEY----- diff --git a/internal/controller/supervisorconfig/testdata/test2.crt b/internal/controller/supervisorconfig/testdata/test2.crt new file mode 100644 index 00000000..7b17500b --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test2.crt @@ -0,0 +1,18 @@ +-----BEGIN CERTIFICATE----- +MIIC/DCCAeQCCQC8V+VeJZWyljANBgkqhkiG9w0BAQsFADBAMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCTlkxETAPBgNVBAcMCE5ldyBZb3JrMREwDwYDVQQKDAhQaW5u +aXBlZDAeFw0yMDEwMjYxNjM5MDlaFw0yMTEwMjYxNjM5MDlaMEAxCzAJBgNVBAYT +AlVTMQswCQYDVQQIDAJOWTERMA8GA1UEBwwITmV3IFlvcmsxETAPBgNVBAoMCFBp +bm5pcGVkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArk4nLgjs0wve +NW8SZ1N0HP0jeQAjqO017bDC6d+jGu8Gl39HdojMLol7EXQz9zlT6Of+uerGHnDr +8xbE3juLrKCihDW7w8nENUBlA+NEEc8yvn3YqMJXMIXbPgWTch+PsEWTi5xWoZEs +JBuLSb9oI2OuzYy5XvT5TWeaLAmeJUO2IQR6UJ6oTGE4nOC6kqJM4Kx8xMKAn7yf +OlKF3vhVmlL+5e9+lpIJN18nDjORATtbZPJPLKELJGVT9repCNuQMuemDnABJWb4 +NPQK8pzZ/hrW/1LdwFnCn6zu0ZlZVzUy6lWl0Hqux1KeMhYjut9nkKMKixCX6nWc +g+Y2X1VO5QIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQBfLq+20Yq24ad9ENh7Wkz0 +W7teg0GN7kNZRuNIRHgXyhsZ/HAI9etfOv4rt/ynyxlnx5j7HRM+Izc6fG3qEQnp +N1RRsHw2As2ilr62g7hHuuEg75/sWDzO1Z7/LjNp3uTE+2Jqb6P+rga2vO1gg8pb ++A8AmU3BmvhOwb2RjaZQvOZGOcWkEZjpJp5mPGptMsl4BzA4kBp4kpzoBBUA34/i +CRP68yTclnKmKvhI0Yg3QlihD8SjMWWa9UUZYGaKE0H8EGHxRldfqGBdjqs6RaaK +xdDFUULvpOFC0lDruWZ/YztiVi3iH2zEiGnxD4OwfapmjhlLcCDlNsNkaHh9UigQ +-----END CERTIFICATE----- diff --git a/internal/controller/supervisorconfig/testdata/test2.key b/internal/controller/supervisorconfig/testdata/test2.key new file mode 100644 index 00000000..042bbceb --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test2.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCuTicuCOzTC941 +bxJnU3Qc/SN5ACOo7TXtsMLp36Ma7waXf0d2iMwuiXsRdDP3OVPo5/656sYecOvz +FsTeO4usoKKENbvDycQ1QGUD40QRzzK+fdiowlcwhds+BZNyH4+wRZOLnFahkSwk +G4tJv2gjY67NjLle9PlNZ5osCZ4lQ7YhBHpQnqhMYTic4LqSokzgrHzEwoCfvJ86 +UoXe+FWaUv7l736Wkgk3XycOM5EBO1tk8k8soQskZVP2t6kI25Ay56YOcAElZvg0 +9ArynNn+Gtb/Ut3AWcKfrO7RmVlXNTLqVaXQeq7HUp4yFiO632eQowqLEJfqdZyD +5jZfVU7lAgMBAAECggEBAKSOFcEJHgN0bdjGPnqbt7/yX33JWuEM6N+4A5tlzQcN +Z4y41Y+bQCAjHLNyn+ijD4uPEdUVRurQMoDxGvSvBIL5t9PXIqeJIRog6/zKnqWt +lbtu9Y8EwemGRV/9RaD1GOMSHGQuOT8Y3bJM6qe58yeN4SYe15ZE8eNYjp1Kiymj +fzSlaMHM/yi8IKL9QtT7Bb9Fclfu8AB3gQM25mCisd+xMFqsg2CWf7zVUDkp8tBi +LkfhJhbGmjpUcQbZGR+bAssBLXh4vBJhLWvfqSDLLD/+pavzcYWHv2XpBzY0VWyq +t4762L6azYuRVFwyeTYTt9rsbVIK7ZOpOxGdeSd318ECgYEA1oatFot7WaFoMp8A +v/7GSNm/txS0um551yt5Ugi7Iv4w/vcsrKHgRXa3ADmT6efFAwePWJTvG1N+iwOU +I+dQBMB4BDMdJAfY4fccOF6J3GNBGgz2DFEhjizcKGu/knot7C9P5vWiSh1U76TZ +e1oqku6AX7/TM3Dia6QbNM7RFtECgYEA0ADenD1e2i2FvSkTkmHrMIjRKt9FesHl +9sPN7dnAaaBi0/7gr3fan11h2DreQV2r29JQhZxFx2EO7DjR+9qFnSO2oueula/X +HfhxKu12aOKMSoSNoL4oBuk5iy/vWh0fwO7S8/a48k99JGXX2Lct4bfR0y3yRFO+ +zHM8T+6C49UCgYEAljlnGgOA1Gov8krgFpLNvZQmKYmpaWgVkDTUVzrf+QgxvUnP +kfAlgd85FUI8ry5rCs0Pd4OL0QHt+mD+Kwo/QaSaJq64eFO6b7pAm8SwG5GxtBFh +d4yUx9/oJ7IUS/mdEOistlpKVEYoBUzWMwgYCh5T7TkCJ+Kj26bmmls9lhECgYBm +24c5i7+T9F7mI6HiCTncTkvg/3fENI4bcMgsjjlwAjfczXUeUA50MCFqY/H0MPYD +RgU7jQOUjJJsjcyI1o6sHjT6accTjli6IVkU+UhMpXrqfpHqox34DOy/v3yE+1Hw +fikjKyZZ7KTdkt8h87NkoxnHbDkZQLBhObrha/id4QKBgBs0Rr+BAOM78MVocPFO +RPuagzFFaU59rOqlPOaRMDZ2jfokC09Maouchc2lJCX9ip4LwdqxiQw8GFRjZEz1 +l+UZd7vAPQUtWQeACYMnmJ2CVNXZhghBQ4ltYTtEJwvB5420bcW7MBSrOYNBCsPr +gDqlX93KhuFxucNTnZj3dbNo +-----END PRIVATE KEY----- diff --git a/internal/controller/supervisorconfig/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go new file mode 100644 index 00000000..5d382bbf --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -0,0 +1,101 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "crypto/tls" + "fmt" + "net/url" + "strings" + + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/klog/v2" + + "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1" + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" +) + +type tlsCertObserverController struct { + issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer +} + +type IssuerHostToTLSCertMapSetter interface { + SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) +} + +func NewTLSCertObserverController( + issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter, + secretInformer corev1informers.SecretInformer, + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "tls-certs-observer-controller", + Syncer: &tlsCertObserverController{ + issuerHostToTLSCertMapSetter: issuerHostToTLSCertMapSetter, + oidcProviderConfigInformer: oidcProviderConfigInformer, + secretInformer: secretInformer, + }, + }, + withInformer( + secretInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + withInformer( + oidcProviderConfigInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + ) +} + +func (c *tlsCertObserverController) Sync(ctx controllerlib.Context) error { + ns := ctx.Key.Namespace + allProviders, err := c.oidcProviderConfigInformer.Lister().OIDCProviderConfigs(ns).List(labels.Everything()) + if err != nil { + return fmt.Errorf("failed to list OIDCProviderConfigs: %w", err) + } + + // Rebuild the whole map on any change to any Secret or OIDCProvider, because either can have changes that + // can cause the map to need to be updated. + issuerHostToTLSCertMap := map[string]*tls.Certificate{} + + for _, provider := range allProviders { + secretName := provider.Spec.SecretName + issuerURL, err := url.Parse(provider.Spec.Issuer) + if err != nil { + klog.InfoS("tlsCertObserverController Sync found an invalid issuer URL", "namespace", ns, "issuer", provider.Spec.Issuer) + continue + } + tlsSecret, err := c.secretInformer.Lister().Secrets(ns).Get(secretName) + if err != nil { + klog.InfoS("tlsCertObserverController Sync could not find TLS cert secret", "namespace", ns, "secretName", secretName) + continue + } + certFromSecret, err := tls.X509KeyPair(tlsSecret.Data["tls.crt"], tlsSecret.Data["tls.key"]) + if err != nil { + klog.InfoS("tlsCertObserverController Sync found a TLS secret with Data in an unexpected format", "namespace", ns, "secretName", secretName) + continue + } + // Lowercase the host part of the URL because hostnames should be treated as case-insensitive. + issuerHostToTLSCertMap[lowercaseHostWithoutPort(issuerURL)] = &certFromSecret + } + + klog.InfoS("tlsCertObserverController Sync updated the TLS cert cache", "issuerHostCount", len(issuerHostToTLSCertMap)) + c.issuerHostToTLSCertMapSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) + + return nil +} + +func lowercaseHostWithoutPort(issuerURL *url.URL) string { + lowercaseHost := strings.ToLower(issuerURL.Host) + colonSegments := strings.Split(lowercaseHost, ":") + return colonSegments[0] +} diff --git a/internal/controller/supervisorconfig/tls_cert_observer_test.go b/internal/controller/supervisorconfig/tls_cert_observer_test.go new file mode 100644 index 00000000..6510b732 --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -0,0 +1,304 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "context" + "crypto/tls" + "io/ioutil" + "net/url" + "testing" + "time" + + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + + "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" + pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" +) + +func TestTLSCertObserverControllerInformerFilters(t *testing.T) { + spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) { + var ( + r *require.Assertions + observableWithInformerOption *testutil.ObservableWithInformerOption + secretsInformerFilter controllerlib.Filter + oidcProviderConfigInformerFilter controllerlib.Filter + ) + + it.Before(func() { + r = require.New(t) + observableWithInformerOption = testutil.NewObservableWithInformerOption() + secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() + oidcProviderConfigInformer := pinnipedinformers.NewSharedInformerFactory(nil, 0).Config().V1alpha1().OIDCProviderConfigs() + _ = NewTLSCertObserverController( + nil, + secretsInformer, + oidcProviderConfigInformer, + observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters + ) + secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) + oidcProviderConfigInformerFilter = observableWithInformerOption.GetFilterForInformer(oidcProviderConfigInformer) + }) + + when("watching Secret objects", func() { + var ( + subject controllerlib.Filter + secret, otherSecret *corev1.Secret + ) + + it.Before(func() { + subject = secretsInformerFilter + secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any Secret changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(secret)) + r.True(subject.Update(secret, otherSecret)) + r.True(subject.Update(otherSecret, secret)) + r.True(subject.Delete(secret)) + }) + }) + }) + + when("watching OIDCProviderConfig objects", func() { + var ( + subject controllerlib.Filter + provider, otherProvider *v1alpha1.OIDCProviderConfig + ) + + it.Before(func() { + subject = oidcProviderConfigInformerFilter + provider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherProvider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any OIDCProviderConfig changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(provider)) + r.True(subject.Update(provider, otherProvider)) + r.True(subject.Update(otherProvider, provider)) + r.True(subject.Delete(provider)) + }) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} + +type fakeIssuerHostToTLSCertMapSetter struct { + setIssuerHostToTLSCertMapWasCalled bool + issuerHostToTLSCertMapReceived map[string]*tls.Certificate +} + +func (f *fakeIssuerHostToTLSCertMapSetter) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) { + f.setIssuerHostToTLSCertMapWasCalled = true + f.issuerHostToTLSCertMapReceived = issuerHostToTLSCertMap +} + +func TestTLSCertObserverControllerSync(t *testing.T) { + spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { + const installedInNamespace = "some-namespace" + + var ( + r *require.Assertions + subject controllerlib.Controller + pinnipedInformerClient *pinnipedfake.Clientset + kubeInformerClient *kubernetesfake.Clientset + pinnipedInformers pinnipedinformers.SharedInformerFactory + kubeInformers kubeinformers.SharedInformerFactory + timeoutContext context.Context + timeoutContextCancel context.CancelFunc + syncContext *controllerlib.Context + issuerHostToTLSCertSetter *fakeIssuerHostToTLSCertMapSetter + ) + + // Defer starting the informers until the last possible moment so that the + // nested Before's can keep adding things to the informer caches. + var startInformersAndController = func() { + // Set this at the last second to allow for injection of server override. + subject = NewTLSCertObserverController( + issuerHostToTLSCertSetter, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ) + + // Set this at the last second to support calling subject.Name(). + syncContext = &controllerlib.Context{ + Context: timeoutContext, + Name: subject.Name(), + Key: controllerlib.Key{ + Namespace: installedInNamespace, + Name: "any-name", + }, + } + + // Must start informers before calling TestRunSynchronously() + kubeInformers.Start(timeoutContext.Done()) + pinnipedInformers.Start(timeoutContext.Done()) + controllerlib.TestRunSynchronously(t, subject) + } + + var readTestFile = func(path string) []byte { + data, err := ioutil.ReadFile(path) + r.NoError(err) + return data + } + + it.Before(func() { + r = require.New(t) + + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) + + kubeInformerClient = kubernetesfake.NewSimpleClientset() + kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) + pinnipedInformerClient = pinnipedfake.NewSimpleClientset() + pinnipedInformers = pinnipedinformers.NewSharedInformerFactory(pinnipedInformerClient, 0) + issuerHostToTLSCertSetter = &fakeIssuerHostToTLSCertMapSetter{} + + unrelatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some other unrelated secret", + Namespace: installedInNamespace, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(unrelatedSecret)) + }) + + it.After(func() { + timeoutContextCancel() + }) + + when("there are no OIDCProviderConfigs and no TLS Secrets yet", func() { + it("sets the issuerHostToTLSCertSetter's map to be empty", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + r.True(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Empty(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived) + }) + }) + + when("there are OIDCProviderConfigs where some have corresponding TLS Secrets and some don't", func() { + var ( + expectedCertificate1, expectedCertificate2 tls.Certificate + ) + + it.Before(func() { + var err error + oidcProviderConfigWithoutSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer1.com"}, // no SecretName field + } + oidcProviderConfigWithoutSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer2.com", SecretName: ""}, + } + oidcProviderConfigWithBadSecret := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://bad-secret-issuer.com", SecretName: "bad-tls-secret-name"}, + } + // Also add one with a URL that cannot be parsed to make sure that the controller is not confused by invalid URLs. + invalidIssuerURL := ":/host//path" + _, err = url.Parse(invalidIssuerURL) //nolint:staticcheck // Yes, this URL is intentionally invalid. + r.Error(err) + oidcProviderConfigWithBadIssuer := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-issuer-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: invalidIssuerURL}, + } + oidcProviderConfigWithGoodSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test without a port number. + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.iSSuer-wiTh-goOd-secRet1.cOm/path", SecretName: "good-tls-secret-name1"}, + } + oidcProviderConfigWithGoodSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test with a port number. + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.issUEr-WIth-gOOd-seCret2.com:1234/path", SecretName: "good-tls-secret-name2"}, + } + testCrt1 := readTestFile("testdata/test.crt") + r.NotEmpty(testCrt1) + testCrt2 := readTestFile("testdata/test2.crt") + r.NotEmpty(testCrt2) + testKey1 := readTestFile("testdata/test.key") + r.NotEmpty(testKey1) + testKey2 := readTestFile("testdata/test2.key") + r.NotEmpty(testKey2) + expectedCertificate1, err = tls.X509KeyPair(testCrt1, testKey1) + r.NoError(err) + expectedCertificate2, err = tls.X509KeyPair(testCrt2, testKey2) + r.NoError(err) + goodTLSSecret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "good-tls-secret-name1", Namespace: installedInNamespace}, + Data: map[string][]byte{"tls.crt": testCrt1, "tls.key": testKey1}, + } + goodTLSSecret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "good-tls-secret-name2", Namespace: installedInNamespace}, + Data: map[string][]byte{"tls.crt": testCrt2, "tls.key": testKey2}, + } + badTLSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-tls-secret-name", Namespace: installedInNamespace}, + Data: map[string][]byte{"junk": nil}, + } + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret2)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithBadSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithBadIssuer)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(goodTLSSecret1)) + r.NoError(kubeInformerClient.Tracker().Add(goodTLSSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(badTLSSecret)) + }) + + it("updates the issuerHostToTLSCertSetter's map to include only the issuers that had valid certs", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.True(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Len(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived, 2) + + // They keys in the map should be lower case and should not include the port numbers, because + // TLS SNI says that SNI hostnames must be DNS names (not ports) and must be case insensitive. + // See https://tools.ietf.org/html/rfc3546#section-3.1 + actualCertificate1 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret1.com"] + r.NotNil(actualCertificate1) + // The actual cert should match the one from the test fixture that was put into the secret. + r.Equal(expectedCertificate1, *actualCertificate1) + actualCertificate2 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret2.com"] + r.NotNil(actualCertificate2) + r.Equal(expectedCertificate2, *actualCertificate2) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} diff --git a/internal/oidc/provider/dynamic_tls_cert_provider.go b/internal/oidc/provider/dynamic_tls_cert_provider.go new file mode 100644 index 00000000..51e3c157 --- /dev/null +++ b/internal/oidc/provider/dynamic_tls_cert_provider.go @@ -0,0 +1,37 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package provider + +import ( + "crypto/tls" + "sync" +) + +type DynamicTLSCertProvider interface { + SetIssuerHostToTLSCertMap(issuerToJWKSMap map[string]*tls.Certificate) + GetTLSCert(lowercaseIssuerHostName string) *tls.Certificate +} + +type dynamicTLSCertProvider struct { + issuerHostToTLSCertMap map[string]*tls.Certificate + mutex sync.RWMutex +} + +func NewDynamicTLSCertProvider() DynamicTLSCertProvider { + return &dynamicTLSCertProvider{ + issuerHostToTLSCertMap: map[string]*tls.Certificate{}, + } +} + +func (p *dynamicTLSCertProvider) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.issuerHostToTLSCertMap = issuerHostToTLSCertMap +} + +func (p *dynamicTLSCertProvider) GetTLSCert(issuerHostName string) *tls.Certificate { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.issuerHostToTLSCertMap[issuerHostName] +}