Support multiple JWT authenticators with structured authn config

Signed-off-by: Anish Ramasekar <anish.ramasekar@gmail.com>
2024-02-21 15:19:25 -08:00
parent dc3f5ec6cc
commit 39e1c9108c
3 changed files with 212 additions and 24 deletions
--- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation.go
+++ b/staging/src/k8s.io/apiserver/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
--- a/staging/src/k8s.io/apiserver/pkg/apis/apiserver/validation/validation_test.go
+++ b/staging/src/k8s.io/apiserver/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",
--- a/test/integration/apiserver/oidc/oidc_test.go
+++ b/test/integration/apiserver/oidc/oidc_test.go
@@ -1057,6 +1057,113 @@ jwt:
 	}
 }

+func TestMultipleJWTAuthenticators(t *testing.T) {
+	defer featuregatetesting.SetFeatureGateDuringTest(t, utilfeature.DefaultFeatureGate, features.StructuredAuthenticationConfiguration, true)()
+
+	caCertContent1, _, caFilePath1, caKeyFilePath1 := generateCert(t)
+	signingPrivateKey1, publicKey1 := rsaGenerateKey(t)
+	oidcServer1 := utilsoidc.BuildAndRunTestServer(t, caFilePath1, caKeyFilePath1, "")
+
+	caCertContent2, _, caFilePath2, caKeyFilePath2 := generateCert(t)
+	signingPrivateKey2, publicKey2 := rsaGenerateKey(t)
+	oidcServer2 := utilsoidc.BuildAndRunTestServer(t, caFilePath2, caKeyFilePath2, "https://example.com")
+
+	authenticationConfig := fmt.Sprintf(`
+apiVersion: apiserver.config.k8s.io/v1alpha1
+kind: AuthenticationConfiguration
+jwt:
+- issuer:
+    url: %s
+    audiences:
+    - foo
+    audienceMatchPolicy: MatchAny
+    certificateAuthority: |
+        %s
+  claimMappings:
+    username:
+      expression: "'k8s-' + claims.sub"
+  claimValidationRules:
+  - expression: 'claims.hd == "example.com"'
+    message: "the hd claim must be set to example.com"
+- issuer:
+    url: "https://example.com"
+    discoveryURL: %s/.well-known/openid-configuration
+    audiences:
+    - bar
+    audienceMatchPolicy: MatchAny
+    certificateAuthority: |
+        %s
+  claimMappings:
+    username:
+      expression: "'k8s-' + claims.sub"
+    groups:
+      expression: '(claims.roles.split(",") + claims.other_roles.split(",")).map(role, "system:" + role)'
+    uid:
+      expression: "claims.uid"
+`, oidcServer1.URL(), indentCertificateAuthority(string(caCertContent1)), oidcServer2.URL(), indentCertificateAuthority(string(caCertContent2)))
+
+	oidcServer1.JwksHandler().EXPECT().KeySet().AnyTimes().DoAndReturn(utilsoidc.DefaultJwksHandlerBehavior(t, publicKey1))
+	oidcServer2.JwksHandler().EXPECT().KeySet().AnyTimes().DoAndReturn(utilsoidc.DefaultJwksHandlerBehavior(t, publicKey2))
+
+	apiServer := startTestAPIServerForOIDC(t, apiServerOIDCConfig{authenticationConfigYAML: authenticationConfig}, publicKey1)
+
+	idTokenLifetime := time.Second * 1200
+	oidcServer1.TokenHandler().EXPECT().Token().Times(1).DoAndReturn(utilsoidc.TokenHandlerBehaviorReturningPredefinedJWT(
+		t,
+		signingPrivateKey1,
+		map[string]interface{}{
+			"iss": oidcServer1.URL(),
+			"sub": defaultOIDCClaimedUsername,
+			"aud": "foo",
+			"exp": time.Now().Add(idTokenLifetime).Unix(),
+			"hd":  "example.com",
+		},
+		defaultStubAccessToken,
+		defaultStubRefreshToken,
+	))
+
+	oidcServer2.TokenHandler().EXPECT().Token().Times(1).DoAndReturn(utilsoidc.TokenHandlerBehaviorReturningPredefinedJWT(
+		t,
+		signingPrivateKey2,
+		map[string]interface{}{
+			"iss":         "https://example.com",
+			"sub":         "not_john_doe",
+			"aud":         "bar",
+			"roles":       "role1,role2",
+			"other_roles": "role3,role4",
+			"exp":         time.Now().Add(idTokenLifetime).Unix(),
+			"uid":         "1234",
+		},
+		defaultStubAccessToken,
+		defaultStubRefreshToken,
+	))
+
+	tokenURL1, err := oidcServer1.TokenURL()
+	require.NoError(t, err)
+
+	tokenURL2, err := oidcServer2.TokenURL()
+	require.NoError(t, err)
+
+	client1 := configureClientFetchingOIDCCredentials(t, apiServer.ClientConfig, caCertContent1, caFilePath1, oidcServer1.URL(), tokenURL1)
+	client2 := configureClientFetchingOIDCCredentials(t, apiServer.ClientConfig, caCertContent2, caFilePath2, oidcServer2.URL(), tokenURL2)
+
+	ctx := testContext(t)
+	res, err := client1.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, authenticationv1.UserInfo{
+		Username: "k8s-john_doe",
+		Groups:   []string{"system:authenticated"},
+	}, res.Status.UserInfo)
+
+	res, err = client2.AuthenticationV1().SelfSubjectReviews().Create(ctx, &authenticationv1.SelfSubjectReview{}, metav1.CreateOptions{})
+	require.NoError(t, err)
+	assert.Equal(t, authenticationv1.UserInfo{
+		Username: "k8s-not_john_doe",
+		Groups:   []string{"system:role1", "system:role2", "system:role3", "system:role4", "system:authenticated"},
+		UID:      "1234",
+	}, res.Status.UserInfo)
+}
+
 func rsaGenerateKey(t *testing.T) (*rsa.PrivateKey, *rsa.PublicKey) {
 	t.Helper()