Supervisor authorize endpoint errors when missing PKCE params

Signed-off-by: Ryan Richard <richardry@vmware.com>

internal/oidc/auth/auth_handler.go

@@ -8,6 +8,8 @@ import (
 	"fmt"
 	"net/http"
 
+	"github.com/ory/fosite/handler/openid"
+
 	"github.com/ory/fosite/compose"
 	"golang.org/x/oauth2"
 	"k8s.io/klog/v2"
@@ -31,10 +33,14 @@ func NewHandler(
 	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.
-		&compose.Config{},
+		oauthConfig,
 
 		// This is the thing that matters right now - the store is used to get information about the
 		// client in the authorization request.
@@ -42,16 +48,18 @@ func NewHandler(
 
 		// 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{},
+		&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.OAuth2AuthorizeExplicitFactory,
 		// compose.OpenIDConnectExplicitFactory,
-		// compose.OAuth2PKCEFactory,
+		compose.OAuth2PKCEFactory,
 	)
 	return httperr.HandlerFunc(func(w http.ResponseWriter, r *http.Request) error {
 		if r.Method != http.MethodPost && r.Method != http.MethodGet {
@@ -70,6 +78,17 @@ func NewHandler(
 			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
+		}
+
 		upstreamIDP, err := chooseUpstreamIDP(idpListGetter)
 		if err != nil {
 			return err

internal/oidc/auth/auth_handler_test.go

@@ -5,6 +5,7 @@ package auth
 
 import (
 	"fmt"
+	"html"
 	"mime"
 	"net/http"
 	"net/http/httptest"
@@ -49,6 +50,20 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			}
 		`)
 
+		fositeMissingCodeChallengeErrorQuery = map[string]string{
+			"error":             "invalid_request",
+			"error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nThis client must include a code_challenge when performing the authorize code flow, but it is missing.",
+			"error_hint":        "This client must include a code_challenge when performing the authorize code flow, but it is missing.",
+			"state":             "some-state-value",
+		}
+
+		fositeMissingCodeChallengeMethodErrorQuery = map[string]string{
+			"error":             "invalid_request",
+			"error_description": "The request is missing a required parameter, includes an invalid parameter value, includes a parameter more than once, or is otherwise malformed\n\nClients must use code_challenge_method=S256, plain is not allowed.",
+			"error_hint":        "Clients must use code_challenge_method=S256, plain is not allowed.",
+			"state":             "some-state-value",
+		}
+
 		fositeUnsupportedResponseTypeErrorQuery = map[string]string{
 			"error":             "unsupported_response_type",
 			"error_description": "The authorization server does not support obtaining a token using this method\n\nThe client is not allowed to request response_type \"unsupported\".",
@@ -103,6 +118,8 @@ func TestAuthorizationEndpoint(t *testing.T) {
 				},
 			},
 		},
+		AuthorizeCodes: map[string]storage.StoreAuthorizeCode{},
+		PKCES:          map[string]fosite.Requester{},
 	}
 
 	happyStateGenerator := func() (state.State, error) { return "test-state", nil }
@@ -111,7 +128,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 
 	// This is the PKCE challenge which is calculated as base64(sha256("test-pkce")). For example:
 	// $ echo -n test-pkce | shasum -a 256 | cut -d" " -f1 | xxd -r -p | base64 | cut -d"=" -f1
-	expectedCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"
+	expectedUpstreamCodeChallenge := "VVaezYqum7reIhoavCHD1n2d-piN3r_mywoYj7fCR7g"
 
 	pathWithQuery := func(path string, query map[string]string) string {
 		values := url.Values{}
@@ -140,6 +157,8 @@ func TestAuthorizationEndpoint(t *testing.T) {
 		"client_id":             "pinniped-cli",
 		"state":                 "some-state-value",
 		"nonce":                 "some-nonce-value",
+		"code_challenge":        "some-challenge",
+		"code_challenge_method": "S256",
 		"redirect_uri":          downstreamRedirectURI,
 	}
 
@@ -169,7 +188,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			"client_id":             "some-client-id",
 			"state":                 "test-state",
 			"nonce":                 "test-nonce",
-			"code_challenge":        expectedCodeChallenge,
+			"code_challenge":        expectedUpstreamCodeChallenge,
 			"code_challenge_method": "S256",
 			"redirect_uri":          issuer + "/callback/some-idp",
 		},
@@ -207,7 +226,10 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			path:            happyGetRequestPath,
 			wantStatus:      http.StatusFound,
 			wantContentType: "text/html; charset=utf-8",
-			wantBodyString:     "",
+			wantBodyString: fmt.Sprintf(`<a href="%s">Found</a>.%s`,
+				html.EscapeString(happyGetRequestExpectedRedirectLocation),
+				"\n\n",
+			),
 			wantLocationHeader: happyGetRequestExpectedRedirectLocation,
 		},
 		{
@@ -225,6 +247,8 @@ func TestAuthorizationEndpoint(t *testing.T) {
 				"scope":                 []string{"openid profile email"},
 				"client_id":             []string{"pinniped-cli"},
 				"state":                 []string{"some-state-value"},
+				"code_challenge":        []string{"some-challenge"},
+				"code_challenge_method": []string{"S256"},
 				"redirect_uri":          []string{downstreamRedirectURI},
 			}.Encode(),
 			wantStatus:         http.StatusFound,
@@ -272,6 +296,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantStatus:         http.StatusFound,
 			wantContentType:    "application/json; charset=utf-8",
 			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeUnsupportedResponseTypeErrorQuery),
+			wantBodyString:     "",
 		},
 		{
 			name:               "downstream scopes do not match what is configured for client",
@@ -285,6 +310,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantStatus:         http.StatusFound,
 			wantContentType:    "application/json; charset=utf-8",
 			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidScopeErrorQuery),
+			wantBodyString:     "",
 		},
 		{
 			name:               "missing response type in request",
@@ -298,6 +324,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantStatus:         http.StatusFound,
 			wantContentType:    "application/json; charset=utf-8",
 			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingResponseTypeErrorQuery),
+			wantBodyString:     "",
 		},
 		{
 			name:            "missing client id in request",
@@ -312,6 +339,34 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantContentType: "application/json; charset=utf-8",
 			wantBodyJSON:    fositeInvalidClientErrorBody,
 		},
+		{
+			name:               "missing PKCE code_challenge in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1
+			issuer:             issuer,
+			idpListGetter:      newIDPListGetter(upstreamOIDCIdentityProvider),
+			generateState:      happyStateGenerator,
+			generatePKCE:       happyPKCEGenerator,
+			generateNonce:      happyNonceGenerator,
+			method:             http.MethodGet,
+			path:               modifiedHappyGetRequestPath(map[string]string{"code_challenge": ""}),
+			wantStatus:         http.StatusFound,
+			wantContentType:    "application/json; charset=utf-8",
+			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeErrorQuery),
+			wantBodyString:     "",
+		},
+		{
+			name:               "missing PKCE code_challenge_method in request", // See https://tools.ietf.org/html/rfc7636#section-4.4.1
+			issuer:             issuer,
+			idpListGetter:      newIDPListGetter(upstreamOIDCIdentityProvider),
+			generateState:      happyStateGenerator,
+			generatePKCE:       happyPKCEGenerator,
+			generateNonce:      happyNonceGenerator,
+			method:             http.MethodGet,
+			path:               modifiedHappyGetRequestPath(map[string]string{"code_challenge_method": ""}),
+			wantStatus:         http.StatusFound,
+			wantContentType:    "application/json; charset=utf-8",
+			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeMissingCodeChallengeMethodErrorQuery),
+			wantBodyString:     "",
+		},
 		{
 			name:               "state does not have enough entropy",
 			issuer:             issuer,
@@ -324,6 +379,7 @@ func TestAuthorizationEndpoint(t *testing.T) {
 			wantStatus:         http.StatusFound,
 			wantContentType:    "application/json; charset=utf-8",
 			wantLocationHeader: urlWithQuery(downstreamRedirectURI, fositeInvalidStateErrorQuery),
+			wantBodyString:     "",
 		},
 		{
 			name:            "error while generating state",
@@ -428,11 +484,10 @@ func TestAuthorizationEndpoint(t *testing.T) {
 		require.Equal(t, test.wantStatus, rsp.Code)
 		requireEqualContentType(t, rsp.Header().Get("Content-Type"), test.wantContentType)
 
-		if test.wantBodyString != "" {
-			require.Equal(t, test.wantBodyString, rsp.Body.String())
-		}
 		if test.wantBodyJSON != "" {
 			require.JSONEq(t, test.wantBodyJSON, rsp.Body.String())
+		} else {
+			require.Equal(t, test.wantBodyString, rsp.Body.String())
 		}
 
 		if test.wantLocationHeader != "" {
@@ -475,11 +530,15 @@ func TestAuthorizationEndpoint(t *testing.T) {
 				"client_id":             "some-other-client-id",
 				"state":                 "test-state",
 				"nonce":                 "test-nonce",
-				"code_challenge":        expectedCodeChallenge,
+				"code_challenge":        expectedUpstreamCodeChallenge,
 				"code_challenge_method": "S256",
 				"redirect_uri":          issuer + "/callback/some-other-idp",
 			},
 		)
+		test.wantBodyString = fmt.Sprintf(`<a href="%s">Found</a>.%s`,
+			html.EscapeString(test.wantLocationHeader),
+			"\n\n",
+		)
 
 		// Run again on the same instance of the subject with the modified upstream IDP settings and the
 		// modified expectations. This should ensure that the implementation is using the in-memory cache