Merge pull request #123568 from enj/enj/i/jwt_username_required

jwt: fail on empty username via CEL expression

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

@@ -769,7 +769,13 @@ func (a *Authenticator) getUsername(ctx context.Context, c claims, claimsUnstruc
 			return "", fmt.Errorf("oidc: error evaluating username claim expression: %w", fmt.Errorf("username claim expression must return a string"))
 		}
 
-		return evalResult.EvalResult.Value().(string), nil
+		username := evalResult.EvalResult.Value().(string)
+
+		if len(username) == 0 {
+			return "", fmt.Errorf("oidc: empty username via CEL expression is not allowed")
+		}
+
+		return username, nil
 	}
 
 	var username string

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

@@ -2982,7 +2982,7 @@ func TestToken(t *testing.T) {
 		// test to ensure omitempty fields not included in user info
 		// are set and accessible for CEL evaluation.
 		{
-			name: "test user validation rule doesn't fail when user info is empty",
+			name: "test user validation rule doesn't fail when user info is empty except username",
 			options: Options{
 				JWTAuthenticator: apiserver.JWTAuthenticator{
 					Issuer: apiserver.Issuer{
@@ -2997,6 +2997,58 @@ func TestToken(t *testing.T) {
 							Expression: "claims.groups",
 						},
 					},
+					UserValidationRules: []apiserver.UserValidationRule{
+						{
+							Expression: `user.username == " "`,
+							Message:    "username must be single space",
+						},
+						{
+							Expression: `user.uid == ""`,
+							Message:    "uid must be empty string",
+						},
+						{
+							Expression: `!('bar' in user.groups)`,
+							Message:    "groups must not contain bar",
+						},
+						{
+							Expression: `!('bar' in user.extra)`,
+							Message:    "extra must not contain bar",
+						},
+					},
+				},
+				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": " ",
+				"groups": null,
+				"exp": %d,
+				"baz": "qux"
+			}`, valid.Unix()),
+			want: &user.DefaultInfo{Name: " "},
+		},
+		{
+			name: "empty username is allowed via claim",
+			options: Options{
+				JWTAuthenticator: apiserver.JWTAuthenticator{
+					Issuer: apiserver.Issuer{
+						URL:       "https://auth.example.com",
+						Audiences: []string{"my-client"},
+					},
+					ClaimMappings: apiserver.ClaimMappings{
+						Username: apiserver.PrefixedClaimOrExpression{
+							Claim:  "username",
+							Prefix: pointer.String(""),
+						},
+						Groups: apiserver.PrefixedClaimOrExpression{
+							Expression: "claims.groups",
+						},
+					},
 					UserValidationRules: []apiserver.UserValidationRule{
 						{
 							Expression: `user.username == ""`,

test/integration/apiserver/oidc/oidc_test.go

@@ -325,6 +325,77 @@ jwt:
 				assert.True(t, apierrors.IsUnauthorized(errorToCheck), errorToCheck)
 			},
 		},
+		{
+			name: "ID token is okay but username is empty",
+			configureInfrastructure: func(t *testing.T, fn authenticationConfigFunc, keyFunc func(t *testing.T) (*rsa.PrivateKey, *rsa.PublicKey)) (
+				oidcServer *utilsoidc.TestServer,
+				apiServer *kubeapiserverapptesting.TestServer,
+				signingPrivateKey *rsa.PrivateKey,
+				caCertContent []byte,
+				caFilePath string,
+			) {
+				caCertContent, _, caFilePath, caKeyFilePath := generateCert(t)
+
+				signingPrivateKey, _ = keyFunc(t)
+
+				oidcServer = utilsoidc.BuildAndRunTestServer(t, caFilePath, caKeyFilePath, "")
+
+				if useAuthenticationConfig {
+					authenticationConfig := fmt.Sprintf(`
+apiVersion: apiserver.config.k8s.io/v1alpha1
+kind: AuthenticationConfiguration
+jwt:
+- issuer:
+    url: %s
+    audiences:
+    - %s
+    certificateAuthority: |
+        %s
+  claimMappings:
+    username:
+      expression: claims.sub
+`, oidcServer.URL(), defaultOIDCClientID, indentCertificateAuthority(string(caCertContent)))
+					apiServer = startTestAPIServerForOIDC(t, apiServerOIDCConfig{authenticationConfigYAML: authenticationConfig}, &signingPrivateKey.PublicKey)
+				} else {
+					apiServer = startTestAPIServerForOIDC(t, apiServerOIDCConfig{
+						oidcURL: oidcServer.URL(), oidcClientID: defaultOIDCClientID, oidcCAFilePath: caFilePath, oidcUsernamePrefix: "-",
+					},
+						&signingPrivateKey.PublicKey)
+				}
+
+				oidcServer.JwksHandler().EXPECT().KeySet().AnyTimes().DoAndReturn(utilsoidc.DefaultJwksHandlerBehavior(t, &signingPrivateKey.PublicKey))
+
+				return oidcServer, apiServer, signingPrivateKey, caCertContent, caFilePath
+			},
+			configureOIDCServerBehaviour: func(t *testing.T, oidcServer *utilsoidc.TestServer, signingPrivateKey *rsa.PrivateKey) {
+				oidcServer.TokenHandler().EXPECT().Token().Times(1).DoAndReturn(utilsoidc.TokenHandlerBehaviorReturningPredefinedJWT(
+					t,
+					signingPrivateKey,
+					map[string]interface{}{
+						"iss": oidcServer.URL(),
+						"sub": "",
+						"aud": defaultOIDCClientID,
+						"exp": time.Now().Add(time.Second * 1200).Unix(),
+					},
+					defaultStubAccessToken,
+					defaultStubRefreshToken,
+				))
+			},
+			configureClient: configureClientFetchingOIDCCredentials,
+			assertErrFn: func(t *testing.T, errorToCheck error) {
+				if useAuthenticationConfig { // since the config uses a CEL expression
+					assert.True(t, apierrors.IsUnauthorized(errorToCheck), errorToCheck)
+				} else {
+					// the claim based approach is still allowed to use empty usernames
+					_ = assert.True(t, apierrors.IsForbidden(errorToCheck), errorToCheck) &&
+						assert.Equal(
+							t,
+							`pods is forbidden: User "" cannot list resource "pods" in API group "" in the namespace "default"`,
+							errorToCheck.Error(),
+						)
+				}
+			},
+		},
 	}
 
 	for _, tt := range tests {