From 81148866e0ef0a8e09341823243222491a89ec69 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Thu, 27 May 2021 09:25:48 -0700 Subject: [PATCH] URL query escape the upstream OIDC subject in the downstream subject URL --- internal/oidc/callback/callback_handler.go | 6 +++- .../oidc/callback/callback_handler_test.go | 31 ++++++++++--------- 2 files changed, 21 insertions(+), 16 deletions(-) diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 5bece1d9..1b14c788 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -228,7 +228,7 @@ func getSubjectAndUsernameFromUpstreamIDToken( return "", "", httperr.New(http.StatusUnprocessableEntity, "subject claim in upstream ID token has invalid format") } - subject := fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, upstreamSubject) + subject := downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString, upstreamSubject) usernameClaimName := upstreamIDPConfig.GetUsernameClaim() if usernameClaimName == "" { @@ -282,6 +282,10 @@ func getSubjectAndUsernameFromUpstreamIDToken( return subject, username, nil } +func downstreamSubjectFromUpstreamOIDC(upstreamIssuerAsString string, upstreamSubject string) string { + return fmt.Sprintf("%s?%s=%s", upstreamIssuerAsString, oidc.IDTokenSubjectClaim, url.QueryEscape(upstreamSubject)) +} + func getGroupsFromUpstreamIDToken( upstreamIDPConfig provider.UpstreamOIDCIdentityProviderI, idTokenClaims map[string]interface{}, diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index c999cba4..583ee943 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -28,9 +28,10 @@ import ( const ( happyUpstreamIDPName = "upstream-idp-name" - upstreamIssuer = "https://my-upstream-issuer.com" - upstreamSubject = "abc123-some-guid" - upstreamUsername = "test-pinniped-username" + upstreamIssuer = "https://my-upstream-issuer.com" + upstreamSubject = "abc123-some guid" // has a space character which should get escaped in URL + queryEscapedUpstreamSubject = "abc123-some+guid" + upstreamUsername = "test-pinniped-username" upstreamUsernameClaim = "the-user-claim" upstreamGroupsClaim = "the-groups-claim" @@ -141,7 +142,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -160,8 +161,8 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, - wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, + wantDownstreamIDTokenUsername: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenGroups: []string{}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted, @@ -180,7 +181,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe@whitehouse.gov", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -201,7 +202,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe@whitehouse.gov", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -223,7 +224,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, // succeed despite `email_verified=false` because we're not using the email claim for anything wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: "joe", wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -268,7 +269,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamSubject, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -287,7 +288,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: []string{"notAnArrayGroup1 notAnArrayGroup2"}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -306,7 +307,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamIDTokenGroups: []string{"group1", "group2"}, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, @@ -445,7 +446,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=&state=` + happyDownstreamState, wantDownstreamIDTokenUsername: upstreamUsername, - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamRequestedScopes: []string{"profile", "email"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, wantDownstreamNonce: downstreamNonce, @@ -467,7 +468,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: downstreamRedirectURI + `\?code=([^&]+)&scope=openid\+offline_access&state=` + happyDownstreamState, wantDownstreamIDTokenUsername: upstreamUsername, - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamRequestedScopes: []string{"openid", "offline_access"}, wantDownstreamGrantedScopes: []string{"openid", "offline_access"}, wantDownstreamIDTokenGroups: upstreamGroupMembership, @@ -548,7 +549,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyDownstreamRedirectLocationRegexp, wantBody: "", - wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + upstreamSubject, + wantDownstreamIDTokenSubject: upstreamIssuer + "?sub=" + queryEscapedUpstreamSubject, wantDownstreamIDTokenUsername: upstreamUsername, wantDownstreamRequestedScopes: happyDownstreamScopesRequested, wantDownstreamGrantedScopes: happyDownstreamScopesGranted,