From e53f2cbd7e6f663b3a6ff47f80eac573c690f97e Mon Sep 17 00:00:00 2001 From: Jesse Doyle Date: Mon, 22 Jun 2020 22:34:40 -0600 Subject: [PATCH 1/5] OAuth2 - PKCE * 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 --- lib/omniauth/strategies/oauth2.rb | 28 +++++++++++++++++++++---- spec/omniauth/strategies/oauth2_spec.rb | 14 +++++++++++++ 2 files changed, 38 insertions(+), 4 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 318f699..2fa6a48 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -29,6 +29,7 @@ def self.inherited(subclass) option :token_options, [] option :auth_token_params, {} option :provider_ignores_state, false + option :pkce, false attr_accessor :access_token @@ -49,18 +50,33 @@ def request_phase end def authorize_params + verifier = SecureRandom.hex(64) + + if options.pkce + # NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A + challenge = Base64 + .urlsafe_encode64(Digest::SHA2.digest(verifier)) + .split("=") + .first + options.authorize_params[:code_challenge] = challenge + options.authorize_params[:code_challenge_method] = "S256" + end + options.authorize_params[:state] = SecureRandom.hex(24) params = options.authorize_params.merge(options_for("authorize")) + if OmniAuth.config.test_mode @env ||= {} @env["rack.session"] ||= {} end + + session["omniauth.pkce.verifier"] = verifier if options.pkce session["omniauth.state"] = params[:state] params end def token_params - options.token_params.merge(options_for("token")) + options.token_params.merge(options_for("token")).merge(pkce_token_params) end def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity @@ -84,17 +100,21 @@ def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength protected + def pkce_token_params + return {} unless options.pkce + + { code_verifier: session.delete("omniauth.pkce.verifier") } + end + def build_access_token verifier = request.params["code"] client.auth_code.get_token(verifier, {:redirect_uri => callback_url}.merge(token_params.to_hash(:symbolize_keys => true)), deep_symbolize(options.auth_token_params)) end def deep_symbolize(options) - hash = {} - options.each do |key, value| + options.each_with_object({}) do |(key, value), hash| hash[key.to_sym] = value.is_a?(Hash) ? deep_symbolize(value) : value end - hash end def options_for(option) diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb index 4765ad5..1832cc0 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -66,6 +66,13 @@ def app expect(instance.authorize_params.keys).to eq(["state"]) expect(instance.session["omniauth.state"]).to eq("qux") end + + it "includes PKCE parameters if enabled" do + instance = subject.new("abc", "def", pkce: true) + expect(instance.authorize_params[:code_challenge]).to be_a(String) + expect(instance.authorize_params[:code_challenge_method]).to eq("S256") + expect(instance.session["omniauth.pkce.verifier"]).to be_a(String) + end end describe "#token_params" do @@ -80,6 +87,13 @@ def app instance = subject.new("abc", "def", :token_options => %i[scope foo], :scope => "bar", :foo => "baz") expect(instance.token_params).to eq("scope" => "bar", "foo" => "baz") end + + it "includes the PKCE code_verifier if enabled" do + instance = subject.new("abc", "def", pkce: true) + # setup session + instance.authorize_params + expect(instance.token_params[:code_verifier]).to be_a(String) + end end describe "#callback_phase" do From a7b3b73599579d09ffab588156b7003e1b07cab3 Mon Sep 17 00:00:00 2001 From: Jesse Doyle Date: Sun, 28 Jun 2020 00:47:11 -0600 Subject: [PATCH 2/5] OAuth2 - PKCE | CI * Resolve all current rubocop violations for rubocop v0.86.0. --- .rubocop.yml | 24 ++++++++++--- Gemfile | 2 ++ Rakefile | 2 ++ lib/omniauth-oauth2.rb | 4 ++- lib/omniauth-oauth2/version.rb | 4 ++- lib/omniauth/strategies/oauth2.rb | 46 +++++++++++++++---------- omniauth-oauth2.gemspec | 2 ++ spec/helper.rb | 2 ++ spec/omniauth/strategies/oauth2_spec.rb | 10 +++--- 9 files changed, 68 insertions(+), 28 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index d5ce27c..3930505 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,15 +1,28 @@ +AllCops: + NewCops: enable + Layout/AccessModifierIndentation: EnforcedStyle: outdent +Layout/LineLength: + AllowURI: true + Enabled: false + Layout/SpaceInsideHashLiteralBraces: EnforcedStyle: no_space +Metrics/AbcSize: + Max: 17 + +Metrics/BlockLength: + Exclude: + - spec/omniauth/strategies/oauth2_spec.rb + Metrics/BlockNesting: Max: 2 -Metrics/LineLength: - AllowURI: true - Enabled: false +Metrics/ClassLength: + Max: 110 Metrics/MethodLength: CountComments: false @@ -19,6 +32,10 @@ Metrics/ParameterLists: Max: 4 CountKeywordArgs: true +Naming/FileName: + Exclude: + - lib/omniauth-oauth2.rb + Style/CollectionMethods: PreferredMethods: map: 'collect' @@ -52,4 +69,3 @@ Style/TrailingCommaInHashLiteral: Style/TrailingCommaInArrayLiteral: EnforcedStyleForMultiline: comma - \ No newline at end of file diff --git a/Gemfile b/Gemfile index 75f41f6..94bd336 100644 --- a/Gemfile +++ b/Gemfile @@ -1,3 +1,5 @@ +# frozen_string_literal: true + source "https://rubygems.org" gem "rake", "~> 12.0" diff --git a/Rakefile b/Rakefile index 8452dcd..7097e8d 100755 --- a/Rakefile +++ b/Rakefile @@ -1,4 +1,6 @@ #!/usr/bin/env rake +# frozen_string_literal: true + require "bundler/gem_tasks" require "rspec/core/rake_task" diff --git a/lib/omniauth-oauth2.rb b/lib/omniauth-oauth2.rb index 894d81c..48d302a 100644 --- a/lib/omniauth-oauth2.rb +++ b/lib/omniauth-oauth2.rb @@ -1,2 +1,4 @@ -require "omniauth-oauth2/version" # rubocop:disable FileName +# frozen_string_literal: true + +require "omniauth-oauth2/version" require "omniauth/strategies/oauth2" diff --git a/lib/omniauth-oauth2/version.rb b/lib/omniauth-oauth2/version.rb index c2c7281..b8d6e73 100644 --- a/lib/omniauth-oauth2/version.rb +++ b/lib/omniauth-oauth2/version.rb @@ -1,5 +1,7 @@ +# frozen_string_literal: true + module OmniAuth module OAuth2 - VERSION = "1.6.0".freeze + VERSION = "1.6.0" end end diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 2fa6a48..b69e6eb 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + require "oauth2" require "omniauth" require "securerandom" @@ -24,7 +26,7 @@ def self.inherited(subclass) option :client_secret, nil option :client_options, {} option :authorize_params, {} - option :authorize_options, [:scope, :state] + option :authorize_options, %i[scope state] option :token_params, {} option :token_options, [] option :auth_token_params, {} @@ -52,15 +54,7 @@ def request_phase def authorize_params verifier = SecureRandom.hex(64) - if options.pkce - # NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A - challenge = Base64 - .urlsafe_encode64(Digest::SHA2.digest(verifier)) - .split("=") - .first - options.authorize_params[:code_challenge] = challenge - options.authorize_params[:code_challenge_method] = "S256" - end + pkce_authorize_params!(verifier) options.authorize_params[:state] = SecureRandom.hex(24) params = options.authorize_params.merge(options_for("authorize")) @@ -70,8 +64,7 @@ def authorize_params @env["rack.session"] ||= {} end - session["omniauth.pkce.verifier"] = verifier if options.pkce - session["omniauth.state"] = params[:state] + build_authorize_session!(params, verifier) params end @@ -79,7 +72,7 @@ def token_params options.token_params.merge(options_for("token")).merge(pkce_token_params) end - def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength, PerceivedComplexity + def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexity, Metrics/MethodLength, Metrics/PerceivedComplexity error = request.params["error_reason"] || request.params["error"] if error fail!(error, CallbackError.new(request.params["error"], request.params["error_description"] || request.params["error_reason"], request.params["error_uri"])) @@ -100,10 +93,27 @@ def callback_phase # rubocop:disable AbcSize, CyclomaticComplexity, MethodLength protected + def build_authorize_session!(params, verifier) + session["omniauth.pkce.verifier"] = verifier if options.pkce + session["omniauth.state"] = params[:state] + end + + def pkce_authorize_params!(verifier) + return unless options.pkce + + # NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A + challenge = Base64 + .urlsafe_encode64(Digest::SHA2.digest(verifier)) + .split("=") + .first + options.authorize_params[:code_challenge] = challenge + options.authorize_params[:code_challenge_method] = "S256" + end + def pkce_token_params return {} unless options.pkce - { code_verifier: session.delete("omniauth.pkce.verifier") } + {:code_verifier => session.delete("omniauth.pkce.verifier")} end def build_access_token @@ -121,10 +131,10 @@ def options_for(option) hash = {} options.send(:"#{option}_options").select { |key| options[key] }.each do |key| hash[key.to_sym] = if options[key].respond_to?(:call) - options[key].call(env) - else - options[key] - end + options[key].call(env) + else + options[key] + end end hash end diff --git a/omniauth-oauth2.gemspec b/omniauth-oauth2.gemspec index 20abb26..f443431 100644 --- a/omniauth-oauth2.gemspec +++ b/omniauth-oauth2.gemspec @@ -1,3 +1,5 @@ +# frozen_string_literal: true + lib = File.expand_path("../lib", __FILE__) $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) require "omniauth-oauth2/version" diff --git a/spec/helper.rb b/spec/helper.rb index 280be5f..561e680 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + $LOAD_PATH.unshift File.expand_path("..", __FILE__) $LOAD_PATH.unshift File.expand_path("../../lib", __FILE__) diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb index 1832cc0..f3c7d51 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -1,6 +1,8 @@ +# frozen_string_literal: true + require "helper" -describe OmniAuth::Strategies::OAuth2 do # rubocop:disable Metrics/BlockLength +describe OmniAuth::Strategies::OAuth2 do def app lambda do |_env| [200, {}, ["Hello."]] @@ -62,13 +64,13 @@ def app end it "includes custom state in the authorize params" do - instance = subject.new("abc", "def", state: Proc.new { "qux" } ) + instance = subject.new("abc", "def", :state => proc { "qux" }) expect(instance.authorize_params.keys).to eq(["state"]) expect(instance.session["omniauth.state"]).to eq("qux") end it "includes PKCE parameters if enabled" do - instance = subject.new("abc", "def", pkce: true) + instance = subject.new("abc", "def", :pkce => true) expect(instance.authorize_params[:code_challenge]).to be_a(String) expect(instance.authorize_params[:code_challenge_method]).to eq("S256") expect(instance.session["omniauth.pkce.verifier"]).to be_a(String) @@ -89,7 +91,7 @@ def app end it "includes the PKCE code_verifier if enabled" do - instance = subject.new("abc", "def", pkce: true) + instance = subject.new("abc", "def", :pkce => true) # setup session instance.authorize_params expect(instance.token_params[:code_verifier]).to be_a(String) From 13dde0ca2183f32138cfda43dd011b68d644f897 Mon Sep 17 00:00:00 2001 From: Jesse Doyle Date: Mon, 10 Aug 2020 13:36:51 -0600 Subject: [PATCH 3/5] OAuth2 - PKCE | CI __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. --- .rubocop.yml | 11 ++++++++++- Gemfile | 2 -- Rakefile | 1 - lib/omniauth-oauth2.rb | 2 -- lib/omniauth-oauth2/version.rb | 4 +--- lib/omniauth/strategies/oauth2.rb | 2 -- omniauth-oauth2.gemspec | 2 -- spec/helper.rb | 2 -- spec/omniauth/strategies/oauth2_spec.rb | 2 -- 9 files changed, 11 insertions(+), 17 deletions(-) diff --git a/.rubocop.yml b/.rubocop.yml index 3930505..985a27e 100644 --- a/.rubocop.yml +++ b/.rubocop.yml @@ -1,6 +1,9 @@ AllCops: NewCops: enable +Gemspec/RequiredRubyVersion: + Enabled: false + Layout/AccessModifierIndentation: EnforcedStyle: outdent @@ -11,8 +14,11 @@ Layout/LineLength: Layout/SpaceInsideHashLiteralBraces: EnforcedStyle: no_space +Lint/MissingSuper: + Enabled: false + Metrics/AbcSize: - Max: 17 + Max: 18 Metrics/BlockLength: Exclude: @@ -52,6 +58,9 @@ Style/DoubleNegation: Style/ExpandPathArguments: Enabled: false +Style/FrozenStringLiteralComment: + Enabled: false + Style/HashSyntax: EnforcedStyle: hash_rockets diff --git a/Gemfile b/Gemfile index 94bd336..75f41f6 100644 --- a/Gemfile +++ b/Gemfile @@ -1,5 +1,3 @@ -# frozen_string_literal: true - source "https://rubygems.org" gem "rake", "~> 12.0" diff --git a/Rakefile b/Rakefile index 7097e8d..fd5db1e 100755 --- a/Rakefile +++ b/Rakefile @@ -1,5 +1,4 @@ #!/usr/bin/env rake -# frozen_string_literal: true require "bundler/gem_tasks" require "rspec/core/rake_task" diff --git a/lib/omniauth-oauth2.rb b/lib/omniauth-oauth2.rb index 48d302a..9986eca 100644 --- a/lib/omniauth-oauth2.rb +++ b/lib/omniauth-oauth2.rb @@ -1,4 +1,2 @@ -# frozen_string_literal: true - require "omniauth-oauth2/version" require "omniauth/strategies/oauth2" diff --git a/lib/omniauth-oauth2/version.rb b/lib/omniauth-oauth2/version.rb index b8d6e73..c2c7281 100644 --- a/lib/omniauth-oauth2/version.rb +++ b/lib/omniauth-oauth2/version.rb @@ -1,7 +1,5 @@ -# frozen_string_literal: true - module OmniAuth module OAuth2 - VERSION = "1.6.0" + VERSION = "1.6.0".freeze end end diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index b69e6eb..d47137a 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "oauth2" require "omniauth" require "securerandom" diff --git a/omniauth-oauth2.gemspec b/omniauth-oauth2.gemspec index f443431..20abb26 100644 --- a/omniauth-oauth2.gemspec +++ b/omniauth-oauth2.gemspec @@ -1,5 +1,3 @@ -# frozen_string_literal: true - lib = File.expand_path("../lib", __FILE__) $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) require "omniauth-oauth2/version" diff --git a/spec/helper.rb b/spec/helper.rb index 561e680..280be5f 100644 --- a/spec/helper.rb +++ b/spec/helper.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - $LOAD_PATH.unshift File.expand_path("..", __FILE__) $LOAD_PATH.unshift File.expand_path("../../lib", __FILE__) diff --git a/spec/omniauth/strategies/oauth2_spec.rb b/spec/omniauth/strategies/oauth2_spec.rb index f3c7d51..3b988f8 100644 --- a/spec/omniauth/strategies/oauth2_spec.rb +++ b/spec/omniauth/strategies/oauth2_spec.rb @@ -1,5 +1,3 @@ -# frozen_string_literal: true - require "helper" describe OmniAuth::Strategies::OAuth2 do From 53ade6bb2f8f01065b10a5edb2d4121538258615 Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Mon, 10 Aug 2020 20:22:59 -0400 Subject: [PATCH 4/5] Minor refactor of pkce --- lib/omniauth/strategies/oauth2.rb | 44 ++++++++++++++++++------------- 1 file changed, 25 insertions(+), 19 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index d47137a..3759cbf 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -30,6 +30,16 @@ def self.inherited(subclass) option :auth_token_params, {} option :provider_ignores_state, false option :pkce, false + option :pkce_verifier, nil + option :pkce_options, { + :code_challenge => proc { |verifier| + Base64.urlsafe_encode64( + Digest::SHA2.digest(verifier), + padding: false + ) + }, + :code_challenge_method => "S256" + } attr_accessor :access_token @@ -50,19 +60,20 @@ def request_phase end def authorize_params - verifier = SecureRandom.hex(64) - - pkce_authorize_params!(verifier) - options.authorize_params[:state] = SecureRandom.hex(24) - params = options.authorize_params.merge(options_for("authorize")) if OmniAuth.config.test_mode @env ||= {} @env["rack.session"] ||= {} end - build_authorize_session!(params, verifier) + params = options.authorize_params + .merge(options_for("authorize")) + .merge(pkce_authorize_params) + + session["omniauth.pkce.verifier"] = options.pkce_verifier if options.pkce + session["omniauth.state"] = params[:state] + params end @@ -91,21 +102,16 @@ def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexi protected - def build_authorize_session!(params, verifier) - session["omniauth.pkce.verifier"] = verifier if options.pkce - session["omniauth.state"] = params[:state] - end - - def pkce_authorize_params!(verifier) - return unless options.pkce + def pkce_authorize_params + return {} unless options.pkce + options.pkce_verifier = SecureRandom.hex(64) # NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A - challenge = Base64 - .urlsafe_encode64(Digest::SHA2.digest(verifier)) - .split("=") - .first - options.authorize_params[:code_challenge] = challenge - options.authorize_params[:code_challenge_method] = "S256" + { + :code_challenge => options.pkce_options[:code_challenge] + .call(options.pkce_verifier), + :code_challenge_method => options.pkce_options[:code_challenge_method] + } end def pkce_token_params From d41419421c6283af14c0025bf021a682f9c7874c Mon Sep 17 00:00:00 2001 From: Bobby McDonald Date: Tue, 11 Aug 2020 11:10:04 -0400 Subject: [PATCH 5/5] Fix rubocop warnings --- lib/omniauth/strategies/oauth2.rb | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/lib/omniauth/strategies/oauth2.rb b/lib/omniauth/strategies/oauth2.rb index 3759cbf..0c50bd6 100644 --- a/lib/omniauth/strategies/oauth2.rb +++ b/lib/omniauth/strategies/oauth2.rb @@ -35,10 +35,10 @@ def self.inherited(subclass) :code_challenge => proc { |verifier| Base64.urlsafe_encode64( Digest::SHA2.digest(verifier), - padding: false + :padding => false, ) }, - :code_challenge_method => "S256" + :code_challenge_method => "S256", } attr_accessor :access_token @@ -59,7 +59,7 @@ def request_phase redirect client.auth_code.authorize_url({:redirect_uri => callback_url}.merge(authorize_params)) end - def authorize_params + def authorize_params# rubocop:disable Metrics/AbcSize, Metrics/MethodLength options.authorize_params[:state] = SecureRandom.hex(24) if OmniAuth.config.test_mode @@ -104,13 +104,14 @@ def callback_phase # rubocop:disable Metrics/AbcSize, Metrics/CyclomaticComplexi def pkce_authorize_params return {} unless options.pkce + options.pkce_verifier = SecureRandom.hex(64) # NOTE: see https://tools.ietf.org/html/rfc7636#appendix-A { :code_challenge => options.pkce_options[:code_challenge] .call(options.pkce_verifier), - :code_challenge_method => options.pkce_options[:code_challenge_method] + :code_challenge_method => options.pkce_options[:code_challenge_method], } end