PKCE implementation (#1784)

* Basic implementation of PKCE Signed-off-by: Tadeusz Magura-Witkowski <tadeuszmw@gmail.com> * @mfmarche on 24 Feb: when code_verifier is set, don't check client_secret In PKCE flow, no client_secret is used, so the check for a valid client_secret would always fail. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * @deric on 16 Jun: return invalid_grant when wrong code_verifier Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Enforce PKCE flow on /token when PKCE flow was started on /auth Also dissallow PKCE on /token, when PKCE flow was not started on /auth Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * fixed error messages when mixed PKCE/no PKCE flow. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * server_test.go: Added PKCE error cases on /token endpoint * Added test for invalid_grant, when wrong code_verifier is sent * Added test for mixed PKCE / no PKCE auth flows. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * cleanup: extracted method checkErrorResponse and type TestDefinition * fixed connector being overwritten Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * /token endpoint: skip client_secret verification only for grand type authorization_code with PKCE extension Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Allow "Authorization" header in CORS handlers * Adds "Authorization" to the default CORS headers{"Accept", "Accept-Language", "Content-Language", "Origin"} Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Add "code_challenge_methods_supported" to discovery endpoint discovery endpoint /dex/.well-known/openid-configuration now has the following entry: "code_challenge_methods_supported": [ "S256", "plain" ] Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Updated tests (mixed-up comments), added a PKCE test * @asoorm added test that checks if downgrade to "plain" on /token endpoint Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * remove redefinition of providedCodeVerifier, fixed spelling (#6) Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> Signed-off-by: Bernd Eckstein <HEllRZA@users.noreply.github.com> * Rename struct CodeChallenge to PKCE Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * PKCE: Check clientSecret when available In authorization_code flow with PKCE, allow empty client_secret on /auth and /token endpoints. But check the client_secret when it is given. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Enable PKCE with public: true dex configuration public on staticClients now enables the following behavior in PKCE: - Public: false, PKCE will always check client_secret. This means PKCE in it's natural form is disabled. - Public: true, PKCE is enabled. It will only check client_secret if the client has sent one. But it allows the code flow if the client didn't sent one. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Redirect error on unsupported code_challenge_method - Check for unsupported code_challenge_method after redirect uri is validated, and use newErr() to return the error. - Add PKCE tests to oauth2_test.go Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Reverted go.mod and go.sum to the state of master Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Don't omit client secret check for PKCE Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Allow public clients (e.g. with PKCE) to have redirect URIs configured Signed-off-by: Martin Heide <martin.heide@faro.com> * Remove "Authorization" as Accepted Headers on CORS, small fixes Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Revert "Allow public clients (e.g. with PKCE) to have redirect URIs configured" This reverts commit b6e297b78537dc44cd3e1374f0b4d34bf89404ac. Signed-off-by: Martin Heide <martin.heide@faro.com> * PKCE on client_secret client error message * When connecting to the token endpoint with PKCE without client_secret, but the client is configured with a client_secret, generate a special error message. Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * Output info message when PKCE without client_secret used on confidential client * removes the special error message Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> * General missing/invalid client_secret message on token endpoint Signed-off-by: Bernd Eckstein <Bernd.Eckstein@faro.com> Co-authored-by: Tadeusz Magura-Witkowski <tadeuszmw@gmail.com> Co-authored-by: Martin Heide <martin.heide@faro.com> Co-authored-by: M. Heide <66078329+heidemn-faro@users.noreply.github.com>
2020-10-26 11:33:40 +01:00
parent 2a282860fa
commit b5519695a6
10 changed files with 439 additions and 53 deletions
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -2,6 +2,8 @@ package server

 import (
 	"context"
+	"crypto/sha256"
+	"encoding/base64"
 	"encoding/json"
 	"errors"
 	"fmt"
@@ -23,6 +25,11 @@ import (
 	"github.com/dexidp/dex/storage"
 )

+const (
+	CodeChallengeMethodPlain = "plain"
+	CodeChallengeMethodS256  = "S256"
+)
+
 // newHealthChecker returns the healthz handler. The handler runs until the
 // provided context is canceled.
 func (s *Server) newHealthChecker(ctx context.Context) http.Handler {
@@ -148,34 +155,36 @@ func (s *Server) handlePublicKeys(w http.ResponseWriter, r *http.Request) {
 }

 type discovery struct {
-	Issuer         string   `json:"issuer"`
-	Auth           string   `json:"authorization_endpoint"`
-	Token          string   `json:"token_endpoint"`
-	Keys           string   `json:"jwks_uri"`
-	UserInfo       string   `json:"userinfo_endpoint"`
-	DeviceEndpoint string   `json:"device_authorization_endpoint"`
-	GrantTypes     []string `json:"grant_types_supported"`
-	ResponseTypes  []string `json:"response_types_supported"`
-	Subjects       []string `json:"subject_types_supported"`
-	IDTokenAlgs    []string `json:"id_token_signing_alg_values_supported"`
-	Scopes         []string `json:"scopes_supported"`
-	AuthMethods    []string `json:"token_endpoint_auth_methods_supported"`
-	Claims         []string `json:"claims_supported"`
+	Issuer            string   `json:"issuer"`
+	Auth              string   `json:"authorization_endpoint"`
+	Token             string   `json:"token_endpoint"`
+	Keys              string   `json:"jwks_uri"`
+	UserInfo          string   `json:"userinfo_endpoint"`
+	DeviceEndpoint    string   `json:"device_authorization_endpoint"`
+	GrantTypes        []string `json:"grant_types_supported"`
+	ResponseTypes     []string `json:"response_types_supported"`
+	Subjects          []string `json:"subject_types_supported"`
+	IDTokenAlgs       []string `json:"id_token_signing_alg_values_supported"`
+	CodeChallengeAlgs []string `json:"code_challenge_methods_supported"`
+	Scopes            []string `json:"scopes_supported"`
+	AuthMethods       []string `json:"token_endpoint_auth_methods_supported"`
+	Claims            []string `json:"claims_supported"`
 }

 func (s *Server) discoveryHandler() (http.HandlerFunc, error) {
 	d := discovery{
-		Issuer:         s.issuerURL.String(),
-		Auth:           s.absURL("/auth"),
-		Token:          s.absURL("/token"),
-		Keys:           s.absURL("/keys"),
-		UserInfo:       s.absURL("/userinfo"),
-		DeviceEndpoint: s.absURL("/device/code"),
-		Subjects:       []string{"public"},
-		GrantTypes:     []string{grantTypeAuthorizationCode, grantTypeRefreshToken, grantTypeDeviceCode},
-		IDTokenAlgs:    []string{string(jose.RS256)},
-		Scopes:         []string{"openid", "email", "groups", "profile", "offline_access"},
-		AuthMethods:    []string{"client_secret_basic"},
+		Issuer:            s.issuerURL.String(),
+		Auth:              s.absURL("/auth"),
+		Token:             s.absURL("/token"),
+		Keys:              s.absURL("/keys"),
+		UserInfo:          s.absURL("/userinfo"),
+		DeviceEndpoint:    s.absURL("/device/code"),
+		Subjects:          []string{"public"},
+		GrantTypes:        []string{grantTypeAuthorizationCode, grantTypeRefreshToken, grantTypeDeviceCode},
+		IDTokenAlgs:       []string{string(jose.RS256)},
+		CodeChallengeAlgs: []string{CodeChallengeMethodS256, CodeChallengeMethodPlain},
+		Scopes:            []string{"openid", "email", "groups", "profile", "offline_access"},
+		AuthMethods:       []string{"client_secret_basic"},
 		Claims: []string{
 			"aud", "email", "email_verified", "exp",
 			"iat", "iss", "locale", "name", "sub",
@@ -643,6 +652,7 @@ func (s *Server) sendCodeResponse(w http.ResponseWriter, r *http.Request, authRe
 				Expiry:        s.now().Add(time.Minute * 30),
 				RedirectURI:   authReq.RedirectURI,
 				ConnectorData: authReq.ConnectorData,
+				PKCE:          authReq.PKCE,
 			}
 			if err := s.storage.CreateAuthCode(code); err != nil {
 				s.logger.Errorf("Failed to create auth code: %v", err)
@@ -756,6 +766,11 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) {
 		return
 	}
 	if client.Secret != clientSecret {
+		if clientSecret == "" {
+			s.logger.Infof("missing client_secret on token request for client: %s", client.ID)
+		} else {
+			s.logger.Infof("invalid client_secret on token request for client: %s", client.ID)
+		}
 		s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized)
 		return
 	}
@@ -773,6 +788,18 @@ func (s *Server) handleToken(w http.ResponseWriter, r *http.Request) {
 	}
 }

+func (s *Server) calculateCodeChallenge(codeVerifier, codeChallengeMethod string) (string, error) {
+	switch codeChallengeMethod {
+	case CodeChallengeMethodPlain:
+		return codeVerifier, nil
+	case CodeChallengeMethodS256:
+		shaSum := sha256.Sum256([]byte(codeVerifier))
+		return base64.RawURLEncoding.EncodeToString(shaSum[:]), nil
+	default:
+		return "", fmt.Errorf("unknown challenge method (%v)", codeChallengeMethod)
+	}
+}
+
 // handle an access token request https://tools.ietf.org/html/rfc6749#section-4.1.3
 func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client storage.Client) {
 	code := r.PostFormValue("code")
@@ -789,6 +816,31 @@ func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client s
 		return
 	}

+	// RFC 7636 (PKCE)
+	codeChallengeFromStorage := authCode.PKCE.CodeChallenge
+	providedCodeVerifier := r.PostFormValue("code_verifier")
+
+	if providedCodeVerifier != "" && codeChallengeFromStorage != "" {
+		calculatedCodeChallenge, err := s.calculateCodeChallenge(providedCodeVerifier, authCode.PKCE.CodeChallengeMethod)
+		if err != nil {
+			s.logger.Error(err)
+			s.tokenErrHelper(w, errServerError, "", http.StatusInternalServerError)
+			return
+		}
+		if codeChallengeFromStorage != calculatedCodeChallenge {
+			s.tokenErrHelper(w, errInvalidGrant, "Invalid code_verifier.", http.StatusBadRequest)
+			return
+		}
+	} else if providedCodeVerifier != "" {
+		// Received no code_challenge on /auth, but a code_verifier on /token
+		s.tokenErrHelper(w, errInvalidRequest, "No PKCE flow started. Cannot check code_verifier.", http.StatusBadRequest)
+		return
+	} else if codeChallengeFromStorage != "" {
+		// Received PKCE request on /auth, but no code_verifier on /token
+		s.tokenErrHelper(w, errInvalidGrant, "Expecting parameter code_verifier in PKCE flow.", http.StatusBadRequest)
+		return
+	}
+
 	if authCode.RedirectURI != redirectURI {
 		s.tokenErrHelper(w, errInvalidRequest, "redirect_uri did not match URI from initial request.", http.StatusBadRequest)
 		return