node-oauth / node-oauth2-server

🚀 The successor to oauthjs/oauth2-server. 🔒 Complete, compliant, maintained and well tested OAuth2 Server for node.js. Includes native async await and PKCE.
https://www.npmjs.com/package/@node-oauth/oauth2-server
MIT License
286 stars 39 forks source link

PKCE Refresh token #288

Open kuselan84 opened 4 months ago

kuselan84 commented 4 months ago

Token handler on PCKE flow is not verifying code_verifier and expecting client_secret.

Providing client_secret will defeat PKCE flow.

Please assist.

jankapunkt commented 4 months ago

Hello @kuselan84 thank you for getting in contact. What is exactly the nature of your issue?

kuselan84 commented 4 months ago

Hi @jankapunkt,

I'm trying implement PKCE flow using guide at PKCE Support.

I send auth request as follows:

POST http://localhost:3000/authorize
Content-Type: application/x-www-form-urlencoded

response_type=code&
client_id=myclient&
redirect_uri=http://localhost:3000/client/app&
state=mystate&
scope=offline_access&
code_challenge=p4eKNknklsnn5fQb9A20lfYv36WHZk_K9y8PRCPooNc&
code_challenge_method=S256

I received code to redirect uri successfully. Then I request token as follows:

POST http://localhost:3000/token
Content-Type: application/x-www-form-urlencoded

grant_type=authorization_code&
client_id=myclient&
code=8b51da4fade34a5ccf0d1dc40816812052e2b3ae&
code_verifier=e4fz4xjFKhlQUWXgsME5dp3CugzrYf1BLUwuywRu9gc

I received access token successfully. Then I request access token again using refresh token as follows:

POST http://localhost:3000/token
Content-Type: application/x-www-form-urlencoded

grant_type=refresh_token&
client_id=myclient&
code_verifier=e4fz4xjFKhlQUWXgsME5dp3CugzrYf1BLUwuywRu9gc&
refresh_token=f5360c80cd7fd774d3a165360796a1747d3de012f6608ce6bcb3997876f7c892

It failed with this error message:

{
  "error": "invalid_client",
  "error_description": "Invalid client: cannot retrieve client credentials"
}

The request works if I send client_secret parameter. However it ignore code_verifier verification.

kuselan84 commented 4 months ago

Perhaps it could be fixed so that neither the code_verifier nor the client_secret are required. If the client securely manages the refresh_token, it alone could suffice for obtaining a new access_token. eg.

POST http://localhost:3000/token
Content-Type: application/x-www-form-urlencoded

grant_type=refresh_token&
client_id=myclient&
refresh_token=f5360c80cd7fd774d3a165360796a1747d3de012f6608ce6bcb3997876f7c892
jankapunkt commented 4 months ago

@kuselan84 I read up again the RFC and some resources and there is indeed no specification on the RFC about the refresh token in combination with PKCE. I also think this is why we did not 100% cover this in our implementation so far. This is very unexpected.

Assumptions

Concerns

Perhaps it could be fixed so that neither the code_verifier nor the client_secret are required. If the client securely manages the refresh_token, it alone could suffice for obtaining a new access_token. eg.

I agree with this. However I'm still unsure how If the client securely manages the refresh_token is actually realized in a PKCE scenario.

Is this even possible in a PKCE scenario? From how I see it, the client is considered unsafe. A stack answer is also inconclusive on this.

Summary

I'm generally positive with you as the RFC and literature is inconclusive and if in doubt we should always decide for the flexibility and let developers decide. However we will have to warn over the concerns in the documentation.

I would still not consider this as a bug, though.

kuselan84 commented 4 months ago

Thank you for the reply, @jankapunkt.

I have opted to disable client_secret on token endpoint like this as per documentation:

router.post('/token', oauth.token({requireClientAuthentication:{refresh_token:false}}));

I'm going to implement Refresh token reuse Detection to protect refresh token.

jankapunkt commented 3 months ago

If there is no veto from @jorenvandeweyer @dhensby @shrihari-prakash I'm going to implement this. The impl. also needs to update the tests accordingly.

dhensby commented 3 months ago

disclaimer: I have not familiarised myself with the PKCE spec recently

