Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

PKCE support? #469

Closed
StFS opened this issue May 26, 2020 · 7 comments · Fixed by #470
Closed

PKCE support? #469

StFS opened this issue May 26, 2020 · 7 comments · Fixed by #470
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@StFS
Copy link
Contributor

StFS commented May 26, 2020

I've been looking for information about how to enable PKCE when using the AuthorizationCodeFlow but haven't found anything.

Am I missing something or is PKCE not supported (out of the box) by this library?

@danoscarmike danoscarmike added the type: question Request for information or clarification. Not an issue. label May 27, 2020
@StFS
Copy link
Contributor Author

StFS commented May 28, 2020

I've added this feature to my fork of this project. I know it's in maintenance mode but is there any chance a PR would be merged?

Tagging @danoscarmike (as the only one who has reacted to this question) and @chingor13 and @rmistry as top contributors.

You can find what I currently have here: https://github.com/googleapis/google-oauth-java-client/compare/master...StFS:pkce-support?expand=1

There is still some documentation work to be done but the code is there and it works. I also added a complete example that works against a pristine keycloak instance that is run via docker.

So:

  1. Is there any chance a PR would be accepted, merged and included in the next release?
  2. If so, can you comment on the changes I have already and tell me if I should be doing something differently?

@StFS
Copy link
Contributor Author

StFS commented Jun 8, 2020

Anyone? @danoscarmike?

@StFS
Copy link
Contributor Author

StFS commented Jun 8, 2020

I'd also like to point out that according to the RFC for native apps, public native app clients MUST implement PKCE. So I'd argue that this issue, which started out as a question, is now a full blown issue. Not only that, it's a security bug.

The reason for the RFC mandating that PKCE should be used is that the authorization code redirect is pointed toward a local web server that does not have TLS. This is in fact what this library does. It starts up a local web server that listens for the authorization code token redirect. Therefore, it's trivial for a malicious agent, running on the client machine, to sniff the authorization code and use it.

I cannot update the issue to reflect this but I would really like somebody to review this. I'm trying to avoid creating our own patched version of this library but there is also the risk that a current user of this library isn't aware of this security risk and is using the library in good faith.

@StFS
Copy link
Contributor Author

StFS commented Jun 8, 2020

Sorry for the obsessive tagging, but desperately hoping to just get some feedback before deciding to create a patched fork.

@elharo , @ericraskin

@chingor13
Copy link
Contributor

This library has been in maintenance mode for a long time with mostly bugfixes being accepted. That said, the RFC looks pretty clear on this and the change looks reasonable.

@chingor13 chingor13 added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p2 Moderately-important priority. Fix may not be included in next release. and removed type: question Request for information or clarification. Not an issue. labels Jun 8, 2020
@StFS
Copy link
Contributor Author

StFS commented Jun 8, 2020

Thank you so much @chingor13 for taking the time to review this. I will update the PR promptly.

@JLLeitschuh
Copy link

This was assigned CVE-2020-7692

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants