PKCE implementation

[HEllRZA]

This pull request adds the PKCE (Proof Key for Code Exchange) implementation. (RFC 7636), Closes #1114.

Based on the implementation of @Teeed (PR #1652)

• add the remarks of @deric (return invalid_grand when wrong code_verifier)
• fix the issue, @mfmarche found.
• Enforce PKCE flow on /token when PKCE flow was started on /auth
• made sure, the correct errors are returned
• add Tests for the error cases
• make sure, the client_secret is only omitted, when code_verifier is received on /token, and grant_type is correct.
• add "Authorization" to allowed CORS headers

I am lookin forward to your review.

Test PKCE

You should use a public client (without secret), so PKCE is not stopped by a missing client_secret

staticClients:
- id: example-app
  name: 'Example App'
  public: true

Note

• Configuring redirectURIs does currently not work with public clients (see Allow public clients (e.g. SPAs using implicit flow or PKCE) to have redirect URLs other than localhost #1822) but public clients allow all localhost callback URIs (e.g. http://localhost:5556/dex/callback)
• When testing with oidc-client.js if you use loadUserInfo: true, it will not work until fix: allow Authorization header when doing CORS #1819 is merged.

Thanks to @Teeed and i hope this PR is of interest to @mfmarche, @deric, @justin-slowik

[Teeed]

Hi,

good job! 👍

[HEllRZA]

@sagikazarmark could you take a look at it? That would be great!

[asoorm]

Is it necessary to support the plain code challenge method?

https://tools.ietf.org/html/rfc7636#section-7.2

Because of this, "plain" SHOULD NOT be used and exists only for
compatibility with deployed implementations where the request path is
already protected. The "plain" method SHOULD NOT be used in new
implementations, unless they cannot support "S256" for some technical
reason.

The "S256" code challenge method or other cryptographically secure
code challenge method extension SHOULD be used. The "plain" code
challenge method relies on the operating system and transport
security not to disclose the request to an attacker.

https://tools.ietf.org/html/draft-ietf-oauth-v2-1-00#section-4.1.1.2

The plain transformation is for compatibility with existing
deployments and for constrained environments that can't use the
"S256" transformation.

[HEllRZA]

@asoorm
Thanks for your input! I also dislike "plain".

Reading the RFC, it sounds to me that the server has to support the code_challenge_method "plain". It is even the default, when no code_challenge_method is specified. The passages you quote discourage the use of "plain" in the client that starts the PKCE request.

Client Sends the Code Challenge with the Authorization Request

The client sends the code challenge as part of the OAuth 2.0
Authorization Request (Section 4.1.1 of [RFC6749]) using the
following additional parameters:

code_challenge
REQUIRED. Code challenge.

code_challenge_method
OPTIONAL, defaults to "plain" if not present in the request. Code
verifier transformation method is "S256" or "plain".

However the client should not use it. It should use "S256" and never downgrade to "plain". "S256" is required on the server.

Clients MUST NOT downgrade to "plain" after trying the "S256" method.
Servers that support PKCE are required to support "S256", and servers
that do not support PKCE will simply ignore the unknown
"code_verifier".

Other Auth Providers also support both (e.g. auth0).
Attacks should be prevented by disallowing to use "plain" on the /token endpoint, when "S256" was used as code_challenge_method on the /auth endpoint to start the flow. I will make sure that i have a test for that!

[tkleczek]

Would be nice to also add some tests to oauth2_test.go

[heidemn-faro]

@bonifaido @sagikazarmark it seems that all requested changes have been implemented in 46c6d9d .
Anything else that needs to be addressed before this can be merged?

[trentis]

I could be doing something wrong but I just tried using this PR with a oidc-client-js implementation. The handleToken method would still always respond with unauthorized at the checking of client secret step. Any ideas why this would be happening?

Is the if statement even required if the handleAuthCode does the verifiying step.

[HEllRZA]

@trentis It was decided to still use the client_secret with PKCE.

So there are two ways to solve this issue for you:

1. Use a client secret with oidc-client-js (that's probably not what you want to do)
2. Configure your dex-client as public client with no client secret.

staticClients:
- id: example-app
  name: 'Example App'
  public: true

Note that configuring redirectURIs does currently not work with public clients (see #1822) but public clients allow all localhost callback URIs (e.g. http://localhost:5556/dex/callback) (IIRC 127.0.0.1 does not work!)

Actually i was thinking about implementing a different error message in case of PKCE used with empty client_secret on the /token endpoint, as i was aware of that pitfall. Something like:
"Missing client_secret. Please configure your dex client as "public" if you want to use PKCE without client_secret"

[HEllRZA]

@trentis Another pitfall:
If you use loadUserInfo: true in oidc-client-js, it will not work until #1819 is merged.

[trentis]

Cheers for the assistance @HEllRZA . Yeah an error might be good took me a while to realize it wasn't oidc-client-js as they do the code challenge automatically. So it particularly could help others. I'll probably end up using a public client and merge in the other PRs to get what I need. Hopefully it gets merged soon

[tkleczek]

Seems inconsistent that we want to specifically call out "missing credentials" in PKCE case and not in general case.
Also I'm wondering whether not specifying client authentication error details but just general "Invalid client credentials." is for security reasons. E.g. the error you introduced would allow to determine whether the client having a specific client_id is registered on the server.

[HEllRZA]

I understand. Should we instead just log it?

[tkleczek]

I would say it's a good idea, many such log entries could indicate that some attack is in place. We could also log when client_id is invalid. Not sure if we would like to specifically call-out "missing credentials" case, but I'm not against it. And let's do it regardless of PKCE.

[HEllRZA]

Ok, Now, there is a log output instead.

[tkleczek]

I was thinking more in lines of:

           if client.Secret != clientSecret {
	        if clientSecret != "" {
	            s.logger.Info("missing client secret")
	        } else {
	            s.logger.Info("invalid client secret")
	        }
	        
		s.tokenErrHelper(w, errInvalidClient, "Invalid client credentials.", http.StatusUnauthorized)					 
                return

[HEllRZA]

Written text has so much potential for misunderstanding ;o)

[sagikazarmark]

@tkleczek @HEllRZA is this PR ready for another look?

[tkleczek]

@sagikazarmark Yes, I believe so.

[HEllRZA]

@tkleczek @sagikazarmark
Yes, it is ripe.

[johanbrandhorst]

Shouldn't this case be OK for PKCE? The whole idea is that the client doesn't have to provide the client secret. The role of the IdP in this case is just to verify Code verifier and Code challenge, which happens in handleAuthCode.

Ref: https://auth0.com/docs/flows/authorization-code-flow-with-proof-key-for-code-exchange-pkce

[johanbrandhorst]

I tried rewriting this locally to move the clientSecret validation into handleAuthCode, handleRefreshToken and handlePasswordGrant respectively and it works. I think we have to remove it from this function for PKCE to work.

[johanbrandhorst]

Here's a rought sketch of the changes I made, though I'd suggest extracting the client secret validation into its own function instead:

diff --git a/server/handlers.go b/server/handlers.go
index 342849ee..1141dffc 100644
--- a/server/handlers.go
+++ b/server/handlers.go
@@ -765,24 +765,15 @@ 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
-	}
 
 	grantType := r.PostFormValue("grant_type")
 	switch grantType {
 	case grantTypeAuthorizationCode:
-		s.handleAuthCode(w, r, client)
+		s.handleAuthCode(w, r, client, clientSecret)
 	case grantTypeRefreshToken:
-		s.handleRefreshToken(w, r, client)
+		s.handleRefreshToken(w, r, client, clientSecret)
 	case grantTypePassword:
-		s.handlePasswordGrant(w, r, client)
+		s.handlePasswordGrant(w, r, client, clientSecret)
 	default:
 		s.tokenErrHelper(w, errInvalidGrant, "", http.StatusBadRequest)
 	}
@@ -801,7 +792,7 @@ func (s *Server) calculateCodeChallenge(codeVerifier, codeChallengeMethod string
 }
 
 // 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) {
+func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client storage.Client, clientSecret string) {
 	code := r.PostFormValue("code")
 	redirectURI := r.PostFormValue("redirect_uri")
 
@@ -839,6 +830,16 @@ func (s *Server) handleAuthCode(w http.ResponseWriter, r *http.Request, client s
 		// 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
+	} else {
+		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
+		}
 	}
 
 	if authCode.RedirectURI != redirectURI {
@@ -1000,7 +1001,17 @@ func (s *Server) exchangeAuthCode(w http.ResponseWriter, authCode storage.AuthCo
 }
 
 // handle a refresh token request https://tools.ietf.org/html/rfc6749#section-6
-func (s *Server) handleRefreshToken(w http.ResponseWriter, r *http.Request, client storage.Client) {
+func (s *Server) handleRefreshToken(w http.ResponseWriter, r *http.Request, client storage.Client, clientSecret string) {
+	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
+	}
+
 	code := r.PostFormValue("refresh_token")
 	scope := r.PostFormValue("scope")
 	if code == "" {
@@ -1227,7 +1238,17 @@ func (s *Server) handleUserInfo(w http.ResponseWriter, r *http.Request) {
 	w.Write(claims)
 }
 
-func (s *Server) handlePasswordGrant(w http.ResponseWriter, r *http.Request, client storage.Client) {
+func (s *Server) handlePasswordGrant(w http.ResponseWriter, r *http.Request, client storage.Client, clientSecret string) {
+	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
+	}
+
 	// Parse the fields
 	if err := r.ParseForm(); err != nil {
 		s.tokenErrHelper(w, errInvalidRequest, "Couldn't parse data", http.StatusBadRequest)

[HEllRZA]

@johanbrandhorst

I initially implemented it in a similar way by skipping the client_secret validation, if the request was PKCE.

We decided that, for that case of using PKCE, the client should be configured as public and have no client_secret. And we should not omit the client_secret if the client is confidential. In fact the current implementation checks the client secret either way. Public client just allows an empty secret.

Quote from @tkleczek:

I agree that PKCE was introduced to mitigate security concerns for public clients, but my reasoning is:
If client is configured as public, then the check should pass as both clientSecret and client.Secret should be empty.
But if for whatever reason sb configures confidential client with PKCE, why not validate client secret as well.

[johanbrandhorst]

That makes sense, thanks!

[sagikazarmark]

LGTM, thanks @HEllRZA and @tkleczek for your work! ❤️

[bonifaido]

LGTM, thank you very much!