My understanding PKCE is primarily aimed at clients that are deemed to be unable to securely store sensitive date (hence, there is no client_secret issued to them as part of their credentials). Therefore, I'd be very cautious about even issuing refresh tokens to clients that cannot store secrets securely...

shrihari-prakash commented 3 months ago

Hello @jankapunkt ,

In my opinion, I think it is normal that the code_verifier is ignored in refresh token exchanges. Because if we take a step back and see the purpose of PKCE itself, for instance in a native app setup when the auth code flow is done and the redirect is sent to your app, a malicious application can register itself as a handler for the redirect URI and get the authorization code which can be abused. The primary thing to note is that there are two parties involved here.

But in case of refresh tokens, there is none of the redirect stuff and it is purely handled within the application. I don't see why a code_verifier is required in this case although as @dhensby mentioned, if the client cannot store refresh tokens securely, there is no point to even sending it to client :).

jankapunkt commented 3 months ago

@dhensby @shrihari-prakash thanks for your input. The biggest problem of all this is, that the standard says NOTHING about all this. Real edge-case here.

dhensby commented 3 months ago

So there are two issues at play here.

  1. Should PKCE flows return refresh tokens at all?
  2. If PKCE flow does issue a refresh token, how is that token exchanged properly for a renewed access token?

Should PKCE flows return refresh tokens?

This is actually less a question about PKCE and more about public vs confidential clients. tl;dr: Public clients can't store secrets, confidential clients can. PKCE code challenges can (and are) used by confidential clients as it's recommended in some of the more advanced standards such as FAPI 2.0.

The OAuth specification essentially says, it's up to the risk tolerance of the authorisation server as to whether refresh tokens are issued:

The client type designation is based on the authorization server's definition of secure authentication and its acceptable exposure levels of client credentials. The authorization server SHOULD NOT make assumptions about the client type.

Personally, I would not issue refresh tokens to any public clients, but of course native apps that have access to store refresh tokens in secure enclaves, certainly feels like an acceptable level of risk following a PKCE flow.

I'd note that in section 1.1 of the PKCE extension there is no mention of refresh tokens forming part of the protocol, whereas the OAuth protocol does mention them in the equivalent section section 1.5 of its spec. Certainly not conclusive, but a fairly clear omission.

My view on this is: PKCE flows issuing refresh tokens is up to the server, the server should be storing meta data about the client (ie: whether it is a public or confidential client) and acting on that information as it feels appropriate. NB: If the client has authentication credentials (ie: it has a client_secret) it must provide that even with the PKCE flow.

