From 2564d1be42f166419a09e5e0ae0dd66f261c8524 Mon Sep 17 00:00:00 2001 From: Andrew Keesler Date: Wed, 4 Nov 2020 12:19:07 -0800 Subject: [PATCH] Supervisor authorize endpoint errors when missing PKCE params Signed-off-by: Ryan Richard --- internal/oidc/auth/auth_handler.go | 27 +++++- internal/oidc/auth/auth_handler_test.go | 115 ++++++++++++++++++------ 2 files changed, 110 insertions(+), 32 deletions(-) diff --git a/internal/oidc/auth/auth_handler.go b/internal/oidc/auth/auth_handler.go index d46384af..8142c25c 100644 --- a/internal/oidc/auth/auth_handler.go +++ b/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 diff --git a/internal/oidc/auth/auth_handler_test.go b/internal/oidc/auth/auth_handler_test.go index d4bee483..d8454137 100644 --- a/internal/oidc/auth/auth_handler_test.go +++ b/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{} @@ -135,12 +152,14 @@ func TestAuthorizationEndpoint(t *testing.T) { } happyGetRequestQueryMap := map[string]string{ - "response_type": "code", - "scope": "openid profile email", - "client_id": "pinniped-cli", - "state": "some-state-value", - "nonce": "some-nonce-value", - "redirect_uri": downstreamRedirectURI, + "response_type": "code", + "scope": "openid profile email", + "client_id": "pinniped-cli", + "state": "some-state-value", + "nonce": "some-nonce-value", + "code_challenge": "some-challenge", + "code_challenge_method": "S256", + "redirect_uri": downstreamRedirectURI, } happyGetRequestPath := pathWithQuery("/some/path", happyGetRequestQueryMap) @@ -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", }, @@ -197,17 +216,20 @@ func TestAuthorizationEndpoint(t *testing.T) { tests := []testCase{ { - name: "happy path using GET", - issuer: issuer, - idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), - generateState: happyStateGenerator, - generatePKCE: happyPKCEGenerator, - generateNonce: happyNonceGenerator, - method: http.MethodGet, - path: happyGetRequestPath, - wantStatus: http.StatusFound, - wantContentType: "text/html; charset=utf-8", - wantBodyString: "", + name: "happy path using GET", + issuer: issuer, + idpListGetter: newIDPListGetter(upstreamOIDCIdentityProvider), + generateState: happyStateGenerator, + generatePKCE: happyPKCEGenerator, + generateNonce: happyNonceGenerator, + method: http.MethodGet, + path: happyGetRequestPath, + wantStatus: http.StatusFound, + wantContentType: "text/html; charset=utf-8", + wantBodyString: fmt.Sprintf(`Found.%s`, + html.EscapeString(happyGetRequestExpectedRedirectLocation), + "\n\n", + ), wantLocationHeader: happyGetRequestExpectedRedirectLocation, }, { @@ -221,11 +243,13 @@ func TestAuthorizationEndpoint(t *testing.T) { path: "/some/path", contentType: "application/x-www-form-urlencoded", body: url.Values{ - "response_type": []string{"code"}, - "scope": []string{"openid profile email"}, - "client_id": []string{"pinniped-cli"}, - "state": []string{"some-state-value"}, - "redirect_uri": []string{downstreamRedirectURI}, + "response_type": []string{"code"}, + "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, wantContentType: "", @@ -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(`Found.%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