From 0cd086cf9cac6c696abaa3d92cda071cb86544a5 Mon Sep 17 00:00:00 2001 From: Margo Crawford Date: Tue, 14 Dec 2021 11:59:52 -0800 Subject: [PATCH] Check username claim is unchanged for oidc. Also add integration tests for claims changing. --- internal/oidc/token/token_handler.go | 18 ++++- internal/oidc/token/token_handler_test.go | 89 +++++++++++++++++++++- internal/upstreamoidc/upstreamoidc.go | 2 +- internal/upstreamoidc/upstreamoidc_test.go | 39 ++++++++++ test/integration/supervisor_login_test.go | 12 +-- 5 files changed, 145 insertions(+), 15 deletions(-) diff --git a/internal/oidc/token/token_handler.go b/internal/oidc/token/token_handler.go index 2630fadb..9e27be10 100644 --- a/internal/oidc/token/token_handler.go +++ b/internal/oidc/token/token_handler.go @@ -6,6 +6,7 @@ package token import ( "context" + "errors" "net/http" "github.com/ory/fosite" @@ -142,12 +143,23 @@ func upstreamOIDCRefresh(ctx context.Context, session *psession.PinnipedSession, oldDownstreamSubject := session.Fosite.Claims.Subject oldSub, err := upstreamoidc.ExtractUpstreamSubjectFromDownstream(oldDownstreamSubject) if err != nil { - return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Could not verify upstream refresh subject against previous value").WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + return errorsx.WithStack(errUpstreamRefreshError.WithHintf("Upstream refresh failed."). + WithWrap(err).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) } if oldSub != newSub { return errorsx.WithStack(errUpstreamRefreshError.WithHintf( - "Subject in upstream refresh does not match previous value").WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + "Upstream refresh failed.").WithWrap(errors.New("subject in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } + usernameClaim := p.GetUsernameClaim() + newUsername := claims[usernameClaim] + // its possible this won't be returned. + // but if it is, verify that it hasn't changed. + if newUsername != nil { + oldUsername := session.Fosite.Claims.Extra["username"] + if oldUsername != newUsername { + return errorsx.WithStack(errUpstreamRefreshError.WithHintf( + "Upstream refresh failed.").WithWrap(errors.New("username in upstream refresh does not match previous value")).WithDebugf("provider name: %q, provider type: %q", s.ProviderName, s.ProviderType)) + } } } diff --git a/internal/oidc/token/token_handler_test.go b/internal/oidc/token/token_handler_test.go index 6fb6e2e8..eb81a280 100644 --- a/internal/oidc/token/token_handler_test.go +++ b/internal/oidc/token/token_handler_test.go @@ -22,8 +22,6 @@ import ( "testing" "time" - "go.pinniped.dev/pkg/oidcclient/oidctypes" - "github.com/ory/fosite" fositeoauth2 "github.com/ory/fosite/handler/oauth2" "github.com/ory/fosite/handler/openid" @@ -54,6 +52,7 @@ import ( "go.pinniped.dev/internal/psession" "go.pinniped.dev/internal/testutil" "go.pinniped.dev/internal/testutil/oidctestutil" + "go.pinniped.dev/pkg/oidcclient/oidctypes" ) const ( @@ -1067,6 +1066,30 @@ func TestRefreshGrant(t *testing.T) { ), }, }, + { + name: "refresh grant with unchanged username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": "some-subject", + "username-claim": goodUsername, + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: happyRefreshTokenResponseForOpenIDAndOfflineAccess( + upstreamOIDCCustomSessionDataWithNewRefreshToken(oidcUpstreamRefreshedRefreshToken), + refreshedUpstreamTokensWithIDAndRefreshTokens(), + ), + }, + }, { name: "happy path refresh grant without openid scope granted (no id token returned)", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( @@ -1617,7 +1640,67 @@ func TestRefreshGrant(t *testing.T) { wantErrorResponseBody: here.Doc(` { "error": "error", - "error_description": "Error during upstream refresh. Subject in upstream refresh does not match previous value" + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with claims but not the subject claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." + } + `), + }, + }, + }, + { + name: "refresh grant with changed username claim", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC( + upstreamOIDCIdentityProviderBuilder().WithUsernameClaim("username-claim").WithValidatedTokens(&oidctypes.Token{ + IDToken: &oidctypes.IDToken{ + Claims: map[string]interface{}{ + "some-claim": "some-value", + "sub": "some-subject", + "username-claim": "some-changed-username", + }, + }, + }).WithRefreshedTokens(refreshedUpstreamTokensWithIDAndRefreshTokens()).Build()), + authcodeExchange: authcodeExchangeInputs{ + customSessionData: initialUpstreamOIDCCustomSessionData(), + modifyAuthRequest: func(r *http.Request) { r.Form.Set("scope", "openid offline_access") }, + want: happyAuthcodeExchangeTokenResponseForOpenIDAndOfflineAccess(initialUpstreamOIDCCustomSessionData()), + }, + refreshRequest: refreshRequestInputs{ + want: tokenEndpointResponseExpectedValues{ + wantUpstreamRefreshCall: happyOIDCUpstreamRefreshCall(), + wantUpstreamOIDCValidateTokenCall: happyUpstreamValidateTokenCall(refreshedUpstreamTokensWithIDAndRefreshTokens()), + wantStatus: http.StatusUnauthorized, + wantErrorResponseBody: here.Doc(` + { + "error": "error", + "error_description": "Error during upstream refresh. Upstream refresh failed." } `), }, diff --git a/internal/upstreamoidc/upstreamoidc.go b/internal/upstreamoidc/upstreamoidc.go index 0ac4dbd5..1c7b7d8c 100644 --- a/internal/upstreamoidc/upstreamoidc.go +++ b/internal/upstreamoidc/upstreamoidc.go @@ -242,7 +242,7 @@ func ExtractUpstreamSubjectFromDownstream(downstreamSubject string) (string, err if !strings.Contains(downstreamSubject, "?sub=") { return "", errors.New("downstream subject did not contain original upstream subject") } - return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil // TODO test for ?sub= occurring twice (imagine if you ran the supervisor with another supervisor as the upstream idp...) + return strings.SplitN(downstreamSubject, "?sub=", 2)[1], nil } // ValidateToken will validate the ID token. It will also merge the claims from the userinfo endpoint response, diff --git a/internal/upstreamoidc/upstreamoidc_test.go b/internal/upstreamoidc/upstreamoidc_test.go index 949a56d7..7d5552f4 100644 --- a/internal/upstreamoidc/upstreamoidc_test.go +++ b/internal/upstreamoidc/upstreamoidc_test.go @@ -910,6 +910,45 @@ func TestProviderConfig(t *testing.T) { } }) + t.Run("ExtractUpstreamSubjectFromDownstream", func(t *testing.T) { + tests := []struct { + name string + downstreamSubject string + wantUpstreamSubject string + wantErr string + }{ + { + name: "happy path", + downstreamSubject: "https://some-issuer?sub=some-subject", + wantUpstreamSubject: "some-subject", + }, + { + name: "subject in a subject", + downstreamSubject: "https://some-other-issuer?sub=https://some-issuer?sub=some-subject", + wantUpstreamSubject: "https://some-issuer?sub=some-subject", + }, + { + name: "doesn't contain sub=", + downstreamSubject: "something-invalid", + wantErr: "downstream subject did not contain original upstream subject", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + actualUpstreamSubject, err := ExtractUpstreamSubjectFromDownstream(tt.downstreamSubject) + if tt.wantErr != "" { + require.Error(t, err) + require.Equal(t, tt.wantErr, err.Error()) + } else { + require.NoError(t, err) + require.Equal(t, tt.wantUpstreamSubject, actualUpstreamSubject) + } + }) + } + + }) + t.Run("ExchangeAuthcodeAndValidateTokens", func(t *testing.T) { tests := []struct { name string diff --git a/test/integration/supervisor_login_test.go b/test/integration/supervisor_login_test.go index b10f650e..db2f617e 100644 --- a/test/integration/supervisor_login_test.go +++ b/test/integration/supervisor_login_test.go @@ -87,10 +87,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Subject = "wrong-subject" }, // the ID token Subject should include the upstream user ID after the upstream issuer name wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", @@ -124,10 +122,8 @@ func TestSupervisorLogin(t *testing.T) { }, requestAuthorization: requestAuthorizationUsingBrowserAuthcodeFlow, breakRefreshSessionData: func(t *testing.T, pinnipedSession *psession.PinnipedSession, _, _ string) { - customSessionData := pinnipedSession.Custom - require.Equal(t, psession.ProviderTypeOIDC, customSessionData.ProviderType) - require.NotEmpty(t, customSessionData.OIDC.UpstreamRefreshToken) - customSessionData.OIDC.UpstreamRefreshToken = "invalid-updated-refresh-token" + fositeSessionData := pinnipedSession.Fosite + fositeSessionData.Claims.Extra["username"] = "some-incorrect-username" }, wantDownstreamIDTokenSubjectToMatch: "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Issuer+"?sub=") + ".+", wantDownstreamIDTokenUsernameToMatch: func(_ string) string { return "^" + regexp.QuoteMeta(env.SupervisorUpstreamOIDC.Username) + "$" },