From 8345ad0bac4fee6d25f033f0445e2e10eae6afbe Mon Sep 17 00:00:00 2001 From: Monis Khan Date: Wed, 28 Feb 2024 12:53:08 -0500 Subject: [PATCH] jwt: fail on empty username via CEL expression Signed-off-by: Monis Khan --- .../pkg/authenticator/token/oidc/oidc.go | 8 ++- .../pkg/authenticator/token/oidc/oidc_test.go | 54 +++++++++++++- test/integration/apiserver/oidc/oidc_test.go | 71 +++++++++++++++++++ 3 files changed, 131 insertions(+), 2 deletions(-) diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go index b0b633e8273..c7e92e086e5 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc.go @@ -767,7 +767,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 diff --git a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go index f8bf78ed2c7..1591570bcd7 100644 --- a/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go +++ b/staging/src/k8s.io/apiserver/plugin/pkg/authenticator/token/oidc/oidc_test.go @@ -2930,7 +2930,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{ @@ -2945,6 +2945,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 == ""`, diff --git a/test/integration/apiserver/oidc/oidc_test.go b/test/integration/apiserver/oidc/oidc_test.go index ffd73756002..8d8b87f9b69 100644 --- a/test/integration/apiserver/oidc/oidc_test.go +++ b/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 {