Answer: Yes, PKCE can (but probably shouldn't for public clients) release refresh tokens.

How is a refresh token obtained through PKCE exchanged?

The specification is annoyingly vague around this. If we start at refreshing an access token it says:

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1.

[Section 3.2.1] says:

Confidential clients or other clients issued client credentials MUST authenticate with the authorization server as described in Section 2.3 when making requests to the token endpoint

Section 2.3 says:

If the client type is confidential, the client and authorization server establish a client authentication method suitable for the security requirements of the authorization server.

So all the authentication rhetoric is around confidential clients...

However it does also sneak in these statements that could apply to PKCE flows:

If the client type is confidential or the client was issued client credentials (or assigned other authentication requirements), the client MUST authenticate with the authorization server as described in Section 3.2.1.

and

The authorization server MAY establish a client authentication method with public clients. However, the authorization server MUST NOT rely on public client authentication for the purpose of identifying the client.

Is the PKCE flow establishing an authentication method with the client, or just a single-use challenge?

I think as with the question of issuing refresh tokens at all, this really comes down to the level of acceptable risk for the authorisation server. On the one hand, I agree with @shrihari-prakash - the code_challenge is used when exchanging the auth_code for an access token (and optional refresh token), as the auth_code is the thing that could be intercepted and the authorisation server wants some assurance that it is communicating with the same client instance that the end-user authorised. Once the tokens have been released to the verified client, the code_challenge has done its job. This is laid out fairly clearly in the PKCE protocol, as it is only talking about associating the challenge with the authorisation code and not any other tokens that may be issued - but the fact that the spec is entirely silent on refresh tokens does make this hard to rely on as definitive.

On the other hand, associating the challenge with the client and for subsequent refreshes does seem sensible if one is going to actually issue refresh tokens and they want some level of assurance that they are refreshing the token with the same client as before. It fits in with the implied spec that the request should include the same authentication that was used in the original token exchange.

Although this is a rather long and meandering post, I think I'm settling that the challenge does not need to be supplied when exchanging the refresh token when using a public client. It is completely academic for a public client to provide the code challenge with a refresh token, if the client is not capable of secure storage, then a refresh token should not be issued. If it is capable of secure storage of the refresh token, then it doesn't need the code challenge to prove itself. A public client without the capability of secure storage wouldn't be able to store the challenge any more securely than the refresh token, so if one can be extracted from the client, then so can the other - providing both provides no real security benefit over just the refresh token.

Lastly, it appears that that Okta (who run Auth0) don't require the challenge when exchanging refresh tokens.


Just to touch on this specific issue as raised by OP:

If the client has a client_secret issued, then it must be providing its secret with every request. It shouldn't be able to use PKCE to circumvent the authentication requirement - if you need a client to do both un-authenticated PKCE and authenticated flows, they should be two separate clients (that is clear in the spec):

A client may be implemented as a distributed set of components, each with a different client type and security context (e.g., a distributed client with both a confidential server-based component and a public browser-based component). If the authorization server does not provide support for such clients or does not provide guidance with regard to their registration, the client SHOULD register each component as a separate client.

@jankapunkt is quite right in his "assumption", that if the client has a client secret, that it must provide that during the refresh exchange.

In regards to resolving this issue, I think we need to add the concept of confidential/public clients to the library because it's not just a case of enable or disable authentication on refresh endpoints, it's a case of appropriately authenticating a client holistically with the entire request context.

jorenvandeweyer commented 3 months ago

To summarise two mechanism are here confused as one.

They should both be implemented independently.

OAuth 2.0 Authorization Code Grant

tools.ietf.org/html/rfc6749#section-1.3.1

The Authorization Code grant type is used by confidential and public clients to exchange an authorization code for an access token. After the user returns to the client via the redirect URL, the application will get the authorization code from the URL and use it to request an access token. It is recommended that all clients use the PKCE extension with this flow as well to provide better security.


PKCE extension

PKCE is not a form of client authentication, and PKCE is not a replacement for a client secret or other client authentication. PKCE is recommended even if a client is using a client secret or other form of client authentication like private_key_jwt.

When the PKCE extension is used in an authentication flow, the server must require the code verifier on the code exchange.

The code verifier should never be used outside this flow (so not in the refresh token flow).

Examples

Confidential Client without PKCE ```ts grant_type=authorization_code& client_id=myclient& client_secret=mysecret& code=8b51da4fade34a5ccf0d1dc40816812052e2b3ae ```
Confidential Client with PKCE ``` grant_type=authorization_code& client_id=myclient& client_secret=mysecret& code=8b51da4fade34a5ccf0d1dc40816812052e2b3ae& code_verifier=e4fz4xjFKhlQUWXgsME5dp3CugzrYf1BLUwuywRu9gc ```
Public Client without PKCE ``` grant_type=authorization_code& client_id=myclient& code=8b51da4fade34a5ccf0d1dc40816812052e2b3ae ```
Public Client with PKCE ``` grant_type=authorization_code& client_id=myclient& code=8b51da4fade34a5ccf0d1dc40816812052e2b3ae& code_verifier=e4fz4xjFKhlQUWXgsME5dp3CugzrYf1BLUwuywRu9gc ```

Confidential & Public Client

This is currently not implemented in the library but can be implemented by the implementor.

We already have an open issue for this improvement. #81


@jankapunkt Which changes are you proposing?

MartinKolarik commented 2 weeks ago

@jorenvandeweyer's summary is correct here.

With grant_type=authorization_code, PKCE may or may not be used, and client_secret may or may not be required. Both decisions are up to the server and any of the four possible combinations is valid.

With grant_type=refresh_token, PKCE does not apply. client_secret may or may not be required, again, based on the decision of the server. For public clients, it may very well make sense to allow the refresh flow without a secret (though right now, this seems difficult to achieve as there's only one server-wide setting that applies to all clients).