Skip to content
New issue

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

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Authorization Code Flow with PKCE #449

Merged
merged 2 commits into from
Nov 8, 2020
Merged

Add support for Authorization Code Flow with PKCE #449

merged 2 commits into from
Nov 8, 2020

Conversation

ls-youssef-jlidat
Copy link
Contributor

@ls-youssef-jlidat ls-youssef-jlidat commented Aug 12, 2020 •
edited by evansims
Loading

Add support for Authorization Code Flow with PKCE

By submitting a PR to this repository, you agree to the terms within the Auth0 Code of Conduct. Please see the contributing guidelines for how to create and submit a high-quality PR for this repo.

Description

This is a draft PR to add support for Authorization Code Flow with Proof Key for Code Exchange (PKCE).
We wanted to use this SDK in our company to implement SSO integration with Auth0, The team managing Auth0 is highly recommending adding this extra layer of security when exchanging code to access token.

I opened this PR just to get your feedbacks first before finalizing it.

References

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

Testing

To test this change, we must enable PKCE verification in Auth0 server first. Then from an Auth0 client we can set the new property enable_pkce to true and proceed to login. We should be able to pass verification and exchange a token.

Checklist

  • I have added documentation for new/changed functionality in this PR or in auth0.com/docs
  • All active GitHub checks for tests, formatting, and security are passing
  • The correct base branch is being used, if not master

@ls-youssef-jlidat ls-youssef-jlidat marked this pull request as ready for review October 27, 2020 18:30
@ls-youssef-jlidat ls-youssef-jlidat requested a review from a team as a code owner October 27, 2020 18:30
@evansims
Copy link
Member

evansims commented Oct 30, 2020 •
edited
Loading

Hi @ls-youssef-jlidat 👋 Thanks for you PR! Really appreciate the work you put into this.

We'd not considered adding PKCE to date as the security issues it was traditionally meant to address are generally more of a concern in things like native apps or single page applications, but I know feelings on this have evolved to now support the idea of PKCE in confidential clients as well.

@evansims
Copy link
Member

evansims commented Nov 3, 2020

Hey again @ls-youssef-jlidat just wanted to give you an update; the team and I are reviewing things and will get back to you on this PR shortly. Thank you again for your work on this.

@ls-youssef-jlidat
Copy link
Contributor Author

ls-youssef-jlidat commented Nov 3, 2020 •
edited
Loading

Hi @ls-youssef-jlidat 👋 Thanks for you PR! Really appreciate the work you put into this.

Out of curiosity, did you have a specific situation or use case in mind for implementing PKCE into the PHP SDK? Is there a specific concern you're looking to address, or are you just rounding out the SDK's feature set?

We'd not considered adding PKCE to date as the security issues it is meant to address are generally more of a concern in things like native apps or single page applications. I'd be curious to hear your inspiration for implementing it on the backend as well

Hi @evansims, Thank you for the feedback!
The team managing Auth0 in our company is enforcing this extra layer of security in all clients including web apps that use a client secret, and that was the only missing piece preventing us from using this SDK in our PHP application. I'm aware that PKCE was originally designed to protect mobile apps and SPAs, I opened this PR just to collect some feedbacks from your team and see if it's providing any value.

evansims
evansims previously approved these changes Nov 6, 2020
View reviewed changes
Copy link
Member

@evansims evansims left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks solid, thanks very much for your contribution! 👍

ls-youssef-jlidat
ls-youssef-jlidat commented Nov 7, 2020
View reviewed changes
Copy link
Contributor Author

@ls-youssef-jlidat ls-youssef-jlidat left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @evansims for the approval, looking forward to see this change in one of your stable versions soon 🙏

src/Helpers/PKCE.php
try {
$bytes = random_bytes($size);
} catch (\Exception $e) {
$bytes = openssl_random_pseudo_bytes($size);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to add ext-openssl: * as a composer requirement just to make sure the openssl extension is enabled in the PHP installation.

src/Helpers/PKCE.php
*
* @see https://tools.ietf.org/html/rfc7636
*/
public static function generateCodeVerifier(int $length = 43): string
Copy link
Contributor Author

@ls-youssef-jlidat ls-youssef-jlidat Nov 7, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Feel free to extract this method somewhere else or rename this helper class so it can be used for anything else (not necessary related to PKCE).

@ls-youssef-jlidat ls-youssef-jlidat force-pushed the add-Auth0-PKCE-support branch 2 times, most recently from c83aa79 to 2c9237c Compare November 7, 2020 17:08
@ls-youssef-jlidat
Add PKCE support
fc85efb 
Add support for Authorization Code Flow with PKCE
@ls-youssef-jlidat
Copy link
Contributor Author

@evansims I force pushed a commit to fix a typo in the author email.

@evansims evansims merged commit b78e6c9 into auth0:master Nov 8, 2020
abhishekrawatlightspeed
abhishekrawatlightspeed reviewed Nov 11, 2020
View reviewed changes
src/Helpers/PKCE.php
{
if ($length < 43 || $length > 128) {
throw new \InvalidArgumentException(
'Code verifier must be crated with a minimum length of 43 characters and a maximum length of 128 characters.'
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think you spelled created wrong

@evansims evansims added this to the 7.5.0 milestone Nov 16, 2020
@github-actions
Copy link
Contributor

This pull request has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@abhishekrawatlightspeed abhishekrawatlightspeed abhishekrawatlightspeed left review comments

@evansims evansims evansims approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
7.5.0
Development

Successfully merging this pull request may close these issues.
3 participants
@ls-youssef-jlidat @evansims @abhishekrawatlightspeed