From bc65af8e0463a4874443a5aa8ff85aafa6f45146 Mon Sep 17 00:00:00 2001 From: Anish Ramasekar Date: Wed, 21 Feb 2024 15:19:25 -0800 Subject: [PATCH] Support multiple JWT authenticators with structured authn config Signed-off-by: Anish Ramasekar Kubernetes-commit: 39e1c9108c0802024ebb01ad2286b2f09f63798e --- pkg/apis/apiserver/validation/validation.go | 39 ++++---- .../apiserver/validation/validation_test.go | 90 +++++++++++++++++-- 2 files changed, 105 insertions(+), 24 deletions(-) diff --git a/pkg/apis/apiserver/validation/validation.go b/pkg/apis/apiserver/validation/validation.go index 2f54a7e5b..db445f433 100644 --- a/pkg/apis/apiserver/validation/validation.go +++ b/pkg/apis/apiserver/validation/validation.go @@ -45,32 +45,33 @@ func ValidateAuthenticationConfiguration(c *api.AuthenticationConfiguration, dis root := field.NewPath("jwt") var allErrs field.ErrorList - // This stricter validation is solely based on what the current implementation supports. - // TODO(aramase): when StructuredAuthenticationConfiguration feature gate is added and wired up, - // relax this check to allow 0 authenticators. This will allow us to support the case where - // API server is initially configured with no authenticators and then authenticators are added - // later via dynamic config. - if len(c.JWT) == 0 { - allErrs = append(allErrs, field.Required(root, fmt.Sprintf(atLeastOneRequiredErrFmt, root))) + // We allow 0 authenticators in the authentication configuration. + // This allows us to support scenarios where the API server is initially set up without + // any authenticators and then authenticators are added later via dynamic config. + + if len(c.JWT) > 64 { + allErrs = append(allErrs, field.TooMany(root, len(c.JWT), 64)) return allErrs } - // This stricter validation is because the --oidc-* flag option is singular. - // TODO(aramase): when StructuredAuthenticationConfiguration feature gate is added and wired up, - // remove the 1 authenticator limit check and add set the limit to 64. - if len(c.JWT) > 1 { - allErrs = append(allErrs, field.TooMany(root, len(c.JWT), 1)) - return allErrs - } - - // TODO(aramase): right now we only support a single JWT authenticator as - // this is wired to the --oidc-* flags. When StructuredAuthenticationConfiguration - // feature gate is added and wired up, we will remove the 1 authenticator limit - // check and add validation for duplicate issuers. + seenIssuers := sets.New[string]() + seenDiscoveryURLs := sets.New[string]() for i, a := range c.JWT { fldPath := root.Index(i) _, errs := validateJWTAuthenticator(a, fldPath, sets.New(disallowedIssuers...), utilfeature.DefaultFeatureGate.Enabled(features.StructuredAuthenticationConfiguration)) allErrs = append(allErrs, errs...) + + if seenIssuers.Has(a.Issuer.URL) { + allErrs = append(allErrs, field.Duplicate(fldPath.Child("issuer").Child("url"), a.Issuer.URL)) + } + seenIssuers.Insert(a.Issuer.URL) + + if len(a.Issuer.DiscoveryURL) > 0 { + if seenDiscoveryURLs.Has(a.Issuer.DiscoveryURL) { + allErrs = append(allErrs, field.Duplicate(fldPath.Child("issuer").Child("discoveryURL"), a.Issuer.DiscoveryURL)) + } + seenDiscoveryURLs.Insert(a.Issuer.DiscoveryURL) + } } return allErrs diff --git a/pkg/apis/apiserver/validation/validation_test.go b/pkg/apis/apiserver/validation/validation_test.go index 92d205c5a..702a200a4 100644 --- a/pkg/apis/apiserver/validation/validation_test.go +++ b/pkg/apis/apiserver/validation/validation_test.go @@ -58,17 +58,97 @@ func TestValidateAuthenticationConfiguration(t *testing.T) { { name: "jwt authenticator is empty", in: &api.AuthenticationConfiguration{}, - want: "jwt: Required value: at least one jwt is required", + want: "", }, { - name: ">1 jwt authenticator", + name: "duplicate issuer across jwt authenticators", in: &api.AuthenticationConfiguration{ JWT: []api.JWTAuthenticator{ - {Issuer: api.Issuer{URL: "https://issuer-url", Audiences: []string{"audience"}}}, - {Issuer: api.Issuer{URL: "https://issuer-url", Audiences: []string{"audience"}}}, + { + 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"), + }, + }, + }, + { + 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"), + }, + }, + }, }, }, - want: "jwt: Too many: 2: must have at most 1 items", + want: `jwt[1].issuer.url: Duplicate value: "https://issuer-url"`, + }, + { + name: "duplicate discoveryURL across jwt authenticators", + in: &api.AuthenticationConfiguration{ + JWT: []api.JWTAuthenticator{ + { + Issuer: api.Issuer{ + URL: "https://issuer-url", + DiscoveryURL: "https://discovery-url/.well-known/openid-configuration", + Audiences: []string{"audience"}, + }, + ClaimValidationRules: []api.ClaimValidationRule{ + { + Claim: "foo", + RequiredValue: "bar", + }, + }, + ClaimMappings: api.ClaimMappings{ + Username: api.PrefixedClaimOrExpression{ + Claim: "sub", + Prefix: pointer.String("prefix"), + }, + }, + }, + { + Issuer: api.Issuer{ + URL: "https://different-issuer-url", + DiscoveryURL: "https://discovery-url/.well-known/openid-configuration", + Audiences: []string{"audience"}, + }, + ClaimValidationRules: []api.ClaimValidationRule{ + { + Claim: "foo", + RequiredValue: "bar", + }, + }, + ClaimMappings: api.ClaimMappings{ + Username: api.PrefixedClaimOrExpression{ + Claim: "sub", + Prefix: pointer.String("prefix"), + }, + }, + }, + }, + }, + want: `jwt[1].issuer.discoveryURL: Duplicate value: "https://discovery-url/.well-known/openid-configuration"`, }, { name: "failed issuer validation",