From f8d76066c5d2ebffefbd46c5b3fdda72ececda45 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Fri, 20 Nov 2020 08:38:23 -0500 Subject: [PATCH] callback_handler.go: assert nonce is stored correctly I think we want to do this here since we are storing all of the other ID token claims? Signed-off-by: Andrew Keesler --- internal/oidc/callback/callback_handler.go | 11 +++++++++-- internal/oidc/callback/callback_handler_test.go | 12 ++++++++---- 2 files changed, 17 insertions(+), 6 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index a9e144b2..e443b9d6 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -97,7 +97,13 @@ func NewHandler( return err } - openIDSession := makeDownstreamSession(downstreamIssuer, downstreamAuthParams.Get("client_id"), username, groups) + openIDSession := makeDownstreamSession( + downstreamIssuer, + downstreamAuthParams.Get("client_id"), + downstreamAuthParams.Get("nonce"), + 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()) @@ -291,7 +297,7 @@ func getGroupsFromUpstreamIDToken( return groups, nil } -func makeDownstreamSession(issuer, clientID, username string, groups []string) *openid.DefaultSession { +func makeDownstreamSession(issuer, clientID, nonce, username string, groups []string) *openid.DefaultSession { now := time.Now() openIDSession := &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ @@ -302,6 +308,7 @@ func makeDownstreamSession(issuer, clientID, username string, groups []string) * IssuedAt: now, RequestedAt: now, AuthTime: now, + Nonce: nonce, }, } if groups != nil { diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index 36f51b21..bcfc3e8f 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -49,6 +49,7 @@ const ( happyUpstreamAuthcode = "upstream-auth-code" downstreamRedirectURI = "http://127.0.0.1/callback" downstreamClientID = "pinniped-cli" + downstreamNonce = "some-nonce-value" timeComparisonFudgeFactor = time.Second * 15 ) @@ -62,7 +63,7 @@ var ( "scope": []string{strings.Join(happyDownstreamScopesRequested, " ")}, "client_id": []string{downstreamClientID}, "state": []string{happyDownstreamState}, - "nonce": []string{"some-nonce-value"}, + "nonce": []string{downstreamNonce}, "code_challenge": []string{"some-challenge"}, "code_challenge_method": []string{"S256"}, "redirect_uri": []string{downstreamRedirectURI}, @@ -119,6 +120,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenSubject string wantDownstreamIDTokenGroups []string wantDownstreamRequestedScopes []string + wantDownstreamNonce string wantExchangeAndValidateTokensCall *oidctestutil.ExchangeAuthcodeAndValidateTokenArgs }{ @@ -135,6 +137,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenSubject: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamNonce: downstreamNonce, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, { @@ -150,6 +153,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, wantDownstreamIDTokenGroups: nil, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamNonce: downstreamNonce, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, { @@ -165,6 +169,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenSubject: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, + wantDownstreamNonce: downstreamNonce, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, @@ -287,6 +292,7 @@ func TestCallbackEndpoint(t *testing.T) { wantDownstreamIDTokenSubject: upstreamUsername, wantDownstreamRequestedScopes: []string{"profile", "email"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, + wantDownstreamNonce: downstreamNonce, wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, }, { @@ -507,6 +513,7 @@ func TestCallbackEndpoint(t *testing.T) { // Check the rest of the downstream ID token's claims. require.Equal(t, downstreamIssuer, actualClaims.Issuer) require.Equal(t, []string{downstreamClientID}, actualClaims.Audience) + require.Equal(t, test.wantDownstreamNonce, actualClaims.Nonce) testutil.RequireTimeInDelta(t, time.Now().Add(time.Minute*5), actualClaims.ExpiresAt, timeComparisonFudgeFactor) testutil.RequireTimeInDelta(t, time.Now(), actualClaims.IssuedAt, timeComparisonFudgeFactor) testutil.RequireTimeInDelta(t, time.Now(), actualClaims.RequestedAt, timeComparisonFudgeFactor) @@ -519,9 +526,6 @@ func TestCallbackEndpoint(t *testing.T) { require.Empty(t, actualClaims.AuthenticationContextClassReference) require.Empty(t, actualClaims.AuthenticationMethodsReference) - // TODO we should put the downstream request's nonce into the ID token, but maybe the token endpoint is responsible for that? - require.Empty(t, actualClaims.Nonce) - // TODO add thorough tests about what should be stored for PKCES and IDSessions } else { require.Empty(t, rsp.Header().Values("Location"))