diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index a6fa63b4..08c42ada 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -24,7 +24,7 @@ import ( ) func NewHandler( - issuer string, + downstreamIssuer string, idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provider, generateCSRF func() (csrftoken.CSRFToken, error), @@ -92,7 +92,7 @@ func NewHandler( Endpoint: oauth2.Endpoint{ AuthURL: upstreamIDP.GetAuthorizationURL().String(), }, - RedirectURL: fmt.Sprintf("%s/callback/%s", issuer, upstreamIDP.GetName()), + RedirectURL: fmt.Sprintf("%s/callback/%s", downstreamIssuer, upstreamIDP.GetName()), Scopes: upstreamIDP.GetScopes(), } diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index df90bde5..9bfe06e5 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -28,6 +28,7 @@ import ( func TestAuthorizationEndpoint(t *testing.T) { const ( + downstreamIssuer = "https://my-downstream-issuer.com/some-path" downstreamRedirectURI = "http://127.0.0.1/callback" downstreamRedirectURIWithDifferentPort = "http://127.0.0.1:42/callback" ) @@ -120,8 +121,6 @@ func TestAuthorizationEndpoint(t *testing.T) { Scopes: []string{"scope1", "scope2"}, } - issuer := "https://my-issuer.com/some-path" - // Configure fosite the same way that the production code would, using NullStorage to turn off storage. oauthStore := oidc.NullStorage{} hmacSecret := []byte("some secret - must have at least 32 bytes") @@ -233,7 +232,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", - "redirect_uri": issuer + "/callback/some-idp", + "redirect_uri": downstreamIssuer + "/callback/some-idp", }) } @@ -270,7 +269,7 @@ func TestAuthorizationEndpoint(t *testing.T) { tests := []testCase{ { name: "happy path using GET without a CSRF cookie", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -288,7 +287,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "happy path using GET with a CSRF cookie", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -306,7 +305,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "happy path using POST", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -326,7 +325,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "happy path when downstream redirect uri matches what is configured for client except for the port number", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -348,7 +347,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "downstream redirect uri does not match what is configured for client", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -365,7 +364,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "downstream client does not exist", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -380,7 +379,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "response type is unsupported", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -396,7 +395,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "downstream scopes do not match what is configured for client", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -412,7 +411,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "missing response type in request", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -428,7 +427,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "missing client id in request", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -443,7 +442,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "missing PKCE code_challenge in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -459,7 +458,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "invalid value for PKCE code_challenge_method in request", // https://tools.ietf.org/html/rfc7636#section-4.3 - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -475,7 +474,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "when PKCE code_challenge_method in request is `plain`", // https://tools.ietf.org/html/rfc7636#section-4.3 - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -491,7 +490,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1 - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -509,7 +508,7 @@ func TestAuthorizationEndpoint(t *testing.T) { // This is just one of the many OIDC validations run by fosite. This test is to ensure that we are running // through that part of the fosite library. name: "prompt param is not allowed to have none and another legal value at the same time", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -525,7 +524,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "OIDC validations are skipped when the openid scope was not requested", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -546,7 +545,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "state does not have enough entropy", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -562,7 +561,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while encoding upstream state param", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -577,7 +576,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while encoding CSRF cookie value for new cookie", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -592,7 +591,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while generating CSRF token", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: func() (csrftoken.CSRFToken, error) { return "", fmt.Errorf("some csrf generator error") }, generatePKCE: happyPKCEGenerator, @@ -607,7 +606,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while generating nonce", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -622,7 +621,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while generating PKCE", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: func() (pkce.Code, error) { return "", fmt.Errorf("some PKCE generator error") }, @@ -637,7 +636,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "error while decoding CSRF cookie", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), generateCSRF: happyCSRFGenerator, generatePKCE: happyPKCEGenerator, @@ -653,7 +652,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "no upstream providers are configured", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(), // empty method: http.MethodGet, path: happyGetRequestPath, @@ -663,7 +662,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "too many upstream providers are configured", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider, &upstreamOIDCIdentityProvider), // more than one not allowed method: http.MethodGet, path: happyGetRequestPath, @@ -673,7 +672,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "PUT is a bad method", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), method: http.MethodPut, path: "/some/path", @@ -683,7 +682,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "PATCH is a bad method", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), method: http.MethodPatch, path: "/some/path", @@ -693,7 +692,7 @@ func TestAuthorizationEndpoint(t *testing.T) { }, { name: "DELETE is a bad method", - issuer: issuer, + issuer: downstreamIssuer, idpListGetter: testutil.NewIDPListGetter(&upstreamOIDCIdentityProvider), method: http.MethodDelete, path: "/some/path", @@ -792,7 +791,7 @@ func TestAuthorizationEndpoint(t *testing.T) { "nonce": happyNonce, "code_challenge": expectedUpstreamCodeChallenge, "code_challenge_method": "S256", - "redirect_uri": issuer + "/callback/some-other-idp", + "redirect_uri": downstreamIssuer + "/callback/some-other-idp", }, ) test.wantBodyString = fmt.Sprintf(`Found.%s`, diff --git a/internal/oidc/callback/callback_handler.go b/internal/oidc/callback/callback_handler.go index 93073cd6..009fa737 100644 --- a/internal/oidc/callback/callback_handler.go +++ b/internal/oidc/callback/callback_handler.go @@ -36,7 +36,12 @@ const ( downstreamGroupsClaim = "oidc.pinniped.dev/groups" ) -func NewHandler(idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provider, stateDecoder, cookieDecoder oidc.Decoder) http.Handler { +func NewHandler( + downstreamIssuer string, + idpListGetter oidc.IDPListGetter, + oauthHelper fosite.OAuth2Provider, + stateDecoder, cookieDecoder oidc.Decoder, +) http.Handler { return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { state, err := validateRequest(r, stateDecoder, cookieDecoder) if err != nil { @@ -106,7 +111,7 @@ func NewHandler(idpListGetter oidc.IDPListGetter, oauthHelper fosite.OAuth2Provi now := time.Now() authorizeResponder, err := oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{ Claims: &jwt.IDTokenClaims{ - Issuer: "https://fosite.my-application.com", // TODO use the right value here + Issuer: downstreamIssuer, Subject: username, Audience: []string{"my-client"}, // TODO use the right value here ExpiresAt: now.Add(time.Minute * 30), // TODO use the right value here diff --git a/internal/oidc/callback/callback_handler_test.go b/internal/oidc/callback/callback_handler_test.go index a375f095..07fc7b18 100644 --- a/internal/oidc/callback/callback_handler_test.go +++ b/internal/oidc/callback/callback_handler_test.go @@ -33,8 +33,8 @@ const ( func TestCallbackEndpoint(t *testing.T) { const ( + downstreamIssuer = "https://my-downstream-issuer.com/path" downstreamRedirectURI = "http://127.0.0.1/callback" - happyUpstreamAuthcode = "upstream-auth-code" ) @@ -207,9 +207,7 @@ func TestCallbackEndpoint(t *testing.T) { wantStatus int wantBody string wantRedirectLocationRegexp string - // TODO: I am unused... - wantAuthcodeStored bool - wantGrantedOpenidScope bool + wantGrantedOpenidScope bool wantExchangeAndValidateTokensCall *testutil.ExchangeAuthcodeAndValidateTokenArgs }{ @@ -221,7 +219,6 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyRedirectLocationRegexp, - wantAuthcodeStored: true, wantGrantedOpenidScope: true, wantBody: "", wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, @@ -234,7 +231,6 @@ func TestCallbackEndpoint(t *testing.T) { csrfCookie: happyCSRFCookie, wantStatus: http.StatusFound, wantRedirectLocationRegexp: happyRedirectLocationRegexp, - wantAuthcodeStored: true, wantGrantedOpenidScope: true, wantBody: "", wantExchangeAndValidateTokensCall: happyExchangeAndValidateTokensArgs, @@ -396,7 +392,7 @@ func TestCallbackEndpoint(t *testing.T) { oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) idpListGetter := testutil.NewIDPListGetter(&test.idp) - subject := NewHandler(idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec) + subject := NewHandler(downstreamIssuer, idpListGetter, oauthHelper, happyStateCodec, happyCookieCodec) req := httptest.NewRequest(test.method, test.path, nil) if test.csrfCookie != "" { req.Header.Set("Cookie", test.csrfCookie) @@ -406,9 +402,15 @@ func TestCallbackEndpoint(t *testing.T) { t.Logf("response: %#v", rsp) t.Logf("response body: %q", rsp.Body.String()) - require.Equal(t, test.wantStatus, rsp.Code) + if test.wantExchangeAndValidateTokensCall != nil { + require.Equal(t, 1, test.idp.ExchangeAuthcodeAndValidateTokensCallCount()) + test.wantExchangeAndValidateTokensCall.Ctx = req.Context() + require.Equal(t, test.wantExchangeAndValidateTokensCall, test.idp.ExchangeAuthcodeAndValidateTokensArgs(0)) + } else { + require.Equal(t, 0, test.idp.ExchangeAuthcodeAndValidateTokensCallCount()) + } - require.False(t, test.wantBody != "" && test.wantRedirectLocationRegexp != "", "test cannot set both body and redirect assertions") + require.Equal(t, test.wantStatus, rsp.Code) if test.wantBody != "" { require.Equal(t, test.wantBody, rsp.Body.String()) @@ -448,19 +450,12 @@ func TestCallbackEndpoint(t *testing.T) { } else { require.NotContains(t, storedRequest.GetGrantedScopes(), "openid") } + require.Equal(t, downstreamIssuer, storedSession.Claims.Issuer) require.Equal(t, "test-pinniped-username", storedSession.Claims.Subject) require.Equal(t, []string{"test-pinniped-group-0", "test-pinniped-group-1"}, storedSession.Claims.Extra["oidc.pinniped.dev/groups"]) } else { require.Empty(t, rsp.Header().Values("Location")) } - - if test.wantExchangeAndValidateTokensCall != nil { - require.Equal(t, 1, test.idp.ExchangeAuthcodeAndValidateTokensCallCount()) - test.wantExchangeAndValidateTokensCall.Ctx = req.Context() - require.Equal(t, test.wantExchangeAndValidateTokensCall, test.idp.ExchangeAuthcodeAndValidateTokensArgs(0)) - } else { - require.Equal(t, 0, test.idp.ExchangeAuthcodeAndValidateTokensCallCount()) - } }) } }