diff --git a/pkg/operator/configobservation/auth/auth_serviceaccountissuer.go b/pkg/operator/configobservation/auth/auth_serviceaccountissuer.go index 79ac4afe9a..95533d78b7 100644 --- a/pkg/operator/configobservation/auth/auth_serviceaccountissuer.go +++ b/pkg/operator/configobservation/auth/auth_serviceaccountissuer.go @@ -125,21 +125,74 @@ func observedConfig(existingConfig map[string]interface{}, "api-audiences": apiServerArgumentValue, } - // If the issuer is not set in KAS, we rely on the config-overrides.yaml to set both - // the issuer and the api-audiences but configure the jwks-uri to point to - // the LB so that it does not default to KAS IP which is not included in the serving certs + // If the issuer is not set in KAS, we rely on config-overrides.yaml to provide both + // the service-account-issuer and api-audiences. In that case, we still configure + // service-account-jwks-uri to point at the API server load balancer endpoint so we + // don’t fall back to the KAS IP, which is not included in the serving cert SANs. + // + // The service-account-jwks-uri is configured based on the issuer type: + // + // Default issuer: + // We set service-account-jwks-uri to the API server load balancer endpoint + // (/openid/v1/jwks) so clients don’t fall back to the KAS IP, + // which is not covered by the serving certificate SANs. + // + // Custom issuer: + // A custom issuer is expected to be an OIDC issuer (a URL). It should fall into + // one of two categories: + // + // 1) Issuer is the external cluster URL. In this case, the API server already + // serves both the discovery document and public keys at: + // - /.well-known/openid-configuration + // - /openid/v1/jwks + // This is effectively the same behavior as the default issuer case. + // + // 2) Issuer is any other URL. In this case, we expect the user to host both the + // OIDC discovery document and JWKS outside of the cluster at: + // - /.well-known/openid-configuration + // - /openid/v1/jwks + // + // Any other JWKS location would require a future enhancement (RFE) and is not + // currently supported. + if observedActiveIssuer == defaultServiceAccountIssuerValue { + // Default issuer → use API LB infrastructureConfig, err := getInfrastructureConfig("cluster") if err != nil { return existingConfig, append(errs, err) } - if apiServerExternalURL := infrastructureConfig.Status.APIServerURL; len(apiServerExternalURL) == 0 { + + apiServerExternalURL := infrastructureConfig.Status.APIServerURL + if len(apiServerExternalURL) == 0 { return existingConfig, append(errs, fmt.Errorf("APIServerURL missing from infrastructure/cluster")) - } else { - apiServerArguments["service-account-jwks-uri"] = []interface{}{apiServerExternalURL + "/openid/v1/jwks"} } - } + apiServerArguments["service-account-jwks-uri"] = []interface{}{ + apiServerExternalURL + "/openid/v1/jwks", + } + } else { + // Custom issuer → only set jwks-uri if the issuer is a valid HTTPS URL, + // as required by the OpenID Discovery 1.0 spec. If it's not, we skip setting + // the flag and log a warning. Per the kube-apiserver docs, a non-HTTPS issuer + // will also cause ServiceAccountIssuerDiscovery to be disabled. + parsed, parseErr := url.Parse(observedActiveIssuer) + switch { + case parseErr != nil: + klog.Warningf("custom service account issuer %q could not be parsed as a URL: %v; "+ + "ServiceAccountIssuerDiscovery will be disabled and service-account-jwks-uri will not be configured. "+ + "It is highly recommended that the issuer be a valid HTTPS URL per the OpenID Discovery spec.", + observedActiveIssuer, parseErr) + case parsed.Scheme != "https": + klog.Warningf("custom service account issuer %q has scheme %q instead of \"https\"; "+ + "ServiceAccountIssuerDiscovery will be disabled and service-account-jwks-uri will not be configured. "+ + "It is highly recommended that the issuer be a valid HTTPS URL per the OpenID Discovery spec.", + observedActiveIssuer, parsed.Scheme) + default: + apiServerArguments["service-account-jwks-uri"] = []interface{}{ + observedActiveIssuer + "/openid/v1/jwks", + } + } + } return map[string]interface{}{"apiServerArguments": apiServerArguments}, errs } diff --git a/pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go b/pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go index f82271090a..8ed066c204 100644 --- a/pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go +++ b/pkg/operator/configobservation/auth/auth_serviceaccountissuer_test.go @@ -31,87 +31,128 @@ func TestObservedConfig(t *testing.T) { issuer string trustedIssuers []string existingIssuer string + existingJWKSURI string authError error infraError error expectedIssuer string expectedTrustedIssuers []string expectedChange bool - expectInternalJWKI bool + expectedJWKSURI string }{ { - name: "no issuer, no previous issuer means we default", - existingIssuer: "", - issuer: defaultServiceAccountIssuerValue, - expectedIssuer: defaultServiceAccountIssuerValue, - expectInternalJWKI: true, + name: "no issuer, no previous issuer means we default", + existingIssuer: "", + existingJWKSURI: "", + issuer: defaultServiceAccountIssuerValue, + expectedIssuer: defaultServiceAccountIssuerValue, + expectedJWKSURI: testLBURI, }, { - name: "no issuer, previous issuer", - existingIssuer: "https://example.com", - issuer: defaultServiceAccountIssuerValue, - expectedIssuer: defaultServiceAccountIssuerValue, - expectInternalJWKI: true, - expectedChange: true, + name: "no issuer, previous issuer", + existingIssuer: "https://example.com", + existingJWKSURI: "", + issuer: defaultServiceAccountIssuerValue, + expectedIssuer: defaultServiceAccountIssuerValue, + expectedJWKSURI: testLBURI, + expectedChange: true, }, { - name: "issuer set, no previous issuer", - existingIssuer: "", - issuer: "https://example.com", - expectedIssuer: "https://example.com", - expectedChange: true, + name: "issuer set, no previous issuer", + existingIssuer: "", + existingJWKSURI: "", + issuer: "https://example.com", + expectedIssuer: "https://example.com", + expectedJWKSURI: "https://example.com/openid/v1/jwks", + expectedChange: true, }, { name: "previous issuer was default, new is custom value", existingIssuer: defaultServiceAccountIssuerValue, + existingJWKSURI: testLBURI, issuer: "https://example.com", expectedIssuer: "https://example.com", trustedIssuers: []string{defaultServiceAccountIssuerValue}, expectedTrustedIssuers: []string{defaultServiceAccountIssuerValue}, - expectInternalJWKI: false, // this proves we remove the internal api LB when custom value is set + expectedJWKSURI: "https://example.com/openid/v1/jwks", expectedChange: true, }, { - name: "issuer set, previous issuer same", - existingIssuer: "https://example.com", - issuer: "https://example.com", - expectedIssuer: "https://example.com", + name: "issuer set, previous issuer same", + existingIssuer: "https://example.com", + existingJWKSURI: "https://example.com/openid/v1/jwks", + issuer: "https://example.com", + expectedIssuer: "https://example.com", + expectedJWKSURI: "https://example.com/openid/v1/jwks", }, { name: "issuer set, previous issuer and trusted issuers same", existingIssuer: "https://example.com", + existingJWKSURI: "https://example.com/openid/v1/jwks", issuer: "https://example.com", trustedIssuers: []string{"https://trusted.example.com"}, expectedIssuer: "https://example.com", expectedTrustedIssuers: []string{"https://trusted.example.com"}, + expectedJWKSURI: "https://example.com/openid/v1/jwks", }, { - name: "issuer set, previous issuer different", - existingIssuer: "https://example.com", - issuer: "https://example2.com", - expectedIssuer: "https://example2.com", - expectedChange: true, + name: "issuer set, previous issuer different", + existingIssuer: "https://example.com", + existingJWKSURI: "https://example.com/openid/v1/jwks", + issuer: "https://example2.com", + expectedIssuer: "https://example2.com", + expectedJWKSURI: "https://example2.com/openid/v1/jwks", + expectedChange: true, }, { - name: "auth getter error", - existingIssuer: "https://example2.com", - issuer: "https://example.com", - authError: expectedErrAuth, - expectedIssuer: "https://example2.com", + name: "auth getter error", + existingIssuer: "https://example2.com", + existingJWKSURI: "https://example2.com/openid/v1/jwks", + issuer: "https://example.com", + authError: expectedErrAuth, + expectedIssuer: "https://example2.com", + expectedJWKSURI: "https://example2.com/openid/v1/jwks", // preserve existing }, { - name: "infra getter error", - existingIssuer: defaultServiceAccountIssuerValue, - issuer: defaultServiceAccountIssuerValue, - infraError: expectedErrInfra, - expectedIssuer: defaultServiceAccountIssuerValue, - expectInternalJWKI: true, + name: "infra getter error", + existingIssuer: defaultServiceAccountIssuerValue, + existingJWKSURI: testLBURI, + issuer: defaultServiceAccountIssuerValue, + infraError: expectedErrInfra, + expectedIssuer: defaultServiceAccountIssuerValue, + expectedJWKSURI: testLBURI, // no previous + infra error -> do NOT set JWKS + }, + { + name: "default issuer, no previous issuer, infra getter error", + existingIssuer: "", + existingJWKSURI: "", + issuer: defaultServiceAccountIssuerValue, + expectedIssuer: "", + infraError: expectedErrInfra, + expectedJWKSURI: "", + }, + { + name: "custom issuer is not a URL, jwks-uri should be skipped", + existingIssuer: "", + existingJWKSURI: "", + issuer: "my-legacy-issuer", + expectedIssuer: "my-legacy-issuer", + expectedJWKSURI: "", // flag should be omitted, not set to "my-legacy-issuer/openid/v1/jwks" + expectedChange: true, + }, + { + name: "custom issuer is http not https, jwks-uri should be skipped", + existingIssuer: "", + existingJWKSURI: "", + issuer: "http://example.com", + expectedIssuer: "http://example.com", + expectedJWKSURI: "", // kube-apiserver rejects non-https, so we skip the flag + expectedChange: true, }, } { t.Run(tc.name, func(t *testing.T) { testRecorder := events.NewInMemoryRecorder("SAIssuerTest", clock.RealClock{}) - newConfig, errs := observedConfig( - unstructuredAPIConfigForIssuer(t, tc.existingIssuer, tc.trustedIssuers), + unstructuredAPIConfigForIssuer(t, tc.existingIssuer, tc.trustedIssuers, tc.existingJWKSURI), func(_ string) (*operatorv1.KubeAPIServer, error) { return kasStatusForIssuer(tc.issuer, tc.trustedIssuers...), tc.authError }, @@ -129,7 +170,7 @@ func TestObservedConfig(t *testing.T) { if tc.authError == nil && tc.infraError == nil { require.Len(t, errs, 0) } - expectedConfig = apiConfigForIssuer(tc.expectedIssuer, tc.expectedTrustedIssuers) + expectedConfig = apiConfigForIssuer(tc.expectedIssuer, tc.expectedTrustedIssuers, tc.expectedJWKSURI) // Check that errors are passed through if tc.authError != nil { @@ -151,19 +192,8 @@ func TestObservedConfig(t *testing.T) { Kind: "KubeAPIServerConfig", }, } - require.NoError(t, json.Unmarshal(jsonConfig, unmarshalledConfig)) - uri, ok := unmarshalledConfig.APIServerArguments["service-account-jwks-uri"] - if tc.expectInternalJWKI { - if !ok { - t.Errorf("expected service-account-jwks-uri to be set, it is not") - } else { - require.Equal(t, uri, kubecontrolplanev1.Arguments{testLBURI}) - } - } - if !tc.expectInternalJWKI && ok { - t.Errorf("expected no service-account-jwks-uri to be set, it is %+v", uri.String()) - } + // Deep comparison of the entire configuration handles the JWKS URI check automatically. require.Equal(t, expectedConfig, unmarshalledConfig, cmp.Diff(expectedConfig, unmarshalledConfig)) require.True(t, tc.expectedChange == (len(testRecorder.Events()) > 0)) }) @@ -194,17 +224,17 @@ func kasStatusForIssuer(active string, trustedIssuers ...string) *operatorv1.Kub } } -func apiConfigForIssuer(issuer string, trustedIssuers []string) *kubecontrolplanev1.KubeAPIServerConfig { +func apiConfigForIssuer(issuer string, trustedIssuers []string, jwksURI string) *kubecontrolplanev1.KubeAPIServerConfig { args := map[string]kubecontrolplanev1.Arguments{ "service-account-issuer": append([]string{issuer}, trustedIssuers...), "api-audiences": append([]string{issuer}, trustedIssuers...), } - if issuer == defaultServiceAccountIssuerValue { - //delete(args, "service-account-issuer") - //delete(args, "api-audiences") - args["service-account-jwks-uri"] = kubecontrolplanev1.Arguments{testLBURI} + // Only include the JWKS URI if it is non-empty. This ensures that the + // service-account-jwks-uri flag is omitted from the configuration + // when not required, rather than being passed as an empty string. + if jwksURI != "" { + args["service-account-jwks-uri"] = kubecontrolplanev1.Arguments{jwksURI} } - return &kubecontrolplanev1.KubeAPIServerConfig{ TypeMeta: metav1.TypeMeta{ Kind: "KubeAPIServerConfig", @@ -216,8 +246,8 @@ func apiConfigForIssuer(issuer string, trustedIssuers []string) *kubecontrolplan // unstructuredAPIConfigForIssuer round-trips through the golang type // to ensure the input to the function under test will match what will // be received at runtime. -func unstructuredAPIConfigForIssuer(t *testing.T, issuer string, trustedIssuers []string) map[string]interface{} { - config := apiConfigForIssuer(issuer, trustedIssuers) +func unstructuredAPIConfigForIssuer(t *testing.T, issuer string, trustedIssuers []string, jwksURI string) map[string]interface{} { + config := apiConfigForIssuer(issuer, trustedIssuers, jwksURI) // Unmarshaling to unstructured requires explicitly setting kind config.TypeMeta = metav1.TypeMeta{ Kind: "KubeAPIServerConfig",