Support for PKCE in server as per OAUTH PKCE RFC

[marcolenzo]

I implemented the OAUTH PKCE as per RFC 7636 https://www.rfc-editor.org/rfc/rfc7636.txt

I tried to be as less intrusive as possible on the existent code. I was wondering if I had to add the code_challenge and code_challenge_method parameters in the AuthorizationRequest but I eventually opted not to and to rely on the existent maps.

This should complete implementation for issue #655

[MrArtichaut]

Is there any plan to merge this into the master ? I've tried a local merge and there is no conflict and unit tests are all green.

The draft-ietf-oauth-native-apps now indicates that PKCE MUST be implemented for public native apps : https://tools.ietf.org/html/draft-ietf-oauth-native-apps-12#section-6 making this pull request even more interesting.

Thanks.

[osgafarov]

Should this PR be assigned to a specific person?
P.S. I am also working on a mobile app with PKCE approach, so merging this PR would be very helpful.

[jgrandja]

Thanks for the PR @marcolenzo.
However, this project is currently in maintenance mode and we're only accepting minor updates and bug fixes. We are not looking to add any new features.

We are focusing our efforts in the new OAuth2/OIDC support in Spring Security 5.

[osgafarov]

@jgrandja Will there be PKCE support in Spring Security 5? If not, what would be the best way to authorize mobile apps to call REST API using OAuth2?

[jgrandja]

@osgafarov Yes, we will provide support for PKCE in Spring Security 5, however, that will be a while still. Our current focus is in re-writing the client and that is what will go GA later this November. We're planning a 5.1 shortly afterward which will include the Resource Server support and then will start working on the Authorization Server side of things. So the Authorization Server support probably won't be out until mid 2018.

[osgafarov]

Thanks @jgrandja for your answer. Will follow the development of the Spring Security 5.

[ghost]

I would suggest that instead of changing the code do it in a filter, like csrf() - so it would be pkce().
At least temporarily we can include without pain, spring security 5 is far away. Either way, everybody who heard about it tries to implement in his own way. At least i saw the code, so it's not too big deal to create some filter in the filterchain and add the implementation, which is independent so i don't need to hack the whole thing. The other thing is, that i'm using some sort of repository to store the client details, and i just don't see from the code changes, how it can be introduced.

[OrangeDog]

@marcolenzo have you got an example (or advice) for implementing this without library changes (or minimal changes that can be applied with aop or replacing simple classes)?

Edit:
It's a little tricky, as there's no way to get the stored code details without expiring the code. Without modifying existing classes at all you'd need to build an orthogonal storage method to map issued codes to the code challenge parameters.

[marcolenzo]

@OrangeDog did you have the chance to revise the content of the PR itself? https://github.com/spring-projects/spring-security-oauth/pull/675/files

An alternative option that does not fiddle with the internals of the library is to port the logic I introduced into a filter as suggested by @ghost #675 (comment)

[OrangeDog]

Having a closer look this doesn't follow the RFC:

• Parameters should be checked on the auth requests, and "invalid_request" used as an error.
• If the verifier fails on the token request, the error is "invalid_grant".
• S256 specifies an ASCII encoding, not UTF8

The error "invalid_code_verifier" is not part of the standard.

[OrangeDog]

You should probably also use constant-time comparisons, not String.equals.

import static java.nio.charset.StandardCharsets.US_ASCII;
import java.security.MessageDigest;
import java.util.Base64;

// ...

MessageDigest md = MessageDigest.getInstance("SHA-256");
Base64.Encoder b64 = Base64.getUrlEncoder().withoutPadding();
byte[] expected = b64.encode(md.digest(verifier.getBytes(US_ASCII)));
return MessageDigest.isEqual(expected, challenge.getBytes(US_ASCII));