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 <richardry@vmware.com>

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(
+		authorizeRequester, err := oauthHelper.NewAuthorizeRequest(r.Context(), r)
-			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{},
-		)
 		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

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{
+		Clients:        map[string]fosite.Client{oidc.PinnipedCLIOIDCClient().ID: oidc.PinnipedCLIOIDCClient()},
-			"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"},
-				},
-			},
-		},
 		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(`<a href="%s">Found</a>.%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)
 

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,
+	)
+}