From 25a91019c26b5113b56f01196b192d80797f70e4 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 23 Oct 2020 16:25:44 -0700 Subject: [PATCH 01/17] Add `spec.secretName` to OPC and handle case-insensitive hostnames - When two different Issuers have the same host (i.e. they differ only by path) then they must have the same secretName. This is because it wouldn't make sense for there to be two different TLS certificates for one host. Find any that do not have the same secret name to put an error status on them and to avoid serving OIDC endpoints for them. The host comparison is case-insensitive. - Issuer hostnames should be treated as case-insensitive, because DNS hostnames are case-insensitive. So https://me.com and https://mE.cOm are duplicate issuers. However, paths are case-sensitive, so https://me.com/A and https://me.com/a are different issuers. Fixed this in the issuer validations and in the OIDC Manager's request router logic. --- .../v1alpha1/types_oidcproviderconfig.go.tmpl | 16 +- ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 11 + generated/1.17/README.adoc | 1 + .../v1alpha1/types_oidcproviderconfig.go | 16 +- .../client/openapi/zz_generated.openapi.go | 7 + ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 11 + generated/1.18/README.adoc | 1 + .../v1alpha1/types_oidcproviderconfig.go | 16 +- .../client/openapi/zz_generated.openapi.go | 7 + ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 11 + generated/1.19/README.adoc | 1 + .../v1alpha1/types_oidcproviderconfig.go | 16 +- .../client/openapi/zz_generated.openapi.go | 7 + ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 11 + .../oidcproviderconfig_watcher.go | 59 ++++- .../oidcproviderconfig_watcher_test.go | 211 +++++++++++++++++- internal/oidc/provider/manager/manager.go | 7 +- .../oidc/provider/manager/manager_test.go | 40 +++- 18 files changed, 411 insertions(+), 38 deletions(-) diff --git a/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl b/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl index 8ecf6c83..38c2f0b2 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,15 @@ 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"` + + // SecretName 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. + // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside + // the cluster using a LoadBalancer Service), and is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named + // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // +optional + SecretName string `json:"secretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml index 727f20ec..528f6727 100644 --- a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,17 @@ spec: for more information." minLength: 1 type: string + secretName: + description: SecretName 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. + SecretName is required if you would like to use the HTTPS endpoints + (e.g. when exposing them outside the cluster using a LoadBalancer + Service), and is not required when you would like to use only the + HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS + secret must contain keys named `tls.crt` and `tls.key` that contain + the certificate and private key to use for TLS. + type: string required: - issuer type: object diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index bc3adc11..a43ac459 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -132,6 +132,7 @@ 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. +| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. |=== diff --git a/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83..38c2f0b2 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,15 @@ 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"` + + // SecretName 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. + // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside + // the cluster using a LoadBalancer Service), and is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named + // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // +optional + SecretName string `json:"secretName,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 5d451690..8ae4db21 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: "", }, }, + "secretName": { + SchemaProps: spec.SchemaProps{ + Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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 727f20ec..528f6727 100644 --- a/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,17 @@ spec: for more information." minLength: 1 type: string + secretName: + description: SecretName 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. + SecretName is required if you would like to use the HTTPS endpoints + (e.g. when exposing them outside the cluster using a LoadBalancer + Service), and is not required when you would like to use only the + HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS + secret must contain keys named `tls.crt` and `tls.key` that contain + the certificate and private key to use for TLS. + type: string required: - issuer type: object diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index d0b69f73..0a6f30fe 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -132,6 +132,7 @@ 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. +| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. |=== diff --git a/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83..38c2f0b2 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,15 @@ 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"` + + // SecretName 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. + // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside + // the cluster using a LoadBalancer Service), and is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named + // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // +optional + SecretName string `json:"secretName,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 bd28cdfd..85a105ef 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: "", }, }, + "secretName": { + SchemaProps: spec.SchemaProps{ + Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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 727f20ec..528f6727 100644 --- a/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,17 @@ spec: for more information." minLength: 1 type: string + secretName: + description: SecretName 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. + SecretName is required if you would like to use the HTTPS endpoints + (e.g. when exposing them outside the cluster using a LoadBalancer + Service), and is not required when you would like to use only the + HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS + secret must contain keys named `tls.crt` and `tls.key` that contain + the certificate and private key to use for TLS. + type: string required: - issuer type: object diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index 2e5b95fa..fa454528 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -132,6 +132,7 @@ 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. +| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. |=== diff --git a/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go b/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go index 8ecf6c83..38c2f0b2 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,15 @@ 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"` + + // SecretName 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. + // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside + // the cluster using a LoadBalancer Service), and is not required when you would like to use only the + // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named + // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // +optional + SecretName string `json:"secretName,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 dd6e0519..015721f8 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: "", }, }, + "secretName": { + SchemaProps: spec.SchemaProps{ + Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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 727f20ec..528f6727 100644 --- a/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,6 +49,17 @@ spec: for more information." minLength: 1 type: string + secretName: + description: SecretName 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. + SecretName is required if you would like to use the HTTPS endpoints + (e.g. when exposing them outside the cluster using a LoadBalancer + Service), and is not required when you would like to use only the + HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS + secret must contain keys named `tls.crt` and `tls.key` that contain + the certificate and private key to use for TLS. + type: string required: - issuer type: object diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index 77acd401..12dc2b63 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,76 @@ 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 addresses -> set of unique secret names. This will help us complain when + // multiple OIDCProviderConfigs have the same issuer address (host) component but specify + // different TLS serving Secrets. Doesn't make sense to have the one address use more than one + // TLS cert. Also make a helper function for forming keys into this map. + uniqueSecretNamesPerIssuerAddress := make(map[string]map[string]bool) + issuerURLToHostKey := func(issuerURL *url.URL) string { + return strings.ToLower(issuerURL.Host) + } + 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[issuerURLToHostKey(issuerURL)] + if setOfSecretNames == nil { + setOfSecretNames = make(map[string]bool) + uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] = setOfSecretNames + } + setOfSecretNames[opc.Spec.SecretName] = 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[issuerURLToHostKey(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 address must use the same secretName: "+issuerURLToHostKey(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 7db8022d..1028b324 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,196 @@ func TestSync(t *testing.T) { }) }) + when("there are OIDCProviderConfigs with the same issuer address 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", + SecretName: "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{ + Issuer: "https://issuer-duplicate-address.com/path2", + SecretName: "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", + SecretName: "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, + SecretName: "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 address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) + + oidcProviderConfigSameIssuerAddress2.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus + oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress2.Status.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/oidc/provider/manager/manager.go b/internal/oidc/provider/manager/manager.go index 0737c3f5..46a5f071 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 d68e0782..01122ad7 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) }) }) }) From 8b7c30cfbd678ea09c794dabdb2e09c537ba364f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 26 Oct 2020 17:03:26 -0700 Subject: [PATCH 02/17] Supervisor listens for HTTPS on port 443 with configurable TLS certs - TLS certificates can be configured on the OIDCProviderConfig using the `secretName` field. - When listening for incoming TLS connections, choose the TLS cert based on the SNI hostname of the incoming request. - Because SNI hostname information on incoming requests does not include the port number of the request, we add a validation that OIDCProviderConfigs where the issuer hostnames (not including port number) are the same must use the same `secretName`. - Note that this approach does not yet support requests made to an IP address instead of a hostname. Also note that `localhost` is considered a hostname by SNI. - Add port 443 as a container port to the pod spec. - A new controller watches for TLS secrets and caches them in memory. That same in-memory cache is used while servicing incoming connections on the TLS port. - Make it easy to configure both port 443 and/or port 80 for various Service types using our ytt templates for the supervisor. - When deploying to kind, add another nodeport and forward it to the host on another port to expose our new HTTPS supervisor port to the host. --- cmd/pinniped-supervisor/main.go | 53 ++- deploy/supervisor/deployment.yaml | 2 + deploy/supervisor/service.yaml | 52 ++- deploy/supervisor/values.yaml | 17 +- hack/lib/kind-config/single-node.yaml | 9 +- hack/lib/tilt/Tiltfile | 8 +- hack/prepare-for-integration-tests.sh | 9 +- .../supervisorconfig/jwks_observer.go | 2 +- .../oidcproviderconfig_watcher.go | 19 +- .../oidcproviderconfig_watcher_test.go | 10 +- .../supervisorconfig/testdata/test.crt | 19 ++ .../supervisorconfig/testdata/test.key | 28 ++ .../supervisorconfig/testdata/test2.crt | 18 ++ .../supervisorconfig/testdata/test2.key | 28 ++ .../supervisorconfig/tls_cert_observer.go | 101 ++++++ .../tls_cert_observer_test.go | 304 ++++++++++++++++++ .../provider/dynamic_tls_cert_provider.go | 37 +++ 17 files changed, 672 insertions(+), 44 deletions(-) create mode 100644 internal/controller/supervisorconfig/testdata/test.crt create mode 100644 internal/controller/supervisorconfig/testdata/test.key create mode 100644 internal/controller/supervisorconfig/testdata/test2.crt create mode 100644 internal/controller/supervisorconfig/testdata/test2.key create mode 100644 internal/controller/supervisorconfig/tls_cert_observer.go create mode 100644 internal/controller/supervisorconfig/tls_cert_observer_test.go create mode 100644 internal/oidc/provider/dynamic_tls_cert_provider.go diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 346f83ca..7b296fa9 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -5,11 +5,13 @@ package main import ( "context" + "crypto/tls" "fmt" "net" "net/http" "os" "os/signal" + "strings" "time" "k8s.io/apimachinery/pkg/util/clock" @@ -28,6 +30,7 @@ import ( "go.pinniped.dev/internal/controllerlib" "go.pinniped.dev/internal/downward" "go.pinniped.dev/internal/oidc/jwks" + "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidc/provider/manager" ) @@ -68,6 +71,7 @@ func startControllers( cfg *supervisor.Config, issuerManager *manager.Manager, dynamicJWKSProvider jwks.DynamicJWKSProvider, + dynamicTLSCertProvider provider.DynamicTLSCertProvider, kubeClient kubernetes.Interface, pinnipedClient pinnipedclientset.Interface, kubeInformers kubeinformers.SharedInformerFactory, @@ -105,6 +109,15 @@ func startControllers( controllerlib.WithInformer, ), singletonWorker, + ). + WithController( + supervisorconfig.NewTLSCertObserverController( + dynamicTLSCertProvider, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ), + singletonWorker, ) kubeInformers.Start(ctx.Done()) @@ -166,19 +179,49 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { })) dynamicJWKSProvider := jwks.NewDynamicJWKSProvider() + dynamicTLSCertProvider := provider.NewDynamicTLSCertProvider() + // OIDC endpoints will be served by the oidProvidersManager, and any non-OIDC paths will fallback to the healthMux. oidProvidersManager := manager.NewManager(healthMux, dynamicJWKSProvider) - startControllers(ctx, cfg, oidProvidersManager, dynamicJWKSProvider, kubeClient, pinnipedClient, kubeInformers, pinnipedInformers) + + startControllers( + ctx, + cfg, + oidProvidersManager, + dynamicJWKSProvider, + dynamicTLSCertProvider, + kubeClient, + pinnipedClient, + kubeInformers, + pinnipedInformers, + ) //nolint: gosec // Intentionally binding to all network interfaces. - l, err := net.Listen("tcp", ":80") + httpListener, err := net.Listen("tcp", ":80") if err != nil { return fmt.Errorf("cannot create listener: %w", err) } - defer l.Close() - start(ctx, l, oidProvidersManager) + defer httpListener.Close() + start(ctx, httpListener, oidProvidersManager) - klog.InfoS("supervisor is ready", "address", l.Addr().String()) + //nolint: gosec // Intentionally binding to all network interfaces. + httpsListener, err := tls.Listen("tcp", ":443", &tls.Config{ + MinVersion: tls.VersionTLS13, + GetCertificate: func(info *tls.ClientHelloInfo) (*tls.Certificate, error) { + klog.InfoS("GetCertificate called", "info.ServerName", info.ServerName) + return dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)), nil + }, + }) + if err != nil { + return fmt.Errorf("cannot create listener: %w", err) + } + defer httpsListener.Close() + start(ctx, httpsListener, oidProvidersManager) + + klog.InfoS("supervisor is ready", + "httpAddress", httpListener.Addr().String(), + "httpsAddress", httpsListener.Addr().String(), + ) gotSignal := waitForSignal() klog.InfoS("supervisor exiting", "signal", gotSignal) diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index afa4efd5..d5161d46 100644 --- a/deploy/supervisor/deployment.yaml +++ b/deploy/supervisor/deployment.yaml @@ -87,6 +87,8 @@ spec: ports: - containerPort: 80 protocol: TCP + - containerPort: 443 + protocol: TCP livenessProbe: httpGet: path: /healthz diff --git a/deploy/supervisor/service.yaml b/deploy/supervisor/service.yaml index 19076e5e..e2841a63 100644 --- a/deploy/supervisor/service.yaml +++ b/deploy/supervisor/service.yaml @@ -4,7 +4,7 @@ #@ load("@ytt:data", "data") #@ load("helpers.lib.yaml", "defaultLabel", "labels", "namespace", "defaultResourceName", "defaultResourceNameWithSuffix") -#@ if data.values.service_nodeport_port: +#@ if data.values.service_http_nodeport_port or data.values.service_https_nodeport_port: --- apiVersion: v1 kind: Service @@ -17,15 +17,27 @@ spec: selector: app: #@ data.values.app_name ports: - - protocol: TCP - port: #@ data.values.service_nodeport_port + #@ if data.values.service_http_nodeport_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_nodeport_port targetPort: 80 - #@ if data.values.service_nodeport_nodeport: - nodePort: #@ data.values.service_nodeport_nodeport + #@ if data.values.service_http_nodeport_nodeport: + nodePort: #@ data.values.service_http_nodeport_nodeport #@ end + #@ end + #@ if data.values.service_https_nodeport_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_nodeport_port + targetPort: 443 + #@ if data.values.service_https_nodeport_nodeport: + nodePort: #@ data.values.service_https_nodeport_nodeport + #@ end + #@ end #@ end -#@ if data.values.service_clusterip_port: +#@ if data.values.service_http_clusterip_port or data.values.service_https_clusterip_port: --- apiVersion: v1 kind: Service @@ -37,12 +49,21 @@ spec: type: ClusterIP selector: #@ defaultLabel() ports: - - protocol: TCP - port: #@ data.values.service_clusterip_port + #@ if data.values.service_http_clusterip_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_clusterip_port targetPort: 80 + #@ end + #@ if data.values.service_https_clusterip_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_clusterip_port + targetPort: 443 + #@ end #@ end -#@ if data.values.service_loadbalancer_port: +#@ if data.values.service_http_loadbalancer_port or data.values.service_https_loadbalancer_port: --- apiVersion: v1 kind: Service @@ -54,7 +75,16 @@ spec: type: LoadBalancer selector: #@ defaultLabel() ports: - - protocol: TCP - port: #@ data.values.service_loadbalancer_port + #@ if data.values.service_http_loadbalancer_port: + - name: http + protocol: TCP + port: #@ data.values.service_http_loadbalancer_port targetPort: 80 + #@ end + #@ if data.values.service_https_loadbalancer_port: + - name: https + protocol: TCP + port: #@ data.values.service_https_loadbalancer_port + targetPort: 443 + #@ end #@ end diff --git a/deploy/supervisor/values.yaml b/deploy/supervisor/values.yaml index ea06cf30..aea4a04e 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -35,10 +35,15 @@ image_tag: latest #! Optional. image_pull_dockerconfigjson: #! e.g. {"auths":{"https://registry.example.com":{"username":"USERNAME","password":"PASSWORD","auth":"BASE64_ENCODED_USERNAME_COLON_PASSWORD"}}} -#! Specify how to expose the Supervisor app as a Service. -#! Typically you would set a value for only one of the following. +#! Specify how to expose the Supervisor app's HTTP and/or HTTPS ports as a Service. +#! Typically you would set a value for only one of the following service types, for either HTTP or HTTPS depending on your needs. +#! An HTTP service should not be exposed outside the cluster. It would not be secure to serve OIDC endpoints to end users via HTTP. #! Setting any of these values means that a Service of that type will be created. -service_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, e.g. 31234 -service_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_nodeport_port` is specified, e.g. 31234 -service_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, e.g. 443 -service_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, e.g. 443 +service_http_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, with port 80 as its `targetPort`; e.g. 31234 +service_http_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_http_nodeport_port` is specified; e.g. 31234 +service_http_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, with port 80 as its `targetPort`; e.g. 443 +service_http_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, with port 80 as its `targetPort`; e.g. 443 +service_https_nodeport_port: #! when specified, creates a NodePort Service with this `port` value, with port 443 as its `targetPort`; e.g. 31243 +service_https_nodeport_nodeport: #! the `nodePort` value of the NodePort Service, optional when `service_http_nodeport_port` is specified; e.g. 31243 +service_https_loadbalancer_port: #! when specified, creates a LoadBalancer Service with this `port` value, with port 443 as its `targetPort`; e.g. 443 +service_https_clusterip_port: #! when specified, creates a ClusterIP Service with this `port` value, with port 443 as its `targetPort`; e.g. 443 diff --git a/hack/lib/kind-config/single-node.yaml b/hack/lib/kind-config/single-node.yaml index 83c36a58..5247f5b5 100644 --- a/hack/lib/kind-config/single-node.yaml +++ b/hack/lib/kind-config/single-node.yaml @@ -6,7 +6,14 @@ nodes: - protocol: TCP # This same port number is hardcoded in the integration test setup # when creating a Service on a kind cluster. It is used to talk to - # the supervisor app. + # the supervisor app via HTTPS. + containerPort: 31243 + hostPort: 12344 + listenAddress: 127.0.0.1 + - protocol: TCP + # This same port number is hardcoded in the integration test setup + # when creating a Service on a kind cluster. It is used to talk to + # the supervisor app via HTTP. containerPort: 31234 hostPort: 12345 listenAddress: 127.0.0.1 diff --git a/hack/lib/tilt/Tiltfile b/hack/lib/tilt/Tiltfile index 899ea4a5..ebd6e3b8 100644 --- a/hack/lib/tilt/Tiltfile +++ b/hack/lib/tilt/Tiltfile @@ -86,7 +86,7 @@ docker_build_with_restart('image/supervisor', '.', # Render the supervisor installation manifest using ytt. # -# 31234 is the same port number hardcoded in the port forwarding of our kind configuration. +# 31234 and 31243 are the same port numbers hardcoded in the port forwarding of our kind configuration. # Don't think that you can just change this! k8s_yaml(local([ 'ytt', @@ -96,8 +96,10 @@ k8s_yaml(local([ '--data-value', 'image_repo=image/supervisor', '--data-value', 'image_tag=tilt-dev', '--data-value-yaml', 'replicas=1', - '--data-value-yaml', 'service_nodeport_port=80', - '--data-value-yaml', 'service_nodeport_nodeport=31234', + '--data-value-yaml', 'service_http_nodeport_port=80', + '--data-value-yaml', 'service_http_nodeport_nodeport=31234', + '--data-value-yaml', 'service_https_nodeport_port=443', + '--data-value-yaml', 'service_https_nodeport_nodeport=31243', '--data-value-yaml', 'custom_labels={mySupervisorCustomLabelName: mySupervisorCustomLabelValue}', ])) # Tell tilt to watch all of those files for changes. diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 19b3cac4..0e06fe1f 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -119,7 +119,7 @@ if ! tilt_mode; then log_note "Checking for running kind clusters..." if ! kind get clusters | grep -q -e '^pinniped$'; then log_note "Creating a kind cluster..." - # single-node.yaml exposes node port 31234 as 127.0.0.1:12345 and port 31235 as 127.0.0.1:12346 + # single-node.yaml exposes node port 31234 as 127.0.0.1:12345, 31243 as 127.0.0.1:12344, and 31235 as 127.0.0.1:12346 kind create cluster --config "$pinniped_path/hack/lib/kind-config/single-node.yaml" --name pinniped else if ! kubectl cluster-info | grep master | grep -q 127.0.0.1; then @@ -224,8 +224,11 @@ if ! tilt_mode; then --data-value "image_repo=$registry_repo" \ --data-value "image_tag=$tag" \ --data-value-yaml "custom_labels=$supervisor_custom_labels" \ - --data-value-yaml 'service_nodeport_port=80' \ - --data-value-yaml 'service_nodeport_nodeport=31234' >"$manifest" + --data-value-yaml 'service_http_nodeport_port=80' \ + --data-value-yaml 'service_http_nodeport_nodeport=31234' \ + --data-value-yaml 'service_https_nodeport_port=443' \ + --data-value-yaml 'service_https_nodeport_nodeport=31243' \ + >"$manifest" kapp deploy --yes --app "$supervisor_app_name" --diff-changes --file "$manifest" diff --git a/internal/controller/supervisorconfig/jwks_observer.go b/internal/controller/supervisorconfig/jwks_observer.go index c9911b13..bec3cef6 100644 --- a/internal/controller/supervisorconfig/jwks_observer.go +++ b/internal/controller/supervisorconfig/jwks_observer.go @@ -40,7 +40,7 @@ func NewJWKSObserverController( ) controllerlib.Controller { return controllerlib.New( controllerlib.Config{ - Name: "certs-observer-controller", + Name: "jwks-observer-controller", Syncer: &jwksObserverController{ issuerToJWKSSetter: issuerToJWKSSetter, oidcProviderConfigInformer: oidcProviderConfigInformer, diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index 12dc2b63..cb17bb46 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -81,14 +81,13 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er return fmt.Sprintf("%s://%s%s", issuerURL.Scheme, strings.ToLower(issuerURL.Host), issuerURL.Path) } - // Make a map of issuer addresses -> set of unique secret names. This will help us complain when - // multiple OIDCProviderConfigs have the same issuer address (host) component but specify + // Make a map of issuer hostnames -> set of unique secret names. This will help us complain when + // multiple OIDCProviderConfigs have the same issuer hostname (excluding port) but specify // different TLS serving Secrets. Doesn't make sense to have the one address use more than one - // TLS cert. Also make a helper function for forming keys into this map. + // TLS cert. Ignore ports because SNI information on the incoming requests is not going to include + // port numbers. Also make a helper function for forming keys into this map. uniqueSecretNamesPerIssuerAddress := make(map[string]map[string]bool) - issuerURLToHostKey := func(issuerURL *url.URL) string { - return strings.ToLower(issuerURL.Host) - } + issuerURLToHostnameKey := lowercaseHostWithoutPort for _, opc := range all { issuerURL, err := url.Parse(opc.Spec.Issuer) @@ -98,10 +97,10 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er issuerCounts[issuerURLToIssuerKey(issuerURL)]++ - setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] + setOfSecretNames := uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] if setOfSecretNames == nil { setOfSecretNames = make(map[string]bool) - uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)] = setOfSecretNames + uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] = setOfSecretNames } setOfSecretNames[opc.Spec.SecretName] = true } @@ -129,13 +128,13 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er } // Skip url parse errors because they will be validated below. - if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostKey(issuerURL)]) > 1 { + if urlParseErr == nil && len(uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)]) > 1 { if err := c.updateStatus( ctx.Context, opc.Namespace, opc.Name, configv1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus, - "Issuers with the same address must use the same secretName: "+issuerURLToHostKey(issuerURL), + "Issuers with the same DNS hostname (address not including port) must use the same secretName: "+issuerURLToHostnameKey(issuerURL), ); err != nil { errs.Add(fmt.Errorf("could not update status: %w", err)) } diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go index 1028b324..7efeffe4 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go @@ -807,7 +807,7 @@ func TestSync(t *testing.T) { }) }) - when("there are OIDCProviderConfigs with the same issuer address using different secretNames", func() { + when("there are OIDCProviderConfigs with the same issuer DNS hostname using different secretNames", func() { var ( oidcProviderConfigSameIssuerAddress1 *v1alpha1.OIDCProviderConfig oidcProviderConfigSameIssuerAddress2 *v1alpha1.OIDCProviderConfig @@ -828,7 +828,9 @@ func TestSync(t *testing.T) { oidcProviderConfigSameIssuerAddress2 = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "provider2", Namespace: namespace}, Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: "https://issuer-duplicate-address.com/path2", + // Validation treats these as the same DNS hostname even though they have different port numbers, + // because SNI information on the incoming requests is not going to include port numbers. + Issuer: "https://issuer-duplicate-address.com:1234/path2", SecretName: "secret2", }, } @@ -888,11 +890,11 @@ func TestSync(t *testing.T) { oidcProviderConfigDifferentIssuerAddress.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigSameIssuerAddress1.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus - oidcProviderConfigSameIssuerAddress1.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress1.Status.Message = "Issuers with the same DNS hostname (address not including port) must use the same secretName: issuer-duplicate-address.com" oidcProviderConfigSameIssuerAddress1.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigSameIssuerAddress2.Status.Status = v1alpha1.SameIssuerHostMustUseSameSecretOIDCProviderStatus - oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same address must use the same secretName: issuer-duplicate-address.com" + oidcProviderConfigSameIssuerAddress2.Status.Message = "Issuers with the same DNS hostname (address not including port) must use the same secretName: issuer-duplicate-address.com" oidcProviderConfigSameIssuerAddress2.Status.LastUpdateTime = timePtr(metav1.NewTime(frozenNow)) oidcProviderConfigWithInvalidIssuerURL.Status.Status = v1alpha1.InvalidOIDCProviderStatus diff --git a/internal/controller/supervisorconfig/testdata/test.crt b/internal/controller/supervisorconfig/testdata/test.crt new file mode 100644 index 00000000..e54883b0 --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test.crt @@ -0,0 +1,19 @@ +-----BEGIN CERTIFICATE----- +MIIDBjCCAe4CCQDDx1zebLLuzzANBgkqhkiG9w0BAQsFADBFMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCQ0ExFjAUBgNVBAcMDVNhbiBGcmFuY2lzY28xETAPBgNVBAoM +CFBpbm5pcGVkMB4XDTIwMTAyNjE2MzcyOVoXDTIxMTAyNjE2MzcyOVowRTELMAkG +A1UEBhMCVVMxCzAJBgNVBAgMAkNBMRYwFAYDVQQHDA1TYW4gRnJhbmNpc2NvMREw +DwYDVQQKDAhQaW5uaXBlZDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCCAQoCggEB +AJik4mXLVHEIGTK679gjNZNFsutcFGhwCg6WTPy+EAEUjGOUEI/Ca7JAnZGGSZpD +bxnWdXwSt+k7taMWzZIiCosXnvrFmlyCO4wlcajDIOTauG6DKop+S2NjydZxuwUR +G1fb2zXm6Kh3dqwbSzCM7i4pPTEhXJLI04fX6gxyETUGr+rs/p44KtELzSU9NKmP +KUyf8wtoSCz00HYu1auV1px/I1JaKdubx9c5zpr93gJDF2euVV5yaLr1BoRr3UVB +Y5Qa0UWPYCWcTvXyeAku4h4yT6B9iZP/reZfpHSmBxSAPrv4Y8oUqal+i92R77WJ +EkBRm5lVym7l/st3iTmpMlsCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEAd+P3Dfkz +REzsdzja0wYb10q1vggAyMtxhvQdG6kND8esWAki/nAgVnXxIq4Eg0Jnanq9SS2Q +Ab6zpRelEB5YeDPZ7Xm6ApLBxqoEciPNqPARK2YJUPFyZJgntsLBeKKojLVE2KqY +DB8ZxKcmh7NPF4KVL3DSoWGwl4UkZt06R+VfxSSuOm/HtxPmdrz5fR6fNYjb4ss8 +sYY6wJTAILzGxpkhiWGXpE6VgdD5qh6+SevRuynHFKiTQ9L1T5aiAEC55VSizmcT +MRkiZHBMQ5pCaPppnaqahWZ757fdk853miG9ckZ58lq7ccCqU0FlaMwf5jjuMb49 +rM1zqYxgeIqwvw== +-----END CERTIFICATE----- diff --git a/internal/controller/supervisorconfig/testdata/test.key b/internal/controller/supervisorconfig/testdata/test.key new file mode 100644 index 00000000..e29ea462 --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCYpOJly1RxCBky +uu/YIzWTRbLrXBRocAoOlkz8vhABFIxjlBCPwmuyQJ2RhkmaQ28Z1nV8ErfpO7Wj +Fs2SIgqLF576xZpcgjuMJXGowyDk2rhugyqKfktjY8nWcbsFERtX29s15uiod3as +G0swjO4uKT0xIVySyNOH1+oMchE1Bq/q7P6eOCrRC80lPTSpjylMn/MLaEgs9NB2 +LtWrldacfyNSWinbm8fXOc6a/d4CQxdnrlVecmi69QaEa91FQWOUGtFFj2AlnE71 +8ngJLuIeMk+gfYmT/63mX6R0pgcUgD67+GPKFKmpfovdke+1iRJAUZuZVcpu5f7L +d4k5qTJbAgMBAAECggEBAJFM4vVjB45Q1yujJovneCgoQJgpnoOLowcfq0kq4rEk +jj57wwgVWc7kExljasydRDSkIFFqwAYUAGKuYiCopsCgS4UKdFV64pQVUIwEslsm +mEkaMnSCo+CILKkkuZGpJw4LCi/VDcLPdPd/Q6ODg3YNa2JJD4XqBPFaZkBSlG6T +5pE++Lwno0t9CWprvE1i/gxMT9CzN0XsBCFZMirqImqpunRs/T7X2uxERXxE+eCY +bi/Q9yGbYVQzuhVyR/aHfuvOfsqrTpdEybQWzgzOwbuX0jgGv88AmcDgYm/d287K +chFHUrpGEJbLIFHv1/XHeVUEB0WXJxK5GO3J34i8DoECgYEAyibL+ZD197Ugn2RB +O1n7tqSWcRR3RlOC83CGdYFkyLc93UOxs4XUGo/xnXA7/2eyNlb8E+W7Z0G1oGln +0+ZkUjZHnJr1d/5uvZILM5/V8JhI2Tz11+gVbnbdKBj+1tdcxqZCuCZX0jkVTdOZ +/i4lDD53oJitWuvM3DkjiRFeV2UCgYEAwU4NrOSuNYaLsSuXbJGQpLDK86FxRRx3 +zAcMKUIKZItRzGab6iwyl+w3xTkouu2+tp3MbLrBoVortWHw8NgR9zibjM6r7L0P +n7uexa1RAT0O5XuGJtjjlJZ4CFbsrBVTAHlGKr8lhXIBJTt5UINBsCQivp+fQdbg +fXq+1kHJJr8CgYB11SGGimnlhq3KWwzvBKeFsfCDX5Oa6ajmL8wgiFjv6mfkJsZZ +R4P4K7mBtN80JASsSg3Lp1iSeqndJDPCP4Rwq3UYova8iBGS7KMc52k0QgAMqM0A +miaL6jtFWTSKlKReoqE3aBo+zslNQS99CvbLaUof0X8TBWm3YJMHHZmpRQKBgQCr +0H+xO/VoF/XT/QXzdxLUf1t03vs5zYrhayYxCcUJBxgmkNFme/BgPpJ3l02PkL+h +u3I29mwiyW3uI2av+61ESylfJ1eC7ayUcoQ1+c31RtsVuAxOPRtTN8bqyrBEaBPF +aQWn+wwTp3hDKrCykmfxcrz7KA+6yo3wmghDkmeDKwKBgCrWwlhVKtigrBKlkraZ +iD+Inq/KBpjgdimJMQDrRFusX6Rp35k1nvRsFVt+JETiDQij/hKqYecbkkWu9wTd +h7F6q0i2Y4yZaiiagK7SSDBd1quVd0+yJ5u3TgTcMVqB17iTCN4YlEEnaCi/S0dM +LlmHgtyFDi6BxB1lheKDewSo +-----END PRIVATE KEY----- diff --git a/internal/controller/supervisorconfig/testdata/test2.crt b/internal/controller/supervisorconfig/testdata/test2.crt new file mode 100644 index 00000000..7b17500b --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test2.crt @@ -0,0 +1,18 @@ +-----BEGIN CERTIFICATE----- +MIIC/DCCAeQCCQC8V+VeJZWyljANBgkqhkiG9w0BAQsFADBAMQswCQYDVQQGEwJV +UzELMAkGA1UECAwCTlkxETAPBgNVBAcMCE5ldyBZb3JrMREwDwYDVQQKDAhQaW5u +aXBlZDAeFw0yMDEwMjYxNjM5MDlaFw0yMTEwMjYxNjM5MDlaMEAxCzAJBgNVBAYT +AlVTMQswCQYDVQQIDAJOWTERMA8GA1UEBwwITmV3IFlvcmsxETAPBgNVBAoMCFBp +bm5pcGVkMIIBIjANBgkqhkiG9w0BAQEFAAOCAQ8AMIIBCgKCAQEArk4nLgjs0wve +NW8SZ1N0HP0jeQAjqO017bDC6d+jGu8Gl39HdojMLol7EXQz9zlT6Of+uerGHnDr +8xbE3juLrKCihDW7w8nENUBlA+NEEc8yvn3YqMJXMIXbPgWTch+PsEWTi5xWoZEs +JBuLSb9oI2OuzYy5XvT5TWeaLAmeJUO2IQR6UJ6oTGE4nOC6kqJM4Kx8xMKAn7yf +OlKF3vhVmlL+5e9+lpIJN18nDjORATtbZPJPLKELJGVT9repCNuQMuemDnABJWb4 +NPQK8pzZ/hrW/1LdwFnCn6zu0ZlZVzUy6lWl0Hqux1KeMhYjut9nkKMKixCX6nWc +g+Y2X1VO5QIDAQABMA0GCSqGSIb3DQEBCwUAA4IBAQBfLq+20Yq24ad9ENh7Wkz0 +W7teg0GN7kNZRuNIRHgXyhsZ/HAI9etfOv4rt/ynyxlnx5j7HRM+Izc6fG3qEQnp +N1RRsHw2As2ilr62g7hHuuEg75/sWDzO1Z7/LjNp3uTE+2Jqb6P+rga2vO1gg8pb ++A8AmU3BmvhOwb2RjaZQvOZGOcWkEZjpJp5mPGptMsl4BzA4kBp4kpzoBBUA34/i +CRP68yTclnKmKvhI0Yg3QlihD8SjMWWa9UUZYGaKE0H8EGHxRldfqGBdjqs6RaaK +xdDFUULvpOFC0lDruWZ/YztiVi3iH2zEiGnxD4OwfapmjhlLcCDlNsNkaHh9UigQ +-----END CERTIFICATE----- diff --git a/internal/controller/supervisorconfig/testdata/test2.key b/internal/controller/supervisorconfig/testdata/test2.key new file mode 100644 index 00000000..042bbceb --- /dev/null +++ b/internal/controller/supervisorconfig/testdata/test2.key @@ -0,0 +1,28 @@ +-----BEGIN PRIVATE KEY----- +MIIEvgIBADANBgkqhkiG9w0BAQEFAASCBKgwggSkAgEAAoIBAQCuTicuCOzTC941 +bxJnU3Qc/SN5ACOo7TXtsMLp36Ma7waXf0d2iMwuiXsRdDP3OVPo5/656sYecOvz +FsTeO4usoKKENbvDycQ1QGUD40QRzzK+fdiowlcwhds+BZNyH4+wRZOLnFahkSwk +G4tJv2gjY67NjLle9PlNZ5osCZ4lQ7YhBHpQnqhMYTic4LqSokzgrHzEwoCfvJ86 +UoXe+FWaUv7l736Wkgk3XycOM5EBO1tk8k8soQskZVP2t6kI25Ay56YOcAElZvg0 +9ArynNn+Gtb/Ut3AWcKfrO7RmVlXNTLqVaXQeq7HUp4yFiO632eQowqLEJfqdZyD +5jZfVU7lAgMBAAECggEBAKSOFcEJHgN0bdjGPnqbt7/yX33JWuEM6N+4A5tlzQcN +Z4y41Y+bQCAjHLNyn+ijD4uPEdUVRurQMoDxGvSvBIL5t9PXIqeJIRog6/zKnqWt +lbtu9Y8EwemGRV/9RaD1GOMSHGQuOT8Y3bJM6qe58yeN4SYe15ZE8eNYjp1Kiymj +fzSlaMHM/yi8IKL9QtT7Bb9Fclfu8AB3gQM25mCisd+xMFqsg2CWf7zVUDkp8tBi +LkfhJhbGmjpUcQbZGR+bAssBLXh4vBJhLWvfqSDLLD/+pavzcYWHv2XpBzY0VWyq +t4762L6azYuRVFwyeTYTt9rsbVIK7ZOpOxGdeSd318ECgYEA1oatFot7WaFoMp8A +v/7GSNm/txS0um551yt5Ugi7Iv4w/vcsrKHgRXa3ADmT6efFAwePWJTvG1N+iwOU +I+dQBMB4BDMdJAfY4fccOF6J3GNBGgz2DFEhjizcKGu/knot7C9P5vWiSh1U76TZ +e1oqku6AX7/TM3Dia6QbNM7RFtECgYEA0ADenD1e2i2FvSkTkmHrMIjRKt9FesHl +9sPN7dnAaaBi0/7gr3fan11h2DreQV2r29JQhZxFx2EO7DjR+9qFnSO2oueula/X +HfhxKu12aOKMSoSNoL4oBuk5iy/vWh0fwO7S8/a48k99JGXX2Lct4bfR0y3yRFO+ +zHM8T+6C49UCgYEAljlnGgOA1Gov8krgFpLNvZQmKYmpaWgVkDTUVzrf+QgxvUnP +kfAlgd85FUI8ry5rCs0Pd4OL0QHt+mD+Kwo/QaSaJq64eFO6b7pAm8SwG5GxtBFh +d4yUx9/oJ7IUS/mdEOistlpKVEYoBUzWMwgYCh5T7TkCJ+Kj26bmmls9lhECgYBm +24c5i7+T9F7mI6HiCTncTkvg/3fENI4bcMgsjjlwAjfczXUeUA50MCFqY/H0MPYD +RgU7jQOUjJJsjcyI1o6sHjT6accTjli6IVkU+UhMpXrqfpHqox34DOy/v3yE+1Hw +fikjKyZZ7KTdkt8h87NkoxnHbDkZQLBhObrha/id4QKBgBs0Rr+BAOM78MVocPFO +RPuagzFFaU59rOqlPOaRMDZ2jfokC09Maouchc2lJCX9ip4LwdqxiQw8GFRjZEz1 +l+UZd7vAPQUtWQeACYMnmJ2CVNXZhghBQ4ltYTtEJwvB5420bcW7MBSrOYNBCsPr +gDqlX93KhuFxucNTnZj3dbNo +-----END PRIVATE KEY----- diff --git a/internal/controller/supervisorconfig/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go new file mode 100644 index 00000000..5d382bbf --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -0,0 +1,101 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "crypto/tls" + "fmt" + "net/url" + "strings" + + "k8s.io/apimachinery/pkg/labels" + corev1informers "k8s.io/client-go/informers/core/v1" + "k8s.io/klog/v2" + + "go.pinniped.dev/generated/1.19/client/informers/externalversions/config/v1alpha1" + pinnipedcontroller "go.pinniped.dev/internal/controller" + "go.pinniped.dev/internal/controllerlib" +) + +type tlsCertObserverController struct { + issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer +} + +type IssuerHostToTLSCertMapSetter interface { + SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) +} + +func NewTLSCertObserverController( + issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter, + secretInformer corev1informers.SecretInformer, + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, + withInformer pinnipedcontroller.WithInformerOptionFunc, +) controllerlib.Controller { + return controllerlib.New( + controllerlib.Config{ + Name: "tls-certs-observer-controller", + Syncer: &tlsCertObserverController{ + issuerHostToTLSCertMapSetter: issuerHostToTLSCertMapSetter, + oidcProviderConfigInformer: oidcProviderConfigInformer, + secretInformer: secretInformer, + }, + }, + withInformer( + secretInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + withInformer( + oidcProviderConfigInformer, + pinnipedcontroller.MatchAnythingFilter(), + controllerlib.InformerOption{}, + ), + ) +} + +func (c *tlsCertObserverController) Sync(ctx controllerlib.Context) error { + ns := ctx.Key.Namespace + allProviders, err := c.oidcProviderConfigInformer.Lister().OIDCProviderConfigs(ns).List(labels.Everything()) + if err != nil { + return fmt.Errorf("failed to list OIDCProviderConfigs: %w", err) + } + + // Rebuild the whole map on any change to any Secret or OIDCProvider, because either can have changes that + // can cause the map to need to be updated. + issuerHostToTLSCertMap := map[string]*tls.Certificate{} + + for _, provider := range allProviders { + secretName := provider.Spec.SecretName + issuerURL, err := url.Parse(provider.Spec.Issuer) + if err != nil { + klog.InfoS("tlsCertObserverController Sync found an invalid issuer URL", "namespace", ns, "issuer", provider.Spec.Issuer) + continue + } + tlsSecret, err := c.secretInformer.Lister().Secrets(ns).Get(secretName) + if err != nil { + klog.InfoS("tlsCertObserverController Sync could not find TLS cert secret", "namespace", ns, "secretName", secretName) + continue + } + certFromSecret, err := tls.X509KeyPair(tlsSecret.Data["tls.crt"], tlsSecret.Data["tls.key"]) + if err != nil { + klog.InfoS("tlsCertObserverController Sync found a TLS secret with Data in an unexpected format", "namespace", ns, "secretName", secretName) + continue + } + // Lowercase the host part of the URL because hostnames should be treated as case-insensitive. + issuerHostToTLSCertMap[lowercaseHostWithoutPort(issuerURL)] = &certFromSecret + } + + klog.InfoS("tlsCertObserverController Sync updated the TLS cert cache", "issuerHostCount", len(issuerHostToTLSCertMap)) + c.issuerHostToTLSCertMapSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) + + return nil +} + +func lowercaseHostWithoutPort(issuerURL *url.URL) string { + lowercaseHost := strings.ToLower(issuerURL.Host) + colonSegments := strings.Split(lowercaseHost, ":") + return colonSegments[0] +} diff --git a/internal/controller/supervisorconfig/tls_cert_observer_test.go b/internal/controller/supervisorconfig/tls_cert_observer_test.go new file mode 100644 index 00000000..6510b732 --- /dev/null +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -0,0 +1,304 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package supervisorconfig + +import ( + "context" + "crypto/tls" + "io/ioutil" + "net/url" + "testing" + "time" + + "github.com/sclevine/spec" + "github.com/sclevine/spec/report" + "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" + kubeinformers "k8s.io/client-go/informers" + kubernetesfake "k8s.io/client-go/kubernetes/fake" + + "go.pinniped.dev/generated/1.19/apis/config/v1alpha1" + pinnipedfake "go.pinniped.dev/generated/1.19/client/clientset/versioned/fake" + pinnipedinformers "go.pinniped.dev/generated/1.19/client/informers/externalversions" + "go.pinniped.dev/internal/controllerlib" + "go.pinniped.dev/internal/testutil" +) + +func TestTLSCertObserverControllerInformerFilters(t *testing.T) { + spec.Run(t, "informer filters", func(t *testing.T, when spec.G, it spec.S) { + var ( + r *require.Assertions + observableWithInformerOption *testutil.ObservableWithInformerOption + secretsInformerFilter controllerlib.Filter + oidcProviderConfigInformerFilter controllerlib.Filter + ) + + it.Before(func() { + r = require.New(t) + observableWithInformerOption = testutil.NewObservableWithInformerOption() + secretsInformer := kubeinformers.NewSharedInformerFactory(nil, 0).Core().V1().Secrets() + oidcProviderConfigInformer := pinnipedinformers.NewSharedInformerFactory(nil, 0).Config().V1alpha1().OIDCProviderConfigs() + _ = NewTLSCertObserverController( + nil, + secretsInformer, + oidcProviderConfigInformer, + observableWithInformerOption.WithInformer, // make it possible to observe the behavior of the Filters + ) + secretsInformerFilter = observableWithInformerOption.GetFilterForInformer(secretsInformer) + oidcProviderConfigInformerFilter = observableWithInformerOption.GetFilterForInformer(oidcProviderConfigInformer) + }) + + when("watching Secret objects", func() { + var ( + subject controllerlib.Filter + secret, otherSecret *corev1.Secret + ) + + it.Before(func() { + subject = secretsInformerFilter + secret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherSecret = &corev1.Secret{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any Secret changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(secret)) + r.True(subject.Update(secret, otherSecret)) + r.True(subject.Update(otherSecret, secret)) + r.True(subject.Delete(secret)) + }) + }) + }) + + when("watching OIDCProviderConfig objects", func() { + var ( + subject controllerlib.Filter + provider, otherProvider *v1alpha1.OIDCProviderConfig + ) + + it.Before(func() { + subject = oidcProviderConfigInformerFilter + provider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-name", Namespace: "any-namespace"}} + otherProvider = &v1alpha1.OIDCProviderConfig{ObjectMeta: metav1.ObjectMeta{Name: "any-other-name", Namespace: "any-other-namespace"}} + }) + + when("any OIDCProviderConfig changes", func() { + it("returns true to trigger the sync method", func() { + r.True(subject.Add(provider)) + r.True(subject.Update(provider, otherProvider)) + r.True(subject.Update(otherProvider, provider)) + r.True(subject.Delete(provider)) + }) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} + +type fakeIssuerHostToTLSCertMapSetter struct { + setIssuerHostToTLSCertMapWasCalled bool + issuerHostToTLSCertMapReceived map[string]*tls.Certificate +} + +func (f *fakeIssuerHostToTLSCertMapSetter) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) { + f.setIssuerHostToTLSCertMapWasCalled = true + f.issuerHostToTLSCertMapReceived = issuerHostToTLSCertMap +} + +func TestTLSCertObserverControllerSync(t *testing.T) { + spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { + const installedInNamespace = "some-namespace" + + var ( + r *require.Assertions + subject controllerlib.Controller + pinnipedInformerClient *pinnipedfake.Clientset + kubeInformerClient *kubernetesfake.Clientset + pinnipedInformers pinnipedinformers.SharedInformerFactory + kubeInformers kubeinformers.SharedInformerFactory + timeoutContext context.Context + timeoutContextCancel context.CancelFunc + syncContext *controllerlib.Context + issuerHostToTLSCertSetter *fakeIssuerHostToTLSCertMapSetter + ) + + // Defer starting the informers until the last possible moment so that the + // nested Before's can keep adding things to the informer caches. + var startInformersAndController = func() { + // Set this at the last second to allow for injection of server override. + subject = NewTLSCertObserverController( + issuerHostToTLSCertSetter, + kubeInformers.Core().V1().Secrets(), + pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), + controllerlib.WithInformer, + ) + + // Set this at the last second to support calling subject.Name(). + syncContext = &controllerlib.Context{ + Context: timeoutContext, + Name: subject.Name(), + Key: controllerlib.Key{ + Namespace: installedInNamespace, + Name: "any-name", + }, + } + + // Must start informers before calling TestRunSynchronously() + kubeInformers.Start(timeoutContext.Done()) + pinnipedInformers.Start(timeoutContext.Done()) + controllerlib.TestRunSynchronously(t, subject) + } + + var readTestFile = func(path string) []byte { + data, err := ioutil.ReadFile(path) + r.NoError(err) + return data + } + + it.Before(func() { + r = require.New(t) + + timeoutContext, timeoutContextCancel = context.WithTimeout(context.Background(), time.Second*3) + + kubeInformerClient = kubernetesfake.NewSimpleClientset() + kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) + pinnipedInformerClient = pinnipedfake.NewSimpleClientset() + pinnipedInformers = pinnipedinformers.NewSharedInformerFactory(pinnipedInformerClient, 0) + issuerHostToTLSCertSetter = &fakeIssuerHostToTLSCertMapSetter{} + + unrelatedSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{ + Name: "some other unrelated secret", + Namespace: installedInNamespace, + }, + } + r.NoError(kubeInformerClient.Tracker().Add(unrelatedSecret)) + }) + + it.After(func() { + timeoutContextCancel() + }) + + when("there are no OIDCProviderConfigs and no TLS Secrets yet", func() { + it("sets the issuerHostToTLSCertSetter's map to be empty", func() { + startInformersAndController() + err := controllerlib.TestSync(t, subject, *syncContext) + r.NoError(err) + + r.True(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Empty(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived) + }) + }) + + when("there are OIDCProviderConfigs where some have corresponding TLS Secrets and some don't", func() { + var ( + expectedCertificate1, expectedCertificate2 tls.Certificate + ) + + it.Before(func() { + var err error + oidcProviderConfigWithoutSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer1.com"}, // no SecretName field + } + oidcProviderConfigWithoutSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "no-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer2.com", SecretName: ""}, + } + oidcProviderConfigWithBadSecret := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-secret-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://bad-secret-issuer.com", SecretName: "bad-tls-secret-name"}, + } + // Also add one with a URL that cannot be parsed to make sure that the controller is not confused by invalid URLs. + invalidIssuerURL := ":/host//path" + _, err = url.Parse(invalidIssuerURL) //nolint:staticcheck // Yes, this URL is intentionally invalid. + r.Error(err) + oidcProviderConfigWithBadIssuer := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "bad-issuer-oidcproviderconfig", + Namespace: installedInNamespace, + }, + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: invalidIssuerURL}, + } + oidcProviderConfigWithGoodSecret1 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig1", + Namespace: installedInNamespace, + }, + // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test without a port number. + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.iSSuer-wiTh-goOd-secRet1.cOm/path", SecretName: "good-tls-secret-name1"}, + } + oidcProviderConfigWithGoodSecret2 := &v1alpha1.OIDCProviderConfig{ + ObjectMeta: metav1.ObjectMeta{ + Name: "good-secret-oidcproviderconfig2", + Namespace: installedInNamespace, + }, + // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test with a port number. + Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.issUEr-WIth-gOOd-seCret2.com:1234/path", SecretName: "good-tls-secret-name2"}, + } + testCrt1 := readTestFile("testdata/test.crt") + r.NotEmpty(testCrt1) + testCrt2 := readTestFile("testdata/test2.crt") + r.NotEmpty(testCrt2) + testKey1 := readTestFile("testdata/test.key") + r.NotEmpty(testKey1) + testKey2 := readTestFile("testdata/test2.key") + r.NotEmpty(testKey2) + expectedCertificate1, err = tls.X509KeyPair(testCrt1, testKey1) + r.NoError(err) + expectedCertificate2, err = tls.X509KeyPair(testCrt2, testKey2) + r.NoError(err) + goodTLSSecret1 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "good-tls-secret-name1", Namespace: installedInNamespace}, + Data: map[string][]byte{"tls.crt": testCrt1, "tls.key": testKey1}, + } + goodTLSSecret2 := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "good-tls-secret-name2", Namespace: installedInNamespace}, + Data: map[string][]byte{"tls.crt": testCrt2, "tls.key": testKey2}, + } + badTLSSecret := &corev1.Secret{ + ObjectMeta: metav1.ObjectMeta{Name: "bad-tls-secret-name", Namespace: installedInNamespace}, + Data: map[string][]byte{"junk": nil}, + } + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithoutSecret2)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithBadSecret)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithBadIssuer)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret1)) + r.NoError(pinnipedInformerClient.Tracker().Add(oidcProviderConfigWithGoodSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(goodTLSSecret1)) + r.NoError(kubeInformerClient.Tracker().Add(goodTLSSecret2)) + r.NoError(kubeInformerClient.Tracker().Add(badTLSSecret)) + }) + + it("updates the issuerHostToTLSCertSetter's map to include only the issuers that had valid certs", func() { + startInformersAndController() + r.NoError(controllerlib.TestSync(t, subject, *syncContext)) + + r.True(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Len(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived, 2) + + // They keys in the map should be lower case and should not include the port numbers, because + // TLS SNI says that SNI hostnames must be DNS names (not ports) and must be case insensitive. + // See https://tools.ietf.org/html/rfc3546#section-3.1 + actualCertificate1 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret1.com"] + r.NotNil(actualCertificate1) + // The actual cert should match the one from the test fixture that was put into the secret. + r.Equal(expectedCertificate1, *actualCertificate1) + actualCertificate2 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret2.com"] + r.NotNil(actualCertificate2) + r.Equal(expectedCertificate2, *actualCertificate2) + }) + }) + }, spec.Parallel(), spec.Report(report.Terminal{})) +} diff --git a/internal/oidc/provider/dynamic_tls_cert_provider.go b/internal/oidc/provider/dynamic_tls_cert_provider.go new file mode 100644 index 00000000..51e3c157 --- /dev/null +++ b/internal/oidc/provider/dynamic_tls_cert_provider.go @@ -0,0 +1,37 @@ +// Copyright 2020 the Pinniped contributors. All Rights Reserved. +// SPDX-License-Identifier: Apache-2.0 + +package provider + +import ( + "crypto/tls" + "sync" +) + +type DynamicTLSCertProvider interface { + SetIssuerHostToTLSCertMap(issuerToJWKSMap map[string]*tls.Certificate) + GetTLSCert(lowercaseIssuerHostName string) *tls.Certificate +} + +type dynamicTLSCertProvider struct { + issuerHostToTLSCertMap map[string]*tls.Certificate + mutex sync.RWMutex +} + +func NewDynamicTLSCertProvider() DynamicTLSCertProvider { + return &dynamicTLSCertProvider{ + issuerHostToTLSCertMap: map[string]*tls.Certificate{}, + } +} + +func (p *dynamicTLSCertProvider) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) { + p.mutex.Lock() // acquire a write lock + defer p.mutex.Unlock() + p.issuerHostToTLSCertMap = issuerHostToTLSCertMap +} + +func (p *dynamicTLSCertProvider) GetTLSCert(issuerHostName string) *tls.Certificate { + p.mutex.RLock() // acquire a read lock + defer p.mutex.RUnlock() + return p.issuerHostToTLSCertMap[issuerHostName] +} From eeb110761ee34595712167727a62c7bb7884e0b5 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 26 Oct 2020 17:25:45 -0700 Subject: [PATCH 03/17] Rename `secretName` to `SNICertificateSecretName` in OIDCProviderConfig --- .../v1alpha1/types_oidcproviderconfig.go.tmpl | 25 ++++++++++++---- ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 29 ++++++++++++------- generated/1.17/README.adoc | 6 +++- .../v1alpha1/types_oidcproviderconfig.go | 25 ++++++++++++---- .../client/openapi/zz_generated.openapi.go | 4 +-- ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 29 ++++++++++++------- generated/1.18/README.adoc | 6 +++- .../v1alpha1/types_oidcproviderconfig.go | 25 ++++++++++++---- .../client/openapi/zz_generated.openapi.go | 4 +-- ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 29 ++++++++++++------- generated/1.19/README.adoc | 6 +++- .../v1alpha1/types_oidcproviderconfig.go | 25 ++++++++++++---- .../client/openapi/zz_generated.openapi.go | 4 +-- ...nfig.pinniped.dev_oidcproviderconfigs.yaml | 29 ++++++++++++------- .../oidcproviderconfig_watcher.go | 2 +- .../oidcproviderconfig_watcher_test.go | 16 +++++----- .../supervisorconfig/tls_cert_observer.go | 2 +- .../tls_cert_observer_test.go | 22 ++++++++++---- 18 files changed, 200 insertions(+), 88 deletions(-) diff --git a/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl b/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl index 38c2f0b2..a2dc9cbc 100644 --- a/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl +++ b/apis/config/v1alpha1/types_oidcproviderconfig.go.tmpl @@ -31,14 +31,27 @@ type OIDCProviderConfigSpec struct { // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` - // SecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // 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. - // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside - // the cluster using a LoadBalancer Service), and is not required when you would like to use only the - // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named - // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // 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 - SecretName string `json:"secretName,omitempty"` + SNICertificateSecretName string `json:"sniCertificateSecretName,omitempty"` } // OIDCProviderConfigStatus is a struct that describes the actual state of an OIDC Provider. diff --git a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml index 528f6727..e8330ab3 100644 --- a/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/deploy/supervisor/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,16 +49,25 @@ spec: for more information." minLength: 1 type: string - secretName: - description: SecretName 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. - SecretName is required if you would like to use the HTTPS endpoints - (e.g. when exposing them outside the cluster using a LoadBalancer - Service), and is not required when you would like to use only the - HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS - secret must contain keys named `tls.crt` and `tls.key` that contain - the certificate and private key to use for TLS. + 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 diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index a43ac459..b6da2c0c 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -132,7 +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. -| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. +| *`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 38c2f0b2..a2dc9cbc 100644 --- a/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.17/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -31,14 +31,27 @@ type OIDCProviderConfigSpec struct { // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` - // SecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // 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. - // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside - // the cluster using a LoadBalancer Service), and is not required when you would like to use only the - // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named - // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // 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 - SecretName string `json:"secretName,omitempty"` + 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 8ae4db21..e8028d0f 100644 --- a/generated/1.17/client/openapi/zz_generated.openapi.go +++ b/generated/1.17/client/openapi/zz_generated.openapi.go @@ -397,9 +397,9 @@ func schema_117_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, - "secretName": { + "sniCertificateSecretName": { SchemaProps: spec.SchemaProps{ - Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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: "", }, diff --git a/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 528f6727..e8330ab3 100644 --- a/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.17/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,16 +49,25 @@ spec: for more information." minLength: 1 type: string - secretName: - description: SecretName 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. - SecretName is required if you would like to use the HTTPS endpoints - (e.g. when exposing them outside the cluster using a LoadBalancer - Service), and is not required when you would like to use only the - HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS - secret must contain keys named `tls.crt` and `tls.key` that contain - the certificate and private key to use for TLS. + 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 diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 0a6f30fe..5b3892d1 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -132,7 +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. -| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. +| *`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 38c2f0b2..a2dc9cbc 100644 --- a/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.18/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -31,14 +31,27 @@ type OIDCProviderConfigSpec struct { // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` - // SecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // 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. - // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside - // the cluster using a LoadBalancer Service), and is not required when you would like to use only the - // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named - // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // 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 - SecretName string `json:"secretName,omitempty"` + 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 85a105ef..737854c0 100644 --- a/generated/1.18/client/openapi/zz_generated.openapi.go +++ b/generated/1.18/client/openapi/zz_generated.openapi.go @@ -397,9 +397,9 @@ func schema_118_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, - "secretName": { + "sniCertificateSecretName": { SchemaProps: spec.SchemaProps{ - Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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: "", }, diff --git a/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 528f6727..e8330ab3 100644 --- a/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.18/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,16 +49,25 @@ spec: for more information." minLength: 1 type: string - secretName: - description: SecretName 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. - SecretName is required if you would like to use the HTTPS endpoints - (e.g. when exposing them outside the cluster using a LoadBalancer - Service), and is not required when you would like to use only the - HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS - secret must contain keys named `tls.crt` and `tls.key` that contain - the certificate and private key to use for TLS. + 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 diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index fa454528..49ed2bcf 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -132,7 +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. -| *`secretName`* __string__ | SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. +| *`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 38c2f0b2..a2dc9cbc 100644 --- a/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go +++ b/generated/1.19/apis/config/v1alpha1/types_oidcproviderconfig.go @@ -31,14 +31,27 @@ type OIDCProviderConfigSpec struct { // +kubebuilder:validation:MinLength=1 Issuer string `json:"issuer"` - // SecretName is an optional name of a Secret in the same namespace, of type `kubernetes.io/tls`, + // 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. - // SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside - // the cluster using a LoadBalancer Service), and is not required when you would like to use only the - // HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named - // `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS. + // 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 - SecretName string `json:"secretName,omitempty"` + 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 015721f8..66e21e84 100644 --- a/generated/1.19/client/openapi/zz_generated.openapi.go +++ b/generated/1.19/client/openapi/zz_generated.openapi.go @@ -398,9 +398,9 @@ func schema_119_apis_config_v1alpha1_OIDCProviderConfigSpec(ref common.Reference Format: "", }, }, - "secretName": { + "sniCertificateSecretName": { SchemaProps: spec.SchemaProps{ - Description: "SecretName 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. SecretName is required if you would like to use the HTTPS endpoints (e.g. when exposing them outside the cluster using a LoadBalancer Service), and is not required when you would like to use only the HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS secret must contain keys named `tls.crt` and `tls.key` that contain the certificate and private key to use for TLS.", + 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: "", }, diff --git a/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml b/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml index 528f6727..e8330ab3 100644 --- a/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml +++ b/generated/1.19/crds/config.pinniped.dev_oidcproviderconfigs.yaml @@ -49,16 +49,25 @@ spec: for more information." minLength: 1 type: string - secretName: - description: SecretName 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. - SecretName is required if you would like to use the HTTPS endpoints - (e.g. when exposing them outside the cluster using a LoadBalancer - Service), and is not required when you would like to use only the - HTTP endpoints (e.g. when terminating TLS at an Ingress). The TLS - secret must contain keys named `tls.crt` and `tls.key` that contain - the certificate and private key to use for TLS. + 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 diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go index cb17bb46..87e9d227 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher.go @@ -102,7 +102,7 @@ func (c *oidcProviderConfigWatcherController) Sync(ctx controllerlib.Context) er setOfSecretNames = make(map[string]bool) uniqueSecretNamesPerIssuerAddress[issuerURLToHostnameKey(issuerURL)] = setOfSecretNames } - setOfSecretNames[opc.Spec.SecretName] = true + setOfSecretNames[opc.Spec.SNICertificateSecretName] = true } errs := multierror.New() diff --git a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go index 7efeffe4..f486da94 100644 --- a/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go +++ b/internal/controller/supervisorconfig/oidcproviderconfig_watcher_test.go @@ -819,8 +819,8 @@ func TestSync(t *testing.T) { oidcProviderConfigSameIssuerAddress1 = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "provider1", Namespace: namespace}, Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: "https://iSSueR-duPlicAte-adDress.cOm/path1", - SecretName: "secret1", + Issuer: "https://iSSueR-duPlicAte-adDress.cOm/path1", + SNICertificateSecretName: "secret1", }, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress1)) @@ -830,8 +830,8 @@ func TestSync(t *testing.T) { 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", - SecretName: "secret2", + Issuer: "https://issuer-duplicate-address.com:1234/path2", + SNICertificateSecretName: "secret2", }, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigSameIssuerAddress2)) @@ -840,8 +840,8 @@ func TestSync(t *testing.T) { oidcProviderConfigDifferentIssuerAddress = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "differentIssuerAddressProvider", Namespace: namespace}, Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: "https://issuer-not-duplicate.com", - SecretName: "secret1", + Issuer: "https://issuer-not-duplicate.com", + SNICertificateSecretName: "secret1", }, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigDifferentIssuerAddress)) @@ -855,8 +855,8 @@ func TestSync(t *testing.T) { oidcProviderConfigWithInvalidIssuerURL = &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{Name: "invalidIssuerURLProvider", Namespace: namespace}, Spec: v1alpha1.OIDCProviderConfigSpec{ - Issuer: invalidIssuerURL, - SecretName: "secret1", + Issuer: invalidIssuerURL, + SNICertificateSecretName: "secret1", }, } r.NoError(pinnipedAPIClient.Tracker().Add(oidcProviderConfigWithInvalidIssuerURL)) diff --git a/internal/controller/supervisorconfig/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go index 5d382bbf..6a4ce599 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer.go +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -68,7 +68,7 @@ func (c *tlsCertObserverController) Sync(ctx controllerlib.Context) error { issuerHostToTLSCertMap := map[string]*tls.Certificate{} for _, provider := range allProviders { - secretName := provider.Spec.SecretName + 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) diff --git a/internal/controller/supervisorconfig/tls_cert_observer_test.go b/internal/controller/supervisorconfig/tls_cert_observer_test.go index 6510b732..c82449e7 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer_test.go +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -203,21 +203,27 @@ func TestTLSCertObserverControllerSync(t *testing.T) { Name: "no-secret-oidcproviderconfig1", Namespace: installedInNamespace, }, - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://no-secret-issuer1.com"}, // no SecretName field + 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", SecretName: ""}, + 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", SecretName: "bad-tls-secret-name"}, + 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" @@ -236,7 +242,10 @@ func TestTLSCertObserverControllerSync(t *testing.T) { Namespace: installedInNamespace, }, // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test without a port number. - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.iSSuer-wiTh-goOd-secRet1.cOm/path", SecretName: "good-tls-secret-name1"}, + Spec: v1alpha1.OIDCProviderConfigSpec{ + Issuer: "https://www.iSSuer-wiTh-goOd-secRet1.cOm/path", + SNICertificateSecretName: "good-tls-secret-name1", + }, } oidcProviderConfigWithGoodSecret2 := &v1alpha1.OIDCProviderConfig{ ObjectMeta: metav1.ObjectMeta{ @@ -244,7 +253,10 @@ func TestTLSCertObserverControllerSync(t *testing.T) { Namespace: installedInNamespace, }, // Issuer hostname should be treated in a case-insensitive way and SNI ignores port numbers. Test with a port number. - Spec: v1alpha1.OIDCProviderConfigSpec{Issuer: "https://www.issUEr-WIth-gOOd-seCret2.com:1234/path", SecretName: "good-tls-secret-name2"}, + 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) From 1f1b6c884ee7eea76afb653ce78b12d8814368cd Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 27 Oct 2020 14:57:25 -0700 Subject: [PATCH 04/17] Add integration test: supervisor TLS termination and SNI virtual hosting - Also reduce the minimum allowed TLS version to v1.2, because v1.3 is not yet supported by some common clients, e.g. the default MacOS curl command --- cmd/pinniped-supervisor/main.go | 7 +- hack/prepare-for-integration-tests.sh | 1 + test/integration/supervisor_discovery_test.go | 242 +++++++++++++++--- test/integration/supervisor_keys_test.go | 2 +- test/library/client.go | 5 +- test/library/env.go | 92 ++++--- 6 files changed, 261 insertions(+), 88 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 7b296fa9..94a924bd 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -206,10 +206,11 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { //nolint: gosec // Intentionally binding to all network interfaces. httpsListener, err := tls.Listen("tcp", ":443", &tls.Config{ - MinVersion: tls.VersionTLS13, + 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) { - klog.InfoS("GetCertificate called", "info.ServerName", info.ServerName) - return dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)), nil + cert := dynamicTLSCertProvider.GetTLSCert(strings.ToLower(info.ServerName)) + klog.InfoS("GetCertificate called for port 443", "info.ServerName", info.ServerName, "foundCert", cert != nil) + return cert, nil }, }) if err != nil { diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index 0e06fe1f..c7b5aa10 100755 --- a/hack/prepare-for-integration-tests.sh +++ b/hack/prepare-for-integration-tests.sh @@ -282,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 diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index b536429c..ebed769a 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,93 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" + corev1 "k8s.io/api/core/v1" 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() + + temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) + + 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 := createSNICertificateSecret(ctx, t, ns, hostname1, 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 := createSNICertificateSecret(ctx, t, ns, hostname1, 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 := createSNICertificateSecret(ctx, t, ns, hostname2, 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 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) - } - }) + temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, client) tests := []struct { Scheme string @@ -61,7 +114,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 { @@ -98,7 +151,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 +159,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,14 +169,14 @@ 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. @@ -135,13 +188,74 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { 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 createSNICertificateSecret(ctx context.Context, t *testing.T, ns string, hostname string, sniCertificateSecretName 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}, 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: sniCertificateSecretName, + 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, sniCertificateSecretName, metav1.DeleteOptions{}) + require.NoError(t, err) + }) + + return ca +} + +func temporarilyRemoveAllOIDCProviderConfigs(ctx context.Context, t *testing.T, ns string, client pinnipedclientset.Interface) { + // 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) + } + }) +} + func jwksURLForIssuer(scheme, host, path string) string { return fmt.Sprintf("%s://%s/%s/jwks.json", scheme, host, strings.TrimPrefix(path, "/")) } @@ -160,14 +274,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 +294,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, `Get "https://localhost:12344/issuer1": remote error: tls: unrecognized name`) +} + func requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear( ctx context.Context, t *testing.T, @@ -188,15 +319,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 +351,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 +381,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 +413,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 +497,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 52f5ba09..19e1978d 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 f12219da..95f798b4 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 ed39a193..22b8d666 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 { From 38802c218482a7cdaa379fd8bfc400cff9d18604 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 27 Oct 2020 16:33:08 -0700 Subject: [PATCH 05/17] Add a way to set a default supervisor TLS cert for when SNI won't work - Setting a Secret in the supervisor's namespace with a special name will cause it to get picked up and served as the supervisor's TLS cert for any request which does not have a matching SNI cert. - This is especially useful for when there is no DNS record for an issuer and the user will be accessing it via IP address. This is not how we would expect it to be used in production, but it might be useful for other cases. - Includes a new integration test - Also suppress all of the warnings about ignoring the error returned by Close() in lines like `defer x.Close()` to make GoLand happier --- cmd/local-user-authenticator/main.go | 4 +- cmd/local-user-authenticator/main_test.go | 6 +- cmd/pinniped-supervisor/main.go | 14 ++- internal/certauthority/certauthority.go | 6 +- internal/certauthority/certauthority_test.go | 3 +- internal/controller/apicerts/certs_manager.go | 1 + .../supervisorconfig/tls_cert_observer.go | 52 +++++++---- .../tls_cert_observer_test.go | 90 ++++++++++++++----- .../provider/dynamic_tls_cert_provider.go | 15 ++++ test/integration/supervisor_discovery_test.go | 59 ++++++++++-- 10 files changed, 192 insertions(+), 58 deletions(-) diff --git a/cmd/local-user-authenticator/main.go b/cmd/local-user-authenticator/main.go index f774500c..f226066f 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 3db5a542..07771971 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 94a924bd..55e35adc 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -201,7 +201,7 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { if err != nil { return fmt.Errorf("cannot create listener: %w", err) } - defer httpListener.Close() + defer func() { _ = httpListener.Close() }() start(ctx, httpListener, oidProvidersManager) //nolint: gosec // Intentionally binding to all network interfaces. @@ -209,14 +209,22 @@ func run(serverInstallationNamespace string, cfg *supervisor.Config) error { 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)) - klog.InfoS("GetCertificate called for port 443", "info.ServerName", info.ServerName, "foundCert", cert != nil) + 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 httpsListener.Close() + defer func() { _ = httpsListener.Close() }() start(ctx, httpsListener, oidProvidersManager) klog.InfoS("supervisor is ready", diff --git a/internal/certauthority/certauthority.go b/internal/certauthority/certauthority.go index 9e5f77dd..ba3e6492 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 72737991..f2dc5b33 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/controller/apicerts/certs_manager.go b/internal/controller/apicerts/certs_manager.go index 541eaf62..c0be7873 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/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go index 6a4ce599..e29930c6 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer.go +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -18,18 +18,21 @@ import ( "go.pinniped.dev/internal/controllerlib" ) +const SecretNameForDefaultTLSCertificate = "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential + type tlsCertObserverController struct { - issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter - oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer - secretInformer corev1informers.SecretInformer + issuerTLSCertSetter IssuerTLSCertSetter + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer } -type IssuerHostToTLSCertMapSetter interface { +type IssuerTLSCertSetter interface { SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*tls.Certificate) + SetDefaultTLSCert(certificate *tls.Certificate) } func NewTLSCertObserverController( - issuerHostToTLSCertMapSetter IssuerHostToTLSCertMapSetter, + issuerTLSCertSetter IssuerTLSCertSetter, secretInformer corev1informers.SecretInformer, oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -38,9 +41,9 @@ func NewTLSCertObserverController( controllerlib.Config{ Name: "tls-certs-observer-controller", Syncer: &tlsCertObserverController{ - issuerHostToTLSCertMapSetter: issuerHostToTLSCertMapSetter, - oidcProviderConfigInformer: oidcProviderConfigInformer, - secretInformer: secretInformer, + issuerTLSCertSetter: issuerTLSCertSetter, + oidcProviderConfigInformer: oidcProviderConfigInformer, + secretInformer: secretInformer, }, }, withInformer( @@ -74,26 +77,41 @@ func (c *tlsCertObserverController) Sync(ctx controllerlib.Context) error { klog.InfoS("tlsCertObserverController Sync found an invalid issuer URL", "namespace", ns, "issuer", provider.Spec.Issuer) continue } - tlsSecret, err := c.secretInformer.Lister().Secrets(ns).Get(secretName) + certFromSecret, err := c.certFromSecret(ns, secretName) if err != nil { - klog.InfoS("tlsCertObserverController Sync could not find TLS cert secret", "namespace", ns, "secretName", secretName) - continue - } - certFromSecret, err := tls.X509KeyPair(tlsSecret.Data["tls.crt"], tlsSecret.Data["tls.key"]) - if err != nil { - klog.InfoS("tlsCertObserverController Sync found a TLS secret with Data in an unexpected format", "namespace", ns, "secretName", secretName) continue } // Lowercase the host part of the URL because hostnames should be treated as case-insensitive. - issuerHostToTLSCertMap[lowercaseHostWithoutPort(issuerURL)] = &certFromSecret + issuerHostToTLSCertMap[lowercaseHostWithoutPort(issuerURL)] = certFromSecret } klog.InfoS("tlsCertObserverController Sync updated the TLS cert cache", "issuerHostCount", len(issuerHostToTLSCertMap)) - c.issuerHostToTLSCertMapSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) + c.issuerTLSCertSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) + + defaultCert, err := c.certFromSecret(ns, SecretNameForDefaultTLSCertificate) + 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, ":") diff --git a/internal/controller/supervisorconfig/tls_cert_observer_test.go b/internal/controller/supervisorconfig/tls_cert_observer_test.go index c82449e7..633bad04 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer_test.go +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -96,31 +96,38 @@ func TestTLSCertObserverControllerInformerFilters(t *testing.T) { }, spec.Parallel(), spec.Report(report.Terminal{})) } -type fakeIssuerHostToTLSCertMapSetter struct { +type fakeIssuerTLSCertSetter struct { setIssuerHostToTLSCertMapWasCalled bool + setDefaultTLSCertWasCalled bool issuerHostToTLSCertMapReceived map[string]*tls.Certificate + setDefaultTLSCertReceived *tls.Certificate } -func (f *fakeIssuerHostToTLSCertMapSetter) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap map[string]*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" var ( - r *require.Assertions - subject controllerlib.Controller - pinnipedInformerClient *pinnipedfake.Clientset - kubeInformerClient *kubernetesfake.Clientset - pinnipedInformers pinnipedinformers.SharedInformerFactory - kubeInformers kubeinformers.SharedInformerFactory - timeoutContext context.Context - timeoutContextCancel context.CancelFunc - syncContext *controllerlib.Context - issuerHostToTLSCertSetter *fakeIssuerHostToTLSCertMapSetter + 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 @@ -128,7 +135,7 @@ func TestTLSCertObserverControllerSync(t *testing.T) { var startInformersAndController = func() { // Set this at the last second to allow for injection of server override. subject = NewTLSCertObserverController( - issuerHostToTLSCertSetter, + issuerTLSCertSetter, kubeInformers.Core().V1().Secrets(), pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), controllerlib.WithInformer, @@ -165,7 +172,7 @@ func TestTLSCertObserverControllerSync(t *testing.T) { kubeInformers = kubeinformers.NewSharedInformerFactory(kubeInformerClient, 0) pinnipedInformerClient = pinnipedfake.NewSimpleClientset() pinnipedInformers = pinnipedinformers.NewSharedInformerFactory(pinnipedInformerClient, 0) - issuerHostToTLSCertSetter = &fakeIssuerHostToTLSCertMapSetter{} + issuerTLSCertSetter = &fakeIssuerTLSCertSetter{} unrelatedSecret := &corev1.Secret{ ObjectMeta: metav1.ObjectMeta{ @@ -181,13 +188,15 @@ func TestTLSCertObserverControllerSync(t *testing.T) { }) when("there are no OIDCProviderConfigs and no TLS Secrets yet", func() { - it("sets the issuerHostToTLSCertSetter's map to be empty", func() { + it("sets the issuerTLSCertSetter's map to be empty", func() { startInformersAndController() err := controllerlib.TestSync(t, subject, *syncContext) r.NoError(err) - r.True(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) - r.Empty(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived) + r.True(issuerTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) + r.Empty(issuerTLSCertSetter.issuerHostToTLSCertMapReceived) + r.True(issuerTLSCertSetter.setDefaultTLSCertWasCalled) + r.Nil(issuerTLSCertSetter.setDefaultTLSCertReceived) }) }) @@ -293,24 +302,61 @@ func TestTLSCertObserverControllerSync(t *testing.T) { r.NoError(kubeInformerClient.Tracker().Add(badTLSSecret)) }) - it("updates the issuerHostToTLSCertSetter's map to include only the issuers that had valid certs", func() { + 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(issuerHostToTLSCertSetter.setIssuerHostToTLSCertMapWasCalled) - r.Len(issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived, 2) + 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 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret1.com"] + 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 := issuerHostToTLSCertSetter.issuerHostToTLSCertMapReceived["www.issuer-with-good-secret2.com"] + 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 called default-tls-certificate", 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: "default-tls-certificate", 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 index 51e3c157..7c48ad9c 100644 --- a/internal/oidc/provider/dynamic_tls_cert_provider.go +++ b/internal/oidc/provider/dynamic_tls_cert_provider.go @@ -10,11 +10,14 @@ import ( 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 } @@ -30,8 +33,20 @@ func (p *dynamicTLSCertProvider) SetIssuerHostToTLSCertMap(issuerHostToTLSCertMa 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/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index ebed769a..38c088ff 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -31,6 +31,49 @@ import ( "go.pinniped.dev/test/library" ) +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() + + temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) + + scheme := "https" + address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 + + hostAndPortSegments := strings.Split(address, ":") + hostname := 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) + + // 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. + specialNameForDefaultTLSCertSecret := "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential + ca := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, specialNameForDefaultTLSCertSecret, 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(ca.Bundle()), issuerUsingIPAddress, nil) +} + func TestSupervisorTLSTerminationWithSNI(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewPinnipedClientset(t) @@ -57,7 +100,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuer1) // Create the Secret. - ca1 := createSNICertificateSecret(ctx, t, ns, hostname1, sniCertificateSecretName1, kubeClient) + 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) @@ -74,7 +117,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuer1) // Create a Secret at the updated name. - ca1update := createSNICertificateSecret(ctx, t, ns, hostname1, sniCertificateSecretName1update, kubeClient) + 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) @@ -90,7 +133,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { requireStatus(t, pinnipedClient, oidcProviderConfig2.Namespace, oidcProviderConfig2.Name, v1alpha1.SuccessOIDCProviderStatus) // Create the Secret. - ca2 := createSNICertificateSecret(ctx, t, ns, hostname2, sniCertificateSecretName2, kubeClient) + 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{ @@ -195,13 +238,13 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { } } -func createSNICertificateSecret(ctx context.Context, t *testing.T, ns string, hostname string, sniCertificateSecretName string, kubeClient kubernetes.Interface) *certauthority.CA { +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}, 1000*time.Hour) + 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. @@ -210,7 +253,7 @@ func createSNICertificateSecret(ctx context.Context, t *testing.T, ns string, ho secret := corev1.Secret{ TypeMeta: metav1.TypeMeta{}, ObjectMeta: metav1.ObjectMeta{ - Name: sniCertificateSecretName, + Name: secretName, Namespace: ns, }, StringData: map[string]string{ @@ -226,7 +269,7 @@ func createSNICertificateSecret(ctx context.Context, t *testing.T, ns string, ho t.Helper() deleteCtx, cancel := context.WithTimeout(context.Background(), 10*time.Second) defer cancel() - err := kubeClient.CoreV1().Secrets(ns).Delete(deleteCtx, sniCertificateSecretName, metav1.DeleteOptions{}) + err := kubeClient.CoreV1().Secrets(ns).Delete(deleteCtx, secretName, metav1.DeleteOptions{}) require.NoError(t, err) }) @@ -308,7 +351,7 @@ func requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t *testing.T, url return err != nil && strings.Contains(err.Error(), "tls: unrecognized name") }, 10*time.Second, 200*time.Millisecond) require.Error(t, err) - require.EqualError(t, err, `Get "https://localhost:12344/issuer1": remote error: tls: unrecognized name`) + require.EqualError(t, err, fmt.Sprintf(`Get "%s": remote error: tls: unrecognized name`, url)) } func requireCreatingOIDCProviderConfigCausesDiscoveryEndpointsToAppear( From 2777c4e9f3670a489645c392c76d9812e4ac8d43 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 27 Oct 2020 16:56:53 -0700 Subject: [PATCH 06/17] Update prepare-for-integration-tests.sh to use ./hack/kind-{up,down}.sh --- hack/prepare-for-integration-tests.sh | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/hack/prepare-for-integration-tests.sh b/hack/prepare-for-integration-tests.sh index c7b5aa10..8c8e777f 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, 31243 as 127.0.0.1:12344, and 31235 as 127.0.0.1:12346 - kind create cluster --config "$pinniped_path/hack/lib/kind-config/single-node.yaml" --name pinniped + # 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." @@ -304,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}" @@ -315,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 From 170d3a39935a325675423572c5524b70bf1d7aa9 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 27 Oct 2020 17:00:00 -0700 Subject: [PATCH 07/17] Forgot to commit some test fixtures in a prior commit --- .../supervisorconfig/testdata/test3.crt | 17 ++++++++++++ .../supervisorconfig/testdata/test3.key | 27 +++++++++++++++++++ 2 files changed, 44 insertions(+) create mode 100644 internal/controller/supervisorconfig/testdata/test3.crt create mode 100644 internal/controller/supervisorconfig/testdata/test3.key diff --git a/internal/controller/supervisorconfig/testdata/test3.crt b/internal/controller/supervisorconfig/testdata/test3.crt new file mode 100644 index 00000000..796a7690 --- /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 00000000..7ad653ae --- /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----- From 978ecda758436ea89b9c1d2c49728500f288b5de Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 08:58:50 -0700 Subject: [PATCH 08/17] Test SNI & default certs being used at the same time in integration test --- test/integration/supervisor_discovery_test.go | 101 ++++++++++-------- 1 file changed, 58 insertions(+), 43 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 38c088ff..ec1d0f09 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -31,49 +31,6 @@ import ( "go.pinniped.dev/test/library" ) -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() - - temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) - - scheme := "https" - address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 - - hostAndPortSegments := strings.Split(address, ":") - hostname := 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) - - // 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. - specialNameForDefaultTLSCertSecret := "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential - ca := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, specialNameForDefaultTLSCertSecret, 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(ca.Bundle()), issuerUsingIPAddress, nil) -} - func TestSupervisorTLSTerminationWithSNI(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewPinnipedClientset(t) @@ -141,6 +98,64 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { }) } +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() + + temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) + + scheme := "https" + address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 + + hostAndPortSegments := strings.Split(address, ":") + hostname := 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. + specialNameForDefaultTLSCertSecret := "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential + defaultCA := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, specialNameForDefaultTLSCertSecret, 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. + _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, 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) From 29e0ce566263dae09385bb1df692fc3a680f196f Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 11:56:50 -0700 Subject: [PATCH 09/17] Configure name of the supervisor default TLS cert secret via ConfigMap Signed-off-by: Andrew Keesler --- cmd/pinniped-supervisor/main.go | 1 + deploy/supervisor/deployment.yaml | 2 ++ internal/config/supervisor/config.go | 18 +++++++++++++ internal/config/supervisor/config_test.go | 26 +++++++++++++++++-- internal/config/supervisor/types.go | 8 +++++- .../supervisorconfig/tls_cert_observer.go | 19 +++++++------- .../tls_cert_observer_test.go | 11 +++++--- test/integration/supervisor_discovery_test.go | 2 +- 8 files changed, 71 insertions(+), 16 deletions(-) diff --git a/cmd/pinniped-supervisor/main.go b/cmd/pinniped-supervisor/main.go index 55e35adc..b82f5026 100644 --- a/cmd/pinniped-supervisor/main.go +++ b/cmd/pinniped-supervisor/main.go @@ -113,6 +113,7 @@ func startControllers( WithController( supervisorconfig.NewTLSCertObserverController( dynamicTLSCertProvider, + cfg.NamesConfig.DefaultTLSCertificateSecret, kubeInformers.Core().V1().Secrets(), pinnipedInformers.Config().V1alpha1().OIDCProviderConfigs(), controllerlib.WithInformer, diff --git a/deploy/supervisor/deployment.yaml b/deploy/supervisor/deployment.yaml index d5161d46..8f91610d 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 != "": diff --git a/internal/config/supervisor/config.go b/internal/config/supervisor/config.go index eb6836c6..c831c71e 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 b616fa5b..8b77c6f2 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 03805956..060b88ed 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/supervisorconfig/tls_cert_observer.go b/internal/controller/supervisorconfig/tls_cert_observer.go index e29930c6..88db4ace 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer.go +++ b/internal/controller/supervisorconfig/tls_cert_observer.go @@ -18,12 +18,11 @@ import ( "go.pinniped.dev/internal/controllerlib" ) -const SecretNameForDefaultTLSCertificate = "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential - type tlsCertObserverController struct { - issuerTLSCertSetter IssuerTLSCertSetter - oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer - secretInformer corev1informers.SecretInformer + issuerTLSCertSetter IssuerTLSCertSetter + defaultTLSCertificateSecretName string + oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer + secretInformer corev1informers.SecretInformer } type IssuerTLSCertSetter interface { @@ -33,6 +32,7 @@ type IssuerTLSCertSetter interface { func NewTLSCertObserverController( issuerTLSCertSetter IssuerTLSCertSetter, + defaultTLSCertificateSecretName string, secretInformer corev1informers.SecretInformer, oidcProviderConfigInformer v1alpha1.OIDCProviderConfigInformer, withInformer pinnipedcontroller.WithInformerOptionFunc, @@ -41,9 +41,10 @@ func NewTLSCertObserverController( controllerlib.Config{ Name: "tls-certs-observer-controller", Syncer: &tlsCertObserverController{ - issuerTLSCertSetter: issuerTLSCertSetter, - oidcProviderConfigInformer: oidcProviderConfigInformer, - secretInformer: secretInformer, + issuerTLSCertSetter: issuerTLSCertSetter, + defaultTLSCertificateSecretName: defaultTLSCertificateSecretName, + oidcProviderConfigInformer: oidcProviderConfigInformer, + secretInformer: secretInformer, }, }, withInformer( @@ -88,7 +89,7 @@ func (c *tlsCertObserverController) Sync(ctx controllerlib.Context) error { klog.InfoS("tlsCertObserverController Sync updated the TLS cert cache", "issuerHostCount", len(issuerHostToTLSCertMap)) c.issuerTLSCertSetter.SetIssuerHostToTLSCertMap(issuerHostToTLSCertMap) - defaultCert, err := c.certFromSecret(ns, SecretNameForDefaultTLSCertificate) + defaultCert, err := c.certFromSecret(ns, c.defaultTLSCertificateSecretName) if err != nil { c.issuerTLSCertSetter.SetDefaultTLSCert(nil) } else { diff --git a/internal/controller/supervisorconfig/tls_cert_observer_test.go b/internal/controller/supervisorconfig/tls_cert_observer_test.go index 633bad04..9b77fa46 100644 --- a/internal/controller/supervisorconfig/tls_cert_observer_test.go +++ b/internal/controller/supervisorconfig/tls_cert_observer_test.go @@ -42,6 +42,7 @@ func TestTLSCertObserverControllerInformerFilters(t *testing.T) { 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 @@ -115,7 +116,10 @@ func (f *fakeIssuerTLSCertSetter) SetDefaultTLSCert(certificate *tls.Certificate func TestTLSCertObserverControllerSync(t *testing.T) { spec.Run(t, "Sync", func(t *testing.T, when spec.G, it spec.S) { - const installedInNamespace = "some-namespace" + const ( + installedInNamespace = "some-namespace" + defaultTLSSecretName = "some-default-secret-name" + ) var ( r *require.Assertions @@ -136,6 +140,7 @@ func TestTLSCertObserverControllerSync(t *testing.T) { // 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, @@ -324,7 +329,7 @@ func TestTLSCertObserverControllerSync(t *testing.T) { r.Equal(expectedCertificate2, *actualCertificate2) }) - when("there is also a default TLS cert secret called default-tls-certificate", func() { + when("there is also a default TLS cert secret with the configured default TLS cert secret name", func() { var ( expectedDefaultCertificate tls.Certificate ) @@ -338,7 +343,7 @@ func TestTLSCertObserverControllerSync(t *testing.T) { expectedDefaultCertificate, err = tls.X509KeyPair(testCrt, testKey) r.NoError(err) defaultTLSCertSecret := &corev1.Secret{ - ObjectMeta: metav1.ObjectMeta{Name: "default-tls-certificate", Namespace: installedInNamespace}, + ObjectMeta: metav1.ObjectMeta{Name: defaultTLSSecretName, Namespace: installedInNamespace}, Data: map[string][]byte{"tls.crt": testCrt, "tls.key": testKey}, } r.NoError(kubeInformerClient.Tracker().Add(defaultTLSCertSecret)) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index ec1d0f09..45fefcdf 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -135,7 +135,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuerUsingIPAddress) // Create a Secret at the special name which represents the default TLS cert. - specialNameForDefaultTLSCertSecret := "default-tls-certificate" //nolint:gosec // this is not a hardcoded credential + specialNameForDefaultTLSCertSecret := "pinniped-supervisor-default-tls-certificate" //nolint:gosec // this is not a hardcoded credential defaultCA := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, specialNameForDefaultTLSCertSecret, kubeClient) // Now that the Secret exists, we should be able to access the endpoints by IP address using the CA. From 2542a8e1755a1a90b29420a7b9daaa860ef65b86 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 28 Oct 2020 12:32:21 -0700 Subject: [PATCH 10/17] Stash and restore any pre-existing default TLS cert in supervisor_discovery_test.go Signed-off-by: Ryan Richard --- test/integration/supervisor_discovery_test.go | 37 ++++++++++++++----- 1 file changed, 28 insertions(+), 9 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 45fefcdf..c693940e 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -18,6 +18,8 @@ import ( "testing" "time" + k8serrors "k8s.io/apimachinery/pkg/api/errors" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" corev1 "k8s.io/api/core/v1" @@ -31,6 +33,10 @@ import ( "go.pinniped.dev/test/library" ) +const ( + specialNameForDefaultTLSCertSecret = "pinniped-supervisor-default-tls-certificate" +) + func TestSupervisorTLSTerminationWithSNI(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewPinnipedClientset(t) @@ -40,7 +46,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, pinnipedClient, kubeClient) scheme := "https" address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 @@ -107,7 +113,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, pinnipedClient) + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, pinnipedClient, kubeClient) scheme := "https" address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 @@ -135,7 +141,6 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { requireEndpointHasTLSErrorBecauseCertificatesAreNotReady(t, issuerUsingIPAddress) // Create a Secret at the special name which represents the default TLS cert. - specialNameForDefaultTLSCertSecret := "pinniped-supervisor-default-tls-certificate" //nolint:gosec // this is not a hardcoded credential defaultCA := createTLSCertificateSecret(ctx, t, ns, "cert-hostname-doesnt-matter", []net.IP{ip}, specialNameForDefaultTLSCertSecret, kubeClient) // Now that the Secret exists, we should be able to access the endpoints by IP address using the CA. @@ -164,7 +169,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigs(ctx, t, ns, client) + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, client, library.NewClientset(t)) tests := []struct { Scheme string @@ -291,16 +296,24 @@ func createTLSCertificateSecret(ctx context.Context, t *testing.T, ns string, ho return ca } -func temporarilyRemoveAllOIDCProviderConfigs(ctx context.Context, t *testing.T, ns string, client pinnipedclientset.Interface) { +func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx context.Context, t *testing.T, ns 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 := client.ConfigV1alpha1().OIDCProviderConfigs(ns).List(ctx, metav1.ListOptions{}) + originalConfigList, err := pinnipedClient.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{}) + err := pinnipedClient.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. + // Also remove the supervisor's default TLS cert + originalSecret, err := kubeClient.CoreV1().Secrets(ns).Get(ctx, specialNameForDefaultTLSCertSecret, metav1.GetOptions{}) + notFound := k8serrors.IsNotFound(err) + require.False(t, err != nil && !notFound, "unexpected error when getting %s", specialNameForDefaultTLSCertSecret) + err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, specialNameForDefaultTLSCertSecret, metav1.DeleteOptions{}) + notFound = k8serrors.IsNotFound(err) + require.False(t, err != nil && !notFound, "unexpected error when deleting %s", specialNameForDefaultTLSCertSecret) + + // 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() @@ -308,7 +321,13 @@ func temporarilyRemoveAllOIDCProviderConfigs(ctx context.Context, t *testing.T, 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{}) + _, 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) } }) From 8ff64d4c1a07b65a7c53f4f21a4ab4759a378d26 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 12:49:41 -0700 Subject: [PATCH 11/17] Require `https` scheme for OIDCProviderConfig Issuer field Signed-off-by: Andrew Keesler --- internal/oidc/provider/oidcprovider.go | 6 +---- internal/oidc/provider/oidcprovider_test.go | 5 ++++ test/integration/supervisor_discovery_test.go | 23 +++++++++---------- 3 files changed, 17 insertions(+), 17 deletions(-) diff --git a/internal/oidc/provider/oidcprovider.go b/internal/oidc/provider/oidcprovider.go index cc427167..3e53c5e2 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 81204e28..828bd671 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 c693940e..0c0e9e45 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -18,11 +18,10 @@ import ( "testing" "time" - k8serrors "k8s.io/apimachinery/pkg/api/errors" - "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" @@ -34,7 +33,7 @@ import ( ) const ( - specialNameForDefaultTLSCertSecret = "pinniped-supervisor-default-tls-certificate" + specialNameForDefaultTLSCertSecret = "pinniped-supervisor-default-tls-certificate" //nolint:gosec // this is not a hardcoded credential ) func TestSupervisorTLSTerminationWithSNI(t *testing.T) { @@ -193,14 +192,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) @@ -246,7 +245,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { 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) From bd04570e513097127cca9ce829b06189ebb05956 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 28 Oct 2020 13:09:20 -0700 Subject: [PATCH 12/17] supervisor_discovery_test.go tests hostnames are treated as case-insensitive Signed-off-by: Ryan Richard --- test/integration/supervisor_discovery_test.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 0c0e9e45..50a9f006 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -118,7 +118,8 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { address := env.SupervisorHTTPSAddress // hostname and port for direct access to the supervisor's port 443 hostAndPortSegments := strings.Split(address, ":") - hostname := hostAndPortSegments[0] + // 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] @@ -154,7 +155,9 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { 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. - _ = requireDiscoveryEndpointsAreWorking(t, scheme, address, string(sniCA.Bundle()), issuerUsingHostname, nil) + // 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) From 01dddd3cae3423d2c607ee394386026b0649998c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 13:42:02 -0700 Subject: [PATCH 13/17] Add some docs for configuring supervisor TLS Signed-off-by: Andrew Keesler --- deploy/supervisor/README.md | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-) diff --git a/deploy/supervisor/README.md b/deploy/supervisor/README.md index 5a751ee4..5ad6d51f 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. From c52874250ad455870b179a14af13d2cec5d6e3ba Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 14:25:01 -0700 Subject: [PATCH 14/17] Fix a mistake in supervisor_discovery_test.go - Should not fail when the default TLS cert does not exist in the test cluster before the test started --- test/integration/supervisor_discovery_test.go | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 50a9f006..4f60a6c1 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -311,9 +311,12 @@ func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx context. originalSecret, err := kubeClient.CoreV1().Secrets(ns).Get(ctx, specialNameForDefaultTLSCertSecret, metav1.GetOptions{}) notFound := k8serrors.IsNotFound(err) require.False(t, err != nil && !notFound, "unexpected error when getting %s", specialNameForDefaultTLSCertSecret) - err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, specialNameForDefaultTLSCertSecret, metav1.DeleteOptions{}) - notFound = k8serrors.IsNotFound(err) - require.False(t, err != nil && !notFound, "unexpected error when deleting %s", specialNameForDefaultTLSCertSecret) + if notFound { + originalSecret = nil + } else { + err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, specialNameForDefaultTLSCertSecret, 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() { From a007fc3bd31433763b576a93dce040d7b19825b3 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 15:22:53 -0700 Subject: [PATCH 15/17] Form paths correctly when the path arg is empty in supervisor_discovery_test.go --- test/integration/supervisor_discovery_test.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index 4f60a6c1..f4e0397b 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -339,10 +339,16 @@ func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx context. } 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, "/")) } From 4af508981a5d38fdfa5c6e8064a7c9b032fd05d1 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 16:11:19 -0700 Subject: [PATCH 16/17] Make default TLS secret name from app name in supervisor_discovery_test.go --- test/integration/supervisor_discovery_test.go | 31 ++++++++++++------- 1 file changed, 19 insertions(+), 12 deletions(-) diff --git a/test/integration/supervisor_discovery_test.go b/test/integration/supervisor_discovery_test.go index f4e0397b..725ecdfd 100644 --- a/test/integration/supervisor_discovery_test.go +++ b/test/integration/supervisor_discovery_test.go @@ -32,10 +32,6 @@ import ( "go.pinniped.dev/test/library" ) -const ( - specialNameForDefaultTLSCertSecret = "pinniped-supervisor-default-tls-certificate" //nolint:gosec // this is not a hardcoded credential -) - func TestSupervisorTLSTerminationWithSNI(t *testing.T) { env := library.IntegrationEnv(t) pinnipedClient := library.NewPinnipedClientset(t) @@ -45,7 +41,7 @@ func TestSupervisorTLSTerminationWithSNI(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, pinnipedClient, kubeClient) + 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 @@ -112,7 +108,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, pinnipedClient, kubeClient) + 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 @@ -141,7 +137,7 @@ func TestSupervisorTLSTerminationWithDefaultCerts(t *testing.T) { 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}, specialNameForDefaultTLSCertSecret, kubeClient) + 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) @@ -171,7 +167,7 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { ctx, cancel := context.WithTimeout(context.Background(), 5*time.Minute) defer cancel() - temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, client, library.NewClientset(t)) + temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx, t, ns, defaultTLSCertSecretName(env), client, library.NewClientset(t)) tests := []struct { Scheme string @@ -260,6 +256,10 @@ func TestSupervisorOIDCDiscovery(t *testing.T) { } } +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) @@ -298,7 +298,14 @@ func createTLSCertificateSecret(ctx context.Context, t *testing.T, ns string, ho return ca } -func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx context.Context, t *testing.T, ns string, pinnipedClient pinnipedclientset.Interface, kubeClient kubernetes.Interface) { +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) @@ -308,13 +315,13 @@ func temporarilyRemoveAllOIDCProviderConfigsAndDefaultTLSCertSecret(ctx context. } // Also remove the supervisor's default TLS cert - originalSecret, err := kubeClient.CoreV1().Secrets(ns).Get(ctx, specialNameForDefaultTLSCertSecret, metav1.GetOptions{}) + 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", specialNameForDefaultTLSCertSecret) + require.False(t, err != nil && !notFound, "unexpected error when getting %s", defaultTLSCertSecretName) if notFound { originalSecret = nil } else { - err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, specialNameForDefaultTLSCertSecret, metav1.DeleteOptions{}) + err = kubeClient.CoreV1().Secrets(ns).Delete(ctx, defaultTLSCertSecretName, metav1.DeleteOptions{}) require.NoError(t, err) } From 059b6e885f5bfad077e0e61e0dcf870cbe4282c2 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 28 Oct 2020 16:45:23 -0700 Subject: [PATCH 17/17] Allow ytt templating of the `loadBalancerIP` for the supervisor --- deploy/supervisor/service.yaml | 3 +++ deploy/supervisor/values.yaml | 5 +++++ 2 files changed, 8 insertions(+) diff --git a/deploy/supervisor/service.yaml b/deploy/supervisor/service.yaml index e2841a63..d8739c40 100644 --- a/deploy/supervisor/service.yaml +++ b/deploy/supervisor/service.yaml @@ -74,6 +74,9 @@ metadata: spec: type: LoadBalancer selector: #@ defaultLabel() + #@ if data.values.service_loadbalancer_ip: + loadBalancerIP: #@ data.values.service_loadbalancer_ip + #@ end ports: #@ if data.values.service_http_loadbalancer_port: - name: http diff --git a/deploy/supervisor/values.yaml b/deploy/supervisor/values.yaml index aea4a04e..f9e1aac0 100644 --- a/deploy/supervisor/values.yaml +++ b/deploy/supervisor/values.yaml @@ -39,6 +39,7 @@ image_pull_dockerconfigjson: #! e.g. {"auths":{"https://registry.example.com":{" #! 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. +#! 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 @@ -47,3 +48,7 @@ service_https_nodeport_port: #! when specified, creates a NodePort Service with 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