PKCE

[visvk]

Implementation of rfc7636 Proof Key for Code Exchange by OAuth Public Clients

Notes:

• ~~model.generateAuthorizationCode is called with 2 new parameters (in total 5) codeChallenge and codeChallengeMethod. Typically, the "code_challenge" and "code_challenge_method" values are stored in encrypted form in the "code" itself but could alternatively be stored on the server associated with the code.~~ This can be a breaking change, so I've removed this part of code
• model.saveAuthorizationCode: added codeChallenge and codeChallengeMethod to the code object. We need to store the code challenge and it's method server side and then return it later on.
• model.getAuthorizationCode when code was issued with PKCE parameters, it should return these parameters in the code object. Then the code_verifier is validated

TODO:

• ~~client_secret should not be required~~ possible with isClientAuthenticationRequired option

Feedback is welcome!

[mjsalinger]

Also please change the PR to point to the dev branch instead of master.

[patrykcieszkowski]

Hang on, is this full implementation OF PKCE?

[visvk]

This PR supports code_challenge parameter and code_challenge_method parameter to be saved with the authorization code. Then it supports validation of code_verifier when requesting access token.

What is missing, code_challenge parameter and code_challenge_method parameter are not passed to the model.generateAuthorizationCode method, because it will be a breaking change.

If is something wrong or missing, please send a feedback.

[patrykcieszkowski]

Yeah, but there's still an issue with missing client_secret, right? Although you proposed to use requireClientAuthentication model option, I don't see it as good enough workaround. That would mean the authorization_code grant stays unsecured. Or am I wrong?

The Invalid client: cannot retrieve client credentials error is being thrown at token-handler.js (line 196. Method getClientCredentials). Do you think adding OR condition at line 190, and verifying that the code_verifier param was passed would do the trick?

But that also means, codeChallenge validation (at authorization-code-grant-type.js line 135) has to be modified. Currently, it only compares the codeChallange and code_verifier if codeChallange got returned from model.getAuthorizationCode. IMO, it should throw an error if code_verifier was passed, but DB query did not return it.

[visvk]

Thank you for the feedback. Yes, I didn't find better solution for ignoring client_secret. As you stated, requireClientAuthentication is not good solution. In this implementation, you can pass client_secret in the PKCE flow, but I know that there are some android oauth packages, that don't support client_secret. Moreover be aware, in the refresh token grant type you must still pass client_secret. In my opinion, better solution would be to implement client.confidential parameter, so there won't be client auth for non-confidential clients (#498 ).

I can add a change in codeChallenge validation, but in that validation, the server does not know if the code was issued by PKCE flow or not, and throw error when code was not issued by PKCE flow is not good. So I assume, the validation should be done only if code.codeChallenge is present.

[patrykcieszkowski]

excuse my ignorance, but Isn't the idea of PKCE flow to eliminate the need of passing client_secret? I thought it's the main point, is to be used as a public auth method. For mobile apps or web apps.

I need that change ASAP. So for the time being, I will pull your PR and modify it for my own needs for my Oauth2 server. But I will keep track of changes you make, though. Maybe you'll come up with something better.

[visvk]

No, idea of PKCE describes technique to mitigate against the authorization code interception attack.

I agree, that public clients should not use client_secret (as per rfc8252), but the server should assume client confidentiality by client_id (confidentiality of app should be stored in client registration process).

You have provided solution, where server assume client confidentiality by passing code_verifier in the request.

Yes, you should make your own fork, because any PR can be changed or deleted and then you can be in troubles.

[mjsalinger]

@visvk Can you rebase this PR to get rid of the conflicts?

[visvk]

Sorry, but there are many conflicts in the rebase process. Merging dev into this PR would be ok? I need to refactor authorize handler to resolve conflicts.

[mjsalinger]

@visvk That should be fine.

[visvk]

@mjsalinger done.

[yhc44]

When can we expect PKCE in official version?

Thanks :)

[yhc44]

Ok i now used your version and changed it a bit and its fully working in my SPA Application.

Thanks for your work @visvk

[visvk]

@yhc44 Do you want to use this feature in your SPA app? Because SPAs would not benefit from PKCE. PKCE is only useful in public native clients (mobile apps).

[yhc44]

@visvk Actually i've read that it is better than implicit grant.

I dont want to use authorization grant with secret. Is it the same as PKCE in SPA?

What do you recommend?

[ReeSilva]

Hey guys. First of all, awesome work on this lib. Save me a loooot of time.

There's any ETA on this feature? This is almost a must have to the authorization server that I'm building for my client.

Thanks in advance.

[toonvanstrijp]

Any idea of when this will be merged?

[benaubin]

@visvk What's the status on this?

[Rmannn]

any news on this PR ?

[mitjap]

@yhc44 Do you want to use this feature in your SPA app? Because SPAs would not benefit from PKCE. PKCE is only useful in public native clients (mobile apps).

I'm quoting oauth.net which states that PKCE is meant for SPAs.

PKCE (RFC 7636) is an extension to the Authorization Code flow to prevent certain attacks and to be able to securely perform the OAuth exchange from public clients.

It is primarily used by mobile and JavaScript apps, but the technique can be applied to any client as well.

[Zireael]

could PKCE be merged into the v5-dev?

[Rmannn]

6 month later... Please is there any plan to merge this ? We are waiting for this feature. Thx

[patrykcieszkowski]

@Rmannn why don't you make a fork of this repo and merge it yourself, while waiting for the official release?

[Rmannn]

@patrykcieszkowski cause apparently they are not going to merge this pr anytime soon. Even in the next release. This Pr was created in 2017... 3 years ago ?! If our team really need to fork, merge, and publish it to npm, we will simply rewrite the whole lib. This library is getting old, PRs are staying there for months, and v5-dev (typescript rewrite) code is going to be discarded.

We are really thankful for this library, but we simply do not want to loose more time waiting for things that are not going to happen

[thomseddon]

This isn't pegged for the next release - it's also a bit of a whopper so would need a bit of time for review, but would be brilliant for 4.1

[mraffo]

@mjsalinger @thomseddon, Hi there, thanks for the library. My question is... have you planned this to any date in particular? Very thanks and congrats!