Skip to content

Add PKCE support in OIDC & OAuth #10928

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

Merged
merged 8 commits into from
Jun 7, 2019
Merged

Add PKCE support in OIDC & OAuth #10928

merged 8 commits into from
Jun 7, 2019

Conversation

Tratcher
Copy link
Member

@Tratcher Tratcher commented Jun 5, 2019

#7734 (Best reviewed with Ignore Whitespace due to formatting improvements).

I've also cleaned up the samples quite a bit and added IdentityServer to them.

Considerations:

  • For OIDC how important is it to scope PKCE to only code flow? Why not use it for everything? @brockallen @leastprivilege
  • I've enabled this by default for OIDC. Tested with ADD and IdentityServer. Servers that don't support it should silently ignore it.
  • I have not enabled it by default for OAuth. Only MSA and IdentityServer supported it, and OAuth servers tend to be far less standards compliant. Individual providers can opt-in.
  • This required an API breaking change for OAuth and we know of at least 9 providers in aspnet-contrib that will have to update. @PinpointTownes
  • This doubles the size of the State for OAuth and OIDC, which may contribute to long url issues.

@Tratcher Tratcher added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Jun 5, 2019
@Tratcher Tratcher added this to the 3.0.0-preview7 milestone Jun 5, 2019
@Tratcher Tratcher requested a review from HaoK June 5, 2019 22:01
@Tratcher Tratcher self-assigned this Jun 5, 2019
@Tratcher Tratcher added the breaking-change This issue / pr will introduce a breaking change, when resolved / merged. label Jun 5, 2019
@brockallen
Copy link

For OIDC how important is it to scope PKCE to only code flow? Why not use it for everything?

implicit there is no use of the token endpoint, thus no need.
hybrid uses the nonce and c_hash from the spec as the protection, so that's redundant (albeit more complicated for the client).

@kevinchalet
Copy link
Contributor

This required an API breaking change for OAuth and we know of at least 9 providers in aspnet-contrib that will have to update. @PinpointTownes

@martincostello more breaking changes for your PR 😄

I have not enabled it by default for OAuth. Only MSA and IdentityServer supported it, and OAuth servers tend to be far less standards compliant. Individual providers can opt-in.

Sounds safer, indeed 😄

@Tratcher Tratcher marked this pull request as ready for review June 6, 2019 16:28
@Tratcher Tratcher requested a review from analogrelay as a code owner June 6, 2019 16:28
@Tratcher Tratcher merged commit 75e0115 into master Jun 7, 2019
@ghost ghost deleted the tratcher/pixie branch June 7, 2019 18:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer breaking-change This issue / pr will introduce a breaking change, when resolved / merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants