From 4c44f583e9e38af74b541c7886f2e0538eb9fdf6 Mon Sep 17 00:00:00 2001 From: Ryan Richard Date: Fri, 6 May 2022 12:00:46 -0700 Subject: [PATCH] Don't add pinniped_idp_name pinniped_idp_type params into upstream state --- internal/oidc/auth/auth_handler.go | 32 +++++++++++++++++++++---- internal/oidc/auth/auth_handler_test.go | 24 ++++++++++++++----- 2 files changed, 46 insertions(+), 10 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 0c3df1e8..ae502d3a 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -7,6 +7,7 @@ package auth import ( "fmt" "net/http" + "net/url" "time" coreosoidc "github.com/coreos/go-oidc/v3/oidc" @@ -53,6 +54,12 @@ func NewHandler( return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) } + // Note that the client might have used supervisoroidc.AuthorizeUpstreamIDPNameParamName and + // supervisoroidc.AuthorizeUpstreamIDPTypeParamName query params to request a certain upstream IDP. + // The Pinniped CLI has been sending these params since v0.9.0. + // Currently, these are ignored because the Supervisor does not yet support logins when multiple IDPs + // are configured. However, these params should be honored in the future when choosing an upstream + // here, e.g. by calling supervisoroidc.FindUpstreamIDPByNameAndType() when the params are present. oidcUpstream, ldapUpstream, idpType, err := chooseUpstreamIDP(idpLister) if err != nil { plog.WarningErr("authorize upstream config", err) @@ -65,7 +72,7 @@ func NewHandler( // The client set a username header, so they are trying to log in with a username/password. return handleAuthRequestForOIDCUpstreamPasswordGrant(r, w, oauthHelperWithStorage, oidcUpstream) } - return handleAuthRequestForOIDCUpstreamAuthcodeGrant(r, w, + return handleAuthRequestForOIDCUpstreamBrowserFlow(r, w, oauthHelperWithoutStorage, generateCSRF, generateNonce, generatePKCE, oidcUpstream, @@ -75,7 +82,7 @@ func NewHandler( ) } - // we know it's an AD/LDAP upstream. + // We know it's an AD/LDAP upstream. if len(r.Header.Values(supervisoroidc.AuthorizeUsernameHeaderName)) > 0 || len(r.Header.Values(supervisoroidc.AuthorizePasswordHeaderName)) > 0 { // The client set a username header, so they are trying to log in with a username/password. @@ -236,7 +243,7 @@ func handleAuthRequestForOIDCUpstreamPasswordGrant( return nil } -func handleAuthRequestForOIDCUpstreamAuthcodeGrant( +func handleAuthRequestForOIDCUpstreamBrowserFlow( r *http.Request, w http.ResponseWriter, oauthHelper fosite.OAuth2Provider, @@ -487,7 +494,12 @@ func upstreamStateParam( encoder oidc.Encoder, ) (string, error) { stateParamData := oidc.UpstreamStateParamData{ - AuthParams: authorizeRequester.GetRequestForm().Encode(), + // The auth params might have included supervisoroidc.AuthorizeUpstreamIDPNameParamName and + // supervisoroidc.AuthorizeUpstreamIDPTypeParamName, but those can be ignored by other handlers + // that are reading from the encoded upstream state param being built here. + // The UpstreamName and UpstreamType struct fields can be used instead. + // Remove those params here to avoid potential confusion about which should be used later. + AuthParams: removeCustomIDPParams(authorizeRequester.GetRequestForm()).Encode(), UpstreamName: upstreamName, UpstreamType: upstreamType, Nonce: nonceValue, @@ -502,6 +514,18 @@ func upstreamStateParam( return encodedStateParamValue, nil } +func removeCustomIDPParams(params url.Values) url.Values { + p := url.Values{} + // Copy all params. + for k, v := range params { + p[k] = v + } + // Remove the unnecessary params. + delete(p, supervisoroidc.AuthorizeUpstreamIDPNameParamName) + delete(p, supervisoroidc.AuthorizeUpstreamIDPTypeParamName) + return p +} + func addCSRFSetCookieHeader(w http.ResponseWriter, csrfValue csrftoken.CSRFToken, codec oidc.Encoder) error { encodedCSRFValue, err := codec.Encode(oidc.CSRFCookieEncodingName, csrfValue) if err != nil { diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index e2b3f000..dc93c42a 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -847,8 +847,6 @@ func TestAuthorizationEndpoint(t *testing.T) { cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"prompt": "login"}), - contentType: formContentType, - body: encodeQuery(happyGetRequestQueryMap), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantBodyStringWithLocationInHref: true, @@ -856,6 +854,24 @@ func TestAuthorizationEndpoint(t *testing.T) { wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(map[string]string{"prompt": "login"}, "", oidcUpstreamName, "oidc"), nil), wantUpstreamStateParamInLocationHeader: true, }, + { + name: "OIDC upstream browser flow happy path with custom IDP name and type query params, which are excluded from the query params in the upstream state", + idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().Build()), + generateCSRF: happyCSRFGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + stateEncoder: happyStateEncoder, + cookieEncoder: happyCookieEncoder, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{"pinniped_idp_name": "currently-ignored", "pinniped_idp_type": "oidc"}), + contentType: formContentType, + wantStatus: http.StatusSeeOther, + wantContentType: htmlContentType, + wantBodyStringWithLocationInHref: true, + wantCSRFValueInCookieHeader: happyCSRF, + wantLocationHeader: expectedRedirectLocationForUpstreamOIDC(expectedUpstreamStateParam(nil, "", oidcUpstreamName, "oidc"), nil), + wantUpstreamStateParamInLocationHeader: true, + }, { name: "OIDC upstream browser flow happy path with extra params that get passed through", idps: oidctestutil.NewUpstreamIDPListerBuilder().WithOIDC(upstreamOIDCIdentityProviderBuilder().WithAdditionalAuthcodeParams(map[string]string{"prompt": "consent", "abc": "123", "def": "456"}).Build()), @@ -866,8 +882,6 @@ func TestAuthorizationEndpoint(t *testing.T) { cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"prompt": "login"}), - contentType: formContentType, - body: encodeQuery(happyGetRequestQueryMap), wantStatus: http.StatusSeeOther, wantContentType: htmlContentType, wantBodyStringWithLocationInHref: true, @@ -885,8 +899,6 @@ func TestAuthorizationEndpoint(t *testing.T) { cookieEncoder: happyCookieEncoder, method: http.MethodGet, path: modifiedHappyGetRequestPath(map[string]string{"prompt": "none"}), - contentType: formContentType, - body: encodeQuery(happyGetRequestQueryMap), wantStatus: http.StatusSeeOther, wantContentType: jsonContentType, wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeLoginRequiredErrorQuery),