Merge pull request kubernetes#123561 from enj/enj/i/validate_jwt_sa_iss

Prevent conflicts between service account and jwt issuers

pkg/kubeapiserver/authenticator/config.go

@@ -162,6 +162,7 @@ func (config Config) New() (authenticator.Request, *spec.SecurityDefinitions, sp
 				JWTAuthenticator:     jwtAuthenticator,
 				CAContentProvider:    oidcCAContent,
 				SupportedSigningAlgs: config.OIDCSigningAlgs,
+				DisallowedIssuers:    config.ServiceAccountIssuers,
 			})
 			if err != nil {
 				return nil, nil, nil, err

pkg/kubeapiserver/options/authentication.go

@@ -533,7 +533,7 @@ func (o *BuiltInAuthenticationOptions) ToAuthenticationConfig() (kubeauthenticat
 	}
 
 	if ret.AuthenticationConfig != nil {
-		if err := apiservervalidation.ValidateAuthenticationConfiguration(ret.AuthenticationConfig).ToAggregate(); err != nil {
+		if err := apiservervalidation.ValidateAuthenticationConfiguration(ret.AuthenticationConfig, ret.ServiceAccountIssuers).ToAggregate(); err != nil {
 			return kubeauthenticator.Config{}, err
 		}
 	}

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go

@@ -41,7 +41,7 @@ import (
 )
 
 // ValidateAuthenticationConfiguration validates a given AuthenticationConfiguration.
-func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) field.ErrorList {
+func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration, disallowedIssuers []string) field.ErrorList {
 	root := field.NewPath("jwt")
 	var allErrs field.ErrorList
 
@@ -69,7 +69,7 @@ func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) fie
 	// check and add validation for duplicate issuers.
 	for i, a := range c.JWT {
 		fldPath := root.Index(i)
-		_, errs := validateJWTAuthenticator(a, fldPath, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration))
+		_, errs := validateJWTAuthenticator(a, fldPath, sets.New(disallowedIssuers...), utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration))
 		allErrs = append(allErrs, errs...)
 	}
 
@@ -79,41 +79,41 @@ func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration) fie
 // CompileAndValidateJWTAuthenticator validates a given JWTAuthenticator and returns a CELMapper with the compiled
 // CEL expressions for claim mappings and validation rules.
 // This is exported for use in oidc package.
-func CompileAndValidateJWTAuthenticator(authenticator api.JWTAuthenticator) (authenticationcel.CELMapper, field.ErrorList) {
-	return validateJWTAuthenticator(authenticator, nil, utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration))
+func CompileAndValidateJWTAuthenticator(authenticator api.JWTAuthenticator, disallowedIssuers []string) (authenticationcel.CELMapper, field.ErrorList) {
+	return validateJWTAuthenticator(authenticator, nil, sets.New(disallowedIssuers...), utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration))
 }
 
