From afcd5e3e363b9a5d37b199b2396d546f7c2698ee Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Mon, 14 Dec 2020 17:05:53 -0800 Subject: [PATCH 01/12] WIP: Adjust subject and username claims Signed-off-by: Ryan Richard --- internal/oidc/callback/callback_handler.go | 98 +++++++++++-------- .../oidc/callback/callback_handler_test.go | 23 +++-- 2 files changed, 75 insertions(+), 46 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 2cea6c2e..3bdcd9ee 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -30,9 +30,10 @@ const ( // The name of the subject claim specified in the OIDC spec. idTokenSubjectClaim = "sub" - // defaultUpstreamUsernameClaim is what we will use to extract the username from an upstream OIDC - // ID token if the upstream OIDC IDP did not tell us to use another claim. - defaultUpstreamUsernameClaim = idTokenSubjectClaim + // idTokenUsernameClaim is a custom claim in the downstream ID token + // whose value is mapped from a claim in the upstream token. + // By default the value is the same as the downstream subject claim's. + idTokenUsernameClaim = "username" // downstreamGroupsClaim is what we will use to encode the groups in the downstream OIDC ID token // information. @@ -88,7 +89,7 @@ func NewHandler( return httperr.New(http.StatusBadGateway, "error exchanging and validating upstream tokens") } - username, err := getUsernameFromUpstreamIDToken(upstreamIDPConfig, token.IDToken.Claims) + subject, username, err := getSubjectAndUsernameFromUpstreamIDToken(upstreamIDPConfig, token.IDToken.Claims) if err != nil { return err } @@ -98,7 +99,7 @@ func NewHandler( return err } - openIDSession := makeDownstreamSession(username, groups) + openIDSession := makeDownstreamSession(subject, username, groups) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { plog.WarningErr("error while generating and saving authcode", err, "upstreamName", upstreamIDPConfig.GetName()) @@ -192,36 +193,54 @@ func readState(r *http.Request, stateDecoder oidc.Decoder) (*oidc.UpstreamStateP return &state, nil } -func getUsernameFromUpstreamIDToken( +func getSubjectAndUsernameFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, -) (string, error) { - usernameClaim := upstreamIDPConfig.GetUsernameClaim() +) (string, string, error) { + // The spec says the "sub" claim is only unique per issuer, + // so we will prepend the issuer string to make it globally unique. + upstreamIssuer := idTokenClaims[idTokenIssuerClaim] + if upstreamIssuer == "" { + plog.Warning( + "issuer claim in upstream ID token missing", + "upstreamName", upstreamIDPConfig.GetName(), + "issClaim", upstreamIssuer, + ) + return "", "", httperr.New(http.StatusUnprocessableEntity, "issuer claim in upstream ID token missing") + } + upstreamIssuerAsString, ok := upstreamIssuer.(string) + if !ok { + plog.Warning( + "issuer claim in upstream ID token has invalid format", + "upstreamName", upstreamIDPConfig.GetName(), + "issClaim", upstreamIssuer, + ) + return "", "", httperr.New(http.StatusUnprocessableEntity, "issuer claim in upstream ID token has invalid format") + } - user := "" + subjectAsInterface, ok := idTokenClaims[idTokenSubjectClaim] + if !ok { + plog.Warning( + "no subject claim in upstream ID token", + "upstreamName", upstreamIDPConfig.GetName(), + ) + return "", "", httperr.New(http.StatusUnprocessableEntity, "no subject claim in upstream ID token") + } + + upstreamSubject, ok := subjectAsInterface.(string) + if !ok { + plog.Warning( + "subject claim in upstream ID token has invalid format", + "upstreamName", upstreamIDPConfig.GetName(), + ) + return "", "", httperr.New(http.StatusUnprocessableEntity, "subject claim in upstream ID token has invalid format") + } + + subject := fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, idTokenSubjectClaim, upstreamSubject) + + usernameClaim := upstreamIDPConfig.GetUsernameClaim() if usernameClaim == "" { - // The spec says the "sub" claim is only unique per issuer, so by default when there is - // no specific username claim configured we will prepend the issuer string to make it globally unique. - upstreamIssuer := idTokenClaims[idTokenIssuerClaim] - if upstreamIssuer == "" { - plog.Warning( - "issuer claim in upstream ID token missing", - "upstreamName", upstreamIDPConfig.GetName(), - "issClaim", upstreamIssuer, - ) - return "", httperr.New(http.StatusUnprocessableEntity, "issuer claim in upstream ID token missing") - } - upstreamIssuerAsString, ok := upstreamIssuer.(string) - if !ok { - plog.Warning( - "issuer claim in upstream ID token has invalid format", - "upstreamName", upstreamIDPConfig.GetName(), - "issClaim", upstreamIssuer, - ) - return "", httperr.New(http.StatusUnprocessableEntity, "issuer claim in upstream ID token has invalid format") - } - user = fmt.Sprintf("%s?%s=", upstreamIssuerAsString, idTokenSubjectClaim) - usernameClaim = defaultUpstreamUsernameClaim + return subject, subject, nil } usernameAsInterface, ok := idTokenClaims[usernameClaim] @@ -232,7 +251,7 @@ func getUsernameFromUpstreamIDToken( "configuredUsernameClaim", upstreamIDPConfig.GetUsernameClaim(), "usernameClaim", usernameClaim, ) - return "", httperr.New(http.StatusUnprocessableEntity, "no username claim in upstream ID token") + return "", "", httperr.New(http.StatusUnprocessableEntity, "no username claim in upstream ID token") } username, ok := usernameAsInterface.(string) @@ -243,10 +262,10 @@ func getUsernameFromUpstreamIDToken( "configuredUsernameClaim", upstreamIDPConfig.GetUsernameClaim(), "usernameClaim", usernameClaim, ) - return "", httperr.New(http.StatusUnprocessableEntity, "username claim in upstream ID token has invalid format") + return "", "", httperr.New(http.StatusUnprocessableEntity, "username claim in upstream ID token has invalid format") } - return fmt.Sprintf("%s%s", user, username), nil + return subject, username, nil } func getGroupsFromUpstreamIDToken( @@ -283,19 +302,20 @@ func getGroupsFromUpstreamIDToken( return groups, nil } -func makeDownstreamSession(username string, groups []string) *openid.DefaultSession { +func makeDownstreamSession(subject string, username string, groups []string) *openid.DefaultSession { now := time.Now().UTC() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ - Subject: username, + Subject: subject, RequestedAt: now, AuthTime: now, }, } + openIDSession.Claims.Extra = map[string]interface{}{ + idTokenUsernameClaim: username, + } if groups != nil { - openIDSession.Claims.Extra = map[string]interface{}{ - downstreamGroupsClaim: groups, - } + openIDSession.Claims.Extra[downstreamGroupsClaim] = groups } return openIDSession } diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 38d12b29..eb7e4eea 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -133,6 +133,7 @@ func TestCallbackEndpoint(t *testing.T) { wantRedirectLocationRegexp string wantDownstreamGrantedScopes []string wantDownstreamIDTokenSubject string + wantDownstreamIDTokenUsername string wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string wantDownstreamNonce string @@ -150,7 +151,8 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamUsername, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, @@ -169,6 +171,7 @@ func TestCallbackEndpoint(t *testing.T) { wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenGroups: nil, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, @@ -186,7 +189,8 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenUsername: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, @@ -312,7 +316,8 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=&state=` + happyDownstreamState, - wantDownstreamIDTokenSubject: upstreamUsername, + wantDownstreamIDTokenUsername: upstreamUsername, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamRequestedScopes: []string{"profile", "email"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamNonce: downstreamNonce, @@ -333,7 +338,8 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid%20offline_access&state=` + happyDownstreamState, - wantDownstreamIDTokenSubject: upstreamUsername, + wantDownstreamIDTokenUsername: upstreamUsername, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, @@ -522,6 +528,7 @@ func TestCallbackEndpoint(t *testing.T) { authcodeDataAndSignature[1], // Authcode store key is authcode signature test.wantDownstreamGrantedScopes, test.wantDownstreamIDTokenSubject, + test.wantDownstreamIDTokenUsername, test.wantDownstreamIDTokenGroups, test.wantDownstreamRequestedScopes, ) @@ -742,6 +749,7 @@ func validateAuthcodeStorage( storeKey string, wantDownstreamGrantedScopes []string, wantDownstreamIDTokenSubject string, + wantDownstreamIDTokenUsername string, wantDownstreamIDTokenGroups []string, wantDownstreamRequestedScopes []string, ) (*fosite.Request, *openid.DefaultSession) { @@ -778,13 +786,14 @@ func validateAuthcodeStorage( // Now confirm the ID token claims. actualClaims := storedSessionFromAuthcode.Claims - // Check the user's identity, which are put into the downstream ID token's subject and groups claims. + // Check the user's identity, which are put into the downstream ID token's subject, username and groups claims. require.Equal(t, wantDownstreamIDTokenSubject, actualClaims.Subject) + require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"]) if wantDownstreamIDTokenGroups != nil { - require.Len(t, actualClaims.Extra, 1) + require.Len(t, actualClaims.Extra, 2) require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualClaims.Extra["groups"]) } else { - require.Empty(t, actualClaims.Extra) + require.Len(t, actualClaims.Extra, 1) require.NotContains(t, actualClaims.Extra, "groups") } From 16dfab0aff3f21cc62ab6bb1006cbf1a79d71ed3 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Mon, 14 Dec 2020 18:27:14 -0800 Subject: [PATCH 02/12] token_handler_test.go: Add tests for username and groups custom claims --- internal/oidc/callback/callback_handler.go | 27 ++----- internal/oidc/oidc.go | 15 ++++ internal/oidc/token/token_handler_test.go | 85 +++++++++++++++++----- 3 files changed, 88 insertions(+), 39 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 3bdcd9ee..97731aec 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -23,23 +23,6 @@ import ( "go.pinniped.dev/internal/plog" ) -const ( - // The name of the issuer claim specified in the OIDC spec. - idTokenIssuerClaim = "iss" - - // The name of the subject claim specified in the OIDC spec. - idTokenSubjectClaim = "sub" - - // idTokenUsernameClaim is a custom claim in the downstream ID token - // whose value is mapped from a claim in the upstream token. - // By default the value is the same as the downstream subject claim's. - idTokenUsernameClaim = "username" - - // downstreamGroupsClaim is what we will use to encode the groups in the downstream OIDC ID token - // information. - downstreamGroupsClaim = "groups" -) - func NewHandler( idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provider, @@ -199,7 +182,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( ) (string, string, error) { // The spec says the "sub" claim is only unique per issuer, // so we will prepend the issuer string to make it globally unique. - upstreamIssuer := idTokenClaims[idTokenIssuerClaim] + upstreamIssuer := idTokenClaims[oidc.IDTokenIssuerClaim] if upstreamIssuer == "" { plog.Warning( "issuer claim in upstream ID token missing", @@ -218,7 +201,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return "", "", httperr.New(http.StatusUnprocessableEntity, "issuer claim in upstream ID token has invalid format") } - subjectAsInterface, ok := idTokenClaims[idTokenSubjectClaim] + subjectAsInterface, ok := idTokenClaims[oidc.IDTokenSubjectClaim] if !ok { plog.Warning( "no subject claim in upstream ID token", @@ -236,7 +219,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return "", "", httperr.New(http.StatusUnprocessableEntity, "subject claim in upstream ID token has invalid format") } - subject := fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, idTokenSubjectClaim, upstreamSubject) + subject := fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, upstreamSubject) usernameClaim := upstreamIDPConfig.GetUsernameClaim() if usernameClaim == "" { @@ -312,10 +295,10 @@ func makeDownstreamSession(subject string, username string, groups []string) *op }, } openIDSession.Claims.Extra = map[string]interface{}{ - idTokenUsernameClaim: username, + oidc.DownstreamUsernameClaim: username, } if groups != nil { - openIDSession.Claims.Extra[downstreamGroupsClaim] = groups + openIDSession.Claims.Extra[oidc.DownstreamGroupsClaim] = groups } return openIDSession } diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index a1cb56fe..d53ee663 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -44,6 +44,21 @@ const ( // CSRFCookieEncodingName is the `name` passed to the encoder for encoding and decoding the CSRF // cookie contents. CSRFCookieEncodingName = "csrf" + + // The name of the issuer claim specified in the OIDC spec. + IDTokenIssuerClaim = "iss" + + // The name of the subject claim specified in the OIDC spec. + IDTokenSubjectClaim = "sub" + + // DownstreamUsernameClaim is a custom claim in the downstream ID token + // whose value is mapped from a claim in the upstream token. + // By default the value is the same as the downstream subject claim's. + DownstreamUsernameClaim = "username" + + // DownstreamGroupsClaim is what we will use to encode the groups in the downstream OIDC ID token + // information. + DownstreamGroupsClaim = "groups" ) // Encoder is the encoding side of the securecookie.Codec interface. diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index f4610293..0f31e1dd 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -53,8 +53,9 @@ const ( goodRedirectURI = "http://127.0.0.1/callback" goodPKCECodeVerifier = "some-pkce-verifier-that-must-be-at-least-43-characters-to-meet-entropy-requirements" goodNonce = "some-nonce-value-with-enough-bytes-to-exceed-min-allowed" - goodSubject = "some-subject" + goodSubject = "https://issuer?sub=some-subject" goodUsername = "some-username" + goodGroups = "group1,groups2" hmacSecret = "this needs to be at least 32 characters to meet entropy requirements" @@ -781,11 +782,13 @@ func TestTokenExchange(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject, rsp, _, _, secrets, storage := exchangeAuthcodeForTokens(t, test.authcodeExchange) - var parsedResponseBody map[string]interface{} - require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedResponseBody)) + t.Parallel() - request := happyTokenExchangeRequest(test.requestedAudience, parsedResponseBody["access_token"].(string)) + subject, rsp, _, _, secrets, storage := exchangeAuthcodeForTokens(t, test.authcodeExchange) + var parsedAuthcodeExchangeResponseBody map[string]interface{} + require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &parsedAuthcodeExchangeResponseBody)) + + request := happyTokenExchangeRequest(test.requestedAudience, parsedAuthcodeExchangeResponseBody["access_token"].(string)) if test.modifyStorage != nil { test.modifyStorage(t, storage, request) } @@ -801,6 +804,10 @@ func TestTokenExchange(t *testing.T) { existingSecrets, err := secrets.List(context.Background(), metav1.ListOptions{}) require.NoError(t, err) + // Wait one second before performing the token exchange so we can see that the new ID token has new issued + // at and expires at dates which are newer than the old tokens. + time.Sleep(1 * time.Second) + subject.ServeHTTP(rsp, req) t.Logf("response: %#v", rsp) t.Logf("response body: %q", rsp.Body.String()) @@ -816,6 +823,12 @@ func TestTokenExchange(t *testing.T) { return } + claimsOfFirstIDToken := map[string]interface{}{} + originalIDToken := parsedAuthcodeExchangeResponseBody["id_token"].(string) + firstIDTokenDecoded, _ := josejwt.ParseSigned(originalIDToken) + err = firstIDTokenDecoded.UnsafeClaimsWithoutVerification(&claimsOfFirstIDToken) + require.NoError(t, err) + var responseBody map[string]interface{} require.NoError(t, json.Unmarshal(rsp.Body.Bytes(), &responseBody)) @@ -823,18 +836,43 @@ func TestTokenExchange(t *testing.T) { require.Equal(t, "N_A", responseBody["token_type"]) require.Equal(t, "urn:ietf:params:oauth:token-type:jwt", responseBody["issued_token_type"]) - // Assert that the returned token has expected claims. + // Parse the returned token. parsedJWT, err := jose.ParseSigned(responseBody["access_token"].(string)) require.NoError(t, err) var tokenClaims map[string]interface{} require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims)) - require.Contains(t, tokenClaims, "iat") - require.Contains(t, tokenClaims, "rat") - require.Contains(t, tokenClaims, "jti") + + // Make sure that these are the only fields in the token. + idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "exp", "iat", "rat", "groups", "username"} + require.ElementsMatch(t, idTokenFields, getMapKeys(tokenClaims)) + + // Assert that the returned token has expected claims values. + require.NotEmpty(t, tokenClaims["jti"]) + require.NotEmpty(t, tokenClaims["auth_time"]) + require.NotEmpty(t, tokenClaims["exp"]) + require.NotEmpty(t, tokenClaims["iat"]) + require.NotEmpty(t, tokenClaims["rat"]) + require.Empty(t, tokenClaims["nonce"]) // ID tokens only contain nonce during an authcode exchange require.Len(t, tokenClaims["aud"], 1) require.Contains(t, tokenClaims["aud"], test.requestedAudience) require.Equal(t, goodSubject, tokenClaims["sub"]) require.Equal(t, goodIssuer, tokenClaims["iss"]) + require.Equal(t, goodUsername, tokenClaims["username"]) + require.Equal(t, goodGroups, tokenClaims["groups"]) + + // Also assert that some are the same as the original downstream ID token. + requireClaimsAreEqual(t, "iss", claimsOfFirstIDToken, tokenClaims) // issuer + requireClaimsAreEqual(t, "sub", claimsOfFirstIDToken, tokenClaims) // subject + requireClaimsAreEqual(t, "rat", claimsOfFirstIDToken, tokenClaims) // requested at + requireClaimsAreEqual(t, "auth_time", claimsOfFirstIDToken, tokenClaims) // auth time + + // Also assert which are the different from the original downstream ID token. + requireClaimsAreNotEqual(t, "jti", claimsOfFirstIDToken, tokenClaims) // JWT ID + requireClaimsAreNotEqual(t, "aud", claimsOfFirstIDToken, tokenClaims) // audience + requireClaimsAreNotEqual(t, "exp", claimsOfFirstIDToken, tokenClaims) // expires at + require.Greater(t, tokenClaims["exp"], claimsOfFirstIDToken["exp"]) + requireClaimsAreNotEqual(t, "iat", claimsOfFirstIDToken, tokenClaims) // issued at + require.Greater(t, tokenClaims["iat"], claimsOfFirstIDToken["iat"]) // Assert that nothing in storage has been modified. newSecrets, err := secrets.List(context.Background(), metav1.ListOptions{}) @@ -1386,11 +1424,15 @@ func simulateAuthEndpointHavingAlreadyRun(t *testing.T, authRequest *http.Reques session := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ Subject: goodSubject, - AuthTime: goodAuthTime, RequestedAt: goodRequestedAtTime, + AuthTime: goodAuthTime, + Extra: map[string]interface{}{ + oidc.DownstreamUsernameClaim: goodUsername, + oidc.DownstreamGroupsClaim: goodGroups, + }, }, - Subject: goodSubject, - Username: goodUsername, + Subject: "", // not used, note that callback_handler.go does not set this + Username: "", // not used, note that callback_handler.go does not set this } authRequester, err := oauthHelper.NewAuthorizeRequest(ctx, authRequest) require.NoError(t, err) @@ -1609,6 +1651,12 @@ func requireValidStoredRequest( require.Empty(t, claims.JTI) // When claims.JTI is empty, Fosite will generate a UUID for this field. require.Equal(t, goodSubject, claims.Subject) + // Our custom claims from the authorize endpoint should still be set. + require.Equal(t, map[string]interface{}{ + "username": goodUsername, + "groups": goodGroups, + }, claims.Extra) + // We are in charge of setting these fields. For the purpose of testing, we ensure that the // sentinel test value is set correctly. require.Equal(t, goodRequestedAtTime, claims.RequestedAt) @@ -1633,7 +1681,6 @@ func requireValidStoredRequest( require.Empty(t, claims.AuthenticationContextClassReference) require.Empty(t, claims.AuthenticationMethodsReference) require.Empty(t, claims.CodeHash) - require.Empty(t, claims.Extra) } // Assert that the session headers are what we think they should be. @@ -1664,9 +1711,9 @@ func requireValidStoredRequest( require.False(t, ok, "expected session to not hold expiration time for access token, but it did") } - // Assert that the session's username and subject are correct. - require.Equal(t, goodUsername, session.Username) - require.Equal(t, goodSubject, session.Subject) + // We don't use these, so they should be empty. + require.Empty(t, session.Username) + require.Empty(t, session.Subject) } func requireValidIDToken( @@ -1698,12 +1745,14 @@ func requireValidIDToken( IssuedAt int64 `json:"iat"` RequestedAt int64 `json:"rat"` AuthTime int64 `json:"auth_time"` + Groups string `json:"groups"` + Username string `json:"username"` } // Note that there is a bug in fosite which prevents the `at_hash` claim from appearing in this ID token // during the initial authcode exchange, but does not prevent `at_hash` from appearing in the refreshed ID token. // We can add a workaround for this later. - idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "exp", "iat", "rat"} + idTokenFields := []string{"sub", "aud", "iss", "jti", "nonce", "auth_time", "exp", "iat", "rat", "groups", "username"} if wantAtHashClaimInIDToken { idTokenFields = append(idTokenFields, "at_hash") } @@ -1717,6 +1766,8 @@ func requireValidIDToken( err := token.Claims(&claims) require.NoError(t, err) require.Equal(t, goodSubject, claims.Subject) + require.Equal(t, goodUsername, claims.Username) + require.Equal(t, goodGroups, claims.Groups) require.Len(t, claims.Audience, 1) require.Equal(t, goodClient, claims.Audience[0]) require.Equal(t, goodIssuer, claims.Issuer) From 43bb7117b7cb2eb6671d7f0145e8bdf2f36119e9 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Dec 2020 08:34:24 -0800 Subject: [PATCH 03/12] Allow upstream group claim values to be either arrays or strings --- internal/oidc/callback/callback_handler.go | 14 ++++--- .../oidc/callback/callback_handler_test.go | 41 ++++++++++++++++--- 2 files changed, 44 insertions(+), 11 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 97731aec..c63a544b 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -254,7 +254,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( func getGroupsFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, -) ([]string, error) { +) (interface{}, error) { groupsClaim := upstreamIDPConfig.GetGroupsClaim() if groupsClaim == "" { return nil, nil @@ -271,8 +271,9 @@ func getGroupsFromUpstreamIDToken( return nil, httperr.New(http.StatusUnprocessableEntity, "no groups claim in upstream ID token") } - groups, ok := groupsAsInterface.([]string) - if !ok { + groupsAsArray, okAsArray := groupsAsInterface.([]string) + groupsAsString, okAsString := groupsAsInterface.(string) + if !okAsArray && !okAsString { plog.Warning( "groups claim in upstream ID token has invalid format", "upstreamName", upstreamIDPConfig.GetName(), @@ -282,10 +283,13 @@ func getGroupsFromUpstreamIDToken( return nil, httperr.New(http.StatusUnprocessableEntity, "groups claim in upstream ID token has invalid format") } - return groups, nil + if okAsArray { + return groupsAsArray, nil + } + return groupsAsString, nil } -func makeDownstreamSession(subject string, username string, groups []string) *openid.DefaultSession { +func makeDownstreamSession(subject string, username string, groups interface{}) *openid.DefaultSession { now := time.Now().UTC() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index eb7e4eea..2bb1411a 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -134,7 +134,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamGrantedScopes []string wantDownstreamIDTokenSubject string wantDownstreamIDTokenUsername string - wantDownstreamIDTokenGroups []string + wantDownstreamIDTokenGroups interface{} wantDownstreamRequestedScopes []string wantDownstreamNonce string wantDownstreamPKCEChallenge string @@ -199,6 +199,25 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, + { + name: "upstream IDP's configured groups claim in the ID token has a non-array value", + idp: happyUpstream().WithIDTokenClaim(upstreamGroupsClaim, "notAnArrayGroup1 notAnArrayGroup2").Build(), + method: http.MethodGet, + path: newRequestPath().WithState(happyState).String(), + csrfCookie: happyCSRFCookie, + wantStatus: http.StatusFound, + wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, + wantBody: "", + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenUsername: upstreamUsername, + wantDownstreamIDTokenGroups: "notAnArrayGroup1 notAnArrayGroup2", + wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamGrantedScopes: happyDownstreamScopesGranted, + wantDownstreamNonce: downstreamNonce, + wantDownstreamPKCEChallenge: downstreamPKCEChallenge, + wantDownstreamPKCEChallengeMethod: downstreamPKCEChallengeMethod, + wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, + }, // Pre-upstream-exchange verification { @@ -682,8 +701,8 @@ func happyUpstream() *upstreamOIDCIdentityProviderBuilder { } } -func (u *upstreamOIDCIdentityProviderBuilder) WithUsernameClaim(claim string) *upstreamOIDCIdentityProviderBuilder { - u.usernameClaim = claim +func (u *upstreamOIDCIdentityProviderBuilder) WithUsernameClaim(value string) *upstreamOIDCIdentityProviderBuilder { + u.usernameClaim = value return u } @@ -750,7 +769,7 @@ func validateAuthcodeStorage( wantDownstreamGrantedScopes []string, wantDownstreamIDTokenSubject string, wantDownstreamIDTokenUsername string, - wantDownstreamIDTokenGroups []string, + wantDownstreamIDTokenGroups interface{}, wantDownstreamRequestedScopes []string, ) (*fosite.Request, *openid.DefaultSession) { t.Helper() @@ -789,9 +808,19 @@ func validateAuthcodeStorage( // Check the user's identity, which are put into the downstream ID token's subject, username and groups claims. require.Equal(t, wantDownstreamIDTokenSubject, actualClaims.Subject) require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"]) - if wantDownstreamIDTokenGroups != nil { + if wantDownstreamIDTokenGroups != nil { //nolint:nestif // there are some nested if's here but its probably fine for a test require.Len(t, actualClaims.Extra, 2) - require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualClaims.Extra["groups"]) + wantArray, ok := wantDownstreamIDTokenGroups.([]string) + if ok { + require.ElementsMatch(t, wantArray, actualClaims.Extra["groups"]) + } else { + wantString, ok := wantDownstreamIDTokenGroups.(string) + if ok { + require.Equal(t, wantString, actualClaims.Extra["groups"]) + } else { + require.Fail(t, "wantDownstreamIDTokenGroups should be of type: either []string or string") + } + } } else { require.Len(t, actualClaims.Extra, 1) require.NotContains(t, actualClaims.Extra, "groups") From 0e60c93cef2a707460f8e5069ae2bac96d38e18b Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Dec 2020 10:36:19 -0800 Subject: [PATCH 04/12] Add UsernameClaim and GroupsClaim to JWTAuthenticator CRD spec Signed-off-by: Margo Crawford --- .../authentication/v1alpha1/types_jwt.go.tmpl | 10 ++++++++++ ...ation.concierge.pinniped.dev_jwtauthenticators.yaml | 10 ++++++++++ generated/1.17/README.adoc | 2 ++ .../concierge/authentication/v1alpha1/types_jwt.go | 10 ++++++++++ ...ation.concierge.pinniped.dev_jwtauthenticators.yaml | 10 ++++++++++ generated/1.18/README.adoc | 2 ++ .../concierge/authentication/v1alpha1/types_jwt.go | 10 ++++++++++ ...ation.concierge.pinniped.dev_jwtauthenticators.yaml | 10 ++++++++++ generated/1.19/README.adoc | 2 ++ .../concierge/authentication/v1alpha1/types_jwt.go | 10 ++++++++++ ...ation.concierge.pinniped.dev_jwtauthenticators.yaml | 10 ++++++++++ 11 files changed, 86 insertions(+) diff --git a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl index 5d0604bd..b08e8279 100644 --- a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl +++ b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl @@ -27,6 +27,16 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` + // UsernameClaim is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + UsernameClaim string `json:"username_claim"` + + // GroupsClaim is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + GroupsClaim string `json:"groups_claim"` + // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` diff --git a/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index ed16bf0c..0865ca3c 100644 --- a/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,6 +51,11 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string + groups_claim: + description: GroupsClaim is the name of the claim which should be + read to extract the user's group membership from the JWT token. + When not specified, it will default to "groups". + type: string issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -66,6 +71,11 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object + username_claim: + description: UsernameClaim is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string required: - audience - issuer diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index 0ccdd1df..1f29282c 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -92,6 +92,8 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. +| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== diff --git a/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go index 5d0604bd..1c43f68f 100644 --- a/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,6 +27,16 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` + // UsernameClaim is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + UsernameClaim string `json:"username_claim"` + + // GroupsClaim is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + GroupsClaim string `json:"groups_claim"` + // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` diff --git a/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index ed16bf0c..0865ca3c 100644 --- a/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,6 +51,11 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string + groups_claim: + description: GroupsClaim is the name of the claim which should be + read to extract the user's group membership from the JWT token. + When not specified, it will default to "groups". + type: string issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -66,6 +71,11 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object + username_claim: + description: UsernameClaim is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string required: - audience - issuer diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 97042b25..2cef60f4 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -92,6 +92,8 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. +| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== diff --git a/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go index 5d0604bd..1c43f68f 100644 --- a/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,6 +27,16 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` + // UsernameClaim is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + UsernameClaim string `json:"username_claim"` + + // GroupsClaim is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + GroupsClaim string `json:"groups_claim"` + // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` diff --git a/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index ed16bf0c..0865ca3c 100644 --- a/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,6 +51,11 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string + groups_claim: + description: GroupsClaim is the name of the claim which should be + read to extract the user's group membership from the JWT token. + When not specified, it will default to "groups". + type: string issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -66,6 +71,11 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object + username_claim: + description: UsernameClaim is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string required: - audience - issuer diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index edda33b8..bfb3c44f 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -92,6 +92,8 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. +| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== diff --git a/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go index 5d0604bd..1c43f68f 100644 --- a/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,6 +27,16 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` + // UsernameClaim is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + UsernameClaim string `json:"username_claim"` + + // GroupsClaim is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + GroupsClaim string `json:"groups_claim"` + // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` diff --git a/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index ed16bf0c..0865ca3c 100644 --- a/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,6 +51,11 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string + groups_claim: + description: GroupsClaim is the name of the claim which should be + read to extract the user's group membership from the JWT token. + When not specified, it will default to "groups". + type: string issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -66,6 +71,11 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object + username_claim: + description: UsernameClaim is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string required: - audience - issuer From 720bc7ae425816d83c1b5d59c28be7024babf0a2 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 15 Dec 2020 13:33:49 -0800 Subject: [PATCH 05/12] jwtcachefiller_test.go: refactor and remove "if short skip" check - Refactor the test to avoid testing a private method and instead always test the results of running the controller. - Also remove the `if testing.Short()` check because it will always be short when running unit tests. This prevented the unit test from ever running, both locally and in CI. Signed-off-by: Ryan Richard --- .../jwtcachefiller/jwtcachefiller_test.go | 578 ++++++++++-------- 1 file changed, 309 insertions(+), 269 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index eaafa798..7c230cd9 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -41,225 +41,10 @@ import ( func TestController(t *testing.T) { t.Parallel() - someJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: "https://some-issuer.com", - Audience: "some-audience", - TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURVVENDQWptZ0F3SUJBZ0lWQUpzNStTbVRtaTJXeUI0bGJJRXBXaUs5a1RkUE1BMEdDU3FHU0liM0RRRUIKQ3dVQU1COHhDekFKQmdOVkJBWVRBbFZUTVJBd0RnWURWUVFLREFkUWFYWnZkR0ZzTUI0WERUSXdNRFV3TkRFMgpNamMxT0ZvWERUSTBNRFV3TlRFMk1qYzFPRm93SHpFTE1Ba0dBMVVFQmhNQ1ZWTXhFREFPQmdOVkJBb01CMUJwCmRtOTBZV3d3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRRERZWmZvWGR4Z2NXTEMKZEJtbHB5a0tBaG9JMlBuUWtsVFNXMno1cGcwaXJjOGFRL1E3MXZzMTRZYStmdWtFTGlvOTRZYWw4R01DdVFrbApMZ3AvUEE5N1VYelhQNDBpK25iNXcwRGpwWWd2dU9KQXJXMno2MFRnWE5NSFh3VHk4ME1SZEhpUFVWZ0VZd0JpCmtkNThzdEFVS1Y1MnBQTU1reTJjNy9BcFhJNmRXR2xjalUvaFBsNmtpRzZ5dEw2REtGYjJQRWV3MmdJM3pHZ2IKOFVVbnA1V05DZDd2WjNVY0ZHNXlsZEd3aGc3cnZ4U1ZLWi9WOEhCMGJmbjlxamlrSVcxWFM4dzdpUUNlQmdQMApYZWhKZmVITlZJaTJtZlczNlVQbWpMdnVKaGpqNDIrdFBQWndvdDkzdWtlcEgvbWpHcFJEVm9wamJyWGlpTUYrCkYxdnlPNGMxQWdNQkFBR2pnWU13Z1lBd0hRWURWUjBPQkJZRUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1IKTUI4R0ExVWRJd1FZTUJhQUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1JNQjBHQTFVZEpRUVdNQlFHQ0NzRwpBUVVGQndNQ0JnZ3JCZ0VGQlFjREFUQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BNEdBMVVkRHdFQi93UUVBd0lCCkJqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFYbEh4M2tIMDZwY2NDTDlEVE5qTnBCYnlVSytGd2R6T2IwWFYKcmpNaGtxdHVmdEpUUnR5T3hKZ0ZKNXhUR3pCdEtKamcrVU1pczBOV0t0VDBNWThVMU45U2c5SDl0RFpHRHBjVQpxMlVRU0Y4dXRQMVR3dnJIUzIrdzB2MUoxdHgrTEFiU0lmWmJCV0xXQ21EODUzRlVoWlFZekkvYXpFM28vd0p1CmlPUklMdUpNUk5vNlBXY3VLZmRFVkhaS1RTWnk3a25FcHNidGtsN3EwRE91eUFWdG9HVnlkb3VUR0FOdFhXK2YKczNUSTJjKzErZXg3L2RZOEJGQTFzNWFUOG5vZnU3T1RTTzdiS1kzSkRBUHZOeFQzKzVZUXJwNGR1Nmh0YUFMbAppOHNaRkhidmxpd2EzdlhxL3p1Y2JEaHEzQzBhZnAzV2ZwRGxwSlpvLy9QUUFKaTZLQT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K"}, - } - otherJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: "https://some-other-issuer.com", - Audience: "some-audience", - TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURVVENDQWptZ0F3SUJBZ0lWQUpzNStTbVRtaTJXeUI0bGJJRXBXaUs5a1RkUE1BMEdDU3FHU0liM0RRRUIKQ3dVQU1COHhDekFKQmdOVkJBWVRBbFZUTVJBd0RnWURWUVFLREFkUWFYWnZkR0ZzTUI0WERUSXdNRFV3TkRFMgpNamMxT0ZvWERUSTBNRFV3TlRFMk1qYzFPRm93SHpFTE1Ba0dBMVVFQmhNQ1ZWTXhFREFPQmdOVkJBb01CMUJwCmRtOTBZV3d3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRRERZWmZvWGR4Z2NXTEMKZEJtbHB5a0tBaG9JMlBuUWtsVFNXMno1cGcwaXJjOGFRL1E3MXZzMTRZYStmdWtFTGlvOTRZYWw4R01DdVFrbApMZ3AvUEE5N1VYelhQNDBpK25iNXcwRGpwWWd2dU9KQXJXMno2MFRnWE5NSFh3VHk4ME1SZEhpUFVWZ0VZd0JpCmtkNThzdEFVS1Y1MnBQTU1reTJjNy9BcFhJNmRXR2xjalUvaFBsNmtpRzZ5dEw2REtGYjJQRWV3MmdJM3pHZ2IKOFVVbnA1V05DZDd2WjNVY0ZHNXlsZEd3aGc3cnZ4U1ZLWi9WOEhCMGJmbjlxamlrSVcxWFM4dzdpUUNlQmdQMApYZWhKZmVITlZJaTJtZlczNlVQbWpMdnVKaGpqNDIrdFBQWndvdDkzdWtlcEgvbWpHcFJEVm9wamJyWGlpTUYrCkYxdnlPNGMxQWdNQkFBR2pnWU13Z1lBd0hRWURWUjBPQkJZRUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1IKTUI4R0ExVWRJd1FZTUJhQUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1JNQjBHQTFVZEpRUVdNQlFHQ0NzRwpBUVVGQndNQ0JnZ3JCZ0VGQlFjREFUQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BNEdBMVVkRHdFQi93UUVBd0lCCkJqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFYbEh4M2tIMDZwY2NDTDlEVE5qTnBCYnlVSytGd2R6T2IwWFYKcmpNaGtxdHVmdEpUUnR5T3hKZ0ZKNXhUR3pCdEtKamcrVU1pczBOV0t0VDBNWThVMU45U2c5SDl0RFpHRHBjVQpxMlVRU0Y4dXRQMVR3dnJIUzIrdzB2MUoxdHgrTEFiU0lmWmJCV0xXQ21EODUzRlVoWlFZekkvYXpFM28vd0p1CmlPUklMdUpNUk5vNlBXY3VLZmRFVkhaS1RTWnk3a25FcHNidGtsN3EwRE91eUFWdG9HVnlkb3VUR0FOdFhXK2YKczNUSTJjKzErZXg3L2RZOEJGQTFzNWFUOG5vZnU3T1RTTzdiS1kzSkRBUHZOeFQzKzVZUXJwNGR1Nmh0YUFMbAppOHNaRkhidmxpd2EzdlhxL3p1Y2JEaHEzQzBhZnAzV2ZwRGxwSlpvLy9QUUFKaTZLQT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K"}, - } - missingTLSJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: "https://some-issuer.com", - Audience: "some-audience", - } - invalidTLSJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: "https://some-other-issuer.com", - Audience: "some-audience", - TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "invalid base64-encoded data"}, - } - - tests := []struct { - name string - cache func(*testing.T, *authncache.Cache, bool) - wantClose bool - syncKey controllerlib.Key - jwtAuthenticators []runtime.Object - wantErr string - wantLogs []string - wantCacheEntries int - }{ - { - name: "not found", - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="Sync() found that the JWTAuthenticator does not exist yet or was deleted"`, - }, - }, - { - name: "valid jwt authenticator with CA", - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *someJWTAuthenticatorSpec, - }, - }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, - }, - wantCacheEntries: 1, - }, - { - name: "updating jwt authenticator with new fields closes previous instance", - cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { - cache.Store( - authncache.Key{ - Name: "test-name", - Namespace: "test-namespace", - Kind: "JWTAuthenticator", - APIGroup: auth1alpha1.SchemeGroupVersion.Group, - }, - newCacheValue(t, *otherJWTAuthenticatorSpec, wantClose), - ) - }, - wantClose: true, - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *someJWTAuthenticatorSpec, - }, - }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, - }, - wantCacheEntries: 1, - }, - { - name: "updating jwt authenticator with the same value does nothing", - cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { - cache.Store( - authncache.Key{ - Name: "test-name", - Namespace: "test-namespace", - Kind: "JWTAuthenticator", - APIGroup: auth1alpha1.SchemeGroupVersion.Group, - }, - newCacheValue(t, *someJWTAuthenticatorSpec, wantClose), - ) - }, - wantClose: false, - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *someJWTAuthenticatorSpec, - }, - }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="actual jwt authenticator and desired jwt authenticator are the same" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, - }, - wantCacheEntries: 1, - }, - { - name: "updating jwt authenticator when cache value is wrong type", - cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { - cache.Store( - authncache.Key{ - Name: "test-name", - Namespace: "test-namespace", - Kind: "JWTAuthenticator", - APIGroup: auth1alpha1.SchemeGroupVersion.Group, - }, - struct{ authenticator.Token }{}, - ) - }, - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *someJWTAuthenticatorSpec, - }, - }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="wrong JWT authenticator type in cache" "actualType"="struct { authenticator.Token }"`, - `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, - }, - wantCacheEntries: 1, - }, - { - name: "valid jwt authenticator without CA", - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *missingTLSJWTAuthenticatorSpec, - }, - }, - wantLogs: []string{ - `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="https://some-issuer.com" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, - }, - wantCacheEntries: 1, - }, - { - name: "invalid jwt authenticator CA", - syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, - jwtAuthenticators: []runtime.Object{ - &auth1alpha1.JWTAuthenticator{ - ObjectMeta: metav1.ObjectMeta{ - Namespace: "test-namespace", - Name: "test-name", - }, - Spec: *invalidTLSJWTAuthenticatorSpec, - }, - }, - wantErr: "failed to build jwt authenticator: invalid TLS configuration: illegal base64 data at input byte 7", - }, - } - for _, tt := range tests { - tt := tt - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - fakeClient := pinnipedfake.NewSimpleClientset(tt.jwtAuthenticators...) - informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) - cache := authncache.New() - testLog := testlogger.New(t) - - if tt.cache != nil { - tt.cache(t, cache, tt.wantClose) - } - - controller := New(cache, informers.Authentication().V1alpha1().JWTAuthenticators(), testLog) - - ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) - defer cancel() - - informers.Start(ctx.Done()) - controllerlib.TestRunSynchronously(t, controller) - - syncCtx := controllerlib.Context{Context: ctx, Key: tt.syncKey} - - if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" { - require.EqualError(t, err, tt.wantErr) - } else { - require.NoError(t, err) - } - require.Equal(t, tt.wantLogs, testLog.Lines()) - require.Equal(t, tt.wantCacheEntries, len(cache.Keys())) - }) - } -} - -func TestNewJWTAuthenticator(t *testing.T) { - t.Parallel() - const ( - goodSubject = "some-subject" - goodAudience = "some-audience" - group0 = "some-group-0" - group1 = "some-group-1" - goodECSigningKeyID = "some-ec-key-id" goodRSASigningKeyID = "some-rsa-key-id" + goodAudience = "some-audience" ) goodECSigningKey, err := ecdsa.GenerateKey(elliptic.P256(), rand.Reader) @@ -299,27 +84,317 @@ func TestNewJWTAuthenticator(t *testing.T) { })) goodIssuer := server.URL - a, err := newJWTAuthenticator(&auth1alpha1.JWTAuthenticatorSpec{ + + someJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ Issuer: goodIssuer, Audience: goodAudience, TLS: tlsSpecFromTLSConfig(server.TLS), - }) - require.NoError(t, err) - t.Cleanup(a.Close) - - // The implementation of AuthenticateToken() that we use waits 10 seconds after creation to - // perform OIDC discovery. Therefore, the JWTAuthenticator is not functional for the first 10 - // seconds. We sleep for 13 seconds in this unit test to give a little bit of cushion to that 10 - // second delay. - // - // We should get rid of this 10 second delay. See - // https://github.com/vmware-tanzu/pinniped/issues/260. - if testing.Short() { - t.Skip("skipping this test when '-short' flag is passed to avoid necessary 13 second sleep") } - time.Sleep(time.Second * 13) + otherJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://some-other-issuer.com", + Audience: goodAudience, + TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "LS0tLS1CRUdJTiBDRVJUSUZJQ0FURS0tLS0tCk1JSURVVENDQWptZ0F3SUJBZ0lWQUpzNStTbVRtaTJXeUI0bGJJRXBXaUs5a1RkUE1BMEdDU3FHU0liM0RRRUIKQ3dVQU1COHhDekFKQmdOVkJBWVRBbFZUTVJBd0RnWURWUVFLREFkUWFYWnZkR0ZzTUI0WERUSXdNRFV3TkRFMgpNamMxT0ZvWERUSTBNRFV3TlRFMk1qYzFPRm93SHpFTE1Ba0dBMVVFQmhNQ1ZWTXhFREFPQmdOVkJBb01CMUJwCmRtOTBZV3d3Z2dFaU1BMEdDU3FHU0liM0RRRUJBUVVBQTRJQkR3QXdnZ0VLQW9JQkFRRERZWmZvWGR4Z2NXTEMKZEJtbHB5a0tBaG9JMlBuUWtsVFNXMno1cGcwaXJjOGFRL1E3MXZzMTRZYStmdWtFTGlvOTRZYWw4R01DdVFrbApMZ3AvUEE5N1VYelhQNDBpK25iNXcwRGpwWWd2dU9KQXJXMno2MFRnWE5NSFh3VHk4ME1SZEhpUFVWZ0VZd0JpCmtkNThzdEFVS1Y1MnBQTU1reTJjNy9BcFhJNmRXR2xjalUvaFBsNmtpRzZ5dEw2REtGYjJQRWV3MmdJM3pHZ2IKOFVVbnA1V05DZDd2WjNVY0ZHNXlsZEd3aGc3cnZ4U1ZLWi9WOEhCMGJmbjlxamlrSVcxWFM4dzdpUUNlQmdQMApYZWhKZmVITlZJaTJtZlczNlVQbWpMdnVKaGpqNDIrdFBQWndvdDkzdWtlcEgvbWpHcFJEVm9wamJyWGlpTUYrCkYxdnlPNGMxQWdNQkFBR2pnWU13Z1lBd0hRWURWUjBPQkJZRUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1IKTUI4R0ExVWRJd1FZTUJhQUZNTWJpSXFhdVkwajRVWWphWDl0bDJzby9LQ1JNQjBHQTFVZEpRUVdNQlFHQ0NzRwpBUVVGQndNQ0JnZ3JCZ0VGQlFjREFUQVBCZ05WSFJNQkFmOEVCVEFEQVFIL01BNEdBMVVkRHdFQi93UUVBd0lCCkJqQU5CZ2txaGtpRzl3MEJBUXNGQUFPQ0FRRUFYbEh4M2tIMDZwY2NDTDlEVE5qTnBCYnlVSytGd2R6T2IwWFYKcmpNaGtxdHVmdEpUUnR5T3hKZ0ZKNXhUR3pCdEtKamcrVU1pczBOV0t0VDBNWThVMU45U2c5SDl0RFpHRHBjVQpxMlVRU0Y4dXRQMVR3dnJIUzIrdzB2MUoxdHgrTEFiU0lmWmJCV0xXQ21EODUzRlVoWlFZekkvYXpFM28vd0p1CmlPUklMdUpNUk5vNlBXY3VLZmRFVkhaS1RTWnk3a25FcHNidGtsN3EwRE91eUFWdG9HVnlkb3VUR0FOdFhXK2YKczNUSTJjKzErZXg3L2RZOEJGQTFzNWFUOG5vZnU3T1RTTzdiS1kzSkRBUHZOeFQzKzVZUXJwNGR1Nmh0YUFMbAppOHNaRkhidmxpd2EzdlhxL3p1Y2JEaHEzQzBhZnAzV2ZwRGxwSlpvLy9QUUFKaTZLQT09Ci0tLS0tRU5EIENFUlRJRklDQVRFLS0tLS0K"}, + } + missingTLSJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ + Issuer: goodIssuer, + Audience: goodAudience, + } + invalidTLSJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ + Issuer: "https://some-other-issuer.com", + Audience: goodAudience, + TLS: &auth1alpha1.TLSSpec{CertificateAuthorityData: "invalid base64-encoded data"}, + } - var tests = []struct { + tests := []struct { + name string + cache func(*testing.T, *authncache.Cache, bool) + syncKey controllerlib.Key + jwtAuthenticators []runtime.Object + wantClose bool + wantErr string + wantLogs []string + wantCacheEntries int + runTestsOnResultingAuthenticator bool + }{ + { + name: "not found", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="Sync() found that the JWTAuthenticator does not exist yet or was deleted"`, + }, + }, + { + name: "valid jwt authenticator with CA", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpec, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + runTestsOnResultingAuthenticator: true, + }, + { + name: "updating jwt authenticator with new fields closes previous instance", + cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { + cache.Store( + authncache.Key{ + Name: "test-name", + Namespace: "test-namespace", + Kind: "JWTAuthenticator", + APIGroup: auth1alpha1.SchemeGroupVersion.Group, + }, + newCacheValue(t, *otherJWTAuthenticatorSpec, wantClose), + ) + }, + wantClose: true, + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpec, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + runTestsOnResultingAuthenticator: true, + }, + { + name: "updating jwt authenticator with the same value does nothing", + cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { + cache.Store( + authncache.Key{ + Name: "test-name", + Namespace: "test-namespace", + Kind: "JWTAuthenticator", + APIGroup: auth1alpha1.SchemeGroupVersion.Group, + }, + newCacheValue(t, *someJWTAuthenticatorSpec, wantClose), + ) + }, + wantClose: false, + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpec, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="actual jwt authenticator and desired jwt authenticator are the same" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + runTestsOnResultingAuthenticator: false, // skip the tests because the authenticator left in the cache is the mock version that was added above + }, + { + name: "updating jwt authenticator when cache value is wrong type", + cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { + cache.Store( + authncache.Key{ + Name: "test-name", + Namespace: "test-namespace", + Kind: "JWTAuthenticator", + APIGroup: auth1alpha1.SchemeGroupVersion.Group, + }, + struct{ authenticator.Token }{}, + ) + }, + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpec, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="wrong JWT authenticator type in cache" "actualType"="struct { authenticator.Token }"`, + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + runTestsOnResultingAuthenticator: true, + }, + { + name: "valid jwt authenticator without CA", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *missingTLSJWTAuthenticatorSpec, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + runTestsOnResultingAuthenticator: false, // skip the tests because the authenticator left in the cache doesn't have the CA for our test discovery server + }, + { + name: "invalid jwt authenticator CA", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *invalidTLSJWTAuthenticatorSpec, + }, + }, + wantErr: "failed to build jwt authenticator: invalid TLS configuration: illegal base64 data at input byte 7", + }, + } + + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + fakeClient := pinnipedfake.NewSimpleClientset(tt.jwtAuthenticators...) + informers := pinnipedinformers.NewSharedInformerFactory(fakeClient, 0) + cache := authncache.New() + testLog := testlogger.New(t) + + if tt.cache != nil { + tt.cache(t, cache, tt.wantClose) + } + + controller := New(cache, informers.Authentication().V1alpha1().JWTAuthenticators(), testLog) + + ctx, cancel := context.WithTimeout(context.Background(), 2*time.Second) + defer cancel() + + informers.Start(ctx.Done()) + controllerlib.TestRunSynchronously(t, controller) + + syncCtx := controllerlib.Context{Context: ctx, Key: tt.syncKey} + + if err := controllerlib.TestSync(t, controller, syncCtx); tt.wantErr != "" { + require.EqualError(t, err, tt.wantErr) + } else { + require.NoError(t, err) + } + require.Equal(t, tt.wantLogs, testLog.Lines()) + require.Equal(t, tt.wantCacheEntries, len(cache.Keys())) + + if !tt.runTestsOnResultingAuthenticator { + return // end of test unless we wanted to run tests on the resulting authenticator from the cache + } + + // The implementation of AuthenticateToken() that we use waits 10 seconds after creation to + // perform OIDC discovery. Therefore, the JWTAuthenticator is not functional for the first 10 + // seconds. We sleep for 13 seconds in this unit test to give a little bit of cushion to that 10 + // second delay. + // + // We should get rid of this 10 second delay. See + // https://github.com/vmware-tanzu/pinniped/issues/260. + time.Sleep(time.Second * 13) + + // We expected the cache to have an entry, so pull that entry from the cache and test it. + expectedCacheKey := authncache.Key{ + APIGroup: auth1alpha1.GroupName, + Kind: "JWTAuthenticator", + Namespace: syncCtx.Key.Namespace, + Name: syncCtx.Key.Name, + } + cachedAuthenticator := cache.Get(expectedCacheKey) + require.NotNil(t, cachedAuthenticator) + + // Schedule it to be closed at the end of the test. + t.Cleanup(cachedAuthenticator.(*jwtAuthenticator).Close) + + const ( + goodSubject = "some-subject" + group0 = "some-group-0" + group1 = "some-group-1" + ) + + for _, test := range testTableForAuthenticateTokenTests( + t, + goodSubject, + goodRSASigningKey, + goodRSASigningAlgo, + goodRSASigningKeyID, + group0, + group1, + ) { + test := test + t.Run(test.name, func(t *testing.T) { + t.Parallel() + + wellKnownClaims := jwt.Claims{ + Issuer: goodIssuer, + Subject: goodSubject, + Audience: []string{goodAudience}, + Expiry: jwt.NewNumericDate(time.Now().Add(time.Hour)), + NotBefore: jwt.NewNumericDate(time.Now().Add(-time.Hour)), + IssuedAt: jwt.NewNumericDate(time.Now().Add(-time.Hour)), + } + var groups interface{} + if test.jwtClaims != nil { + test.jwtClaims(&wellKnownClaims, &groups) + } + + var signingKey interface{} = goodECSigningKey + signingAlgo := goodECSigningAlgo + signingKID := goodECSigningKeyID + if test.jwtSignature != nil { + test.jwtSignature(&signingKey, &signingAlgo, &signingKID) + } + + jwt := createJWT(t, signingKey, signingAlgo, signingKID, &wellKnownClaims, groups) + rsp, authenticated, err := cachedAuthenticator.AuthenticateToken(context.Background(), jwt) + if test.wantErrorRegexp != "" { + require.Error(t, err) + require.Regexp(t, test.wantErrorRegexp, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, test.wantResponse, rsp) + require.Equal(t, test.wantAuthenticated, authenticated) + } + }) + } + }) + } +} + +func testTableForAuthenticateTokenTests( + t *testing.T, + goodSubject string, + goodRSASigningKey *rsa.PrivateKey, + goodRSASigningAlgo jose.SignatureAlgorithm, + goodRSASigningKeyID string, + group0 string, + group1 string, +) []struct { + name string + jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}) + jwtSignature func(key *interface{}, algo *jose.SignatureAlgorithm, kid *string) + wantResponse *authenticator.Response + wantAuthenticated bool + wantErrorRegexp string +} { + tests := []struct { name string jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}) jwtSignature func(key *interface{}, algo *jose.SignatureAlgorithm, kid *string) @@ -429,7 +504,7 @@ func TestNewJWTAuthenticator(t *testing.T) { jwtClaims: func(claims *jwt.Claims, _ *interface{}) { claims.Expiry = jwt.NewNumericDate(time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC)) }, - wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-02-02 23:09:04 -0456 LMT\)`, + wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-02-02 20:12:08 -0752 LMT\)`, }, { name: "bad token without exp", @@ -459,43 +534,8 @@ func TestNewJWTAuthenticator(t *testing.T) { wantErrorRegexp: `oidc: verify token: oidc: id token signed with unsupported algorithm, expected \["RS256" "ES256"\] got "ES384"`, }, } - for _, test := range tests { - test := test - t.Run(test.name, func(t *testing.T) { - t.Parallel() - wellKnownClaims := jwt.Claims{ - Issuer: goodIssuer, - Subject: goodSubject, - Audience: []string{goodAudience}, - Expiry: jwt.NewNumericDate(time.Now().Add(time.Hour)), - NotBefore: jwt.NewNumericDate(time.Now().Add(-time.Hour)), - IssuedAt: jwt.NewNumericDate(time.Now().Add(-time.Hour)), - } - var groups interface{} - if test.jwtClaims != nil { - test.jwtClaims(&wellKnownClaims, &groups) - } - - var signingKey interface{} = goodECSigningKey - signingAlgo := goodECSigningAlgo - signingKID := goodECSigningKeyID - if test.jwtSignature != nil { - test.jwtSignature(&signingKey, &signingAlgo, &signingKID) - } - - jwt := createJWT(t, signingKey, signingAlgo, signingKID, &wellKnownClaims, groups) - rsp, authenticated, err := a.AuthenticateToken(context.Background(), jwt) - if test.wantErrorRegexp != "" { - require.Error(t, err) - require.Regexp(t, test.wantErrorRegexp, err.Error()) - } else { - require.NoError(t, err) - require.Equal(t, test.wantResponse, rsp) - require.Equal(t, test.wantAuthenticated, authenticated) - } - }) - } + return tests } func tlsSpecFromTLSConfig(tls *tls.Config) *auth1alpha1.TLSSpec { From 05ab8f375e8c3b6337b6dfd6ad96cb013c29e2dc Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Dec 2020 14:37:38 -0800 Subject: [PATCH 06/12] Default to "username" claim in jwtcachefiller Signed-off-by: Margo Crawford --- .../jwtcachefiller/jwtcachefiller.go | 2 +- .../jwtcachefiller/jwtcachefiller_test.go | 64 +++++++++++-------- 2 files changed, 39 insertions(+), 27 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index de31961a..f37a7ff3 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -29,7 +29,7 @@ import ( // These default values come from the way that the Supervisor issues and signs tokens. We make these // the defaults for a JWTAuthenticator so that they can easily integrate with the Supervisor. const ( - defaultUsernameClaim = "sub" + defaultUsernameClaim = "username" defaultGroupsClaim = "groups" ) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 7c230cd9..b2c78a56 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -19,11 +19,10 @@ import ( "testing" "time" - "gopkg.in/square/go-jose.v2/jwt" - "github.com/golang/mock/gomock" "github.com/stretchr/testify/require" "gopkg.in/square/go-jose.v2" + "gopkg.in/square/go-jose.v2/jwt" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apiserver/pkg/authentication/authenticator" @@ -324,19 +323,20 @@ func TestController(t *testing.T) { t.Cleanup(cachedAuthenticator.(*jwtAuthenticator).Close) const ( - goodSubject = "some-subject" - group0 = "some-group-0" - group1 = "some-group-1" + goodSubject = "some-subject" + group0 = "some-group-0" + group1 = "some-group-1" + goodUsername = "pinny123" ) for _, test := range testTableForAuthenticateTokenTests( t, - goodSubject, goodRSASigningKey, goodRSASigningAlgo, goodRSASigningKeyID, group0, group1, + goodUsername, ) { test := test t.Run(test.name, func(t *testing.T) { @@ -351,8 +351,9 @@ func TestController(t *testing.T) { IssuedAt: jwt.NewNumericDate(time.Now().Add(-time.Hour)), } var groups interface{} + username := goodUsername if test.jwtClaims != nil { - test.jwtClaims(&wellKnownClaims, &groups) + test.jwtClaims(&wellKnownClaims, &groups, &username) } var signingKey interface{} = goodECSigningKey @@ -362,7 +363,7 @@ func TestController(t *testing.T) { test.jwtSignature(&signingKey, &signingAlgo, &signingKID) } - jwt := createJWT(t, signingKey, signingAlgo, signingKID, &wellKnownClaims, groups) + jwt := createJWT(t, signingKey, signingAlgo, signingKID, &wellKnownClaims, groups, username) rsp, authenticated, err := cachedAuthenticator.AuthenticateToken(context.Background(), jwt) if test.wantErrorRegexp != "" { require.Error(t, err) @@ -380,15 +381,15 @@ func TestController(t *testing.T) { func testTableForAuthenticateTokenTests( t *testing.T, - goodSubject string, goodRSASigningKey *rsa.PrivateKey, goodRSASigningAlgo jose.SignatureAlgorithm, goodRSASigningKeyID string, group0 string, group1 string, + goodUsername string, ) []struct { name string - jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}) + jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}, username *string) jwtSignature func(key *interface{}, algo *jose.SignatureAlgorithm, kid *string) wantResponse *authenticator.Response wantAuthenticated bool @@ -396,7 +397,7 @@ func testTableForAuthenticateTokenTests( } { tests := []struct { name string - jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}) + jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}, username *string) jwtSignature func(key *interface{}, algo *jose.SignatureAlgorithm, kid *string) wantResponse *authenticator.Response wantAuthenticated bool @@ -406,7 +407,7 @@ func testTableForAuthenticateTokenTests( name: "good token without groups and with EC signature", wantResponse: &authenticator.Response{ User: &user.DefaultInfo{ - Name: goodSubject, + Name: goodUsername, }, }, wantAuthenticated: true, @@ -420,19 +421,19 @@ func testTableForAuthenticateTokenTests( }, wantResponse: &authenticator.Response{ User: &user.DefaultInfo{ - Name: goodSubject, + Name: goodUsername, }, }, wantAuthenticated: true, }, { name: "good token with groups as array", - jwtClaims: func(_ *jwt.Claims, groups *interface{}) { + jwtClaims: func(_ *jwt.Claims, groups *interface{}, username *string) { *groups = []string{group0, group1} }, wantResponse: &authenticator.Response{ User: &user.DefaultInfo{ - Name: goodSubject, + Name: goodUsername, Groups: []string{group0, group1}, }, }, @@ -440,12 +441,12 @@ func testTableForAuthenticateTokenTests( }, { name: "good token with groups as string", - jwtClaims: func(_ *jwt.Claims, groups *interface{}) { + jwtClaims: func(_ *jwt.Claims, groups *interface{}, username *string) { *groups = group0 }, wantResponse: &authenticator.Response{ User: &user.DefaultInfo{ - Name: goodSubject, + Name: goodUsername, Groups: []string{group0}, }, }, @@ -453,26 +454,26 @@ func testTableForAuthenticateTokenTests( }, { name: "good token with nbf unset", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.NotBefore = nil }, wantResponse: &authenticator.Response{ User: &user.DefaultInfo{ - Name: goodSubject, + Name: goodUsername, }, }, wantAuthenticated: true, }, { name: "bad token with groups as map", - jwtClaims: func(_ *jwt.Claims, groups *interface{}) { + jwtClaims: func(_ *jwt.Claims, groups *interface{}, username *string) { *groups = map[string]string{"not an array": "or a string"} }, wantErrorRegexp: "oidc: parse groups claim \"groups\": json: cannot unmarshal object into Go value of type string", }, { name: "bad token with wrong issuer", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Issuer = "wrong-issuer" }, wantResponse: nil, @@ -480,39 +481,46 @@ func testTableForAuthenticateTokenTests( }, { name: "bad token with no audience", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Audience = nil }, wantErrorRegexp: `oidc: verify token: oidc: expected audience "some-audience" got \[\]`, }, { name: "bad token with wrong audience", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Audience = []string{"wrong-audience"} }, wantErrorRegexp: `oidc: verify token: oidc: expected audience "some-audience" got \["wrong-audience"\]`, }, { name: "bad token with nbf in the future", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.NotBefore = jwt.NewNumericDate(time.Date(3020, 2, 3, 4, 5, 6, 7, time.UTC)) }, wantErrorRegexp: `oidc: verify token: oidc: current time .* before the nbf \(not before\) time: 3020-.*`, }, { name: "bad token with exp in past", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Expiry = jwt.NewNumericDate(time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC)) }, wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-02-02 20:12:08 -0752 LMT\)`, }, { name: "bad token without exp", - jwtClaims: func(claims *jwt.Claims, _ *interface{}) { + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Expiry = nil }, wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-01-01 00:00:00 \+0000 UTC\)`, }, + { + name: "token does not have username claim", + jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { + *username = "" + }, + wantErrorRegexp: `oidc: parse username claims "username": claim not present`, + }, { name: "signing key is wrong", jwtSignature: func(key *interface{}, algo *jose.SignatureAlgorithm, kid *string) { @@ -560,6 +568,7 @@ func createJWT( kid string, claims *jwt.Claims, groups interface{}, + username string, ) string { t.Helper() @@ -573,6 +582,9 @@ func createJWT( if groups != nil { builder = builder.Claims(map[string]interface{}{"groups": groups}) } + if username != "" { + builder = builder.Claims(map[string]interface{}{"username": username}) + } jwt, err := builder.CompactSerialize() require.NoError(t, err) From a10d219049b9bba42e90c6bfdd32fc477c214e8a Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 15 Dec 2020 16:11:53 -0800 Subject: [PATCH 07/12] Pass through custom groups claim and username claim Signed-off-by: Ryan Richard --- .../jwtcachefiller/jwtcachefiller.go | 12 ++- .../jwtcachefiller/jwtcachefiller_test.go | 94 +++++++++++++++++-- 2 files changed, 95 insertions(+), 11 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index f37a7ff3..682a5ee6 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -169,12 +169,20 @@ func newJWTAuthenticator(spec *auth1alpha1.JWTAuthenticatorSpec) (*jwtAuthentica caFile = temp.Name() } + usernameClaim := spec.UsernameClaim + if usernameClaim == "" { + usernameClaim = defaultUsernameClaim + } + groupsClaim := spec.GroupsClaim + if groupsClaim == "" { + groupsClaim = defaultGroupsClaim + } authenticator, err := oidc.New(oidc.Options{ IssuerURL: spec.Issuer, ClientID: spec.Audience, - UsernameClaim: defaultUsernameClaim, - GroupsClaim: defaultGroupsClaim, + UsernameClaim: usernameClaim, + GroupsClaim: groupsClaim, SupportedSigningAlgs: defaultSupportedSigningAlgos(), CAFile: caFile, }) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index b2c78a56..1c2fe19b 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -89,6 +89,18 @@ func TestController(t *testing.T) { Audience: goodAudience, TLS: tlsSpecFromTLSConfig(server.TLS), } + someJWTAuthenticatorSpecWithUsernameClaim := &auth1alpha1.JWTAuthenticatorSpec{ + Issuer: goodIssuer, + Audience: goodAudience, + TLS: tlsSpecFromTLSConfig(server.TLS), + UsernameClaim: "my-custom-username-claim", + } + someJWTAuthenticatorSpecWithGroupsClaim := &auth1alpha1.JWTAuthenticatorSpec{ + Issuer: goodIssuer, + Audience: goodAudience, + TLS: tlsSpecFromTLSConfig(server.TLS), + GroupsClaim: "my-custom-groups-claim", + } otherJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ Issuer: "https://some-other-issuer.com", Audience: goodAudience, @@ -113,6 +125,8 @@ func TestController(t *testing.T) { wantErr string wantLogs []string wantCacheEntries int + wantUsernameClaim string + wantGroupsClaim string runTestsOnResultingAuthenticator bool }{ { @@ -140,6 +154,44 @@ func TestController(t *testing.T) { wantCacheEntries: 1, runTestsOnResultingAuthenticator: true, }, + { + name: "valid jwt authenticator with custom username claim", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpecWithUsernameClaim, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + wantUsernameClaim: someJWTAuthenticatorSpecWithUsernameClaim.UsernameClaim, + runTestsOnResultingAuthenticator: true, + }, + { + name: "valid jwt authenticator with custom groups claim", + syncKey: controllerlib.Key{Namespace: "test-namespace", Name: "test-name"}, + jwtAuthenticators: []runtime.Object{ + &auth1alpha1.JWTAuthenticator{ + ObjectMeta: metav1.ObjectMeta{ + Namespace: "test-namespace", + Name: "test-name", + }, + Spec: *someJWTAuthenticatorSpecWithGroupsClaim, + }, + }, + wantLogs: []string{ + `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, + }, + wantCacheEntries: 1, + wantGroupsClaim: someJWTAuthenticatorSpecWithGroupsClaim.GroupsClaim, + runTestsOnResultingAuthenticator: true, + }, { name: "updating jwt authenticator with new fields closes previous instance", cache: func(t *testing.T, cache *authncache.Cache, wantClose bool) { @@ -329,6 +381,14 @@ func TestController(t *testing.T) { goodUsername = "pinny123" ) + if tt.wantUsernameClaim == "" { + tt.wantUsernameClaim = "username" + } + + if tt.wantGroupsClaim == "" { + tt.wantGroupsClaim = "groups" + } + for _, test := range testTableForAuthenticateTokenTests( t, goodRSASigningKey, @@ -337,6 +397,8 @@ func TestController(t *testing.T) { group0, group1, goodUsername, + tt.wantUsernameClaim, + tt.wantGroupsClaim, ) { test := test t.Run(test.name, func(t *testing.T) { @@ -363,7 +425,17 @@ func TestController(t *testing.T) { test.jwtSignature(&signingKey, &signingAlgo, &signingKID) } - jwt := createJWT(t, signingKey, signingAlgo, signingKID, &wellKnownClaims, groups, username) + jwt := createJWT( + t, + signingKey, + signingAlgo, + signingKID, + &wellKnownClaims, + tt.wantGroupsClaim, + groups, + tt.wantUsernameClaim, + username, + ) rsp, authenticated, err := cachedAuthenticator.AuthenticateToken(context.Background(), jwt) if test.wantErrorRegexp != "" { require.Error(t, err) @@ -387,6 +459,8 @@ func testTableForAuthenticateTokenTests( group0 string, group1 string, goodUsername string, + expectedUsernameClaim string, + expectedGroupsClaim string, ) []struct { name string jwtClaims func(wellKnownClaims *jwt.Claims, groups *interface{}, username *string) @@ -469,7 +543,7 @@ func testTableForAuthenticateTokenTests( jwtClaims: func(_ *jwt.Claims, groups *interface{}, username *string) { *groups = map[string]string{"not an array": "or a string"} }, - wantErrorRegexp: "oidc: parse groups claim \"groups\": json: cannot unmarshal object into Go value of type string", + wantErrorRegexp: "oidc: parse groups claim \"" + expectedGroupsClaim + "\": json: cannot unmarshal object into Go value of type string", }, { name: "bad token with wrong issuer", @@ -519,7 +593,7 @@ func testTableForAuthenticateTokenTests( jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { *username = "" }, - wantErrorRegexp: `oidc: parse username claims "username": claim not present`, + wantErrorRegexp: `oidc: parse username claims "` + expectedUsernameClaim + `": claim not present`, }, { name: "signing key is wrong", @@ -567,8 +641,10 @@ func createJWT( signingAlgo jose.SignatureAlgorithm, kid string, claims *jwt.Claims, - groups interface{}, - username string, + groupsClaim string, + groupsValue interface{}, + usernameClaim string, + usernameValue string, ) string { t.Helper() @@ -579,11 +655,11 @@ func createJWT( require.NoError(t, err) builder := jwt.Signed(sig).Claims(claims) - if groups != nil { - builder = builder.Claims(map[string]interface{}{"groups": groups}) + if groupsValue != nil { + builder = builder.Claims(map[string]interface{}{groupsClaim: groupsValue}) } - if username != "" { - builder = builder.Claims(map[string]interface{}{"username": username}) + if usernameValue != "" { + builder = builder.Claims(map[string]interface{}{usernameClaim: usernameValue}) } jwt, err := builder.CompactSerialize() require.NoError(t, err) From 91af51d38e1b5521417e827efca02647ad77e81c Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 15 Dec 2020 17:16:08 -0800 Subject: [PATCH 08/12] Fix integration tests to work with the username and sub claims Signed-off-by: Margo Crawford --- .../concierge_credentialrequest_test.go | 15 ++++++++------- test/integration/supervisor_login_test.go | 6 +++++- test/library/client.go | 9 +++++---- 3 files changed, 18 insertions(+), 12 deletions(-) diff --git a/test/integration/concierge_credentialrequest_test.go b/test/integration/concierge_credentialrequest_test.go index 5e2458e0..67e2574e 100644 --- a/test/integration/concierge_credentialrequest_test.go +++ b/test/integration/concierge_credentialrequest_test.go @@ -62,18 +62,19 @@ func TestSuccessfulCredentialRequest(t *testing.T) { { name: "jwt authenticator", authenticator: func(t *testing.T) corev1.TypedLocalObjectReference { - return library.CreateTestJWTAuthenticator(ctx, t) + return library.CreateTestJWTAuthenticator(ctx, t, "email") }, token: func(t *testing.T) (string, string, []string) { pinnipedExe := buildPinnipedCLI(t) credOutput, _ := runPinniedLoginOIDC(ctx, t, pinnipedExe) token := credOutput.Status.Token - // By default, the JWTAuthenticator expects the username to be in the "sub" claim and the + // By default, the JWTAuthenticator expects the username to be in the "username" claim and the // groups to be in the "groups" claim. - username, groups := getJWTSubAndGroupsClaims(t, token) + // We are configuring pinniped to set the username to be the "email" claim from the token. + username, groups := getJWTEmailAndGroupsClaims(t, token) - return credOutput.Status.Token, username, groups + return token, username, groups }, }, } @@ -233,18 +234,18 @@ func safeDerefStringPtr(s *string) string { return *s } -func getJWTSubAndGroupsClaims(t *testing.T, jwt string) (string, []string) { +func getJWTEmailAndGroupsClaims(t *testing.T, jwt string) (string, []string) { t.Helper() token, err := jwtpkg.ParseSigned(jwt) require.NoError(t, err) var claims struct { - Sub string `json:"sub"` + Email string `json:"email"` Groups []string `json:"groups"` } err = token.UnsafeClaimsWithoutVerification(&claims) require.NoError(t, err) - return claims.Sub, claims.Groups + return claims.Email, claims.Groups } diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index ed4ca1eb..6d934f33 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -169,7 +169,7 @@ func TestSupervisorLogin(t *testing.T) { tokenResponse, err := downstreamOAuth2Config.Exchange(oidcHTTPClientContext, authcode, pkceParam.Verifier()) require.NoError(t, err) - expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat"} + expectedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "nonce", "rat", "username"} verifyTokenResponse(t, tokenResponse, discovery, downstreamOAuth2Config, env.SupervisorTestUpstream.Issuer, nonceParam, expectedIDTokenClaims) // token exchange on the original token @@ -226,6 +226,10 @@ func verifyTokenResponse( idTokenClaimNames = append(idTokenClaimNames, k) } require.ElementsMatch(t, expectedIDTokenClaims, idTokenClaimNames) + expectedUsernamePrefix := upstreamIssuerName + "?sub=" + require.True(t, strings.HasPrefix(idTokenClaims["username"].(string), expectedUsernamePrefix)) + require.Greater(t, len(idTokenClaims["username"].(string)), len(expectedUsernamePrefix), + "the ID token Username should include the upstream user ID after the upstream issuer name") // Some light verification of the other tokens that were returned. require.NotEmpty(t, tokenResponse.AccessToken) diff --git a/test/library/client.go b/test/library/client.go index d11869d2..56c9bd22 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -170,7 +170,7 @@ func CreateTestWebhookAuthenticator(ctx context.Context, t *testing.T) corev1.Ty // authenticator within the test namespace. // // CreateTestJWTAuthenticator gets the OIDC issuer info from IntegrationEnv().CLITestUpstream. -func CreateTestJWTAuthenticator(ctx context.Context, t *testing.T) corev1.TypedLocalObjectReference { +func CreateTestJWTAuthenticator(ctx context.Context, t *testing.T, usernameClaim string) corev1.TypedLocalObjectReference { t.Helper() testEnv := IntegrationEnv(t) @@ -193,9 +193,10 @@ func CreateTestJWTAuthenticator(ctx context.Context, t *testing.T) corev1.TypedL jwtAuthenticator, err := jwtAuthenticators.Create(createContext, &auth1alpha1.JWTAuthenticator{ ObjectMeta: testObjectMeta(t, "jwt-authenticator"), Spec: auth1alpha1.JWTAuthenticatorSpec{ - Issuer: testEnv.CLITestUpstream.Issuer, - Audience: testEnv.CLITestUpstream.ClientID, - TLS: tlsSpec, + Issuer: testEnv.CLITestUpstream.Issuer, + Audience: testEnv.CLITestUpstream.ClientID, + TLS: tlsSpec, + UsernameClaim: usernameClaim, }, }, metav1.CreateOptions{}) require.NoError(t, err, "could not create test JWTAuthenticator") From dcb19150fc5b0f913ef30e7a324bbff1c10c7c91 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 16 Dec 2020 09:42:19 -0800 Subject: [PATCH 09/12] Nest claim configs one level deeper in JWTAuthenticatorSpec Signed-off-by: Margo Crawford --- .../authentication/v1alpha1/types_jwt.go.tmpl | 25 +++++++++++++------ ...cierge.pinniped.dev_jwtauthenticators.yaml | 25 +++++++++++-------- generated/1.17/README.adoc | 21 ++++++++++++++-- .../authentication/v1alpha1/types_jwt.go | 25 +++++++++++++------ .../v1alpha1/zz_generated.deepcopy.go | 17 +++++++++++++ ...cierge.pinniped.dev_jwtauthenticators.yaml | 25 +++++++++++-------- generated/1.18/README.adoc | 21 ++++++++++++++-- .../authentication/v1alpha1/types_jwt.go | 25 +++++++++++++------ .../v1alpha1/zz_generated.deepcopy.go | 17 +++++++++++++ ...cierge.pinniped.dev_jwtauthenticators.yaml | 25 +++++++++++-------- generated/1.19/README.adoc | 21 ++++++++++++++-- .../authentication/v1alpha1/types_jwt.go | 25 +++++++++++++------ .../v1alpha1/zz_generated.deepcopy.go | 17 +++++++++++++ ...cierge.pinniped.dev_jwtauthenticators.yaml | 25 +++++++++++-------- .../jwtcachefiller/jwtcachefiller.go | 4 +-- .../jwtcachefiller/jwtcachefiller_test.go | 24 ++++++++++-------- test/library/client.go | 2 +- 17 files changed, 253 insertions(+), 91 deletions(-) diff --git a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl index b08e8279..0087fad4 100644 --- a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl +++ b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl @@ -27,21 +27,30 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` - // UsernameClaim is the name of the claim which should be read to extract the - // username from the JWT token. When not specified, it will default to "username". + // Claims allows customization of the claims that will be mapped to user identity + // for Kubernetes access. // +optional - UsernameClaim string `json:"username_claim"` - - // GroupsClaim is the name of the claim which should be read to extract the user's - // group membership from the JWT token. When not specified, it will default to "groups". - // +optional - GroupsClaim string `json:"groups_claim"` + Claims JWTTokenClaims `json:"claims"` // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` } +// JWTTokenClaims allows customization of the claims that will be mapped to user identity +// for Kubernetes access. +type JWTTokenClaims struct { + // Groups is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + Groups string `json:"groups"` + + // Username is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + Username string `json:"username"` +} + // JWTAuthenticator describes the configuration of a JWT authenticator. // // Upon receiving a signed JWT, a JWTAuthenticator will performs some validation on it (e.g., valid diff --git a/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index 0865ca3c..e800411e 100644 --- a/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/deploy/concierge/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,11 +51,21 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string - groups_claim: - description: GroupsClaim is the name of the claim which should be - read to extract the user's group membership from the JWT token. - When not specified, it will default to "groups". - type: string + claims: + description: Claims allows customization of the claims that will be + mapped to user identity for Kubernetes access. + properties: + groups: + description: Groups is the name of the claim which should be read + to extract the user's group membership from the JWT token. When + not specified, it will default to "groups". + type: string + username: + description: Username is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string + type: object issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -71,11 +81,6 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object - username_claim: - description: UsernameClaim is the name of the claim which should be - read to extract the username from the JWT token. When not specified, - it will default to "username". - type: string required: - audience - issuer diff --git a/generated/1.17/README.adoc b/generated/1.17/README.adoc index 1f29282c..fa198e84 100644 --- a/generated/1.17/README.adoc +++ b/generated/1.17/README.adoc @@ -92,8 +92,7 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. -| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". -| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`claims`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-jwttokenclaims[$$JWTTokenClaims$$]__ | Claims allows customization of the claims that will be mapped to user identity for Kubernetes access. | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== @@ -115,6 +114,24 @@ Status of a JWT authenticator. |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-jwttokenclaims"] +==== JWTTokenClaims + +JWTTokenClaims allows customization of the claims that will be mapped to user identity for Kubernetes access. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-jwtauthenticatorspec[$$JWTAuthenticatorSpec$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`groups`* __string__ | Groups is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`username`* __string__ | Username is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-17-apis-concierge-authentication-v1alpha1-tlsspec"] ==== TLSSpec diff --git a/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go index 1c43f68f..3e159148 100644 --- a/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.17/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,21 +27,30 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` - // UsernameClaim is the name of the claim which should be read to extract the - // username from the JWT token. When not specified, it will default to "username". + // Claims allows customization of the claims that will be mapped to user identity + // for Kubernetes access. // +optional - UsernameClaim string `json:"username_claim"` - - // GroupsClaim is the name of the claim which should be read to extract the user's - // group membership from the JWT token. When not specified, it will default to "groups". - // +optional - GroupsClaim string `json:"groups_claim"` + Claims JWTTokenClaims `json:"claims"` // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` } +// JWTTokenClaims allows customization of the claims that will be mapped to user identity +// for Kubernetes access. +type JWTTokenClaims struct { + // Groups is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + Groups string `json:"groups"` + + // Username is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + Username string `json:"username"` +} + // JWTAuthenticator describes the configuration of a JWT authenticator. // // Upon receiving a signed JWT, a JWTAuthenticator will performs some validation on it (e.g., valid diff --git a/generated/1.17/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go b/generated/1.17/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go index 93d4e837..c7c6968a 100644 --- a/generated/1.17/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.17/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go @@ -92,6 +92,7 @@ func (in *JWTAuthenticatorList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JWTAuthenticatorSpec) DeepCopyInto(out *JWTAuthenticatorSpec) { *out = *in + out.Claims = in.Claims if in.TLS != nil { in, out := &in.TLS, &out.TLS *out = new(TLSSpec) @@ -133,6 +134,22 @@ func (in *JWTAuthenticatorStatus) DeepCopy() *JWTAuthenticatorStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *JWTTokenClaims) DeepCopyInto(out *JWTTokenClaims) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTTokenClaims. +func (in *JWTTokenClaims) DeepCopy() *JWTTokenClaims { + if in == nil { + return nil + } + out := new(JWTTokenClaims) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TLSSpec) DeepCopyInto(out *TLSSpec) { *out = *in diff --git a/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index 0865ca3c..e800411e 100644 --- a/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.17/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,11 +51,21 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string - groups_claim: - description: GroupsClaim is the name of the claim which should be - read to extract the user's group membership from the JWT token. - When not specified, it will default to "groups". - type: string + claims: + description: Claims allows customization of the claims that will be + mapped to user identity for Kubernetes access. + properties: + groups: + description: Groups is the name of the claim which should be read + to extract the user's group membership from the JWT token. When + not specified, it will default to "groups". + type: string + username: + description: Username is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string + type: object issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -71,11 +81,6 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object - username_claim: - description: UsernameClaim is the name of the claim which should be - read to extract the username from the JWT token. When not specified, - it will default to "username". - type: string required: - audience - issuer diff --git a/generated/1.18/README.adoc b/generated/1.18/README.adoc index 2cef60f4..327bc8fa 100644 --- a/generated/1.18/README.adoc +++ b/generated/1.18/README.adoc @@ -92,8 +92,7 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. -| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". -| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`claims`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-jwttokenclaims[$$JWTTokenClaims$$]__ | Claims allows customization of the claims that will be mapped to user identity for Kubernetes access. | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== @@ -115,6 +114,24 @@ Status of a JWT authenticator. |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-jwttokenclaims"] +==== JWTTokenClaims + +JWTTokenClaims allows customization of the claims that will be mapped to user identity for Kubernetes access. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-jwtauthenticatorspec[$$JWTAuthenticatorSpec$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`groups`* __string__ | Groups is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`username`* __string__ | Username is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-18-apis-concierge-authentication-v1alpha1-tlsspec"] ==== TLSSpec diff --git a/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go index 1c43f68f..3e159148 100644 --- a/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.18/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,21 +27,30 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` - // UsernameClaim is the name of the claim which should be read to extract the - // username from the JWT token. When not specified, it will default to "username". + // Claims allows customization of the claims that will be mapped to user identity + // for Kubernetes access. // +optional - UsernameClaim string `json:"username_claim"` - - // GroupsClaim is the name of the claim which should be read to extract the user's - // group membership from the JWT token. When not specified, it will default to "groups". - // +optional - GroupsClaim string `json:"groups_claim"` + Claims JWTTokenClaims `json:"claims"` // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` } +// JWTTokenClaims allows customization of the claims that will be mapped to user identity +// for Kubernetes access. +type JWTTokenClaims struct { + // Groups is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + Groups string `json:"groups"` + + // Username is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + Username string `json:"username"` +} + // JWTAuthenticator describes the configuration of a JWT authenticator. // // Upon receiving a signed JWT, a JWTAuthenticator will performs some validation on it (e.g., valid diff --git a/generated/1.18/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go b/generated/1.18/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go index 93d4e837..c7c6968a 100644 --- a/generated/1.18/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.18/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go @@ -92,6 +92,7 @@ func (in *JWTAuthenticatorList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JWTAuthenticatorSpec) DeepCopyInto(out *JWTAuthenticatorSpec) { *out = *in + out.Claims = in.Claims if in.TLS != nil { in, out := &in.TLS, &out.TLS *out = new(TLSSpec) @@ -133,6 +134,22 @@ func (in *JWTAuthenticatorStatus) DeepCopy() *JWTAuthenticatorStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *JWTTokenClaims) DeepCopyInto(out *JWTTokenClaims) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTTokenClaims. +func (in *JWTTokenClaims) DeepCopy() *JWTTokenClaims { + if in == nil { + return nil + } + out := new(JWTTokenClaims) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TLSSpec) DeepCopyInto(out *TLSSpec) { *out = *in diff --git a/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index 0865ca3c..e800411e 100644 --- a/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.18/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,11 +51,21 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string - groups_claim: - description: GroupsClaim is the name of the claim which should be - read to extract the user's group membership from the JWT token. - When not specified, it will default to "groups". - type: string + claims: + description: Claims allows customization of the claims that will be + mapped to user identity for Kubernetes access. + properties: + groups: + description: Groups is the name of the claim which should be read + to extract the user's group membership from the JWT token. When + not specified, it will default to "groups". + type: string + username: + description: Username is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string + type: object issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -71,11 +81,6 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object - username_claim: - description: UsernameClaim is the name of the claim which should be - read to extract the username from the JWT token. When not specified, - it will default to "username". - type: string required: - audience - issuer diff --git a/generated/1.19/README.adoc b/generated/1.19/README.adoc index bfb3c44f..3fa12249 100644 --- a/generated/1.19/README.adoc +++ b/generated/1.19/README.adoc @@ -92,8 +92,7 @@ Spec for configuring a JWT authenticator. | Field | Description | *`issuer`* __string__ | Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT claim. | *`audience`* __string__ | Audience is the required value of the "aud" JWT claim. -| *`username_claim`* __string__ | UsernameClaim is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". -| *`groups_claim`* __string__ | GroupsClaim is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`claims`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-jwttokenclaims[$$JWTTokenClaims$$]__ | Claims allows customization of the claims that will be mapped to user identity for Kubernetes access. | *`tls`* __xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-tlsspec[$$TLSSpec$$]__ | TLS configuration for communicating with the OIDC provider. |=== @@ -115,6 +114,24 @@ Status of a JWT authenticator. |=== +[id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-jwttokenclaims"] +==== JWTTokenClaims + +JWTTokenClaims allows customization of the claims that will be mapped to user identity for Kubernetes access. + +.Appears In: +**** +- xref:{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-jwtauthenticatorspec[$$JWTAuthenticatorSpec$$] +**** + +[cols="25a,75a", options="header"] +|=== +| Field | Description +| *`groups`* __string__ | Groups is the name of the claim which should be read to extract the user's group membership from the JWT token. When not specified, it will default to "groups". +| *`username`* __string__ | Username is the name of the claim which should be read to extract the username from the JWT token. When not specified, it will default to "username". +|=== + + [id="{anchor_prefix}-go-pinniped-dev-generated-1-19-apis-concierge-authentication-v1alpha1-tlsspec"] ==== TLSSpec diff --git a/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go b/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go index 1c43f68f..3e159148 100644 --- a/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go +++ b/generated/1.19/apis/concierge/authentication/v1alpha1/types_jwt.go @@ -27,21 +27,30 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` - // UsernameClaim is the name of the claim which should be read to extract the - // username from the JWT token. When not specified, it will default to "username". + // Claims allows customization of the claims that will be mapped to user identity + // for Kubernetes access. // +optional - UsernameClaim string `json:"username_claim"` - - // GroupsClaim is the name of the claim which should be read to extract the user's - // group membership from the JWT token. When not specified, it will default to "groups". - // +optional - GroupsClaim string `json:"groups_claim"` + Claims JWTTokenClaims `json:"claims"` // TLS configuration for communicating with the OIDC provider. // +optional TLS *TLSSpec `json:"tls,omitempty"` } +// JWTTokenClaims allows customization of the claims that will be mapped to user identity +// for Kubernetes access. +type JWTTokenClaims struct { + // Groups is the name of the claim which should be read to extract the user's + // group membership from the JWT token. When not specified, it will default to "groups". + // +optional + Groups string `json:"groups"` + + // Username is the name of the claim which should be read to extract the + // username from the JWT token. When not specified, it will default to "username". + // +optional + Username string `json:"username"` +} + // JWTAuthenticator describes the configuration of a JWT authenticator. // // Upon receiving a signed JWT, a JWTAuthenticator will performs some validation on it (e.g., valid diff --git a/generated/1.19/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go b/generated/1.19/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go index 93d4e837..c7c6968a 100644 --- a/generated/1.19/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go +++ b/generated/1.19/apis/concierge/authentication/v1alpha1/zz_generated.deepcopy.go @@ -92,6 +92,7 @@ func (in *JWTAuthenticatorList) DeepCopyObject() runtime.Object { // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *JWTAuthenticatorSpec) DeepCopyInto(out *JWTAuthenticatorSpec) { *out = *in + out.Claims = in.Claims if in.TLS != nil { in, out := &in.TLS, &out.TLS *out = new(TLSSpec) @@ -133,6 +134,22 @@ func (in *JWTAuthenticatorStatus) DeepCopy() *JWTAuthenticatorStatus { return out } +// DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. +func (in *JWTTokenClaims) DeepCopyInto(out *JWTTokenClaims) { + *out = *in + return +} + +// DeepCopy is an autogenerated deepcopy function, copying the receiver, creating a new JWTTokenClaims. +func (in *JWTTokenClaims) DeepCopy() *JWTTokenClaims { + if in == nil { + return nil + } + out := new(JWTTokenClaims) + in.DeepCopyInto(out) + return out +} + // DeepCopyInto is an autogenerated deepcopy function, copying the receiver, writing into out. in must be non-nil. func (in *TLSSpec) DeepCopyInto(out *TLSSpec) { *out = *in diff --git a/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml b/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml index 0865ca3c..e800411e 100644 --- a/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml +++ b/generated/1.19/crds/authentication.concierge.pinniped.dev_jwtauthenticators.yaml @@ -51,11 +51,21 @@ spec: description: Audience is the required value of the "aud" JWT claim. minLength: 1 type: string - groups_claim: - description: GroupsClaim is the name of the claim which should be - read to extract the user's group membership from the JWT token. - When not specified, it will default to "groups". - type: string + claims: + description: Claims allows customization of the claims that will be + mapped to user identity for Kubernetes access. + properties: + groups: + description: Groups is the name of the claim which should be read + to extract the user's group membership from the JWT token. When + not specified, it will default to "groups". + type: string + username: + description: Username is the name of the claim which should be + read to extract the username from the JWT token. When not specified, + it will default to "username". + type: string + type: object issuer: description: Issuer is the OIDC issuer URL that will be used to discover public signing keys. Issuer is also used to validate the "iss" JWT @@ -71,11 +81,6 @@ spec: If omitted, a default set of system roots will be trusted. type: string type: object - username_claim: - description: UsernameClaim is the name of the claim which should be - read to extract the username from the JWT token. When not specified, - it will default to "username". - type: string required: - audience - issuer diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go index 682a5ee6..03fea206 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller.go @@ -169,11 +169,11 @@ func newJWTAuthenticator(spec *auth1alpha1.JWTAuthenticatorSpec) (*jwtAuthentica caFile = temp.Name() } - usernameClaim := spec.UsernameClaim + usernameClaim := spec.Claims.Username if usernameClaim == "" { usernameClaim = defaultUsernameClaim } - groupsClaim := spec.GroupsClaim + groupsClaim := spec.Claims.Groups if groupsClaim == "" { groupsClaim = defaultGroupsClaim } diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 1c2fe19b..1f2d350f 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -90,16 +90,20 @@ func TestController(t *testing.T) { TLS: tlsSpecFromTLSConfig(server.TLS), } someJWTAuthenticatorSpecWithUsernameClaim := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: goodIssuer, - Audience: goodAudience, - TLS: tlsSpecFromTLSConfig(server.TLS), - UsernameClaim: "my-custom-username-claim", + Issuer: goodIssuer, + Audience: goodAudience, + TLS: tlsSpecFromTLSConfig(server.TLS), + Claims: auth1alpha1.JWTTokenClaims{ + Username: "my-custom-username-claim", + }, } someJWTAuthenticatorSpecWithGroupsClaim := &auth1alpha1.JWTAuthenticatorSpec{ - Issuer: goodIssuer, - Audience: goodAudience, - TLS: tlsSpecFromTLSConfig(server.TLS), - GroupsClaim: "my-custom-groups-claim", + Issuer: goodIssuer, + Audience: goodAudience, + TLS: tlsSpecFromTLSConfig(server.TLS), + Claims: auth1alpha1.JWTTokenClaims{ + Groups: "my-custom-groups-claim", + }, } otherJWTAuthenticatorSpec := &auth1alpha1.JWTAuthenticatorSpec{ Issuer: "https://some-other-issuer.com", @@ -170,7 +174,7 @@ func TestController(t *testing.T) { `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, }, wantCacheEntries: 1, - wantUsernameClaim: someJWTAuthenticatorSpecWithUsernameClaim.UsernameClaim, + wantUsernameClaim: someJWTAuthenticatorSpecWithUsernameClaim.Claims.Username, runTestsOnResultingAuthenticator: true, }, { @@ -189,7 +193,7 @@ func TestController(t *testing.T) { `jwtcachefiller-controller "level"=0 "msg"="added new jwt authenticator" "issuer"="` + goodIssuer + `" "jwtAuthenticator"={"name":"test-name","namespace":"test-namespace"}`, }, wantCacheEntries: 1, - wantGroupsClaim: someJWTAuthenticatorSpecWithGroupsClaim.GroupsClaim, + wantGroupsClaim: someJWTAuthenticatorSpecWithGroupsClaim.Claims.Groups, runTestsOnResultingAuthenticator: true, }, { diff --git a/test/library/client.go b/test/library/client.go index 9e83dea1..4b108662 100644 --- a/test/library/client.go +++ b/test/library/client.go @@ -182,7 +182,7 @@ func CreateTestJWTAuthenticatorForCLIUpstream(ctx context.Context, t *testing.T) Audience: testEnv.CLITestUpstream.ClientID, // The default UsernameClaim is "username" but the upstreams that we use for // integration tests won't necessarily have that claim, so use "sub" here. - UsernameClaim: "sub", + Claims: auth1alpha1.JWTTokenClaims{Username: "sub"}, } // If the test upstream does not have a CA bundle specified, then don't configure one in the // JWTAuthenticator. Leaving TLSSpec set to nil will result in OIDC discovery using the OS's root From 1d4012cabf717b258daf7b58e4b07e29d5110b4d Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 16 Dec 2020 10:17:17 -0800 Subject: [PATCH 10/12] jwtcachefiller_test.go: don't assert about time zones in errors Because the library that we are using which returns that error formats the timestamp in localtime, which is LMT when running on a laptop, but is UTC when running in CI. Signed-off-by: Ryan Richard --- .../authenticator/jwtcachefiller/jwtcachefiller_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go index 1f2d350f..9cec76e6 100644 --- a/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go +++ b/internal/controller/authenticator/jwtcachefiller/jwtcachefiller_test.go @@ -583,14 +583,14 @@ func testTableForAuthenticateTokenTests( jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Expiry = jwt.NewNumericDate(time.Date(1, 2, 3, 4, 5, 6, 7, time.UTC)) }, - wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-02-02 20:12:08 -0752 LMT\)`, + wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: .+`, }, { name: "bad token without exp", jwtClaims: func(claims *jwt.Claims, _ *interface{}, username *string) { claims.Expiry = nil }, - wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: 0001-01-01 00:00:00 \+0000 UTC\)`, + wantErrorRegexp: `oidc: verify token: oidc: token is expired \(Token Expiry: .+`, }, { name: "token does not have username claim", From 406fc955018c101512a054902a5ccdf5895342ce Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Wed, 16 Dec 2020 11:49:59 -0800 Subject: [PATCH 11/12] Empty commit to trigger CI Signed-off-by: Ryan Richard From 653224c2ad8edfd2ebd4b49f3743e3a9bd9f3136 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Wed, 16 Dec 2020 12:21:30 -0800 Subject: [PATCH 12/12] types_jwt.go.tmpl: Replace spaces with tabs --- apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl index 0087fad4..3e159148 100644 --- a/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl +++ b/apis/concierge/authentication/v1alpha1/types_jwt.go.tmpl @@ -27,8 +27,8 @@ type JWTAuthenticatorSpec struct { // +kubebuilder:validation:MinLength=1 Audience string `json:"audience"` - // Claims allows customization of the claims that will be mapped to user identity - // for Kubernetes access. + // Claims allows customization of the claims that will be mapped to user identity + // for Kubernetes access. // +optional Claims JWTTokenClaims `json:"claims"` @@ -41,12 +41,12 @@ type JWTAuthenticatorSpec struct { // for Kubernetes access. type JWTTokenClaims struct { // Groups is the name of the claim which should be read to extract the user's - // group membership from the JWT token. When not specified, it will default to "groups". + // group membership from the JWT token. When not specified, it will default to "groups". // +optional Groups string `json:"groups"` // Username is the name of the claim which should be read to extract the - // username from the JWT token. When not specified, it will default to "username". + // username from the JWT token. When not specified, it will default to "username". // +optional Username string `json:"username"` }