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.

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

Add PKCE support when Authorization Code flow is used #5361

Open
wants to merge 11 commits into
base: master
from

Conversation

@poveilleux
Copy link

commented May 19, 2019

Description

  • Added a new usePkce flag that can be enabled to use PKCE with the Autorization Code flow. This flag is used when the /authorize request is sent (oauth2authorize.js) to determine if the code_challenge has to be sent (part of the PKCE protocol). The code_challenge is a SHA256 of the code_verifier which is sent on the /token request.
  • When the code_challenge is sent, the code_verifier is stored in the auth object to be sent on the /token request.
  • The code_verifier needs to have a length of at least 43 characters as per the spec.

Motivation and Context

  • This makes the Authorization Code flow more secure because the code can only be used by the client that provided the code_challenge on the /authorize request.
  • Fixes #5348

How Has This Been Tested?

My setup:

  1. An OpenID Connect compliant STS using IdentityServer4. I configured a test Client that had RequirePkce enabled with the Authorization Code flow. With these settings, it was not possible to use the Code flow without sending the required data for PKCE.
  2. An API using Swashbuckle.AspNetCore and ASP.Net Core to generate a swagger.json configured to use my STS.

How I tested and implemented the fix:

  1. I set up the dev-server to use the swagger.json from my API. To be able to request an access token, I needed to have PKCE working.
  2. I made the "Authorize" request from SwaggerUI. I was getting an error on the /authorize request about the missing code_challenge. I made the required changes to make that part work.
  3. I tested again and I got an error on the /token request. I made the required changes to send the code_verifier and it worked (meaning I got the access token to make requests to the API).

Impact on the rest of the code:

  • None, the use of PKCE is opt-in with a usePkce flag on the authConfigs. This way, it is not a breaking change for people wanting to upgrade to this new version of SwaggerUI.

Checklist

My PR contains...

  • No code changes (src/ is unmodified: changes to documentation, CI, metadata, etc.)
  • Dependency changes (any modification to dependencies in package.json)
  • Bug fixes (non-breaking change which fixes an issue)
  • Improvements (misc. changes to existing features)
  • Features (non-breaking change which adds functionality)

My changes...

  • are breaking changes to a public API (config options, System API, major UI change, etc).
  • are breaking changes to a private API (Redux, component props, utility functions, etc.).
  • are breaking changes to a developer API (npm script behavior changes, new dev system dependencies, etc).
  • are not breaking changes.

Documentation

  • My changes do not require a change to the project documentation.
  • My changes require a change to the project documentation.
  • If yes to above: I have updated the documentation accordingly.

Automated tests

  • My changes can not or do not need to be tested.
  • My changes can and should be tested by unit and/or integration tests.
  • If yes to above: I have added tests to cover my changes.
  • If yes to above: I have taken care to cover edge cases in my tests.
  • All new and existing tests passed.
poveilleux added 2 commits May 15, 2019
@poveilleux

This comment has been minimized.

Copy link
Author

commented May 19, 2019

This is weird. Locally I get errors in test/e2e-cypress/tests/features/oauth2-flows/application.js and test/e2e-cypress/tests/features/oauth2-flows/password.js but they run just fine in Jenkins.

I am also getting an error on the test/core/system/system.jsx about render() that should have a return statement (eslint error) but I did not change this.

EDIT:
I moved the "eslint disable next line" one line down and it fixed the issue.

@poveilleux poveilleux marked this pull request as ready for review May 19, 2019

poveilleux added 3 commits May 19, 2019
@lsivakumar

This comment has been minimized.

Copy link

commented May 29, 2019

Hi, Is this getting merged to master soon ?

@poveilleux

This comment has been minimized.

Copy link
Author

commented May 29, 2019

@lsivakumar I'd like to, but I am not sure how I can poke the people maintaining SwaggerUI.

poveilleux added 2 commits May 29, 2019
@shockey

This comment has been minimized.

Copy link
Member

commented Jun 15, 2019

@poveilleux, thank you for your work here!

I received an email about this PR, thought it'd be helpful to copy here:

Can [this PR] be merged to master?

Hi!

We're currently sitting on a big backlog of PRs that we need to work through, but rest assured that we haven't forgotten about this!

@poveilleux

This comment has been minimized.

Copy link
Author

commented Jun 15, 2019

Thanks @shockey for the feedback. At least I know it was not forgotten!

poveilleux added 3 commits Jun 15, 2019
@poveilleux

This comment has been minimized.

Copy link
Author

commented Jul 29, 2019

Hi @shockey, is there anything I can do to help you pushing this pull request into master faster? Thanks

@fuzzzerd

This comment has been minimized.

Copy link

commented Jul 30, 2019

I too would love to see this merged so work on adding it downstream can begin.

@Pete-PlaytimeSolutions

This comment has been minimized.

Copy link

commented Aug 12, 2019

Since the latest draft of the IETF OAuth 2.0 Security Best Current Practice recommendations suggests that using Implicit Flow is no longer the preferred flow option for web applications. See

Clients SHOULD instead use the response type "code" (aka authorization code grant type) as specified in Section 3.1.1

I would also love to see pull request fast tracked,
in order to be able to implement the latest security best practices.

@fuzzzerd

This comment has been minimized.

Copy link

commented Sep 6, 2019

Are there plans to merge this? If so, when?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.