From a36f7c6c07b8b39532955e05b6ccab2aca3eed4c Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 15:04:50 -0800 Subject: [PATCH] Test that the port of localhost redirect URI is ignored during validation Also move definition of our oauth client and the general fosite configuration to a helper so we can use the same config to construct the handler for both test and production code. Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 55 +++++-------------------- internal/oidc/auth/auth_handler_test.go | 41 +++++++++++------- internal/oidc/oidc.go | 39 ++++++++++++++++++ 3 files changed, 76 insertions(+), 59 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index 8142c25c3..7f89eaee0 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/internal/oidc/auth/auth_handler.go @@ -10,7 +10,8 @@ import ( "github.com/ory/fosite/handler/openid" - "github.com/ory/fosite/compose" + "github.com/ory/fosite" + "golang.org/x/oauth2" "k8s.io/klog/v2" @@ -28,39 +29,11 @@ type IDPListGetter interface { func NewHandler( issuer string, idpListGetter IDPListGetter, - oauthStore interface{}, + oauthHelper fosite.OAuth2Provider, generateState func() (state.State, error), generatePKCE func() (pkce.Code, error), generateNonce func() (nonce.Nonce, error), ) http.Handler { - oauthConfig := &compose.Config{ - EnforcePKCEForPublicClients: true, - } - secret := []byte("some-cool-secret-that-is-32bytes") // TODO use a real secret once we care about real authorization codes - oauthHelper := compose.Compose( - // Empty Config for right now since we aren't using anything in it. We may want to inject this - // in the future since it has some really nice configuration knobs like token lifetime. - oauthConfig, - - // This is the thing that matters right now - the store is used to get information about the - // client in the authorization request. - oauthStore, - - // Shouldn't need any of this filled in as of right now - we aren't doing auth code stuff, - // issuing ID tokens, or signing anything yet. - &compose.CommonStrategy{ - CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, secret, nil), - }, - - // hasher, shouldn't need this right now - we aren't doing any client auth...yet? - nil, - - // We will _probably_ want the below handlers somewhere in the code, but I'm not sure where yet, - // and we don't need them for the tests to pass currently, so they are commented out. - compose.OAuth2AuthorizeExplicitFactory, - // compose.OpenIDConnectExplicitFactory, - compose.OAuth2PKCEFactory, - ) return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error { if r.Method != http.MethodPost && r.Method != http.MethodGet { // https://openid.net/specs/openid-connect-core-1_0.html#AuthRequest @@ -69,21 +42,7 @@ func NewHandler( return httperr.Newf(http.StatusMethodNotAllowed, "%s (try GET or POST)", r.Method) } - authorizeRequester, err := oauthHelper.NewAuthorizeRequest( - r.Context(), // TODO: maybe another context here since this one will expire? - r, - ) - if err != nil { - oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) - return nil - } - - // Perform validations - _, err = oauthHelper.NewAuthorizeResponse( - r.Context(), // TODO: maybe another context here since this one will expire? - authorizeRequester, - &openid.DefaultSession{}, - ) + authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r) if err != nil { oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) return nil @@ -94,6 +53,12 @@ func NewHandler( return err } + _, err = oauthHelper.NewAuthorizeResponse(r.Context(), authorizeRequester, &openid.DefaultSession{}) + if err != nil { + oauthHelper.WriteAuthorizeError(w, authorizeRequester, err) + return nil + } + stateValue, nonceValue, pkceValue, err := generateParams(generateState, generateNonce, generatePKCE) if err != nil { return err diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index 11b2a6f8c..1da217a96 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/internal/oidc/auth/auth_handler_test.go @@ -18,6 +18,7 @@ import ( "github.com/stretchr/testify/require" "go.pinniped.dev/internal/here" + "go.pinniped.dev/internal/oidc" "go.pinniped.dev/internal/oidc/provider" "go.pinniped.dev/internal/oidcclient/nonce" "go.pinniped.dev/internal/oidcclient/pkce" @@ -112,22 +113,15 @@ func TestAuthorizationEndpoint(t *testing.T) { issuer := "https://my-issuer.com/some-path" + // Configure fosite the same way that the production code would, except use in-memory storage. oauthStore := &storage.MemoryStore{ - Clients: map[string]fosite.Client{ - "pinniped-cli": &fosite.DefaultOpenIDConnectClient{ - DefaultClient: &fosite.DefaultClient{ - ID: "pinniped-cli", - Public: true, - RedirectURIs: []string{downstreamRedirectURI}, - ResponseTypes: []string{"code"}, - GrantTypes: []string{"authorization_code"}, - Scopes: []string{"openid", "profile", "email"}, - }, - }, - }, + Clients: map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()}, AuthorizeCodes: map[string]storage.StoreAuthorizeCode{}, PKCES: map[string]fosite.Requester{}, } + hmacSecret := []byte("some secret - must have at least 32 bytes") + require.GreaterOrEqual(t, len(hmacSecret), 32, "fosite requires that hmac secrets have at least 32 bytes") + oauthHelper := oidc.FositeOauth2Helper(oauthStore, hmacSecret) happyStateGenerator := func() (state.State, error) { return "test-state", nil } happyPKCEGenerator := func() (pkce.Code, error) { return "test-pkce", nil } @@ -291,6 +285,25 @@ func TestAuthorizationEndpoint(t *testing.T) { wantContentType: "application/json; charset=utf-8", wantBodyJSON: fositeInvalidRedirectURIErrorBody, }, + { + name: "downstream redirect uri matches what is configured for client except for the port number", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: modifiedHappyGetRequestPath(map[string]string{ + "redirect_uri": "http://127.0.0.1:42/callback", + }), + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(happyGetRequestExpectedRedirectLocation), + "\n\n", + ), + wantLocationHeader: happyGetRequestExpectedRedirectLocation, + }, { name: "response type is unsupported", issuer: issuer, @@ -534,7 +547,7 @@ func TestAuthorizationEndpoint(t *testing.T) { for _, test := range tests { test := test t.Run(test.name, func(t *testing.T) { - subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) }) } @@ -543,7 +556,7 @@ func TestAuthorizationEndpoint(t *testing.T) { test := tests[0] require.Equal(t, "happy path using GET", test.name) // re-use the happy path test case - subject := NewHandler(test.issuer, test.idpListGetter, oauthStore, test.generateState, test.generatePKCE, test.generateNonce) + subject := NewHandler(test.issuer, test.idpListGetter, oauthHelper, test.generateState, test.generatePKCE, test.generateNonce) runOneTestCase(t, test, subject) diff --git a/internal/oidc/oidc.go b/internal/oidc/oidc.go index d78f199c6..2fd61d3d1 100644 --- a/internal/oidc/oidc.go +++ b/internal/oidc/oidc.go @@ -4,9 +4,48 @@ // Package oidc contains common OIDC functionality needed by Pinniped. package oidc +import ( + "github.com/ory/fosite" + "github.com/ory/fosite/compose" +) + const ( WellKnownEndpointPath = "/.well-known/openid-configuration" AuthorizationEndpointPath = "/oauth2/authorize" TokenEndpointPath = "/oauth2/token" //nolint:gosec // ignore lint warning that this is a credential JWKSEndpointPath = "/jwks.json" ) + +func PinnipedCLIOIDCClient() *fosite.DefaultOpenIDConnectClient { + return &fosite.DefaultOpenIDConnectClient{ + DefaultClient: &fosite.DefaultClient{ + ID: "pinniped-cli", + Public: true, + RedirectURIs: []string{"http://127.0.0.1/callback"}, + ResponseTypes: []string{"code"}, + GrantTypes: []string{"authorization_code"}, + Scopes: []string{"openid", "profile", "email"}, + }, + } +} + +// Note that Fosite requires the HMAC secret to be 32 bytes. +func FositeOauth2Helper(oauthStore interface{}, hmacSecretOfLength32 []byte) fosite.OAuth2Provider { + oauthConfig := &compose.Config{ + EnforcePKCEForPublicClients: true, + } + + return compose.Compose( + oauthConfig, + oauthStore, + &compose.CommonStrategy{ + CoreStrategy: compose.NewOAuth2HMACStrategy(oauthConfig, hmacSecretOfLength32, nil), + }, + nil, // hasher, defaults to using BCrypt when nil. Used for hashing client secrets. + compose.OAuth2AuthorizeExplicitFactory, + // compose.OAuth2RefreshTokenGrantFactory, + // compose.OpenIDConnectExplicitFactory, + // compose.OpenIDConnectRefreshFactory, + compose.OAuth2PKCEFactory, + ) +}