From 0bb2c7beb759247ea78e8299574bdd8cded48ba9 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Tue, 9 Aug 2022 16:07:23 -0700 Subject: [PATCH] Always add the `azp` claim to ID tokens to show the original client ID When the token exchange grant type is used to get a cluster-scoped ID token, the returned token has a new audience value. The client ID of the client which performed the authorization was lost. This didn't matter before, since the only client was `pinniped-cli`, but now that dynamic clients can be registered, the information would be lost in the cluster-scoped ID token. It could be useful for logging, tracing, or auditing, so preserve the information by putting the client ID into the `azp` claim in every ID token (authcode exchange, clsuter-scoped, and refreshed ID tokens). --- internal/oidc/auth/auth_handler.go | 6 ++++-- internal/oidc/callback/callback_handler.go | 3 ++- .../downstreamsession/downstream_session.go | 19 +++++++++++++++---- internal/oidc/login/post_login_handler.go | 3 ++- internal/oidc/token/token_handler_test.go | 10 ++++++++-- .../testutil/oidctestutil/oidctestutil.go | 12 ++++++++---- test/integration/supervisor_login_test.go | 13 +++++++++++-- 7 files changed, 50 insertions(+), 16 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index bf7e1764..a0245fba 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -149,7 +149,8 @@ func handleAuthRequestForLDAPUpstreamCLIFlow( username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(ldapUpstream, idpType, authenticateResponse, username) - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, + authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true) return nil @@ -250,7 +251,8 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( return nil } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, + authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, true) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index f3a37b9d..0409c252 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -79,7 +79,8 @@ func NewHandler( return httperr.Wrap(http.StatusUnprocessableEntity, err.Error(), err) } - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, + authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData) authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, openIDSession) if err != nil { diff --git a/internal/oidc/downstreamsession/downstream_session.go b/internal/oidc/downstreamsession/downstream_session.go index cec13d1e..809a48f4 100644 --- a/internal/oidc/downstreamsession/downstream_session.go +++ b/internal/oidc/downstreamsession/downstream_session.go @@ -41,7 +41,14 @@ const ( ) // MakeDownstreamSession creates a downstream OIDC session. -func MakeDownstreamSession(subject string, username string, groups []string, grantedScopes []string, custom *psession.CustomSessionData) *psession.PinnipedSession { +func MakeDownstreamSession( + subject string, + username string, + groups []string, + grantedScopes []string, + clientID string, + custom *psession.CustomSessionData, +) *psession.PinnipedSession { now := time.Now().UTC() openIDSession := &psession.PinnipedSession{ Fosite: &openid.DefaultSession{ @@ -56,13 +63,17 @@ func MakeDownstreamSession(subject string, username string, groups []string, gra if groups == nil { groups = []string{} } - openIDSession.IDTokenClaims().Extra = map[string]interface{}{} + + extras := map[string]interface{}{} + extras[oidcapi.IDTokenClaimAuthorizedParty] = clientID if slices.Contains(grantedScopes, oidcapi.ScopeUsername) { - openIDSession.IDTokenClaims().Extra[oidcapi.IDTokenClaimUsername] = username + extras[oidcapi.IDTokenClaimUsername] = username } if slices.Contains(grantedScopes, oidcapi.ScopeGroups) { - openIDSession.IDTokenClaims().Extra[oidcapi.IDTokenClaimGroups] = groups + extras[oidcapi.IDTokenClaimGroups] = groups } + openIDSession.IDTokenClaims().Extra = extras + return openIDSession } diff --git a/internal/oidc/login/post_login_handler.go b/internal/oidc/login/post_login_handler.go index a9fe251a..a5a2d04e 100644 --- a/internal/oidc/login/post_login_handler.go +++ b/internal/oidc/login/post_login_handler.go @@ -83,7 +83,8 @@ func NewPostHandler(issuerURL string, upstreamIDPs oidc.UpstreamIdentityProvider username = authenticateResponse.User.GetName() groups := authenticateResponse.User.GetGroups() customSessionData := downstreamsession.MakeDownstreamLDAPOrADCustomSessionData(ldapUpstream, idpType, authenticateResponse, username) - openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, authorizeRequester.GetGrantedScopes(), customSessionData) + openIDSession := downstreamsession.MakeDownstreamSession(subject, username, groups, + authorizeRequester.GetGrantedScopes(), authorizeRequester.GetClient().GetID(), customSessionData) oidc.PerformAuthcodeRedirect(r, w, oauthHelper, authorizeRequester, openIDSession, false) return nil diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index efecad95..5e9ce5c4 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -1398,7 +1398,7 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn require.NoError(t, json.Unmarshal(parsedJWT.UnsafePayloadWithoutVerification(), &tokenClaims)) // Make sure that these are the only fields in the token. - idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "username"} + idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "username", "azp"} if test.authcodeExchange.want.wantGroups != nil { idTokenFields = append(idTokenFields, "groups") } @@ -1412,6 +1412,7 @@ func TestTokenEndpointTokenExchange(t *testing.T) { // tests for grant_type "urn require.NotEmpty(t, tokenClaims["rat"]) require.Len(t, tokenClaims["aud"], 1) require.Contains(t, tokenClaims["aud"], test.requestedAudience) + require.Equal(t, test.authcodeExchange.want.wantClientID, tokenClaims["azp"]) require.Equal(t, goodSubject, tokenClaims["sub"]) require.Equal(t, goodIssuer, tokenClaims["iss"]) if test.authcodeExchange.want.wantUsername != "" { @@ -4027,6 +4028,9 @@ func simulateAuthEndpointHavingAlreadyRun( session.Fosite.Claims.Extra["groups"] = goodGroups } + // The authorization endpoint sets the authorized party to the client ID of the original requester. + session.Fosite.Claims.Extra["azp"] = authRequester.GetClient().GetID() + authResponder, err := oauthHelper.NewAuthorizeResponse(ctx, authRequester, session) require.NoError(t, err) return authResponder @@ -4291,6 +4295,7 @@ func requireValidStoredRequest( if wantGroups != nil { expectedExtra["groups"] = toSliceOfInterface(wantGroups) } + expectedExtra["azp"] = wantClientID require.Equal(t, expectedExtra, claims.Extra) // We are in charge of setting these fields. For the purpose of testing, we ensure that the @@ -4412,7 +4417,7 @@ func requireValidIDToken( // 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", "auth_time", "exp", "iat", "rat"} + idTokenFields := []string{"sub", "aud", "iss", "jti", "auth_time", "exp", "iat", "rat", "azp"} if wantAtHashClaimInIDToken { idTokenFields = append(idTokenFields, "at_hash") } @@ -4439,6 +4444,7 @@ func requireValidIDToken( require.Equal(t, wantGroupsInIDToken, claims.Groups) require.Len(t, claims.Audience, 1) require.Equal(t, wantClientID, claims.Audience[0]) + require.Equal(t, wantClientID, m["azp"]) require.Equal(t, goodIssuer, claims.Issuer) require.NotEmpty(t, claims.JTI) diff --git a/internal/testutil/oidctestutil/oidctestutil.go b/internal/testutil/oidctestutil/oidctestutil.go index 2056e2d1..f5de96e7 100644 --- a/internal/testutil/oidctestutil/oidctestutil.go +++ b/internal/testutil/oidctestutil/oidctestutil.go @@ -1062,27 +1062,31 @@ func validateAuthcodeStorage( // Now confirm the ID token claims. actualClaims := storedSessionFromAuthcode.Fosite.Claims + // Should always have an azp claim. + require.Equal(t, wantDownstreamClientID, actualClaims.Extra["azp"]) + wantDownstreamIDTokenExtraClaimsCount := 1 // should always have azp claim + // 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) - wantDownstreamIDTokenUsernameClaimToExist := 1 if wantDownstreamIDTokenUsername == "" { - wantDownstreamIDTokenUsernameClaimToExist = 0 require.NotContains(t, actualClaims.Extra, "username") } else { + wantDownstreamIDTokenExtraClaimsCount++ // should also have username claim require.Equal(t, wantDownstreamIDTokenUsername, actualClaims.Extra["username"]) } if slices.Contains(wantDownstreamGrantedScopes, "groups") { - require.Len(t, actualClaims.Extra, wantDownstreamIDTokenUsernameClaimToExist+1) + wantDownstreamIDTokenExtraClaimsCount++ // should also have groups claim actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] require.NotNil(t, actualDownstreamIDTokenGroups) require.ElementsMatch(t, wantDownstreamIDTokenGroups, actualDownstreamIDTokenGroups) } else { require.Emptyf(t, wantDownstreamIDTokenGroups, "test case did not want the groups scope to be granted, "+ "but wanted something in the groups claim, which doesn't make sense. please review the test case's expectations.") - require.Len(t, actualClaims.Extra, wantDownstreamIDTokenUsernameClaimToExist) actualDownstreamIDTokenGroups := actualClaims.Extra["groups"] require.Nil(t, actualDownstreamIDTokenGroups) } + // Make sure that we asserted on every extra claim. + require.Len(t, actualClaims.Extra, wantDownstreamIDTokenExtraClaimsCount) // Check the rest of the downstream ID token's claims. Fosite wants us to set these (in UTC time). testutil.RequireTimeInDelta(t, time.Now().UTC(), actualClaims.RequestedAt, timeComparisonFudgeFactor) diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index 069923ff..1aee71c1 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -1996,7 +1996,7 @@ func testSupervisorLogin( } 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", "azp"} if slices.Contains(wantDownstreamScopes, "username") { // If the test wants the username scope to have been granted, then also expect the claim in the ID token. expectedIDTokenClaims = append(expectedIDTokenClaims, "username") @@ -2044,7 +2044,7 @@ func testSupervisorLogin( require.NoError(t, err) // When refreshing, expect to get an "at_hash" claim, but no "nonce" claim. - expectRefreshedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "rat", "at_hash"} + expectRefreshedIDTokenClaims := []string{"iss", "exp", "sub", "aud", "auth_time", "iat", "jti", "rat", "azp", "at_hash"} if slices.Contains(wantDownstreamScopes, "username") { // If the test wants the username scope to have been granted, then also expect the claim in the refreshed ID token. expectRefreshedIDTokenClaims = append(expectRefreshedIDTokenClaims, "username") @@ -2148,6 +2148,10 @@ func verifyTokenResponse( } require.ElementsMatch(t, expectedIDTokenClaims, idTokenClaimNames) + // There should always be an "azp" claim, and the value should be the client ID of the client which made + // the authorization request. + require.Equal(t, downstreamOAuth2Config.ClientID, idTokenClaims["azp"]) + // Check username claim of the ID token, if one is expected. Asserting on the lack of a username claim is // handled above where the full list of claims are asserted. if wantDownstreamIDTokenUsernameToMatch != "" { @@ -2423,6 +2427,11 @@ func doTokenExchange( indentedClaims, err := json.MarshalIndent(claims, " ", " ") require.NoError(t, err) t.Logf("exchanged token claims:\n%s", string(indentedClaims)) + + // The original client ID should be preserved in the azp claim, therefore preserving this information + // about the original source of the authorization for tracing/auditing purposes, since the "aud" claim + // has been updated to have a new value. + require.Equal(t, config.ClientID, claims["azp"]) } func expectSecurityHeaders(t *testing.T, response *http.Response, expectFositeToOverrideSome bool) {