diff --git a/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl b/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl index 8ecf6c83c..a2dc9cbca 100644 --- a/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl +++ b/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl @@ -12,9 +12,10 @@ import ( type OIDCProviderStatus string const ( - SuccessOIDCProviderStatus = OIDCProviderStatus("Success") - DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") - InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") + SuccessOIDCProviderStatus = OIDCProviderStatus("Success") + DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") + SameIssuerHostMustUseSameSecretOIDCProviderStatus = OIDCProviderStatus("SameIssuerHostMustUseSameSecret") + InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") ) // OIDCProviderConfigSpec is a struct that describes an OIDC Provider. @@ -29,6 +30,28 @@ type OIDCProviderConfigSpec struct { // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` + + // SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. + // When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that + // contain the certificate and private key to use for TLS. + // + // Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + // + // SNICertificateSecretName is required if you would like to use different TLS certificates for + // issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same + // DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + // + // SNICertificateSecretName is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you + // would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, + // which is configured elsewhere. + // + // When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work + // for IP addresses. + // + // +optional + SNICertificateSecretName string `json:"sniCertificateSecretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/cmd/local-user-authenticator/main.go b/cmd/local-user-authenticator/main.go index f774500c4..f226066fa 100644 --- a/cmd/local-user-authenticator/main.go +++ b/cmd/local-user-authenticator/main.go @@ -109,7 +109,7 @@ func (w *webhook) ServeHTTP(rsp http.ResponseWriter, req *http.Request) { if err != nil { return } - defer req.Body.Close() + defer func() { _ = req.Body.Close() }() secret, err := w.secretInformer.Lister().Secrets(namespace).Get(username) notFound := k8serrors.IsNotFound(err) @@ -379,7 +379,7 @@ func run() error { if err != nil { return fmt.Errorf("cannot create listener: %w", err) } - defer l.Close() + defer func() { _ = l.Close() }() err = startWebhook(ctx, l, dynamicCertProvider, kubeInformers.Core().V1().Secrets()) if err != nil { diff --git a/cmd/local-user-authenticator/main_test.go b/cmd/local-user-authenticator/main_test.go index 3db5a542d..07771971f 100644 --- a/cmd/local-user-authenticator/main_test.go +++ b/cmd/local-user-authenticator/main_test.go @@ -106,7 +106,7 @@ func TestWebhook(t *testing.T) { l, err := net.Listen("tcp", "127.0.0.1:0") require.NoError(t, err) - defer l.Close() + defer func() { _ = l.Close() }() require.NoError(t, w.start(ctx, l)) client := newClient(caBundle, serverName) @@ -412,7 +412,7 @@ func TestWebhook(t *testing.T) { Body: body, }) require.NoError(t, err) - defer rsp.Body.Close() + defer func() { _ = rsp.Body.Close() }() require.Equal(t, test.wantStatus, rsp.StatusCode) @@ -470,7 +470,7 @@ func newCertProvider(t *testing.T) (dynamiccert.Provider, []byte, string) { ca, err := certauthority.New(pkix.Name{CommonName: serverName + " CA"}, time.Hour*24) require.NoError(t, err) - cert, err := ca.Issue(pkix.Name{CommonName: serverName}, []string{serverName}, time.Hour*24) + cert, err := ca.Issue(pkix.Name{CommonName: serverName}, []string{serverName}, nil, time.Hour*24) require.NoError(t, err) certPEM, keyPEM, err := certauthority.ToPEM(cert) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 346f83ca8..b82f50266 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,16 @@ func startControllers( controllerlib.WithInformer, ), singletonWorker, + ). + WithController( + supervisorconfig.NewTLSCertObserverController( + dynamicTLSCertProvider, + cfg.NamesConfig.DefaultTLSCertificateSecret, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ), + singletonWorker, ) kubeInformers.Start(ctx.Done()) @@ -166,19 +180,58 @@ 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 func() { _ = 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.VersionTLS12, // Allow v1.2 because clients like the default `curl` on MacOS don't support 1.3 yet. + GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { + cert := dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)) + defaultCert := dynamicTLSCertProvider.GetDefaultTLSCert() + klog.InfoS("GetCertificate called for port 443", + "info.ServerName", info.ServerName, + "foundSNICert", cert != nil, + "foundDefaultCert", defaultCert != nil, + ) + if cert == nil { + cert = defaultCert + } + return cert, nil + }, + }) + if err != nil { + return fmt.Errorf("cannot create listener: %w", err) + } + defer func() { _ = 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/README.md b/deploy/supervisor/README.md index 5a751ee48..5ad6d51f5 100644 --- a/deploy/supervisor/README.md +++ b/deploy/supervisor/README.md @@ -147,13 +147,37 @@ spec: # The hostname would typically match the DNS name of the public ingress or load balancer for the cluster. # Any path can be specified, which allows a single hostname to have multiple different issuers. The path is optional. issuer: https://my-issuer.example.com/any/path + # Optionally configure the name of a Secret in the same namespace, of type `kubernetes.io/tls`, + # which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. + sniCertificateSecretName: my-tls-cert-secret ``` -If you are using a LoadBalancer Service to expose the Supervisor app outside your cluster, then you will -also need to configure the OIDCProviderConfig with TLS certificates, so the app can terminate TLS. -You can create the certificates however you like, for example you could use [cert-manager](https://cert-manager.io/). -Keep in mind that your users will load some of these endpoints in their web browsers, so the TLS certificates -should be signed by a Certificate Authority that will be trusted by those browsers. +#### Configuring TLS for the Supervisor OIDC Endpoints If you have terminated TLS outside the app, for example using an Ingress with TLS certificates, then you do not need to configure TLS certificates on the OIDCProviderConfig. + +If you are using a LoadBalancer Service to expose the Supervisor app outside your cluster, then you will +also need to configure the Supervisor app to terminate TLS. There are two places to configure TLS certificates: + +1. Each `OIDCProviderConfig` can be configured with TLS certificates, using the `sniCertificateSecretName` field. + +1. The default TLS certificate for all OIDC providers can be configured by creating a Secret called +`pinniped-supervisor-default-tls-certificate` in the same namespace in which the Supervisor was installed. + +The default TLS certificate will be used for all OIDC providers which did not declare an `sniCertificateSecretName`. +Also, the `sniCertificateSecretName` will be ignored for incoming requests to the OIDC endpoints +that use an IP address as the host, so those requests will always present the default TLS certificates +to the client. When the request includes the hostname, and that hostname matches the hostname of an `Issuer`, +then the TLS certificate defined by the `sniCertificateSecretName` will be used. If that issuer did not +define `sniCertificateSecretName` then the default TLS certificate will be used. If neither exists, +then the client will get a TLS error because the server will not present any TLS certificate. + +It is recommended that you have a DNS entry for your load balancer or Ingress, and that you configure the +OIDC provider's `Issuer` using that DNS hostname, and that the TLS certificate for that provider also +covers that same hostname. + +You can create the certificate Secrets however you like, for example you could use [cert-manager](https://cert-manager.io/) +or `kubectl create secret tls`. +Keep in mind that your users will load some of these endpoints in their web browsers, so the TLS certificates +should be signed by a Certificate Authority that will be trusted by their browsers. diff --git a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml index 727f20eca..e8330ab3a 100644 --- a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,26 @@ spec: for more information." minLength: 1 type: string + sniCertificateSecretName: + description: "SNICertificateSecretName is an optional name of a Secret + in the same namespace, of type `kubernetes.io/tls`, which contains + the TLS serving certificate for the HTTPS endpoints served by this + OIDC Provider. When provided, the TLS Secret named here must contain + keys named `tls.crt` and `tls.key` that contain the certificate + and private key to use for TLS. \n Server Name Indication (SNI) + is an extension to the Transport Layer Security (TLS) supported + by all major browsers. \n SNICertificateSecretName is required if + you would like to use different TLS certificates for issuers of + different hostnames. SNI requests do not include port numbers, so + all issuers with the same DNS hostname must use the same SNICertificateSecretName + value even if they have different port numbers. \n SNICertificateSecretName + is not required when you would like to use only the HTTP endpoints + (e.g. when terminating TLS at an Ingress). It is also not required + when you would like all requests to this OIDC Provider's HTTPS endpoints + to use the default TLS certificate, which is configured elsewhere. + \n When your Issuer URL's host is an IP address, then this field + is ignored. SNI does not work for IP addresses." + type: string required: - issuer type: object diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index afa4efd5a..8f91610d3 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -30,6 +30,8 @@ metadata: data: #@yaml/text-templated-strings pinniped.yaml: | + names: + defaultTLSCertificateSecret: (@= defaultResourceNameWithSuffix("default-tls-certificate") @) labels: (@= json.encode(labels()).rstrip() @) --- #@ if data.values.image_pull_dockerconfigjson and data.values.image_pull_dockerconfigjson != "": @@ -87,6 +89,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 19076e5e7..d8739c409 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 @@ -53,8 +74,20 @@ metadata: spec: type: LoadBalancer selector: #@ defaultLabel() + #@ if data.values.service_loadbalancer_ip: + loadBalancerIP: #@ data.values.service_loadbalancer_ip + #@ end 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 ea06cf302..f9e1aac02 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -35,10 +35,20 @@ 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 +#! Note that all port numbers should be numbers (not strings), i.e. use ytt's `--data-value-yaml` instead of `--data-value`. +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 +#! The `loadBalancerIP` value of the LoadBalancer Service. +#! Ignored unless service_http_loadbalancer_port and/or service_https_loadbalancer_port are provided. +#! Optional. +service_loadbalancer_ip: #! e.g. 1.2.3.4 diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index bc3adc110..b6da2c0c1 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -132,6 +132,11 @@ OIDCProviderConfigSpec is a struct that describes an OIDC Provider. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC Provider's issuer, per the OIDC Discovery Metadata document, as well as the identifier that it will use for the iss claim in issued JWTs. This field will also be used as the base URL for any endpoints used by the OIDC Provider (e.g., if your issuer is https://example.com/foo, then your authorization endpoint will look like https://example.com/foo/some/path/to/auth/endpoint). See https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. +| *`sniCertificateSecretName`* __string__ | SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + SNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + SNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere. + When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses. |=== diff --git a/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83c..a2dc9cbca 100644 --- a/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -12,9 +12,10 @@ import ( type OIDCProviderStatus string const ( - SuccessOIDCProviderStatus = OIDCProviderStatus("Success") - DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") - InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") + SuccessOIDCProviderStatus = OIDCProviderStatus("Success") + DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") + SameIssuerHostMustUseSameSecretOIDCProviderStatus = OIDCProviderStatus("SameIssuerHostMustUseSameSecret") + InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") ) // OIDCProviderConfigSpec is a struct that describes an OIDC Provider. @@ -29,6 +30,28 @@ type OIDCProviderConfigSpec struct { // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` + + // SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. + // When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that + // contain the certificate and private key to use for TLS. + // + // Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + // + // SNICertificateSecretName is required if you would like to use different TLS certificates for + // issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same + // DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + // + // SNICertificateSecretName is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you + // would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, + // which is configured elsewhere. + // + // When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work + // for IP addresses. + // + // +optional + SNICertificateSecretName string `json:"sniCertificateSecretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/generated/1.17/client/openapi/zz_generated.openapi.go b/generated/1.17/client/openapi/zz_generated.openapi.go index 5d4516900..e8028d0f6 100644 --- a/generated/1.17/client/openapi/zz_generated.openapi.go +++ b/generated/1.17/client/openapi/zz_generated.openapi.go @@ -397,6 +397,13 @@ func schema_117_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, + "sniCertificateSecretName": { + SchemaProps: spec.SchemaProps{ + Description: "SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.\n\nServer Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers.\n\nSNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers.\n\nSNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere.\n\nWhen your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses.", + Type: []string{"string"}, + Format: "", + }, + }, }, Required: []string{"issuer"}, }, diff --git a/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 727f20eca..e8330ab3a 100644 --- a/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,26 @@ spec: for more information." minLength: 1 type: string + sniCertificateSecretName: + description: "SNICertificateSecretName is an optional name of a Secret + in the same namespace, of type `kubernetes.io/tls`, which contains + the TLS serving certificate for the HTTPS endpoints served by this + OIDC Provider. When provided, the TLS Secret named here must contain + keys named `tls.crt` and `tls.key` that contain the certificate + and private key to use for TLS. \n Server Name Indication (SNI) + is an extension to the Transport Layer Security (TLS) supported + by all major browsers. \n SNICertificateSecretName is required if + you would like to use different TLS certificates for issuers of + different hostnames. SNI requests do not include port numbers, so + all issuers with the same DNS hostname must use the same SNICertificateSecretName + value even if they have different port numbers. \n SNICertificateSecretName + is not required when you would like to use only the HTTP endpoints + (e.g. when terminating TLS at an Ingress). It is also not required + when you would like all requests to this OIDC Provider's HTTPS endpoints + to use the default TLS certificate, which is configured elsewhere. + \n When your Issuer URL's host is an IP address, then this field + is ignored. SNI does not work for IP addresses." + type: string required: - issuer type: object diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index d0b69f737..5b3892d17 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -132,6 +132,11 @@ OIDCProviderConfigSpec is a struct that describes an OIDC Provider. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC Provider's issuer, per the OIDC Discovery Metadata document, as well as the identifier that it will use for the iss claim in issued JWTs. This field will also be used as the base URL for any endpoints used by the OIDC Provider (e.g., if your issuer is https://example.com/foo, then your authorization endpoint will look like https://example.com/foo/some/path/to/auth/endpoint). See https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. +| *`sniCertificateSecretName`* __string__ | SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + SNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + SNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere. + When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses. |=== diff --git a/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83c..a2dc9cbca 100644 --- a/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -12,9 +12,10 @@ import ( type OIDCProviderStatus string const ( - SuccessOIDCProviderStatus = OIDCProviderStatus("Success") - DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") - InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") + SuccessOIDCProviderStatus = OIDCProviderStatus("Success") + DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") + SameIssuerHostMustUseSameSecretOIDCProviderStatus = OIDCProviderStatus("SameIssuerHostMustUseSameSecret") + InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") ) // OIDCProviderConfigSpec is a struct that describes an OIDC Provider. @@ -29,6 +30,28 @@ type OIDCProviderConfigSpec struct { // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` + + // SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. + // When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that + // contain the certificate and private key to use for TLS. + // + // Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + // + // SNICertificateSecretName is required if you would like to use different TLS certificates for + // issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same + // DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + // + // SNICertificateSecretName is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you + // would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, + // which is configured elsewhere. + // + // When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work + // for IP addresses. + // + // +optional + SNICertificateSecretName string `json:"sniCertificateSecretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/generated/1.18/client/openapi/zz_generated.openapi.go b/generated/1.18/client/openapi/zz_generated.openapi.go index bd28cdfdd..737854c01 100644 --- a/generated/1.18/client/openapi/zz_generated.openapi.go +++ b/generated/1.18/client/openapi/zz_generated.openapi.go @@ -397,6 +397,13 @@ func schema_118_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, + "sniCertificateSecretName": { + SchemaProps: spec.SchemaProps{ + Description: "SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.\n\nServer Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers.\n\nSNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers.\n\nSNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere.\n\nWhen your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses.", + Type: []string{"string"}, + Format: "", + }, + }, }, Required: []string{"issuer"}, }, diff --git a/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 727f20eca..e8330ab3a 100644 --- a/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,26 @@ spec: for more information." minLength: 1 type: string + sniCertificateSecretName: + description: "SNICertificateSecretName is an optional name of a Secret + in the same namespace, of type `kubernetes.io/tls`, which contains + the TLS serving certificate for the HTTPS endpoints served by this + OIDC Provider. When provided, the TLS Secret named here must contain + keys named `tls.crt` and `tls.key` that contain the certificate + and private key to use for TLS. \n Server Name Indication (SNI) + is an extension to the Transport Layer Security (TLS) supported + by all major browsers. \n SNICertificateSecretName is required if + you would like to use different TLS certificates for issuers of + different hostnames. SNI requests do not include port numbers, so + all issuers with the same DNS hostname must use the same SNICertificateSecretName + value even if they have different port numbers. \n SNICertificateSecretName + is not required when you would like to use only the HTTP endpoints + (e.g. when terminating TLS at an Ingress). It is also not required + when you would like all requests to this OIDC Provider's HTTPS endpoints + to use the default TLS certificate, which is configured elsewhere. + \n When your Issuer URL's host is an IP address, then this field + is ignored. SNI does not work for IP addresses." + type: string required: - issuer type: object diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index 2e5b95fa7..49ed2bcf9 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -132,6 +132,11 @@ OIDCProviderConfigSpec is a struct that describes an OIDC Provider. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC Provider's issuer, per the OIDC Discovery Metadata document, as well as the identifier that it will use for the iss claim in issued JWTs. This field will also be used as the base URL for any endpoints used by the OIDC Provider (e.g., if your issuer is https://example.com/foo, then your authorization endpoint will look like https://example.com/foo/some/path/to/auth/endpoint). See https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. +| *`sniCertificateSecretName`* __string__ | SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + SNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + SNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere. + When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses. |=== diff --git a/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83c..a2dc9cbca 100644 --- a/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -12,9 +12,10 @@ import ( type OIDCProviderStatus string const ( - SuccessOIDCProviderStatus = OIDCProviderStatus("Success") - DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") - InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") + SuccessOIDCProviderStatus = OIDCProviderStatus("Success") + DuplicateOIDCProviderStatus = OIDCProviderStatus("Duplicate") + SameIssuerHostMustUseSameSecretOIDCProviderStatus = OIDCProviderStatus("SameIssuerHostMustUseSameSecret") + InvalidOIDCProviderStatus = OIDCProviderStatus("Invalid") ) // OIDCProviderConfigSpec is a struct that describes an OIDC Provider. @@ -29,6 +30,28 @@ type OIDCProviderConfigSpec struct { // https://openid.net/specs/openid-connect-discovery-1_0.html#rfc.section.3 for more information. // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` + + // SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. + // When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that + // contain the certificate and private key to use for TLS. + // + // Server Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers. + // + // SNICertificateSecretName is required if you would like to use different TLS certificates for + // issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same + // DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers. + // + // SNICertificateSecretName is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you + // would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, + // which is configured elsewhere. + // + // When your Issuer URL's host is an IP address, then this field is ignored. SNI does not work + // for IP addresses. + // + // +optional + SNICertificateSecretName string `json:"sniCertificateSecretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/generated/1.19/client/openapi/zz_generated.openapi.go b/generated/1.19/client/openapi/zz_generated.openapi.go index dd6e05198..66e21e84c 100644 --- a/generated/1.19/client/openapi/zz_generated.openapi.go +++ b/generated/1.19/client/openapi/zz_generated.openapi.go @@ -398,6 +398,13 @@ func schema_119_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, + "sniCertificateSecretName": { + SchemaProps: spec.SchemaProps{ + Description: "SNICertificateSecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, which contains the TLS serving certificate for the HTTPS endpoints served by this OIDC Provider. When provided, the TLS Secret named here must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.\n\nServer Name Indication (SNI) is an extension to the Transport Layer Security (TLS) supported by all major browsers.\n\nSNICertificateSecretName is required if you would like to use different TLS certificates for issuers of different hostnames. SNI requests do not include port numbers, so all issuers with the same DNS hostname must use the same SNICertificateSecretName value even if they have different port numbers.\n\nSNICertificateSecretName is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). It is also not required when you would like all requests to this OIDC Provider's HTTPS endpoints to use the default TLS certificate, which is configured elsewhere.\n\nWhen your Issuer URL's host is an IP address, then this field is ignored. SNI does not work for IP addresses.", + Type: []string{"string"}, + Format: "", + }, + }, }, Required: []string{"issuer"}, }, diff --git a/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 727f20eca..e8330ab3a 100644 --- a/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,26 @@ spec: for more information." minLength: 1 type: string + sniCertificateSecretName: + description: "SNICertificateSecretName is an optional name of a Secret + in the same namespace, of type `kubernetes.io/tls`, which contains + the TLS serving certificate for the HTTPS endpoints served by this + OIDC Provider. When provided, the TLS Secret named here must contain + keys named `tls.crt` and `tls.key` that contain the certificate + and private key to use for TLS. \n Server Name Indication (SNI) + is an extension to the Transport Layer Security (TLS) supported + by all major browsers. \n SNICertificateSecretName is required if + you would like to use different TLS certificates for issuers of + different hostnames. SNI requests do not include port numbers, so + all issuers with the same DNS hostname must use the same SNICertificateSecretName + value even if they have different port numbers. \n SNICertificateSecretName + is not required when you would like to use only the HTTP endpoints + (e.g. when terminating TLS at an Ingress). It is also not required + when you would like all requests to this OIDC Provider's HTTPS endpoints + to use the default TLS certificate, which is configured elsewhere. + \n When your Issuer URL's host is an IP address, then this field + is ignored. SNI does not work for IP addresses." + type: string required: - issuer type: object diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index 83c36a582..5247f5b5d 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 899ea4a5f..ebd6e3b85 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 19b3cac4d..8c8e777ff 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -109,18 +109,18 @@ fi if ! tilt_mode; then if [[ "$clean_kind" == "yes" ]]; then - log_note "Deleting running kind clusters to prepare from a clean slate..." - kind delete cluster --name pinniped + log_note "Deleting running kind cluster to prepare from a clean slate..." + ./hack/kind-down.sh fi # # Setup kind and build the app # - log_note "Checking for running kind clusters..." + log_note "Checking for running kind cluster..." 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 - kind create cluster --config "$pinniped_path/hack/lib/kind-config/single-node.yaml" --name pinniped + # Our kind config 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 + ./hack/kind-up.sh else if ! kubectl cluster-info | grep master | grep -q 127.0.0.1; then log_error "Seems like your kubeconfig is not targeting a local cluster." @@ -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" @@ -279,6 +282,7 @@ export PINNIPED_TEST_SUPERVISOR_NAMESPACE=${supervisor_namespace} export PINNIPED_TEST_SUPERVISOR_APP_NAME=${supervisor_app_name} export PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS='${supervisor_custom_labels}' export PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS="127.0.0.1:12345" +export PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS="localhost:12344" export PINNIPED_TEST_CLI_OIDC_ISSUER=http://127.0.0.1:12346/dex export PINNIPED_TEST_CLI_OIDC_CLIENT_ID=pinniped-cli export PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT=48095 @@ -300,7 +304,7 @@ goland_vars=$(grep -v '^#' /tmp/integration-test-env | grep -E '^export .+=' | s log_note log_note "🚀 Ready to run integration tests! For example..." log_note " cd $pinniped_path" -log_note ' source /tmp/integration-test-env && go test -v -count 1 ./test/integration' +log_note ' source /tmp/integration-test-env && go test -v -race -count 1 ./test/integration' log_note log_note 'Want to run integration tests in GoLand? Copy/paste this "Environment" value for GoLand run configurations:' log_note " ${goland_vars}PINNIPED_TEST_CLUSTER_CAPABILITY_FILE=${kind_capabilities_file}" @@ -311,5 +315,5 @@ if ! tilt_mode; then log_note log_note "To delete the deployments, run:" log_note " kapp delete -a local-user-authenticator -y && kapp delete -a $concierge_app_name -y && kapp delete -a $supervisor_app_name -y" - log_note "When you're finished, use 'kind delete cluster --name pinniped' to tear down the cluster." + log_note "When you're finished, use './hack/kind-down.sh' to tear down the cluster." fi diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 9e5f77dde..ba3e64922 100644 --- a/internal/certauthority/certauthority.go +++ b/internal/certauthority/certauthority.go @@ -17,6 +17,7 @@ import ( "fmt" "io" "math/big" + "net" "time" ) @@ -136,7 +137,7 @@ func (c *CA) Bundle() []byte { } // Issue a new server certificate for the given identity and duration. -func (c *CA) Issue(subject pkix.Name, dnsNames []string, ttl time.Duration) (*tls.Certificate, error) { +func (c *CA) Issue(subject pkix.Name, dnsNames []string, ips []net.IP, ttl time.Duration) (*tls.Certificate, error) { // Choose a random 128 bit serial number. serialNumber, err := randomSerial(c.env.serialRNG) if err != nil { @@ -171,6 +172,7 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ttl time.Duration) (*tl BasicConstraintsValid: true, IsCA: false, DNSNames: dnsNames, + IPAddresses: ips, } certBytes, err := x509.CreateCertificate(rand.Reader, &template, caCert, &privateKey.PublicKey, c.signer) if err != nil { @@ -194,7 +196,7 @@ func (c *CA) Issue(subject pkix.Name, dnsNames []string, ttl time.Duration) (*tl // IssuePEM issues a new server certificate for the given identity and duration, returning it as a pair of // PEM-formatted byte slices for the certificate and private key. func (c *CA) IssuePEM(subject pkix.Name, dnsNames []string, ttl time.Duration) ([]byte, []byte, error) { - return toPEM(c.Issue(subject, dnsNames, ttl)) + return toPEM(c.Issue(subject, dnsNames, nil, ttl)) } func toPEM(cert *tls.Certificate, err error) ([]byte, []byte, error) { diff --git a/internal/certauthority/certauthority_test.go b/internal/certauthority/certauthority_test.go index 72737991b..f2dc5b332 100644 --- a/internal/certauthority/certauthority_test.go +++ b/internal/certauthority/certauthority_test.go @@ -11,6 +11,7 @@ import ( "fmt" "io" "io/ioutil" + "net" "strings" "testing" "time" @@ -282,7 +283,7 @@ func TestIssue(t *testing.T) { for _, tt := range tests { tt := tt t.Run(tt.name, func(t *testing.T) { - got, err := tt.ca.Issue(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, 10*time.Minute) + got, err := tt.ca.Issue(pkix.Name{CommonName: "Test Server"}, []string{"example.com"}, []net.IP{net.IPv4(1, 2, 3, 4)}, 10*time.Minute) if tt.wantErr != "" { require.EqualError(t, err, tt.wantErr) require.Nil(t, got) diff --git a/internal/config/supervisor/config.go b/internal/config/supervisor/config.go index eb6836c66..c831c71ef 100644 --- a/internal/config/supervisor/config.go +++ b/internal/config/supervisor/config.go @@ -8,8 +8,11 @@ package supervisor import ( "fmt" "io/ioutil" + "strings" "sigs.k8s.io/yaml" + + "go.pinniped.dev/internal/constable" ) // FromPath loads an Config from a provided local file path, inserts any @@ -30,5 +33,20 @@ func FromPath(path string) (*Config, error) { config.Labels = make(map[string]string) } + if err := validateNames(&config.NamesConfig); err != nil { + return nil, fmt.Errorf("validate names: %w", err) + } + return &config, nil } + +func validateNames(names *NamesConfigSpec) error { + missingNames := []string{} + if names.DefaultTLSCertificateSecret == "" { + missingNames = append(missingNames, "defaultTLSCertificateSecret") + } + if len(missingNames) > 0 { + return constable.Error("missing required names: " + strings.Join(missingNames, ", ")) + } + return nil +} diff --git a/internal/config/supervisor/config_test.go b/internal/config/supervisor/config_test.go index b616fa5b1..8b77c6f29 100644 --- a/internal/config/supervisor/config_test.go +++ b/internal/config/supervisor/config_test.go @@ -18,6 +18,7 @@ func TestFromPath(t *testing.T) { name string yaml string wantConfig *Config + wantError string }{ { name: "Happy", @@ -26,23 +27,40 @@ func TestFromPath(t *testing.T) { labels: myLabelKey1: myLabelValue1 myLabelKey2: myLabelValue2 + names: + defaultTLSCertificateSecret: my-secret-name `), wantConfig: &Config{ Labels: map[string]string{ "myLabelKey1": "myLabelValue1", "myLabelKey2": "myLabelValue2", }, + NamesConfig: NamesConfigSpec{ + DefaultTLSCertificateSecret: "my-secret-name", + }, }, }, { name: "When only the required fields are present, causes other fields to be defaulted", yaml: here.Doc(` --- + names: + defaultTLSCertificateSecret: my-secret-name `), wantConfig: &Config{ Labels: map[string]string{}, + NamesConfig: NamesConfigSpec{ + DefaultTLSCertificateSecret: "my-secret-name", + }, }, }, + { + name: "Missing defaultTLSCertificateSecret name", + yaml: here.Doc(` + --- + `), + wantError: "validate names: missing required names: defaultTLSCertificateSecret", + }, } for _, test := range tests { test := test @@ -62,8 +80,12 @@ func TestFromPath(t *testing.T) { // Test FromPath() config, err := FromPath(f.Name()) - require.NoError(t, err) - require.Equal(t, test.wantConfig, config) + if test.wantError != "" { + require.EqualError(t, err, test.wantError) + } else { + require.NoError(t, err) + require.Equal(t, test.wantConfig, config) + } }) } } diff --git a/internal/config/supervisor/types.go b/internal/config/supervisor/types.go index 03805956d..060b88edf 100644 --- a/internal/config/supervisor/types.go +++ b/internal/config/supervisor/types.go @@ -5,5 +5,11 @@ package supervisor // Config contains knobs to setup an instance of the Pinniped Supervisor. type Config struct { - Labels map[string]string `json:"labels"` + Labels map[string]string `json:"labels"` + NamesConfig NamesConfigSpec `json:"names"` +} + +// NamesConfigSpec configures the names of some Kubernetes resources for the Supervisor. +type NamesConfigSpec struct { + DefaultTLSCertificateSecret string `json:"defaultTLSCertificateSecret"` } diff --git a/internal/controller/apicerts/certs_manager.go b/internal/controller/apicerts/certs_manager.go index 541eaf62d..c0be78731 100644 --- a/internal/controller/apicerts/certs_manager.go +++ b/internal/controller/apicerts/certs_manager.go @@ -103,6 +103,7 @@ func (c *certsManagerController) Sync(ctx controllerlib.Context) error { aggregatedAPIServerTLSCert, err := aggregatedAPIServerCA.Issue( pkix.Name{CommonName: serviceEndpoint}, []string{serviceEndpoint}, + nil, c.certDuration, ) if err != nil { diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index c9911b135..bec3cef61 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 77acd401e..87e9d227f 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -6,6 +6,8 @@ package supervisorconfig import ( "context" "fmt" + "net/url" + "strings" "go.pinniped.dev/internal/multierror" @@ -71,29 +73,75 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er return err } + // Make a map of issuer strings -> count of how many times we saw that issuer string. + // This will help us complain when there are duplicate issuer strings. + // Also make a helper function for forming keys into this map. issuerCounts := make(map[string]int) + issuerURLToIssuerKey := func(issuerURL *url.URL) string { + return fmt.Sprintf("%s://%s%s", issuerURL.Scheme, strings.ToLower(issuerURL.Host), issuerURL.Path) + } + + // 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. 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) + issuerURLToHostnameKey := lowercaseHostWithoutPort + for _, opc := range all { - issuerCounts[opc.Spec.Issuer]++ + issuerURL, err := url.Parse(opc.Spec.Issuer) + if err != nil { + continue // Skip url parse errors because they will be validated again below. + } + + issuerCounts[issuerURLToIssuerKey(issuerURL)]++ + + setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] + if setOfSecretNames == nil { + setOfSecretNames = make(map[string]bool) + uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] = setOfSecretNames + } + setOfSecretNames[opc.Spec.SNICertificateSecretName] = true } errs := multierror.New() oidcProviders := make([]*provider.OIDCProvider, 0) for _, opc := range all { - if issuerCount := issuerCounts[opc.Spec.Issuer]; issuerCount > 1 { + issuerURL, urlParseErr := url.Parse(opc.Spec.Issuer) + + // Skip url parse errors because they will be validated below. + if urlParseErr == nil { + if issuerCount := issuerCounts[issuerURLToIssuerKey(issuerURL)]; issuerCount > 1 { + if err := c.updateStatus( + ctx.Context, + opc.Namespace, + opc.Name, + configv1alpha1.DuplicateOIDCProviderStatus, + "Duplicate issuer: "+opc.Spec.Issuer, + ); err != nil { + errs.Add(fmt.Errorf("could not update status: %w", err)) + } + continue + } + } + + // Skip url parse errors because they will be validated below. + if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)]) > 1 { if err := c.updateStatus( ctx.Context, opc.Namespace, opc.Name, - configv1alpha1.DuplicateOIDCProviderStatus, - "Duplicate issuer: "+opc.Spec.Issuer, + configv1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus, + "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)) } continue } - oidcProvider, err := provider.NewOIDCProvider(opc.Spec.Issuer) + oidcProvider, err := provider.NewOIDCProvider(opc.Spec.Issuer) // This validates the Issuer URL. if err != nil { if err := c.updateStatus( ctx.Context, diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go index 7db8022d0..f486da94d 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go @@ -6,6 +6,7 @@ package supervisorconfig import ( "context" "errors" + "net/url" "reflect" "sync" "testing" @@ -667,22 +668,24 @@ func TestSync(t *testing.T) { ) it.Before(func() { + // Hostnames are case-insensitive, so consider them to be duplicates if they only differ by case. + // Paths are case-sensitive, so having a path that differs only by case makes a new issuer. oidcProviderConfigDuplicate1 = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "duplicate1", Namespace: namespace}, - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com"}, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://iSSueR-duPlicAte.cOm/a"}, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDuplicate1)) r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDuplicate1)) oidcProviderConfigDuplicate2 = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "duplicate2", Namespace: namespace}, - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com"}, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com/a"}, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDuplicate2)) r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDuplicate2)) oidcProviderConfig = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "not-duplicate", Namespace: namespace}, - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-not-duplicate.com"}, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://issuer-duplicate.com/A"}, // different path } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfig)) r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfig)) @@ -715,11 +718,11 @@ func TestSync(t *testing.T) { oidcProviderConfig.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigDuplicate1.Status.Status = v1alpha1.DuplicateOIDCProviderStatus - oidcProviderConfigDuplicate1.Status.Message = "Duplicate issuer: https://issuer-duplicate.com" + oidcProviderConfigDuplicate1.Status.Message = "Duplicate issuer: https://iSSueR-duPlicAte.cOm/a" oidcProviderConfigDuplicate1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigDuplicate2.Status.Status = v1alpha1.DuplicateOIDCProviderStatus - oidcProviderConfigDuplicate2.Status.Message = "Duplicate issuer: https://issuer-duplicate.com" + oidcProviderConfigDuplicate2.Status.Message = "Duplicate issuer: https://issuer-duplicate.com/a" oidcProviderConfigDuplicate2.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) expectedActions := []coretesting.Action{ @@ -770,10 +773,10 @@ func TestSync(t *testing.T) { it("returns the get errors", func() { expectedError := here.Doc(` - 3 error(s): - - could not update status: get failed: some get error - - could not update status: get failed: some get error - - could not update status: get failed: some get error`) + 3 error(s): + - could not update status: get failed: some get error + - could not update status: get failed: some get error + - could not update status: get failed: some get error`) startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) r.EqualError(err, expectedError) @@ -804,6 +807,198 @@ func TestSync(t *testing.T) { }) }) + when("there are OIDCProviderConfigs with the same issuer DNS hostname using different secretNames", func() { + var ( + oidcProviderConfigSameIssuerAddress1 *v1alpha1.OIDCProviderConfig + oidcProviderConfigSameIssuerAddress2 *v1alpha1.OIDCProviderConfig + oidcProviderConfigDifferentIssuerAddress *v1alpha1.OIDCProviderConfig + oidcProviderConfigWithInvalidIssuerURL *v1alpha1.OIDCProviderConfig + ) + + it.Before(func() { + oidcProviderConfigSameIssuerAddress1 = &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "provider1", Namespace: namespace}, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: "https://iSSueR-duPlicAte-adDress.cOm/path1", + SNICertificateSecretName: "secret1", + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress1)) + r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigSameIssuerAddress1)) + oidcProviderConfigSameIssuerAddress2 = &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "provider2", Namespace: namespace}, + Spec: v1alpha1.OIDCProviderConfigSpec{ + // 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", + SNICertificateSecretName: "secret2", + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress2)) + r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigSameIssuerAddress2)) + + oidcProviderConfigDifferentIssuerAddress = &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "differentIssuerAddressProvider", Namespace: namespace}, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: "https://issuer-not-duplicate.com", + SNICertificateSecretName: "secret1", + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDifferentIssuerAddress)) + r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigDifferentIssuerAddress)) + + // Also add one with a URL that cannot be parsed to make sure that the error handling + // for the duplicate issuers and secret names are not confused by invalid URLs. + invalidIssuerURL := ":/host//path" + _, err := url.Parse(invalidIssuerURL) //nolint:staticcheck // Yes, this URL is intentionally invalid. + r.Error(err) + oidcProviderConfigWithInvalidIssuerURL = &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{Name: "invalidIssuerURLProvider", Namespace: namespace}, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: invalidIssuerURL, + SNICertificateSecretName: "secret1", + }, + } + r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigWithInvalidIssuerURL)) + r.NoError(opcInformerClient.Tracker().Add(oidcProviderConfigWithInvalidIssuerURL)) + }) + + it("calls the ProvidersSetter with the non-duplicate", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + nonDuplicateProvider, err := provider.NewOIDCProvider(oidcProviderConfigDifferentIssuerAddress.Spec.Issuer) + r.NoError(err) + + r.True(providersSetter.SetProvidersWasCalled) + r.Equal( + []*provider.OIDCProvider{ + nonDuplicateProvider, + }, + providersSetter.OIDCProvidersReceived, + ) + }) + + it("updates the statuses", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + oidcProviderConfigDifferentIssuerAddress.Status.Status = v1alpha1.SuccessOIDCProviderStatus + oidcProviderConfigDifferentIssuerAddress.Status.Message = "Provider successfully created" + oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) + + oidcProviderConfigSameIssuerAddress1.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus + 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 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 + oidcProviderConfigWithInvalidIssuerURL.Status.Message = `Invalid: could not parse issuer as URL: parse ":/host//path": missing protocol scheme` + oidcProviderConfigWithInvalidIssuerURL.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) + + expectedActions := []coretesting.Action{ + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress1.Namespace, + oidcProviderConfigSameIssuerAddress1.Name, + ), + coretesting.NewUpdateAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress1.Namespace, + oidcProviderConfigSameIssuerAddress1, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress2.Namespace, + oidcProviderConfigSameIssuerAddress2.Name, + ), + coretesting.NewUpdateAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress2.Namespace, + oidcProviderConfigSameIssuerAddress2, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigDifferentIssuerAddress.Namespace, + oidcProviderConfigDifferentIssuerAddress.Name, + ), + coretesting.NewUpdateAction( + oidcProviderConfigGVR, + oidcProviderConfigDifferentIssuerAddress.Namespace, + oidcProviderConfigDifferentIssuerAddress, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigWithInvalidIssuerURL.Namespace, + oidcProviderConfigWithInvalidIssuerURL.Name, + ), + coretesting.NewUpdateAction( + oidcProviderConfigGVR, + oidcProviderConfigWithInvalidIssuerURL.Namespace, + oidcProviderConfigWithInvalidIssuerURL, + ), + } + r.ElementsMatch(expectedActions, pinnipedAPIClient.Actions()) + }) + + when("we cannot talk to the API", func() { + it.Before(func() { + pinnipedAPIClient.PrependReactor( + "get", + "oidcproviderconfigs", + func(_ coretesting.Action) (bool, runtime.Object, error) { + return true, nil, errors.New("some get error") + }, + ) + }) + + it("returns the get errors", func() { + expectedError := here.Doc(` + 4 error(s): + - could not update status: get failed: some get error + - could not update status: get failed: some get error + - could not update status: get failed: some get error + - could not update status: get failed: some get error`) + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.EqualError(err, expectedError) + + oidcProviderConfigDifferentIssuerAddress.Status.Status = v1alpha1.SuccessOIDCProviderStatus + oidcProviderConfigDifferentIssuerAddress.Status.Message = "Provider successfully created" + oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) + + expectedActions := []coretesting.Action{ + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress1.Namespace, + oidcProviderConfigSameIssuerAddress1.Name, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigSameIssuerAddress2.Namespace, + oidcProviderConfigSameIssuerAddress2.Name, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigDifferentIssuerAddress.Namespace, + oidcProviderConfigDifferentIssuerAddress.Name, + ), + coretesting.NewGetAction( + oidcProviderConfigGVR, + oidcProviderConfigWithInvalidIssuerURL.Namespace, + oidcProviderConfigWithInvalidIssuerURL.Name, + ), + } + r.ElementsMatch(expectedActions, pinnipedAPIClient.Actions()) + }) + }) + }) + when("there are no OIDCProviderConfigs in the informer", func() { it("keeps waiting for one", func() { startInformersAndController() diff --git a/internal/controller/supervisorconfig/testdata/test.crt b/internal/controller/supervisorconfig/testdata/test.crt new file mode 100644 index 000000000..e54883b08 --- /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 000000000..e29ea4627 --- /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 000000000..7b17500b4 --- /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 000000000..042bbceb1 --- /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/testdata/test3.crt b/internal/controller/supervisorconfig/testdata/test3.crt new file mode 100644 index 000000000..796a7690f --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test3.crt @@ -0,0 +1,17 @@ +-----BEGIN CERTIFICATE----- +MIICyDCCAbCgAwIBAgIBADANBgkqhkiG9w0BAQsFADAVMRMwEQYDVQQDEwprdWJl +cm5ldGVzMB4XDTIwMDcyNTIxMDQxOFoXDTMwMDcyMzIxMDQxOFowFTETMBEGA1UE +AxMKa3ViZXJuZXRlczCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEBAL3K +hYv2gIQ1Dwzh2cWMid+ofAnvLIfV2Xv61vTLGprUI+XUqB4/gtf6X6UNn0Lett2n +d8p4wy7hw73hU/ggdvmWJvqBrSjc3JGfy+kj66fKXX+PTlbL7QbwiRvcSqIXIWlV +lHHxECWrED8jCulw/NVqfook/h5iNUCT9yswSJr/0fImiVnoTlIoEYG2eCNejZ5c +g39uD3ZTqd9ZxWwSLLnI+2kpJnZBPcd1ZQ8AQqzDgZtYRCqacn5gckQUKZWKQlxo +Eft6g1XHJouAWAZw7hEtk0v8rG0/eKF7wamxFi6BFVlbjWBsB4T9rApbdBWTKeCJ +Hv8fv5RMFSzpT3uzTO8CAwEAAaMjMCEwDgYDVR0PAQH/BAQDAgKkMA8GA1UdEwEB +/wQFMAMBAf8wDQYJKoZIhvcNAQELBQADggEBACh5RhbxqJe+Z/gc17cZhKNmdiwu +I2pLp3QBfwvN+Wbmajzw/7rYhY0d8JYVTJzXSCPWi6UAKxAtXOLF8WIIf9i39n6R +uKOBGW14FzzGyRJiD3qaG/JTvEW+SLhwl68Ndr5LHSnbugAqq31abcQy6Zl9v5A8 +JKC97Lj/Sn8rj7opKy4W3oq7NCQsAb0zh4IllRF6UvSnJySfsg7xdXHHpxYDHtOS +XcOu5ySUIZTgFe9RfeUZlGZ5xn0ckMlQ7qW2Wx1q0OVWw5us4NtkGqKrHG4Tn1X7 +uwo/Yytn5sDxrDv1/oii6AZOCsTPre4oD3wz4nmVzCVJcgrqH4Q24hT8WNg= +-----END CERTIFICATE----- diff --git a/internal/controller/supervisorconfig/testdata/test3.key b/internal/controller/supervisorconfig/testdata/test3.key new file mode 100644 index 000000000..7ad653aee --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test3.key @@ -0,0 +1,27 @@ +-----BEGIN RSA PRIVATE KEY----- +MIIEogIBAAKCAQEAvcqFi/aAhDUPDOHZxYyJ36h8Ce8sh9XZe/rW9MsamtQj5dSo +Hj+C1/pfpQ2fQt623ad3ynjDLuHDveFT+CB2+ZYm+oGtKNzckZ/L6SPrp8pdf49O +VsvtBvCJG9xKohchaVWUcfEQJasQPyMK6XD81Wp+iiT+HmI1QJP3KzBImv/R8iaJ +WehOUigRgbZ4I16NnlyDf24PdlOp31nFbBIsucj7aSkmdkE9x3VlDwBCrMOBm1hE +KppyfmByRBQplYpCXGgR+3qDVccmi4BYBnDuES2TS/ysbT94oXvBqbEWLoEVWVuN +YGwHhP2sClt0FZMp4Ike/x+/lEwVLOlPe7NM7wIDAQABAoIBAFC1tUEmHNUcM0BJ +M3D9KQzB+63F1mwVlx1QOOV1EeVR3co5Ox1R6PSr9sycFGQ9jgqI0zp5TJe9Tp6L +GkhklfPh1MWnK9o6wlnzWKXWrrp2Jni+mpPyuOPAmq4Maniv2XeP+0bROwqpyojv +AA7yC7M+TH226ZJGNVs3EV9+cwHml0yuzBfIJn/rv/w2g+WRKM/MC0S7k2d8bRlA +NycKVGAGBhKTltjoVYOeh6aHEpSjK8zfaePjo5dYJvoVIli60YCgcJOU/8jXT+Np +1Fm7tRvAtj3pUp0Sqdaf2RUzh9jfJp2VFCHuSJ6TPqArOyQojtMcTHF0TiW7xrHP +xOCRIAECgYEAwGBPU7vdthMJBg+ORUoGQQaItTeJvQwIqJvbKD2osp4jhS1dGZBw +W30GKEc/gd8JNtOq9BBnMicPF7hktuy+bSPv41XPud67rSSO7Tsw20C10gFRq06B +zIJWFAUqK3IkvVc3VDmtSLSDox4QZ/BdqaMlQ5y5JCsC5kThmkZFlO8CgYEA/I9X +YHi6RioMJE1fqOHJL4DDjlezmcuRrD7fE5InKbtJZ2JhGYOX/C0KXnHTOWTCDxxN +FBvpvD6Xv5o3PhB9Z6k2fqvJ4GS8urkG/KU4xcC+bak+9ava8oaiSqG16zD9NH2P +jJ60NrbLl1J0pU9fiwuFVUKJ4hDZOfN9RqYdyAECgYAVwo8WhJiGgM6zfcz073OX +pVqPTPHqjVLpZ3+5pIfRdGvGI6R1QM5EuvaYVb7MPOM47WZX5wcVOC/P2g6iVlMP +21HGIC2384a9BfaYxOo40q/+SiHnw6CQ9mkwKIllkqqvNA9RGpkMMUb2i28For2l +c4vCgxa6DZdtXns6TRqPxwKBgCfY5cxOv/T6BVhk7MbUeM2J31DB/ZAyUhV/Bess +kAlBh19MYk2IOZ6L7KriApV3lDaWHIMjtEkDByYvyq98Io0MYZCywfMpca10K+oI +l2B7/I+IuGpCZxUEsO5dfTpSTGDPvqpND9niFVUWqVi7oTNq6ep9yQtl5SADjqxq +4SABAoGAIm0hUg1wtcS46cGLy6PIkPM5tocTSghtz4vFsuk/i4QA9GBoBO2gH6ty ++kJHmeaXt2dmgySp0QAWit5UlceEumB0NXnAdJZQxeGSFSyYkDWhwXd8wDceKo/1 +LfCU6Dk8IN/SsppVUWXQ2rlORvxlrHeCio8o0kS9Yiu55WMYg4g= +-----END RSA 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 000000000..88db4ace9 --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -0,0 +1,120 @@ +// 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 { + issuerTLSCertSetter IssuerTLSCertSetter + defaultTLSCertificateSecretName string + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer +} + +type IssuerTLSCertSetter interface { + SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) + SetDefaultTLSCert(certificate *tls.Certificate) +} + +func NewTLSCertObserverController( + issuerTLSCertSetter IssuerTLSCertSetter, + defaultTLSCertificateSecretName string, + secretInformer corev1informers.SecretInformer, + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "tls-certs-observer-controller", + Syncer: &tlsCertObserverController{ + issuerTLSCertSetter: issuerTLSCertSetter, + defaultTLSCertificateSecretName: defaultTLSCertificateSecretName, + 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.SNICertificateSecretName + 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 + } + certFromSecret, err := c.certFromSecret(ns, secretName) + if err != nil { + 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.issuerTLSCertSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) + + defaultCert, err := c.certFromSecret(ns, c.defaultTLSCertificateSecretName) + if err != nil { + c.issuerTLSCertSetter.SetDefaultTLSCert(nil) + } else { + c.issuerTLSCertSetter.SetDefaultTLSCert(defaultCert) + } + + return nil +} + +func (c *tlsCertObserverController) certFromSecret(ns string, secretName string) (*tls.Certificate, error) { + 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) + return nil, err + } + 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) + return nil, err + } + return &certFromSecret, 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 000000000..9b77fa468 --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -0,0 +1,367 @@ +// 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, + "", // don't care about the secret name for this test + 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 fakeIssuerTLSCertSetter struct { + setIssuerHostToTLSCertMapWasCalled bool + setDefaultTLSCertWasCalled bool + issuerHostToTLSCertMapReceived map[string]*tls.Certificate + setDefaultTLSCertReceived *tls.Certificate +} + +func (f *fakeIssuerTLSCertSetter) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) { + f.setIssuerHostToTLSCertMapWasCalled = true + f.issuerHostToTLSCertMapReceived = issuerHostToTLSCertMap +} + +func (f *fakeIssuerTLSCertSetter) SetDefaultTLSCert(certificate *tls.Certificate) { + f.setDefaultTLSCertWasCalled = true + f.setDefaultTLSCertReceived = certificate +} + +func TestTLSCertObserverControllerSync(t *testing.T) { + spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { + const ( + installedInNamespace = "some-namespace" + defaultTLSSecretName = "some-default-secret-name" + ) + + 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 + issuerTLSCertSetter *fakeIssuerTLSCertSetter + ) + + // 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( + issuerTLSCertSetter, + defaultTLSSecretName, + 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) + issuerTLSCertSetter = &fakeIssuerTLSCertSetter{} + + 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 issuerTLSCertSetter's map to be empty", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + r.True(issuerTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Empty(issuerTLSCertSetter.issuerHostToTLSCertMapReceived) + r.True(issuerTLSCertSetter.setDefaultTLSCertWasCalled) + r.Nil(issuerTLSCertSetter.setDefaultTLSCertReceived) + }) + }) + + 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 SNICertificateSecretName field + } + oidcProviderConfigWithoutSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: "https://no-secret-issuer2.com", + SNICertificateSecretName: "", + }, + } + oidcProviderConfigWithBadSecret := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: "https://bad-secret-issuer.com", + SNICertificateSecretName: "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", + SNICertificateSecretName: "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", + SNICertificateSecretName: "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 issuerTLSCertSetter's map to include only the issuers that had valid certs", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.True(issuerTLSCertSetter.setDefaultTLSCertWasCalled) + r.Nil(issuerTLSCertSetter.setDefaultTLSCertReceived) + + r.True(issuerTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Len(issuerTLSCertSetter.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 := issuerTLSCertSetter.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 := issuerTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret2.com"] + r.NotNil(actualCertificate2) + r.Equal(expectedCertificate2, *actualCertificate2) + }) + + when("there is also a default TLS cert secret with the configured default TLS cert secret name", func() { + var ( + expectedDefaultCertificate tls.Certificate + ) + + it.Before(func() { + var err error + testCrt := readTestFile("testdata/test3.crt") + r.NotEmpty(testCrt) + testKey := readTestFile("testdata/test3.key") + r.NotEmpty(testKey) + expectedDefaultCertificate, err = tls.X509KeyPair(testCrt, testKey) + r.NoError(err) + defaultTLSCertSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: defaultTLSSecretName, Namespace: installedInNamespace}, + Data: map[string][]byte{"tls.crt": testCrt, "tls.key": testKey}, + } + r.NoError(kubeInformerClient.Tracker().Add(defaultTLSCertSecret)) + }) + + it("updates the issuerTLSCertSetter's map as before but also updates the default certificate", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.True(issuerTLSCertSetter.setDefaultTLSCertWasCalled) + actualDefaultCertificate := issuerTLSCertSetter.setDefaultTLSCertReceived + r.NotNil(actualDefaultCertificate) + r.Equal(expectedDefaultCertificate, *actualDefaultCertificate) + + r.True(issuerTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Len(issuerTLSCertSetter.issuerHostToTLSCertMapReceived, 2) + }) + }) + }) + }, 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 000000000..7c48ad9c8 --- /dev/null +++ b/internal/oidc/provider/dynamic_tls_cert_provider.go @@ -0,0 +1,52 @@ +// 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) + SetDefaultTLSCert(certificate *tls.Certificate) + GetTLSCert(lowercaseIssuerHostName string) *tls.Certificate + GetDefaultTLSCert() *tls.Certificate +} + +type dynamicTLSCertProvider struct { + issuerHostToTLSCertMap map[string]*tls.Certificate + defaultCert *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) SetDefaultTLSCert(certificate *tls.Certificate) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.defaultCert = certificate +} + +func (p *dynamicTLSCertProvider) GetTLSCert(issuerHostName string) *tls.Certificate { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.issuerHostToTLSCertMap[issuerHostName] +} + +func (p *dynamicTLSCertProvider) GetDefaultTLSCert() *tls.Certificate { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.defaultCert +} diff --git a/internal/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 0737c3f50..46a5f071d 100644 --- a/internal/oidc/provider/manager/manager.go +++ b/internal/oidc/provider/manager/manager.go @@ -5,6 +5,7 @@ package manager import ( "net/http" + "strings" "sync" "k8s.io/klog/v2" @@ -53,10 +54,10 @@ func (m *Manager) SetProviders(oidcProviders ...*provider.OIDCProvider) { m.providerHandlers = make(map[string]http.Handler) for _, incomingProvider := range oidcProviders { - wellKnownURL := incomingProvider.IssuerHost() + "/" + incomingProvider.IssuerPath() + oidc.WellKnownEndpointPath + wellKnownURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.WellKnownEndpointPath m.providerHandlers[wellKnownURL] = discovery.NewHandler(incomingProvider.Issuer()) - jwksURL := incomingProvider.IssuerHost() + "/" + incomingProvider.IssuerPath() + oidc.JWKSEndpointPath + jwksURL := strings.ToLower(incomingProvider.IssuerHost()) + "/" + incomingProvider.IssuerPath() + oidc.JWKSEndpointPath m.providerHandlers[jwksURL] = jwks.NewHandler(incomingProvider.Issuer(), m.dynamicJWKSProvider) klog.InfoS("oidc provider manager added or updated issuer", "issuer", incomingProvider.Issuer()) @@ -85,5 +86,5 @@ func (m *Manager) findHandler(req *http.Request) http.Handler { m.mu.RLock() defer m.mu.RUnlock() - return m.providerHandlers[req.Host+"/"+req.URL.Path] + return m.providerHandlers[strings.ToLower(req.Host)+"/"+req.URL.Path] } diff --git a/internal/oidc/provider/manager/manager_test.go b/internal/oidc/provider/manager/manager_test.go index d68e07820..01122ad79 100644 --- a/internal/oidc/provider/manager/manager_test.go +++ b/internal/oidc/provider/manager/manager_test.go @@ -33,18 +33,20 @@ func TestManager(t *testing.T) { ) issuer1 := "https://example.com/some/path" + issuer1DifferentCaseHostname := "https://eXamPle.coM/some/path" issuer1KeyID := "issuer1-key" issuer2 := "https://example.com/some/path/more/deeply/nested/path" // note that this is a sub-path of the other issuer url + issuer2DifferentCaseHostname := "https://exAmPlE.Com/some/path/more/deeply/nested/path" issuer2KeyID := "issuer2-key" newGetRequest := func(url string) *http.Request { return httptest.NewRequest(http.MethodGet, url, nil) } - requireDiscoveryRequestToBeHandled := func(issuer, requestURLSuffix string) { + requireDiscoveryRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedIssuerInResponse string) { recorder := httptest.NewRecorder() - subject.ServeHTTP(recorder, newGetRequest(issuer+oidc.WellKnownEndpointPath+requestURLSuffix)) + subject.ServeHTTP(recorder, newGetRequest(requestIssuer+oidc.WellKnownEndpointPath+requestURLSuffix)) r.False(fallbackHandlerWasCalled) @@ -56,7 +58,7 @@ func TestManager(t *testing.T) { r.NoError(err) // Minimal check to ensure that the right discovery endpoint was called - r.Equal(issuer, parsedDiscoveryResult.Issuer) + r.Equal(expectedIssuerInResponse, parsedDiscoveryResult.Issuer) } requireJWKSRequestToBeHandled := func(requestIssuer, requestURLSuffix, expectedJWKKeyID string) { @@ -145,13 +147,23 @@ func TestManager(t *testing.T) { }) it("routes matching requests to the appropriate provider", func() { - requireDiscoveryRequestToBeHandled(issuer1, "") - requireDiscoveryRequestToBeHandled(issuer2, "") - requireDiscoveryRequestToBeHandled(issuer2, "?some=query") + requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) + + // Hostnames are case-insensitive, so test that we can handle that. + requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) + + // Hostnames are case-insensitive, so test that we can handle that. + requireJWKSRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2KeyID) }) }) @@ -170,13 +182,23 @@ func TestManager(t *testing.T) { }) it("still routes matching requests to the appropriate provider", func() { - requireDiscoveryRequestToBeHandled(issuer1, "") - requireDiscoveryRequestToBeHandled(issuer2, "") - requireDiscoveryRequestToBeHandled(issuer2, "?some=query") + requireDiscoveryRequestToBeHandled(issuer1, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2, "?some=query", issuer2) + + // Hostnames are case-insensitive, so test that we can handle that. + requireDiscoveryRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2) + requireDiscoveryRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2) requireJWKSRequestToBeHandled(issuer1, "", issuer1KeyID) requireJWKSRequestToBeHandled(issuer2, "", issuer2KeyID) requireJWKSRequestToBeHandled(issuer2, "?some=query", issuer2KeyID) + + // Hostnames are case-insensitive, so test that we can handle that. + requireJWKSRequestToBeHandled(issuer1DifferentCaseHostname, "", issuer1KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "", issuer2KeyID) + requireJWKSRequestToBeHandled(issuer2DifferentCaseHostname, "?some=query", issuer2KeyID) }) }) }) diff --git a/internal/oidc/provider/oidcprovider.go b/internal/oidc/provider/oidcprovider.go index cc4271674..3e53c5e2a 100644 --- a/internal/oidc/provider/oidcprovider.go +++ b/internal/oidc/provider/oidcprovider.go @@ -37,7 +37,7 @@ func (p *OIDCProvider) validate() error { return fmt.Errorf("could not parse issuer as URL: %w", err) } - if issuerURL.Scheme != "https" && p.removeMeAfterWeNoLongerNeedHTTPIssuerSupport(issuerURL.Scheme) { + if issuerURL.Scheme != "https" { return constable.Error(`issuer must have "https" scheme`) } @@ -74,7 +74,3 @@ func (p *OIDCProvider) IssuerHost() string { func (p *OIDCProvider) IssuerPath() string { return p.issuerPath } - -func (p *OIDCProvider) removeMeAfterWeNoLongerNeedHTTPIssuerSupport(scheme string) bool { - return scheme != "http" -} diff --git a/internal/oidc/provider/oidcprovider_test.go b/internal/oidc/provider/oidcprovider_test.go index 81204e284..828bd6711 100644 --- a/internal/oidc/provider/oidcprovider_test.go +++ b/internal/oidc/provider/oidcprovider_test.go @@ -58,6 +58,11 @@ func TestOIDCProviderValidations(t *testing.T) { name: "with path", issuer: "https://tuna.com/fish/marlin", }, + { + name: "with http scheme", + issuer: "http://tuna.com", + wantError: `issuer must have "https" scheme`, + }, { name: "trailing slash in path", issuer: "https://tuna.com/", diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index b536429c8..725ecdfd5 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -7,9 +7,11 @@ import ( "context" "crypto/tls" "crypto/x509" + "crypto/x509/pkix" "encoding/json" "fmt" "io/ioutil" + "net" "net/http" "net/url" "strings" @@ -18,42 +20,154 @@ import ( "github.com/stretchr/testify/assert" "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/client-go/kubernetes" "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" pinnipedclientset "go.pinniped.dev/generated/1.19/client/clientset/versioned" + "go.pinniped.dev/internal/certauthority" "go.pinniped.dev/internal/here" "go.pinniped.dev/test/library" ) +func TestSupervisorTLSTerminationWithSNI(t *testing.T) { + env := library.IntegrationEnv(t) + pinnipedClient := library.NewPinnipedClientset(t) + kubeClient := library.NewClientset(t) + + ns := env.SupervisorNamespace + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), pinnipedClient, kubeClient) + + scheme := "https" + address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 + + hostname1 := strings.Split(address, ":")[0] + issuer1 := fmt.Sprintf("%s://%s/issuer1", scheme, address) + sniCertificateSecretName1 := "integration-test-sni-cert-1" + + // Create an OIDCProviderConfig with an sniCertificateSecretName. + oidcProviderConfig1 := library.CreateTestOIDCProvider(ctx, t, issuer1, sniCertificateSecretName1) + requireStatus(t, pinnipedClient, oidcProviderConfig1.Namespace, oidcProviderConfig1.Name, v1alpha1.SuccessOIDCProviderStatus) + + // The sniCertificateSecretName Secret does not exist, so the endpoints should fail with TLS errors. + requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuer1) + + // Create the Secret. + ca1 := createTLSCertificateSecret(ctx, t, ns, hostname1, nil, sniCertificateSecretName1, kubeClient) + + // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1.Bundle()), issuer1, nil) + + // Update the config to take away the sniCertificateSecretName. + sniCertificateSecretName1update := "integration-test-sni-cert-1-update" + oidcProviderConfig1LatestVersion, err := pinnipedClient.ConfigV1alpha1().OIDCProviderConfigs(ns).Get(ctx, oidcProviderConfig1.Name, metav1.GetOptions{}) + require.NoError(t, err) + oidcProviderConfig1LatestVersion.Spec.SNICertificateSecretName = sniCertificateSecretName1update + _, err = pinnipedClient.ConfigV1alpha1().OIDCProviderConfigs(ns).Update(ctx, oidcProviderConfig1LatestVersion, metav1.UpdateOptions{}) + require.NoError(t, err) + + // The the endpoints should fail with TLS errors again. + requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuer1) + + // Create a Secret at the updated name. + ca1update := createTLSCertificateSecret(ctx, t, ns, hostname1, nil, sniCertificateSecretName1update, kubeClient) + + // Now that the Secret exists at the new name, we should be able to access the endpoints by hostname using the CA. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, string(ca1update.Bundle()), issuer1, nil) + + // To test SNI virtual hosting, send requests to discovery endpoints when the public address is different from the issuer name. + hostname2 := "some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com" + hostnamePort2 := "2684" + issuer2 := fmt.Sprintf("%s://%s:%s/issuer2", scheme, hostname2, hostnamePort2) + sniCertificateSecretName2 := "integration-test-sni-cert-2" + + // Create an OIDCProviderConfig with an sniCertificateSecretName. + oidcProviderConfig2 := library.CreateTestOIDCProvider(ctx, t, issuer2, sniCertificateSecretName2) + requireStatus(t, pinnipedClient, oidcProviderConfig2.Namespace, oidcProviderConfig2.Name, v1alpha1.SuccessOIDCProviderStatus) + + // Create the Secret. + ca2 := createTLSCertificateSecret(ctx, t, ns, hostname2, nil, sniCertificateSecretName2, kubeClient) + + // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, hostname2+":"+hostnamePort2, string(ca2.Bundle()), issuer2, map[string]string{ + hostname2 + ":" + hostnamePort2: address, + }) +} + +func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { + env := library.IntegrationEnv(t) + pinnipedClient := library.NewPinnipedClientset(t) + kubeClient := library.NewClientset(t) + + ns := env.SupervisorNamespace + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) + defer cancel() + + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), pinnipedClient, kubeClient) + + scheme := "https" + address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 + + hostAndPortSegments := strings.Split(address, ":") + // hostnames are case-insensitive, so test mis-matching the case of the issuer URL and the request URL + hostname := strings.ToLower(hostAndPortSegments[0]) + port := "443" + if len(hostAndPortSegments) > 1 { + port = hostAndPortSegments[1] + } + ips, err := net.DefaultResolver.LookupIP(ctx, "ip4", hostname) + require.NoError(t, err) + ip := ips[0] + ipAsString := ip.String() + ipWithPort := ipAsString + ":" + port + + issuerUsingIPAddress := fmt.Sprintf("%s://%s/issuer1", scheme, ipWithPort) + issuerUsingHostname := fmt.Sprintf("%s://%s/issuer1", scheme, address) + + // Create an OIDCProviderConfig without an sniCertificateSecretName. + oidcProviderConfig1 := library.CreateTestOIDCProvider(ctx, t, issuerUsingIPAddress, "") + requireStatus(t, pinnipedClient, oidcProviderConfig1.Namespace, oidcProviderConfig1.Name, v1alpha1.SuccessOIDCProviderStatus) + + // There is no default TLS cert and the sniCertificateSecretName was not set, so the endpoints should fail with TLS errors. + requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuerUsingIPAddress) + + // Create a Secret at the special name which represents the default TLS cert. + defaultCA := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, defaultTLSCertSecretName(env), kubeClient) + + // Now that the Secret exists, we should be able to access the endpoints by IP address using the CA. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) + + // Create an OIDCProviderConfig with an sniCertificateSecretName. + sniCertificateSecretName := "integration-test-sni-cert-1" + oidcProviderConfig2 := library.CreateTestOIDCProvider(ctx, t, issuerUsingHostname, sniCertificateSecretName) + requireStatus(t, pinnipedClient, oidcProviderConfig2.Namespace, oidcProviderConfig2.Name, v1alpha1.SuccessOIDCProviderStatus) + + // Create the Secret. + sniCA := createTLSCertificateSecret(ctx, t, ns, hostname, nil, sniCertificateSecretName, kubeClient) + + // Now that the Secret exists, we should be able to access the endpoints by hostname using the CA from the SNI cert. + // Hostnames are case-insensitive, so the request should still work even if the case of the hostname is different + // from the case of the issuer URL's hostname. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, strings.ToUpper(hostname)+":"+port, string(sniCA.Bundle()), issuerUsingHostname, nil) + + // And we can still access the other issuer using the default cert. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, ipWithPort, string(defaultCA.Bundle()), issuerUsingIPAddress, nil) +} + func TestSupervisorOIDCDiscovery(t *testing.T) { env := library.IntegrationEnv(t) client := library.NewPinnipedClientset(t) ns := env.SupervisorNamespace - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - // Temporarily remove any existing OIDCProviderConfigs from the cluster so we can test from a clean slate. - originalConfigList, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).List(ctx, metav1.ListOptions{}) - require.NoError(t, err) - for _, config := range originalConfigList.Items { - err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, config.Name, metav1.DeleteOptions{}) - require.NoError(t, err) - } - - // When this test has finished, recreate any OIDCProviderConfigs that had existed on the cluster before this test. - t.Cleanup(func() { - cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) - defer cancel() - - for _, config := range originalConfigList.Items { - thisConfig := config - thisConfig.ResourceVersion = "" // Get rid of resource version since we can't create an object with one. - _, err := client.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(cleanupCtx, &thisConfig, metav1.CreateOptions{}) - require.NoError(t, err) - } - }) + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, library.NewClientset(t)) tests := []struct { Scheme string @@ -61,7 +175,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { CABundle string }{ {Scheme: "http", Address: env.SupervisorHTTPAddress}, - {Scheme: "https", Address: env.SupervisorHTTPSAddress, CABundle: env.SupervisorHTTPSCABundle}, + {Scheme: "https", Address: env.SupervisorHTTPSIngressAddress, CABundle: env.SupervisorHTTPSIngressCABundle}, } for _, test := range tests { @@ -77,14 +191,14 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // Test that there is no default discovery endpoint available when there are no OIDCProviderConfigs. requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, fmt.Sprintf("%s://%s", scheme, addr)) - // Define several unique issuer strings. - issuer1 := fmt.Sprintf("%s://%s/nested/issuer1", scheme, addr) - issuer2 := fmt.Sprintf("%s://%s/nested/issuer2", scheme, addr) - issuer3 := fmt.Sprintf("%s://%s/issuer3", scheme, addr) - issuer4 := fmt.Sprintf("%s://%s/issuer4", scheme, addr) - issuer5 := fmt.Sprintf("%s://%s/issuer5", scheme, addr) - issuer6 := fmt.Sprintf("%s://%s/issuer6", scheme, addr) - badIssuer := fmt.Sprintf("%s://%s/badIssuer?cannot-use=queries", scheme, addr) + // Define several unique issuer strings. Always use https in the issuer name even when we are accessing the http port. + issuer1 := fmt.Sprintf("https://%s/nested/issuer1", addr) + issuer2 := fmt.Sprintf("https://%s/nested/issuer2", addr) + issuer3 := fmt.Sprintf("https://%s/issuer3", addr) + issuer4 := fmt.Sprintf("https://%s/issuer4", addr) + issuer5 := fmt.Sprintf("https://%s/issuer5", addr) + issuer6 := fmt.Sprintf("https://%s/issuer6", addr) + badIssuer := fmt.Sprintf("https://%s/badIssuer?cannot-use=queries", addr) // When OIDCProviderConfig are created in sequence they each cause a discovery endpoint to appear only for as long as the OIDCProviderConfig exists. config1, jwks1 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer1, client) @@ -98,7 +212,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // When multiple OIDCProviderConfigs exist at the same time they each serve a unique discovery endpoint. config3, jwks3 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer3, client) config4, jwks4 := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer4, client) - requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3) // discovery for issuer3 is still working after issuer4 started working + requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer3, nil) // discovery for issuer3 is still working after issuer4 started working // The auto-created JWK's were different from each other. require.NotEqual(t, jwks3.Keys[0]["x"], jwks4.Keys[0]["x"]) require.NotEqual(t, jwks3.Keys[0]["y"], jwks4.Keys[0]["y"]) @@ -106,7 +220,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // Editing a provider to change the issuer name updates the endpoints that are being served. updatedConfig4 := editOIDCProviderConfigIssuerName(t, config4, client, ns, issuer5) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer4) - jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5) + jwks5 := requireDiscoveryEndpointsAreWorking(t, scheme, addr, caBundle, issuer5, nil) // The JWK did not change when the issuer name was updated. require.Equal(t, jwks4.Keys[0], jwks5.Keys[0]) @@ -116,37 +230,132 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { // When the same issuer is added twice, both issuers are marked as duplicates, and neither provider is serving. config6Duplicate1, _ := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer6, client) - config6Duplicate2 := library.CreateTestOIDCProvider(ctx, t, issuer6) + config6Duplicate2 := library.CreateTestOIDCProvider(ctx, t, issuer6, "") requireStatus(t, client, ns, config6Duplicate1.Name, v1alpha1.DuplicateOIDCProviderStatus) requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.DuplicateOIDCProviderStatus) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, issuer6) // If we delete the first duplicate issuer, the second duplicate issuer starts serving. requireDelete(t, client, ns, config6Duplicate1.Name) - requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6) + requireWellKnownEndpointIsWorking(t, scheme, addr, caBundle, issuer6, nil) requireStatus(t, client, ns, config6Duplicate2.Name, v1alpha1.SuccessOIDCProviderStatus) // When we finally delete all issuers, the endpoint should be down. requireDeletingOIDCProviderConfigCausesDiscoveryEndpointsToDisappear(t, config6Duplicate2, client, ns, scheme, addr, caBundle, issuer6) // "Host" headers can be used to send requests to discovery endpoints when the public address is different from the issuer name. - issuer7 := fmt.Sprintf("%s://some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com:2684/issuer7", scheme) + issuer7 := "https://some-issuer-host-and-port-that-doesnt-match-public-supervisor-address.com:2684/issuer7" config7, _ := requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear(ctx, t, scheme, addr, caBundle, issuer7, client) requireDeletingOIDCProviderConfigCausesDiscoveryEndpointsToDisappear(t, config7, client, ns, scheme, addr, caBundle, issuer7) // When we create a provider with an invalid issuer, the status is set to invalid. - badConfig := library.CreateTestOIDCProvider(ctx, t, badIssuer) + badConfig := library.CreateTestOIDCProvider(ctx, t, badIssuer, "") requireStatus(t, client, ns, badConfig.Name, v1alpha1.InvalidOIDCProviderStatus) requireDiscoveryEndpointsAreNotFound(t, scheme, addr, caBundle, badIssuer) requireDeletingOIDCProviderConfigCausesDiscoveryEndpointsToDisappear(t, badConfig, client, ns, scheme, addr, caBundle, badIssuer) } } +func defaultTLSCertSecretName(env *library.TestEnv) string { + return env.SupervisorAppName + "-default-tls-certificate" //nolint:gosec // this is not a hardcoded credential +} + +func createTLSCertificateSecret(ctx context.Context, t *testing.T, ns string, hostname string, ips []net.IP, secretName string, kubeClient kubernetes.Interface) *certauthority.CA { + // Create a CA. + ca, err := certauthority.New(pkix.Name{CommonName: "Acme Corp"}, 1000*time.Hour) + require.NoError(t, err) + + // Using the CA, create a TLS server cert. + tlsCert, err := ca.Issue(pkix.Name{CommonName: hostname}, []string{hostname}, ips, 1000*time.Hour) + require.NoError(t, err) + + // Write the serving cert to the SNI secret. + tlsCertChainPEM, tlsPrivateKeyPEM, err := certauthority.ToPEM(tlsCert) + require.NoError(t, err) + secret := corev1.Secret{ + TypeMeta: metav1.TypeMeta{}, + ObjectMeta: metav1.ObjectMeta{ + Name: secretName, + Namespace: ns, + }, + StringData: map[string]string{ + "tls.crt": string(tlsCertChainPEM), + "tls.key": string(tlsPrivateKeyPEM), + }, + } + _, err = kubeClient.CoreV1().Secrets(ns).Create(ctx, &secret, metav1.CreateOptions{}) + require.NoError(t, err) + + // Delete the Secret when the test ends. + t.Cleanup(func() { + t.Helper() + deleteCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) + defer cancel() + err := kubeClient.CoreV1().Secrets(ns).Delete(deleteCtx, secretName, metav1.DeleteOptions{}) + require.NoError(t, err) + }) + + return ca +} + +func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret( + ctx context.Context, + t *testing.T, + ns string, + defaultTLSCertSecretName string, + pinnipedClient pinnipedclientset.Interface, + kubeClient kubernetes.Interface, +) { + // Temporarily remove any existing OIDCProviderConfigs from the cluster so we can test from a clean slate. + originalConfigList, err := pinnipedClient.ConfigV1alpha1().OIDCProviderConfigs(ns).List(ctx, metav1.ListOptions{}) + require.NoError(t, err) + for _, config := range originalConfigList.Items { + err := pinnipedClient.ConfigV1alpha1().OIDCProviderConfigs(ns).Delete(ctx, config.Name, metav1.DeleteOptions{}) + require.NoError(t, err) + } + + // Also remove the supervisor's default TLS cert + originalSecret, err := kubeClient.CoreV1().Secrets(ns).Get(ctx, defaultTLSCertSecretName, metav1.GetOptions{}) + notFound := k8serrors.IsNotFound(err) + require.False(t, err != nil && !notFound, "unexpected error when getting %s", defaultTLSCertSecretName) + if notFound { + originalSecret = nil + } else { + err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, defaultTLSCertSecretName, metav1.DeleteOptions{}) + require.NoError(t, err) + } + + // When this test has finished, recreate any OIDCProviderConfigs and default secret that had existed on the cluster before this test. + t.Cleanup(func() { + cleanupCtx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + for _, config := range originalConfigList.Items { + thisConfig := config + thisConfig.ResourceVersion = "" // Get rid of resource version since we can't create an object with one. + _, err := pinnipedClient.ConfigV1alpha1().OIDCProviderConfigs(ns).Create(cleanupCtx, &thisConfig, metav1.CreateOptions{}) + require.NoError(t, err) + } + + if originalSecret != nil { + originalSecret.ResourceVersion = "" // Get rid of resource version since we can't create an object with one. + _, err = kubeClient.CoreV1().Secrets(ns).Create(cleanupCtx, originalSecret, metav1.CreateOptions{}) + require.NoError(t, err) + } + }) +} + func jwksURLForIssuer(scheme, host, path string) string { + if path == "" { + return fmt.Sprintf("%s://%s/jwks.json", scheme, host) + } return fmt.Sprintf("%s://%s/%s/jwks.json", scheme, host, strings.TrimPrefix(path, "/")) } func wellKnownURLForIssuer(scheme, host, path string) string { + if path == "" { + return fmt.Sprintf("%s://%s/.well-known/openid-configuration", scheme, host) + } return fmt.Sprintf("%s://%s/%s/.well-known/openid-configuration", scheme, host, strings.TrimPrefix(path, "/")) } @@ -160,14 +369,14 @@ func requireDiscoveryEndpointsAreNotFound(t *testing.T, supervisorScheme, superv func requireEndpointNotFound(t *testing.T, url, host, caBundle string) { t.Helper() - httpClient := newHTTPClient(caBundle) + httpClient := newHTTPClient(t, caBundle, nil) ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() requestNonExistentPath, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) require.NoError(t, err) - requestNonExistentPath.Header.Add("Host", host) + requestNonExistentPath.Host = host var response *http.Response assert.Eventually(t, func() bool { @@ -180,6 +389,23 @@ func requireEndpointNotFound(t *testing.T, url, host, caBundle string) { require.NoError(t, err) } +func requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t *testing.T, url string) { + t.Helper() + httpClient := newHTTPClient(t, "", nil) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) + defer cancel() + + request, err := http.NewRequestWithContext(ctx, http.MethodGet, url, nil) + require.NoError(t, err) + + assert.Eventually(t, func() bool { + _, err = httpClient.Do(request) //nolint:bodyclose + return err != nil && strings.Contains(err.Error(), "tls: unrecognized name") + }, 10*time.Second, 200*time.Millisecond) + require.Error(t, err) + require.EqualError(t, err, fmt.Sprintf(`Get "%s": remote error: tls: unrecognized name`, url)) +} + func requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear( ctx context.Context, t *testing.T, @@ -188,15 +414,15 @@ func requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear( client pinnipedclientset.Interface, ) (*v1alpha1.OIDCProviderConfig, *ExpectedJWKSResponseFormat) { t.Helper() - newOIDCProviderConfig := library.CreateTestOIDCProvider(ctx, t, issuerName) - jwksResult := requireDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName) + newOIDCProviderConfig := library.CreateTestOIDCProvider(ctx, t, issuerName, "") + jwksResult := requireDiscoveryEndpointsAreWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, nil) requireStatus(t, client, newOIDCProviderConfig.Namespace, newOIDCProviderConfig.Name, v1alpha1.SuccessOIDCProviderStatus) return newOIDCProviderConfig, jwksResult } -func requireDiscoveryEndpointsAreWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string) *ExpectedJWKSResponseFormat { - requireWellKnownEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName) - jwksResult := requireJWKSEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName) +func requireDiscoveryEndpointsAreWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) *ExpectedJWKSResponseFormat { + requireWellKnownEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides) + jwksResult := requireJWKSEndpointIsWorking(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName, dnsOverrides) return jwksResult } @@ -220,11 +446,11 @@ func requireDeletingOIDCProviderConfigCausesDiscoveryEndpointsToDisappear( requireDiscoveryEndpointsAreNotFound(t, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName) } -func requireWellKnownEndpointIsWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string) { +func requireWellKnownEndpointIsWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) { t.Helper() issuerURL, err := url.Parse(issuerName) require.NoError(t, err) - response, responseBody := requireSuccessEndpointResponse(t, wellKnownURLForIssuer(supervisorScheme, supervisorAddress, issuerURL.Path), issuerName, supervisorCABundle) //nolint:bodyclose + response, responseBody := requireSuccessEndpointResponse(t, wellKnownURLForIssuer(supervisorScheme, supervisorAddress, issuerURL.Path), issuerName, supervisorCABundle, dnsOverrides) //nolint:bodyclose // Check that the response matches our expectations. expectedResultTemplate := here.Doc(`{ @@ -250,12 +476,17 @@ type ExpectedJWKSResponseFormat struct { Keys []map[string]string } -func requireJWKSEndpointIsWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string) *ExpectedJWKSResponseFormat { +func requireJWKSEndpointIsWorking(t *testing.T, supervisorScheme, supervisorAddress, supervisorCABundle, issuerName string, dnsOverrides map[string]string) *ExpectedJWKSResponseFormat { t.Helper() issuerURL, err := url.Parse(issuerName) require.NoError(t, err) - response, responseBody := requireSuccessEndpointResponse(t, jwksURLForIssuer(supervisorScheme, supervisorAddress, issuerURL.Path), issuerName, supervisorCABundle) //nolint:bodyclose + response, responseBody := requireSuccessEndpointResponse(t, //nolint:bodyclose + jwksURLForIssuer(supervisorScheme, supervisorAddress, issuerURL.Path), + issuerName, + supervisorCABundle, + dnsOverrides, + ) var result ExpectedJWKSResponseFormat err = json.Unmarshal([]byte(responseBody), &result) @@ -277,9 +508,10 @@ func requireJWKSEndpointIsWorking(t *testing.T, supervisorScheme, supervisorAddr return &result } -func requireSuccessEndpointResponse(t *testing.T, endpointURL, issuer, caBundle string) (*http.Response, string) { +func requireSuccessEndpointResponse(t *testing.T, endpointURL, issuer, caBundle string, dnsOverrides map[string]string) (*http.Response, string) { t.Helper() - httpClient := newHTTPClient(caBundle) + httpClient := newHTTPClient(t, caBundle, dnsOverrides) + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Minute) defer cancel() @@ -360,12 +592,33 @@ func requireStatus(t *testing.T, client pinnipedclientset.Interface, ns, name st require.Equalf(t, status, opc.Status.Status, "unexpected status (message = '%s')", opc.Status.Message) } -func newHTTPClient(caBundle string) *http.Client { +func newHTTPClient(t *testing.T, caBundle string, dnsOverrides map[string]string) *http.Client { c := &http.Client{} + + realDialer := &net.Dialer{} + overrideDialContext := func(ctx context.Context, network, addr string) (net.Conn, error) { + replacementAddr, hasKey := dnsOverrides[addr] + if hasKey { + t.Logf("DialContext replacing addr %s with %s", addr, replacementAddr) + addr = replacementAddr + } else if dnsOverrides != nil { + t.Fatal("dnsOverrides was provided but not used, which was probably a mistake") + } + return realDialer.DialContext(ctx, network, addr) + } + if caBundle != "" { // CA bundle is optional caCertPool := x509.NewCertPool() caCertPool.AppendCertsFromPEM([]byte(caBundle)) - c.Transport = &http.Transport{TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS13, RootCAs: caCertPool}} + c.Transport = &http.Transport{ + DialContext: overrideDialContext, + TLSClientConfig: &tls.Config{MinVersion: tls.VersionTLS13, RootCAs: caCertPool}, + } + } else { + c.Transport = &http.Transport{ + DialContext: overrideDialContext, + } } + return c } diff --git a/test/integration/supervisor_keys_test.go b/test/integration/supervisor_keys_test.go index 52f5ba09c..19e1978dd 100644 --- a/test/integration/supervisor_keys_test.go +++ b/test/integration/supervisor_keys_test.go @@ -27,7 +27,7 @@ func TestSupervisorOIDCKeys(t *testing.T) { defer cancel() // Create our OPC under test. - opc := library.CreateTestOIDCProvider(ctx, t, "") + opc := library.CreateTestOIDCProvider(ctx, t, "", "") // Ensure a secret is created with the OPC's JWKS. var updatedOPC *configv1alpha1.OIDCProviderConfig diff --git a/test/library/client.go b/test/library/client.go index f12219da2..95f798b45 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -165,7 +165,7 @@ func CreateTestWebhookIDP(ctx context.Context, t *testing.T) corev1.TypedLocalOb // // If the provided issuer is not the empty string, then it will be used for the // OIDCProviderConfig.Spec.Issuer field. Else, a random issuer will be generated. -func CreateTestOIDCProvider(ctx context.Context, t *testing.T, issuer string) *configv1alpha1.OIDCProviderConfig { +func CreateTestOIDCProvider(ctx context.Context, t *testing.T, issuer, sniCertificateSecretName string) *configv1alpha1.OIDCProviderConfig { t.Helper() testEnv := IntegrationEnv(t) @@ -186,7 +186,8 @@ func CreateTestOIDCProvider(ctx context.Context, t *testing.T, issuer string) *c Annotations: map[string]string{"pinniped.dev/testName": t.Name()}, }, Spec: configv1alpha1.OIDCProviderConfigSpec{ - Issuer: issuer, + Issuer: issuer, + SNICertificateSecretName: sniCertificateSecretName, }, }, metav1.CreateOptions{}) require.NoError(t, err, "could not create test OIDCProviderConfig") diff --git a/test/library/env.go b/test/library/env.go index ed39a193d..22b8d6665 100644 --- a/test/library/env.go +++ b/test/library/env.go @@ -26,17 +26,18 @@ const ( type TestEnv struct { t *testing.T - ConciergeNamespace string `json:"conciergeNamespace"` - SupervisorNamespace string `json:"supervisorNamespace"` - ConciergeAppName string `json:"conciergeAppName"` - SupervisorAppName string `json:"supervisorAppName"` - SupervisorCustomLabels map[string]string `json:"supervisorCustomLabels"` - ConciergeCustomLabels map[string]string `json:"conciergeCustomLabels"` - Capabilities map[Capability]bool `json:"capabilities"` - TestWebhook idpv1alpha1.WebhookIdentityProviderSpec `json:"testWebhook"` - SupervisorHTTPAddress string `json:"supervisorHttpAddress"` - SupervisorHTTPSAddress string `json:"supervisorHttpsAddress"` - SupervisorHTTPSCABundle string `json:"supervisorHttpsCABundle"` + ConciergeNamespace string `json:"conciergeNamespace"` + SupervisorNamespace string `json:"supervisorNamespace"` + ConciergeAppName string `json:"conciergeAppName"` + SupervisorAppName string `json:"supervisorAppName"` + SupervisorCustomLabels map[string]string `json:"supervisorCustomLabels"` + ConciergeCustomLabels map[string]string `json:"conciergeCustomLabels"` + Capabilities map[Capability]bool `json:"capabilities"` + TestWebhook idpv1alpha1.WebhookIdentityProviderSpec `json:"testWebhook"` + SupervisorHTTPAddress string `json:"supervisorHttpAddress"` + SupervisorHTTPSAddress string `json:"supervisorHttpsAddress"` + SupervisorHTTPSIngressAddress string `json:"supervisorHttpsIngressAddress"` + SupervisorHTTPSIngressCABundle string `json:"supervisorHttpsIngressCABundle"` TestUser struct { Token string `json:"token"` @@ -75,51 +76,62 @@ func IntegrationEnv(t *testing.T) *TestEnv { err := yaml.Unmarshal([]byte(capabilitiesDescriptionYAML), &result) require.NoErrorf(t, err, "capabilities specification was invalid YAML") - needEnv := func(key string) string { - t.Helper() - value := os.Getenv(key) - require.NotEmptyf(t, value, "must specify %s env var for integration tests", key) - return value - } + loadEnvVars(t, &result) - result.ConciergeNamespace = needEnv("PINNIPED_TEST_CONCIERGE_NAMESPACE") - result.ConciergeAppName = needEnv("PINNIPED_TEST_CONCIERGE_APP_NAME") - result.TestUser.ExpectedUsername = needEnv("PINNIPED_TEST_USER_USERNAME") - result.TestUser.ExpectedGroups = strings.Split(strings.ReplaceAll(needEnv("PINNIPED_TEST_USER_GROUPS"), " ", ""), ",") - result.TestUser.Token = needEnv("PINNIPED_TEST_USER_TOKEN") - result.TestWebhook.Endpoint = needEnv("PINNIPED_TEST_WEBHOOK_ENDPOINT") - result.SupervisorNamespace = needEnv("PINNIPED_TEST_SUPERVISOR_NAMESPACE") - result.SupervisorAppName = needEnv("PINNIPED_TEST_SUPERVISOR_APP_NAME") - result.TestWebhook.TLS = &idpv1alpha1.TLSSpec{CertificateAuthorityData: needEnv("PINNIPED_TEST_WEBHOOK_CA_BUNDLE")} + result.t = t + return &result +} + +func needEnv(t *testing.T, key string) string { + t.Helper() + value := os.Getenv(key) + require.NotEmptyf(t, value, "must specify %s env var for integration tests", key) + return value +} + +func loadEnvVars(t *testing.T, result *TestEnv) { + t.Helper() + + result.ConciergeNamespace = needEnv(t, "PINNIPED_TEST_CONCIERGE_NAMESPACE") + result.ConciergeAppName = needEnv(t, "PINNIPED_TEST_CONCIERGE_APP_NAME") + result.TestUser.ExpectedUsername = needEnv(t, "PINNIPED_TEST_USER_USERNAME") + result.TestUser.ExpectedGroups = strings.Split(strings.ReplaceAll(needEnv(t, "PINNIPED_TEST_USER_GROUPS"), " ", ""), ",") + result.TestUser.Token = needEnv(t, "PINNIPED_TEST_USER_TOKEN") + result.TestWebhook.Endpoint = needEnv(t, "PINNIPED_TEST_WEBHOOK_ENDPOINT") + result.SupervisorNamespace = needEnv(t, "PINNIPED_TEST_SUPERVISOR_NAMESPACE") + result.SupervisorAppName = needEnv(t, "PINNIPED_TEST_SUPERVISOR_APP_NAME") + result.TestWebhook.TLS = &idpv1alpha1.TLSSpec{CertificateAuthorityData: needEnv(t, "PINNIPED_TEST_WEBHOOK_CA_BUNDLE")} result.SupervisorHTTPAddress = os.Getenv("PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS") - result.SupervisorHTTPSAddress = os.Getenv("PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS") + result.SupervisorHTTPSIngressAddress = os.Getenv("PINNIPED_TEST_SUPERVISOR_HTTPS_INGRESS_ADDRESS") + result.SupervisorHTTPSIngressCABundle = os.Getenv("PINNIPED_TEST_SUPERVISOR_HTTPS_INGRESS_CA_BUNDLE") // optional require.NotEmptyf(t, - result.SupervisorHTTPAddress+result.SupervisorHTTPSAddress, - "must specify either PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS or PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS env var (or both) for integration tests", + result.SupervisorHTTPAddress+result.SupervisorHTTPSIngressAddress, + "must specify either PINNIPED_TEST_SUPERVISOR_HTTP_ADDRESS or PINNIPED_TEST_SUPERVISOR_HTTPS_INGRESS_ADDRESS env var (or both) for integration tests", + ) + result.SupervisorHTTPSAddress = needEnv(t, "PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS") + require.NotRegexp(t, "^[0-9]", result.SupervisorHTTPSAddress, + "PINNIPED_TEST_SUPERVISOR_HTTPS_ADDRESS must be a hostname with an optional port and cannot be an IP address", ) - result.SupervisorHTTPSCABundle = os.Getenv("PINNIPED_TEST_SUPERVISOR_HTTPS_CA_BUNDLE") // optional - conciergeCustomLabelsYAML := needEnv("PINNIPED_TEST_CONCIERGE_CUSTOM_LABELS") + conciergeCustomLabelsYAML := needEnv(t, "PINNIPED_TEST_CONCIERGE_CUSTOM_LABELS") var conciergeCustomLabels map[string]string - err = yaml.Unmarshal([]byte(conciergeCustomLabelsYAML), &conciergeCustomLabels) + err := yaml.Unmarshal([]byte(conciergeCustomLabelsYAML), &conciergeCustomLabels) require.NoErrorf(t, err, "PINNIPED_TEST_CONCIERGE_CUSTOM_LABELS must be a YAML map of string to string") result.ConciergeCustomLabels = conciergeCustomLabels require.NotEmpty(t, result.ConciergeCustomLabels, "PINNIPED_TEST_CONCIERGE_CUSTOM_LABELS cannot be empty") - supervisorCustomLabelsYAML := needEnv("PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS") + supervisorCustomLabelsYAML := needEnv(t, "PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS") var supervisorCustomLabels map[string]string err = yaml.Unmarshal([]byte(supervisorCustomLabelsYAML), &supervisorCustomLabels) require.NoErrorf(t, err, "PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS must be a YAML map of string to string") result.SupervisorCustomLabels = supervisorCustomLabels require.NotEmpty(t, result.SupervisorCustomLabels, "PINNIPED_TEST_SUPERVISOR_CUSTOM_LABELS cannot be empty") - result.OIDCUpstream.Issuer = needEnv("PINNIPED_TEST_CLI_OIDC_ISSUER") - result.OIDCUpstream.ClientID = needEnv("PINNIPED_TEST_CLI_OIDC_CLIENT_ID") - result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(needEnv("PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT")) - result.OIDCUpstream.Username = needEnv("PINNIPED_TEST_CLI_OIDC_USERNAME") - result.OIDCUpstream.Password = needEnv("PINNIPED_TEST_CLI_OIDC_PASSWORD") - result.t = t - return &result + result.OIDCUpstream.Issuer = needEnv(t, "PINNIPED_TEST_CLI_OIDC_ISSUER") + result.OIDCUpstream.ClientID = needEnv(t, "PINNIPED_TEST_CLI_OIDC_CLIENT_ID") + result.OIDCUpstream.LocalhostPort, _ = strconv.Atoi(needEnv(t, "PINNIPED_TEST_CLI_OIDC_LOCALHOST_PORT")) + result.OIDCUpstream.Username = needEnv(t, "PINNIPED_TEST_CLI_OIDC_USERNAME") + result.OIDCUpstream.Password = needEnv(t, "PINNIPED_TEST_CLI_OIDC_PASSWORD") } func (e *TestEnv) HasCapability(cap Capability) bool {