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

Consider adding Code + PKCE support in the OpenID Connect handler #7734

Closed
leastprivilege opened this issue Feb 20, 2019 · 11 comments
Closed
Assignees
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one

Comments

@leastprivilege
Copy link
Contributor

leastprivilege commented Feb 20, 2019

Traditional code flow in OAuth 2 has the code substitution problem (https://tools.ietf.org/id/draft-ietf-oauth-security-topics-06.html#rfc.section.3.5). This was one of the motivations to create the OIDC hybrid flow (aka code id_token), where the id_token contains the hash of the code (aka detached signature) to protect against this attack. The downside of this approach is, that an id_token must be sent over the front channel.

PKCE (Proof Key for Code Exchange) solves the same problem in a different and simpler way. The spec (https://tools.ietf.org/html/rfc7636) reads like this is only useful for native client applications, but it turned out it is useful in many more cases.

The advantages of using PKCE are:

  • can go back to code response type which means no id_token must be sent over the front-channel which eliminates privacy concerns (e.g. leaking the subject id over the browser)
  • since there is no id_token on the front channel, no JWT validation must be done
  • since the id_token comes over the HTTPS back-channel only, JWT validation is optional
  • the client implementation is considerably simpler

You might have followed the latest discussions around OIDC/OAuth security considerations and also the SPA guidance. There is a movement to simplify the process around selecting the right flows - basically it boils down to

  • use client credentials for non-interactive server to server
  • use code + PKCE for the rest

It is not hard to add this feature today - I created a POC here:

https://github.com/leastprivilege/AspNetCoreSecuritySamples/tree/aspnetcore21/OidcCodePkce

especially

https://github.com/leastprivilege/AspNetCoreSecuritySamples/blob/aspnetcore21/OidcCodePkce/src/AspNetCoreSecurity/Startup.cs#L77-L118

Something to consider for 3.0

@Tratcher Tratcher added the area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer label Feb 20, 2019
@Eilon
Copy link
Member

Eilon commented Apr 26, 2019

Adding to the list of potential features for 3.1.

@jmprieur - out of curiosity, if we had this, is this something that would work on AzureAD?

@brockallen
Copy link

you're welcome @leastprivilege

@jmprieur
Copy link
Contributor

@Eilon :MSAL.NET uses it in desktop/mobile apps. Adding @brentschmaltz for his opinion for Web apps.

@Tratcher
Copy link
Member

The OAuth base class should also support this. See #9448, #9092

@brentschmaltz
Copy link
Contributor

brentschmaltz commented May 30, 2019

This seems like a valuable feature.
For the OIDC handler, the sample that @leastprivilege provided shows that a simple property on OpenIdConnectOptions should trigger the scenario.

For the OAuthHandler, ExchangeCodeAsync has a fixed parameter set which may require some changes.

@Tratcher
Copy link
Member

Tratcher commented Jun 4, 2019

I have this in progress. It was easy enough for OIDC and I verified it against AAD.

I'm trying to do OAuth support as well but it doesn't seem to be widely supported there. I tried Google, Facebook, Github, and MSA but only MSA seem to support PKCE (because they're the same endpoints as AAD). Any suggestions for a provider that does support it?

@brockallen
Copy link

brockallen commented Jun 5, 2019

I have this in progress. It was easy enough for OIDC and I verified it against AAD.

There's also http://demo.identityserver.io/ available for such testing.

Any suggestions for a provider that does support it?

And you can prolly use the demo IS for that too.

@Tratcher
Copy link
Member

Tratcher commented Jun 5, 2019

@brockallen thanks that worked for OIDC and OAuth. Note IdentityServer's token endpoint could provide a more detailed error for the failure scenario.
Microsoft.IdentityModel.Protocols.OpenIdConnect.OpenIdConnectProtocolException: Message contains error: 'invalid_grant', error_description: 'error_description is null', error_uri: 'error_uri is null'.

Tratcher added a commit that referenced this issue Jun 5, 2019
Tratcher added a commit that referenced this issue Jun 5, 2019
@Tratcher Tratcher added the Done This issue has been fixed label Jun 7, 2019
@Tratcher Tratcher added the enhancement This issue represents an ask for new feature or an enhancement to an existing one label Jun 7, 2019
@Tratcher Tratcher closed this as completed Jun 7, 2019
@brockallen
Copy link

Note IdentityServer's token endpoint could provide a more detailed error for the failure scenario.

ha, check the logs! :P

@Tratcher
Copy link
Member

Tratcher commented Jun 7, 2019

😁 Fair. The spec suggests setting error_description too.

@brockallen
Copy link

yea, we always err'd on the side of trying to expose less info. but fair point.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 3, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-auth Includes: Authn, Authz, OAuth, OIDC, Bearer Done This issue has been fixed enhancement This issue represents an ask for new feature or an enhancement to an existing one
Projects
None yet
Development

No branches or pull requests

7 participants