-func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field.Path, structuredAuthnFeatureEnabled bool) (authenticationcel.CELMapper, field.ErrorList) {
+func validateJWTAuthenticator(authenticator api.JWTAuthenticator, fldPath *field.Path, disallowedIssuers sets.Set[string], structuredAuthnFeatureEnabled bool) (authenticationcel.CELMapper, field.ErrorList) {
 	var allErrs field.ErrorList
 
 	compiler := authenticationcel.NewCompiler(environment.MustBaseEnvSet(environment.DefaultCompatibilityVersion()))
 	mapper := &authenticationcel.CELMapper{}
 
-	allErrs = append(allErrs, validateIssuer(authenticator.Issuer, fldPath.Child("issuer"))...)
+	allErrs = append(allErrs, validateIssuer(authenticator.Issuer, disallowedIssuers, fldPath.Child("issuer"))...)
 	allErrs = append(allErrs, validateClaimValidationRules(compiler, mapper, authenticator.ClaimValidationRules, fldPath.Child("claimValidationRules"), structuredAuthnFeatureEnabled)...)
 	allErrs = append(allErrs, validateClaimMappings(compiler, mapper, authenticator.ClaimMappings, fldPath.Child("claimMappings"), structuredAuthnFeatureEnabled)...)
 	allErrs = append(allErrs, validateUserValidationRules(compiler, mapper, authenticator.UserValidationRules, fldPath.Child("userValidationRules"), structuredAuthnFeatureEnabled)...)
 
 	return *mapper, allErrs
 }
 
-func validateIssuer(issuer api.Issuer, fldPath *field.Path) field.ErrorList {
+func validateIssuer(issuer api.Issuer, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
 
-	allErrs = append(allErrs, validateIssuerURL(issuer.URL, fldPath.Child("url"))...)
+	allErrs = append(allErrs, validateIssuerURL(issuer.URL, disallowedIssuers, fldPath.Child("url"))...)
 	allErrs = append(allErrs, validateIssuerDiscoveryURL(issuer.URL, issuer.DiscoveryURL, fldPath.Child("discoveryURL"))...)
 	allErrs = append(allErrs, validateAudiences(issuer.Audiences, issuer.AudienceMatchPolicy, fldPath.Child("audiences"), fldPath.Child("audienceMatchPolicy"))...)
 	allErrs = append(allErrs, validateCertificateAuthority(issuer.CertificateAuthority, fldPath.Child("certificateAuthority"))...)
 
 	return allErrs
 }
 
-func validateIssuerURL(issuerURL string, fldPath *field.Path) field.ErrorList {
+func validateIssuerURL(issuerURL string, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList {
 	if len(issuerURL) == 0 {
 		return field.ErrorList{field.Required(fldPath, "URL is required")}
 	}
 
-	return validateURL(issuerURL, fldPath)
+	return validateURL(issuerURL, disallowedIssuers, fldPath)
 }
 
 func validateIssuerDiscoveryURL(issuerURL, issuerDiscoveryURL string, fldPath *field.Path) field.ErrorList {
@@ -127,13 +127,18 @@ func validateIssuerDiscoveryURL(issuerURL, issuerDiscoveryURL string, fldPath *f
 		allErrs = append(allErrs, field.Invalid(fldPath, issuerDiscoveryURL, "discoveryURL must be different from URL"))
 	}
 
-	allErrs = append(allErrs, validateURL(issuerDiscoveryURL, fldPath)...)
+	// issuerDiscoveryURL is not an issuer URL and does not need to validated against any set of disallowed issuers
+	allErrs = append(allErrs, validateURL(issuerDiscoveryURL, nil, fldPath)...)
 	return allErrs
 }
 
-func validateURL(issuerURL string, fldPath *field.Path) field.ErrorList {
+func validateURL(issuerURL string, disallowedIssuers sets.Set[string], fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList
 
+	if disallowedIssuers.Has(issuerURL) {
+		allErrs = append(allErrs, field.Invalid(fldPath, issuerURL, fmt.Sprintf("URL must not overlap with disallowed issuers: %s", sets.List(disallowedIssuers))))
+	}
+
 	u, err := url.Parse(issuerURL)
 	if err != nil {
 		allErrs = append(allErrs, field.Invalid(fldPath, issuerURL, err.Error()))

staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go

@@ -50,9 +50,10 @@ func TestValidateAuthenticationConfiguration(t *testing.T) {
 	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StructuredAuthenticationConfiguration, true)()
 
 	testCases := []struct {
-		name string
-		in   *api.AuthenticationConfiguration
-		want string
+		name              string
+		in                *api.AuthenticationConfiguration
+		disallowedIssuers []string
+		want              string
 	}{
 		{
 			name: "jwt authenticator is empty",
@@ -174,6 +175,33 @@ func TestValidateAuthenticationConfiguration(t *testing.T) {
 			},
 			want: `jwt[0].userValidationRules[1].expression: Duplicate value: "user.username == 'foo'"`,
 		},
+		{
+			name: "valid authentication configuration with disallowed issuer",
+			in: &api.AuthenticationConfiguration{
+				JWT: []api.JWTAuthenticator{
+					{
+						Issuer: api.Issuer{
+							URL:       "https://issuer-url",
+							Audiences: []string{"audience"},
+						},
+						ClaimValidationRules: []api.ClaimValidationRule{
+							{
+								Claim:         "foo",
+								RequiredValue: "bar",
+							},
+						},
+						ClaimMappings: api.ClaimMappings{
+							Username: api.PrefixedClaimOrExpression{
+								Claim:  "sub",
+								Prefix: pointer.String("prefix"),
+							},
+						},
+					},
+				},
+			},
+			disallowedIssuers: []string{"a", "b", "https://issuer-url", "c"},
+			want:              `jwt[0].issuer.url: Invalid value: "https://issuer-url": URL must not overlap with disallowed issuers: [a b c https://issuer-url]`,
+		},
 		{
 			name: "valid authentication configuration",
 			in: &api.AuthenticationConfiguration{
@@ -204,7 +232,7 @@ func TestValidateAuthenticationConfiguration(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			got := ValidateAuthenticationConfiguration(tt.in).ToAggregate()
+			got := ValidateAuthenticationConfiguration(tt.in, tt.disallowedIssuers).ToAggregate()
 			if d := cmp.Diff(tt.want, errString(got)); d != "" {
 				t.Fatalf("AuthenticationConfiguration validation mismatch (-want +got):\n%s", d)
 			}
@@ -216,9 +244,10 @@ func TestValidateIssuerURL(t *testing.T) {
 	fldPath := field.NewPath("issuer", "url")
 
 	testCases := []struct {
-		name string
-		in   string
-		want string
+		name              string
+		in                string
+		disallowedIssuers sets.Set[string]
+		want              string
 	}{
 		{
 			name: "url is empty",
@@ -250,6 +279,12 @@ func TestValidateIssuerURL(t *testing.T) {
 			in:   "https://issuer-url#fragment",
 			want: `issuer.url: Invalid value: "https://issuer-url#fragment": URL must not contain a fragment`,
 		},
+		{
+			name:              "valid url that is disallowed",
+			in:                "https://issuer-url",
+			disallowedIssuers: sets.New("https://issuer-url"),
+			want:              `issuer.url: Invalid value: "https://issuer-url": URL must not overlap with disallowed issuers: [https://issuer-url]`,
+		},
 		{
 			name: "valid url",
 			in:   "https://issuer-url",
@@ -259,7 +294,7 @@ func TestValidateIssuerURL(t *testing.T) {
 
 	for _, tt := range testCases {
 		t.Run(tt.name, func(t *testing.T) {
-			got := validateIssuerURL(tt.in, fldPath).ToAggregate()
+			got := validateIssuerURL(tt.in, tt.disallowedIssuers, fldPath).ToAggregate()
 			if d := cmp.Diff(tt.want, errString(got)); d != "" {
 				t.Fatalf("URL validation mismatch (-want +got):\n%s", d)
 			}

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go

@@ -94,6 +94,8 @@ type Options struct {
 	// https://openid.net/specs/openid-connect-core-1_0.html#IDTokenValidation
 	SupportedSigningAlgs []string
 
+	DisallowedIssuers []string
+
 	// now is used for testing. It defaults to time.Now.
 	now func() time.Time
 }
@@ -227,7 +229,7 @@ var allowedSigningAlgs = map[string]bool{
 }
 
 func New(opts Options) (authenticator.Token, error) {
-	celMapper, fieldErr := apiservervalidation.CompileAndValidateJWTAuthenticator(opts.JWTAuthenticator)
+	celMapper, fieldErr := apiservervalidation.CompileAndValidateJWTAuthenticator(opts.JWTAuthenticator, opts.DisallowedIssuers)
 	if err := fieldErr.ToAggregate(); err != nil {
 		return nil, err
 	}

staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go

@@ -2772,6 +2772,58 @@ func TestToken(t *testing.T) {
 			}`, valid.Unix()),
 			wantInitErr: `claimMappings.extra[2].key: Duplicate value: "example.org/foo"`,
 		},
+		{
+			name: "disallowed issuer via configured value",
+			options: Options{
+				JWTAuthenticator: apiserver.JWTAuthenticator{
+					Issuer: apiserver.Issuer{
+						URL:       "https://auth.example.com",
+						Audiences: []string{"my-client"},
+					},
+					ClaimMappings: apiserver.ClaimMappings{
+						Username: apiserver.PrefixedClaimOrExpression{
+							Expression: "claims.username",
+						},
+						Groups: apiserver.PrefixedClaimOrExpression{
+							Expression: "claims.groups",
+						},
+						UID: apiserver.ClaimOrExpression{
+							Expression: "claims.uid",
+						},
+						Extra: []apiserver.ExtraMapping{
+							{
+								Key:             "example.org/foo",
+								ValueExpression: "claims.foo",
+							},
+							{
+								Key:             "example.org/bar",
+								ValueExpression: "claims.bar",
+							},
+						},
+					},
+				},
+				DisallowedIssuers: []string{"https://auth.example.com"},
+				now:               func() time.Time { return now },
+			},
+			signingKey: loadRSAPrivKey(t, "testdata/rsa_1.pem", jose.RS256),
+			pubKeys: []*jose.JSONWebKey{
+				loadRSAKey(t, "testdata/rsa_1.pem", jose.RS256),
+			},
+			claims: fmt.Sprintf(`{
+				"iss": "https://auth.example.com",
+				"aud": "my-client",
+				"username": "jane",
+				"groups": ["team1", "team2"],
+				"exp": %d,
+				"uid": "1234",
+				"foo": "bar",
+				"bar": [
+					"baz",
+					"qux"
+				]
+			}`, valid.Unix()),
+			wantInitErr: `issuer.url: Invalid value: "https://auth.example.com": URL must not overlap with disallowed issuers: [https://auth.example.com]`,
+		},
 		{
 			name: "extra claim mapping, empty string value for key",
 			options: Options{