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) }) }) })