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

OAuth2 - PKCE #131

Merged
merged 4 commits into from Aug 11, 2020
Merged

OAuth2 - PKCE #131

merged 4 commits into from Aug 11, 2020

Conversation

jessedoyle
Copy link
Contributor

@jessedoyle jessedoyle commented Jun 23, 2020

resolves: #130

Overview

Hi There! Thanks for building such a great library!

This PR implements the optional PKCE (proof key for code exchange) protocol when using an OAuth2 code grant and the feature is enabled via strategy options.

For security purposes, PKCE is now recommended for all OAuth2 code grant interactions by the Internet Engineering Task Force.

PKCE requires the client to "prove" that the identity requesting an access token is the same actor that successfully authorized the OAuth2 request.

With this PR, you can enable PKCE by simply setting the pkce strategy option to true.

i.e.

...
provider(
  :my_oauth2_strategy,
  pkce: true
)
...

Notes

  • Add a pkce option to the oauth2 strategy that defaults
    to false.
  • When the option is true, the client will authorize with the
    provider using PKCE (proof key for code exchange) 1. This
    enhances the security footprint of the interaction and is now
    recommended by the IETF for all OAuth2 code grant interactions.
  • At a high level, PKCE works as follows:
    1. Generate a new random code verifier string value with a
      minimum length of 43 characters and a maximum length of
      128 characters.
    2. Take the SHA256 hash value of the code verifier string and
      perform a URL-safe Base64 encode of the result as defined
      in 2.
    3. Pass code_challenge={Base64(SHA256(code_verifier)}
      and code_challenge_method=S256 query parameters with
      the client OAuth2 authorize request.
    4. In the callback_phase, pass the code_verifier in plaintext
      to the provider as a query parameter to the OAuth2 token
      endpoint. This provides strong guarantees to the OAuth provider
      that the client is the same entity that requested authorization.

* Add a `pkce` option to the oauth2 strategy that defaults
  to `false`.
* When the option is true, the client will authorize with the
  provider using PKCE (proof key for code exchange) [1]. This
  enhances the security footprint of the interaction and is now
  recommended by the IETF for all OAuth2 code grant interactions.
* At a high level, PKCE works as follows:
  1. Generate a new random code verifier string value with a
     minimum length of 43 characters and a maximum length of
     128 characters.
  2. Take the SHA256 hash value of the code verifier string and
     perform a URL-safe Base64 encode of the result as defined
     in [2].
  3. Pass `code_challenge={Base64(SHA256(code_verifier)}`
     and `code_challenge_method=S256` query parameters with
     the client OAuth2 authorize request.
  4. In the callback_phase, pass the `code_verifier` in plaintext
     to the provider as a query parameter to the OAuth2 token
     endpoint. This provides strong guarantees to the OAuth provider
     that the client is the same entity that requested authorization.

[1]: https://tools.ietf.org/html/rfc7636
[2]: https://tools.ietf.org/html/rfc7636#appendix-A
@jessedoyle
Copy link
Contributor Author

It looks like CI is failing on Ruby 2.4.4 due to RuboCop violations.

I'd be happy to resolve these issues, but it looks some lint exceptions are being thrown for files that aren't changed in this PR. How do you normally handle this?

@jessedoyle jessedoyle mentioned this pull request Jun 24, 2020
* Resolve all current rubocop violations for rubocop
  v0.86.0.
@jessedoyle
Copy link
Contributor Author

jessedoyle commented Jun 28, 2020

@tmilewski, @supernova32 - I've resolved the rubocop violations and CI is passing. Do you think we can merge this?

@aaronpk
Copy link

aaronpk commented Aug 10, 2020

Great to see this! Would love to see this merged.

@jessedoyle
Copy link
Contributor Author

Great to see this! Would love to see this merged.

It looks like there's not a ton of movement in the repo anymore with the last release occurring back in 2018.

We're running off of a fork as PKCE is a requirement for our apps. It would be awesome to get this merged upstream!

I'd be happy to help get this merged in any way that I can.

Pinging the owners of the gem on RubyGems one last time (sorry for the spam): @mbleigh, @sferik, @tmilewski, @BobbyMcWho

@BobbyMcWho
Copy link
Member

👋 thanks for reaching out, sorry it's taken so long to get back to you on this, I've only recently gained maintainer-status on this project and a few other omniauth related ones, and unfortunately don't have a ton of time to to provide the level of maintenance I'd like to 😞

That being said, this looks like a reasonable change, I'd like to figure out a way to bypass the CI warnings without adding the frozen string literal comments, due to the fact that those comments are not supported on ruby < 2.3, and I believe we still technically support those ruby versions.

@jessedoyle
Copy link
Contributor Author

@BobbyMcWho - Thanks for the response!

I'd like to figure out a way to bypass the CI warnings without adding the frozen string literal comments, due to the fact that those comments are not supported on ruby < 2.3, and I believe we still technically support those ruby versions.

We should be able to add an exception for those comments in the rubocop.yml file. I'll remove them and add the exception if you're okay with that?

@BobbyMcWho
Copy link
Member

BobbyMcWho commented Aug 10, 2020

Yeah, that sounds good to me, I do see that @supernova32 removed support for those ruby < 2.4 in 35bc27b, but I think for now, let's leave the magic comments out of scope for this specific change, and we'll reconsider those in the future.

@jessedoyle
Copy link
Contributor Author

It's looking like there's a couple more errors introduced by newer versions of Rubocop:

Inspecting 8 files
....WC..

Offenses:

lib/omniauth/strategies/oauth2.rb:17:7: W: Lint/MissingSuper: Call super to invoke callback defined in the parent class.
      def self.inherited(subclass) ...
      ^^^^^^^^^^^^^^^^^^^^^^^^^^^^
lib/omniauth/strategies/oauth2.rb:128:7: C: Metrics/AbcSize: Assignment Branch Condition size for options_for is too high. [<4, 17, 4> 17.92/17]
      def options_for(option) ...
      ^^^^^^^^^^^^^^^^^^^^^^^
lib/omniauth/strategies/oauth2.rb:145:9: W: Lint/MissingSuper: Call super to initialize state of the parent class.
        def initialize(error, error_reason = nil, error_uri = nil) ...
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
omniauth-oauth2.gemspec:1:1: C: Gemspec/RequiredRubyVersion: required_ruby_version should be specified.
lib = File.expand_path("../lib", __FILE__)
^

8 files inspected, 4 offenses detected

It's usually common practice to lock rubocop to a particular version so the build isn't a moving target. With that being said, would you prefer exceptions? Or for me to fix the warning?

__review__

* Remove Ruby 2.3 frozen string magic comments as requested because
  it is out of scope for this feature.
* Add an exception to `rubocop.yml` so that the missing
  magic comment is not a CI failure.
* Add exceptions for `Lint/MissingSuper` and
  `Gemspec/REquiredRubyVersion` to minimize churn on the feature.
@jessedoyle
Copy link
Contributor Author

@BobbyMcWho - I ended up adding exceptions to the rubocop.yml for new warnings to minimize churn to get this feature merged!

@BobbyMcWho
Copy link
Member

@jessedoyle I opened a PR to your branch with a minor refactor jessedoyle#1

@jessedoyle
Copy link
Contributor Author

@BobbyMcWho - Thanks, I've merged your changes!

@BobbyMcWho
Copy link
Member

Hmm, I just realized that I don't actually have merge access to this repo, @supernova32 is that something you're able to provide?

@suprnova32
Copy link
Member

I created a new team for OAuth2 and added you to it, Bobby. You should be able to merge now.

BobbyMcWho added a commit that referenced this pull request Aug 11, 2020
@BobbyMcWho BobbyMcWho merged commit 53ade6b into omniauth:master Aug 11, 2020
@jessedoyle
Copy link
Contributor Author

Thanks everyone!

@BobbyMcWho
Copy link
Member

Thanks for sticking through with us @jessedoyle, this has been released in v1.7.0 and has been pushed to rubygems under the same.

@jessedoyle jessedoyle deleted the pkce branch August 11, 2020 15:56
@renatojf
Copy link

renatojf commented Sep 2, 2020

Might be a silly question, but does this actually work?
https://github.com/omniauth/omniauth-oauth2/blob/master/lib/omniauth/strategies/oauth2.rb#L108

This generates a new verifier on each request, and you cannot control it in order to re-send it on the callbacks phase.. Is it supposed to be like this and I got the whole thing wrong?

@BobbyMcWho
Copy link
Member

@renatojf have you tested this against an auth provider and had it fail? If it’s not working definitely submit an issue, but as I understand it what happens is we create the verifier here, store it in the session, and send it back in the token exchange transaction

@jessedoyle
Copy link
Contributor Author

@renatojf - We've been using this feature in production against an AWS Cognito user pool.

In most cases you shouldn't need to control the PKCE verifier as it's automatically merged into the token request params here:

options.token_params.merge(options_for("token")).merge(pkce_token_params)
.

The verifier is stored in the session using the omniauth.pkce.verifier if you need to access it directly though.

Finally, the PKCE spec indicates that the code verifier should be a high-entropy cryptographic string for each request between 43 and 128 characters: https://tools.ietf.org/html/rfc7636#section-4.1

@jessedoyle
Copy link
Contributor Author

With that being said, I'm sure it's possible to add an option proc that would control the generation PKCE verifier if some auth providers are doing custom/non-standard PKCE.

@aaronpk
Copy link

aaronpk commented Sep 2, 2020

With that being said, I'm sure it's possible to add an option proc that would control the generation PKCE verifier if some auth providers are doing custom/non-standard PKCE.

Please tell me providers are not doing this. PKCE is specified pretty strictly so the built-in random code verifier generator here should always work.

@aaronpk
Copy link

aaronpk commented Sep 2, 2020

Might be a silly question, but does this actually work?
https://github.com/omniauth/omniauth-oauth2/blob/master/lib/omniauth/strategies/oauth2.rb#L108

This generates a new verifier on each request, and you cannot control it in order to re-send it on the callbacks phase.. Is it supposed to be like this and I got the whole thing wrong?

This took me a minute to track down as well, but what's happening is line 72 runs the pkce_authorize_params function which generates the PKCE verifier and stores it in options.pkce_verifier, then it is saved in the session on line 74. A bit of a tricky code path to follow but it's definitely doing what is intended.

@renatojf
Copy link

renatojf commented Sep 2, 2020

This part I understand @aaronpk, but correct me if I'm wrong. Shouldn't it be sent once again on the callback phase?
The reason why I'm asking is because I am trying to implement a strategy for Okta and I keep getting forbidden responses 😔

@renatojf
Copy link

renatojf commented Sep 2, 2020

I'm sorry, I'm using my phone and I just missed a lot of posts before the one I replied. I will provide a bit more details once I am on a PC.
I am having some troubles with Okta and validating the PKCE

@aaronpk
Copy link

aaronpk commented Sep 2, 2020

This part I understand @aaronpk, but correct me if I'm wrong. Shouldn't it be sent once again on the callback phase?
The reason why I'm asking is because I am trying to implement a strategy for Okta and I keep getting forbidden responses 😔

Yes it includes it in the token request here

@renatojf
Copy link

renatojf commented Sep 2, 2020

I think I found my issue. I am running a Rails 6 app in API mode. Although there's a session object, I think it is only being used as a dummy session. Every time I try to log the session['omniauth.pkce.verifier'], it comes empty 😓

@jessedoyle
Copy link
Contributor Author

I think I found my issue. I am running a Rails 6 app in API mode. Although there's a session object, I think it is only being used as a dummy session. Every time I try to log the session['omniauth.pkce.verifier'], it comes empty 😓

Yup! I've run into the exact same problem in the past! Nice, I was going to suggest looking if session persistence was working. Glad you got it figured out!

@BobbyMcWho
Copy link
Member

If you’re running in API mode, and just doing server to server communication, would using the OAuth2 gem directly work better for you @renatojf? https://github.com/oauth-xx/oauth2

@renatojf
Copy link

renatojf commented Sep 3, 2020

I am already looking in that direction @BobbyMcWho . Thank you all very much for the support !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

PKCE Support
5 participants