From 41d31abf5bfe32ed1fc26305737368ac2d3f9d64 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 17:12:42 -0700 Subject: [PATCH 01/26] Add .gitlab-ci.yml to test code --- .gitlab-ci.yml | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) create mode 100644 .gitlab-ci.yml diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml new file mode 100644 index 00000000..0313a624 --- /dev/null +++ b/.gitlab-ci.yml @@ -0,0 +1,27 @@ +.test-template: &test + cache: + paths: + - vendor/ruby + before_script: + - gem install bundler --no-document + - bundle config set --local path 'vendor/ruby' + - bundle install -j $(nproc) --path vendor # Install dependencies into ./vendor/ruby + - ruby -v # Print out ruby version for debugging + script: + - bundle exec rake test + +rspec-2.5: + image: "ruby:2.5" + <<: *test + +rspec-2.6: + image: "ruby:2.6" + <<: *test + +rspec-2.7: + image: "ruby:2.7" + <<: *test + +rspec-3.0: + image: "ruby:3.0" + <<: *test From 503cd73c319185344eea103493f0cf244f9077bd Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 17:15:26 -0700 Subject: [PATCH 02/26] Rename .gemspec and update home page --- ..._connect.gemspec => gitlab-omniauth-openid-connect.gemspec | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) rename omniauth_openid_connect.gemspec => gitlab-omniauth-openid-connect.gemspec (91%) diff --git a/omniauth_openid_connect.gemspec b/gitlab-omniauth-openid-connect.gemspec similarity index 91% rename from omniauth_openid_connect.gemspec rename to gitlab-omniauth-openid-connect.gemspec index dc0be0e9..41d7aa09 100644 --- a/omniauth_openid_connect.gemspec +++ b/gitlab-omniauth-openid-connect.gemspec @@ -5,13 +5,13 @@ $LOAD_PATH.unshift(lib) unless $LOAD_PATH.include?(lib) require 'omniauth/openid_connect/version' Gem::Specification.new do |spec| - spec.name = 'omniauth_openid_connect' + spec.name = 'gitlab-omniauth-openid-connect' spec.version = OmniAuth::OpenIDConnect::VERSION spec.authors = ['John Bohn', 'Ilya Shcherbinin'] spec.email = ['jjbohn@gmail.com', 'm0n9oose@gmail.com'] spec.summary = 'OpenID Connect Strategy for OmniAuth' spec.description = 'OpenID Connect Strategy for OmniAuth.' - spec.homepage = 'https://github.com/m0n9oose/omniauth_openid_connect' + spec.homepage = 'https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect' spec.license = 'MIT' spec.files = `git ls-files -z`.split("\x0") From 633bc202b465ec930167bcb27fd7404c8d355690 Mon Sep 17 00:00:00 2001 From: shmokmt <32533860+shmokmt@users.noreply.github.com> Date: Thu, 25 Mar 2021 14:27:02 +0900 Subject: [PATCH 03/26] Update dependencies --- gitlab-omniauth-openid-connect.gemspec | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/gitlab-omniauth-openid-connect.gemspec b/gitlab-omniauth-openid-connect.gemspec index 41d7aa09..9231b1f7 100644 --- a/gitlab-omniauth-openid-connect.gemspec +++ b/gitlab-omniauth-openid-connect.gemspec @@ -19,17 +19,17 @@ Gem::Specification.new do |spec| spec.test_files = spec.files.grep(%r{^(test|spec|features)/}) spec.require_paths = ['lib'] - spec.add_dependency 'addressable', '~> 2.5' + spec.add_dependency 'addressable', '~> 2.7' spec.add_dependency 'omniauth', '~> 1.9' - spec.add_dependency 'openid_connect', '~> 1.1' + spec.add_dependency 'openid_connect', '~> 1.2' spec.add_development_dependency 'coveralls', '~> 0.8' - spec.add_development_dependency 'faker', '~> 1.6' + spec.add_development_dependency 'faker', '~> 2.17' spec.add_development_dependency 'guard', '~> 2.14' - spec.add_development_dependency 'guard-bundler', '~> 2.2' + spec.add_development_dependency 'guard-bundler', '~> 3.0' spec.add_development_dependency 'guard-minitest', '~> 2.4' - spec.add_development_dependency 'minitest', '~> 5.1' - spec.add_development_dependency 'mocha', '~> 1.7' - spec.add_development_dependency 'rake', '~> 12.0' - spec.add_development_dependency 'rubocop', '~> 0.63' - spec.add_development_dependency 'simplecov', '~> 0.12' + spec.add_development_dependency 'minitest', '~> 5.14' + spec.add_development_dependency 'mocha', '~> 1.12' + spec.add_development_dependency 'rake', '~> 13.0' + spec.add_development_dependency 'rubocop', '~> 1.12' + spec.add_development_dependency 'simplecov', '~> 0.16' end From e05891d5eb04fa1f3e6fb7630ab9c8116a907b94 Mon Sep 17 00:00:00 2001 From: shmokmt <32533860+shmokmt@users.noreply.github.com> Date: Thu, 25 Mar 2021 14:27:25 +0900 Subject: [PATCH 04/26] Run test on 3.0 --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index b15dfe38..a3c33d87 100644 --- a/.travis.yml +++ b/.travis.yml @@ -4,5 +4,6 @@ rvm: - 2.5 - 2.6 - 2.7 + - 3.0 - jruby-head - ruby-head From 922ed8db7d98dfd92f96f84f6ff257f0fed60ed7 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 16:54:51 -0700 Subject: [PATCH 05/26] Fix handling of JWT without key ID When a key ID (`kid`) is not included in the JWT, that means we don't know anything about which signing key to use. The json-jwt library expects the `kid` value to be present if a JWK Set is presented. If a missing `kid` exception is raised, we now iterate through each key to find one that works. Closes https://github.com/m0n9oose/omniauth_openid_connect/issues/64 --- lib/omniauth/strategies/openid_connect.rb | 42 +++++++++++++++-- .../strategies/openid_connect_test.rb | 46 +++++++++++++++++++ test/strategy_test_case.rb | 15 +++++- 3 files changed, 98 insertions(+), 5 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index c7e86f70..abc93987 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -177,9 +177,13 @@ def authorize_uri end def public_key - return config.jwks if options.discovery - - key_or_secret + @public_key ||= begin + if options.discovery + config.jwks + else + key_or_secret + end + end end private @@ -226,7 +230,37 @@ def access_token end def decode_id_token(id_token) - ::OpenIDConnect::ResponseObject::IdToken.decode(id_token, public_key) + decode!(id_token, public_key) + rescue JSON::JWK::Set::KidNotFound + # Either the JWT doesn't have kid specified or the set of keys doesn't + # have a matching key. Since we can't tell the first case from the second, + # try each key individually to see if one works. + # https://github.com/nov/json-jwt/pull/92#issuecomment-824654949 + decoded = decode_with_each_key!(id_token) + + raise unless decoded + + decoded + end + + def decode!(id_token, key) + ::OpenIDConnect::ResponseObject::IdToken.decode(id_token, key) + end + + def decode_with_each_key!(id_token) + return unless public_key.is_a?(JSON::JWK::Set) + + public_key.each do |key| + begin + decoded = decode!(id_token, key) + rescue JSON::JWK::Set::KidNotFound, JSON::JWS::VerificationFailed + next + end + + return decoded if decoded + end + + nil end def client_options diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index b72f6ce0..e6d1cec5 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -222,6 +222,52 @@ def test_callback_phase_with_id_token strategy.callback_phase end + def test_callback_phase_with_id_token_no_kid + rsa_private = OpenSSL::PKey::RSA.generate(2048) + other_rsa_private = OpenSSL::PKey::RSA.generate(2048) + + key = JSON::JWK.new(rsa_private) + other_key = JSON::JWK.new(other_rsa_private) + token = JSON::JWT.new(payload).sign(rsa_private, :RS256).to_s + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => token, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.client_signing_alg = :RS256 + strategy.options.client_jwk_signing_key = { 'keys' => [other_key, key] }.to_json + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + + def test_callback_phase_with_id_token_no_matching_key + rsa_private = OpenSSL::PKey::RSA.generate(2048) + other_rsa_private = OpenSSL::PKey::RSA.generate(2048) + + nonce = SecureRandom.hex(16) + key = JSON::JWK.new(rsa_private) + other_key = JSON::JWK.new(other_rsa_private) + token = JSON::JWT.new(payload).sign(rsa_private, :RS256).to_s + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => token, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.client_signing_alg = :RS256 + strategy.options.client_jwk_signing_key = { 'keys' => [other_key] }.to_json + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + + assert_raises JSON::JWK::Set::KidNotFound do + strategy.callback_phase + end + end + def test_callback_phase_with_discovery code = SecureRandom.hex(16) state = SecureRandom.hex(16) diff --git a/test/strategy_test_case.rb b/test/strategy_test_case.rb index 6e7b15ef..8bda6af2 100644 --- a/test/strategy_test_case.rb +++ b/test/strategy_test_case.rb @@ -3,17 +3,30 @@ class DummyApp def call(env); end end - attr_accessor :identifier, :secret + attr_accessor :identifier, :secret, :issuer, :nonce def setup @identifier = '1234' @secret = '1234asdgat3' + @issuer = "https://server.example.com" + @nonce = SecureRandom.hex(16) end def client strategy.client end + def payload + { + "iss": issuer, + "aud": identifier, + "sub": "248289761001", + "nonce": nonce, + "exp": Time.now.to_i + 1000, + "iat": Time.now.to_i + 1000 + } + end + def user_info @user_info ||= OpenIDConnect::ResponseObject::UserInfo.new( sub: SecureRandom.hex(16), From 770661f3ba61baa9d6e0020d8cbb4b326b23a8ac Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 20:55:03 -0700 Subject: [PATCH 06/26] Clean up nonce creation in tests A random value is created in each test case, so we don't need to initialize this again. --- .../omniauth/strategies/openid_connect_test.rb | 15 --------------- 1 file changed, 15 deletions(-) diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index e6d1cec5..54cf7dd4 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -163,7 +163,6 @@ def test_uid def test_callback_phase(session = {}, params = {}) code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -195,7 +194,6 @@ def test_callback_phase(session = {}, params = {}) def test_callback_phase_with_id_token code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('id_token' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -247,7 +245,6 @@ def test_callback_phase_with_id_token_no_matching_key rsa_private = OpenSSL::PKey::RSA.generate(2048) other_rsa_private = OpenSSL::PKey::RSA.generate(2048) - nonce = SecureRandom.hex(16) key = JSON::JWK.new(rsa_private) other_key = JSON::JWK.new(other_rsa_private) token = JSON::JWT.new(payload).sign(rsa_private, :RS256).to_s @@ -271,7 +268,6 @@ def test_callback_phase_with_id_token_no_matching_key def test_callback_phase_with_discovery code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) jwks = JSON::JWK::Set.new(JSON.parse(File.read('test/fixtures/jwks.json'))['keys']) request.stubs(:params).returns('code' => code, 'state' => state) @@ -314,7 +310,6 @@ def test_callback_phase_with_discovery def test_callback_phase_with_error state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('error' => 'invalid_request') request.stubs(:path_info).returns('') @@ -326,7 +321,6 @@ def test_callback_phase_with_error def test_callback_phase_with_invalid_state code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => 'foobar') request.stubs(:path_info).returns('') @@ -337,7 +331,6 @@ def test_callback_phase_with_invalid_state def test_callback_phase_without_code state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('state' => state) request.stubs(:path_info).returns('') @@ -349,7 +342,6 @@ def test_callback_phase_without_code def test_callback_phase_without_id_token state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('state' => state) request.stubs(:path_info).returns('') strategy.options.response_type = 'id_token' @@ -362,7 +354,6 @@ def test_callback_phase_without_id_token def test_callback_phase_without_id_token_symbol state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('state' => state) request.stubs(:path_info).returns('') strategy.options.response_type = :id_token @@ -376,7 +367,6 @@ def test_callback_phase_without_id_token_symbol def test_callback_phase_with_timeout code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -396,7 +386,6 @@ def test_callback_phase_with_timeout def test_callback_phase_with_etimeout code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -416,7 +405,6 @@ def test_callback_phase_with_etimeout def test_callback_phase_with_socket_error code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -436,7 +424,6 @@ def test_callback_phase_with_socket_error def test_callback_phase_with_rack_oauth2_client_error code = SecureRandom.hex(16) state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) request.stubs(:params).returns('code' => code, 'state' => state) request.stubs(:path_info).returns('') @@ -565,7 +552,6 @@ def test_dynamic_state def test_option_client_auth_method state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) opts = strategy.options.client_options opts[:host] = 'foobar.com' @@ -628,7 +614,6 @@ def test_public_key_with_hmac def test_id_token_auth_hash state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) strategy.options.response_type = 'id_token' strategy.options.issuer = 'example.com' From ed647d03f3aa7a9cd292a29f886ef949e9e0b75d Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Tue, 20 Apr 2021 23:24:02 -0700 Subject: [PATCH 07/26] Fetch key from JWKS URI if available In non-standard OpenID Connect providers, such as Azure B2C, discovery does not work because the discovery URL does not match the issuer field. If a JWKS URI is provided when discovery is disabled, we should make an HTTP request for the keys and use the response. Closes https://github.com/m0n9oose/omniauth_openid_connect/issues/72 --- lib/omniauth/strategies/openid_connect.rb | 27 +++++++++------ .../strategies/openid_connect_test.rb | 33 +++++++++++++++++++ 2 files changed, 50 insertions(+), 10 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index abc93987..c39f584f 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -180,14 +180,20 @@ def public_key @public_key ||= begin if options.discovery config.jwks - else + elsif key_or_secret key_or_secret + elsif client_options.jwks_uri + fetch_key end end end private + def fetch_key + @fetch_key ||= parse_jwk_key(::OpenIDConnect.http_client.get_content(client_options.jwks_uri)) + end + def issuer resource = "#{ client_options.scheme }://#{ client_options.host }" resource = "#{ resource }:#{ client_options.port }" if client_options.port @@ -297,16 +303,17 @@ def session end def key_or_secret - case options.client_signing_alg - when :HS256, :HS384, :HS512 - client_options.secret - when :RS256, :RS384, :RS512 - if options.client_jwk_signing_key - parse_jwk_key(options.client_jwk_signing_key) - elsif options.client_x509_signing_key - parse_x509_key(options.client_x509_signing_key) + @key_or_secret ||= + case options.client_signing_alg + when :HS256, :HS384, :HS512 + client_options.secret + when :RS256, :RS384, :RS512 + if options.client_jwk_signing_key + parse_jwk_key(options.client_jwk_signing_key) + elsif options.client_x509_signing_key + parse_x509_key(options.client_x509_signing_key) + end end - end end def parse_x509_key(key) diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index 54cf7dd4..b3f56caa 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -308,6 +308,39 @@ def test_callback_phase_with_discovery strategy.callback_phase end + def test_callback_phase_with_jwks_uri + code = SecureRandom.hex(16) + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => code, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = 'example.com' + strategy.options.client_options.jwks_uri = 'https://jwks.example.com' + strategy.options.response_type = 'id_token' + + HTTPClient + .any_instance.stubs(:get_content) + .with(strategy.options.client_options.jwks_uri) + .returns(File.read('test/fixtures/jwks.json')) + + strategy.unstub(:user_info) + access_token = stub('OpenIDConnect::AccessToken') + access_token.stubs(:access_token) + access_token.stubs(:refresh_token) + access_token.stubs(:expires_in) + access_token.stubs(:scope) + access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + + id_token = stub('OpenIDConnect::ResponseObject::IdToken') + id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email') + id_token.stubs(:verify!).with(issuer: strategy.options.issuer, client_id: @identifier, nonce: nonce).returns(true) + ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token) + id_token.expects(:verify!) + + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + def test_callback_phase_with_error state = SecureRandom.hex(16) request.stubs(:params).returns('error' => 'invalid_request') From 5688347ea0a349c58ca2feb1532c7d26547f8afc Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 21:14:03 -0700 Subject: [PATCH 08/26] Update README.md with link to original project --- README.md | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index d74ba19e..214e5e61 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,9 @@ # OmniAuth::OpenIDConnect +This project was forked from +[m0n9oose/omniauth_openid_connect](https://github.com/m0n9oose/omniauth_openid_connect) +since a number of important bug fixes have not been merged in the past year. + Originally was [omniauth-openid-connect](https://github.com/jjbohn/omniauth-openid-connect) I've forked this repository and launch as separate gem because maintaining of original was dropped. @@ -10,7 +14,7 @@ I've forked this repository and launch as separate gem because maintaining of or Add this line to your application's Gemfile: - gem 'omniauth_openid_connect' + gem 'gitlab-omniauth-openid-connect', require: 'omniauth_openid_connect' And then execute: @@ -19,7 +23,7 @@ And then execute: Or install it yourself as: $ gem install omniauth_openid_connect - + ## Supported Ruby Versions OmniAuth::OpenIDConnect is tested under 2.4, 2.5, 2.6, 2.7 From 8b6c2fd02f3aed5abbca9a88ebfb95f43d8d48ef Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 21:32:18 -0700 Subject: [PATCH 09/26] Update CHANGELOG.md --- CHANGELOG.md | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f4d5d75..3ff2b0ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,8 @@ +# v0.4.0 (04.23.2021) + +- [Fetch key from JWKS URI if available](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/3) +- [Fix handling of JWT without key ID](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/2) + # v0.3.5 (07.06.2020) - bugfix: Info from decoded id_token is not exposed into `request.env['omniauth.auth']` [#61](https://github.com/m0n9oose/omniauth_openid_connect/pull/61) From 34dc46496ae8e5d71353d85c0f1954986ea395c8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 21:37:05 -0700 Subject: [PATCH 10/26] Release v0.4.0 --- CHANGELOG.md | 1 + lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ff2b0ac..f54a5db3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,7 @@ - [Fetch key from JWKS URI if available](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/3) - [Fix handling of JWT without key ID](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/2) +- [Add .gitlab-ci.yml and test with Ruby 3.0](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/1) # v0.3.5 (07.06.2020) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 9265aa02..ed83b8cf 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.3.5' + VERSION = '0.4.0' end end From bee781f32971d8482cd59d8f7484ad5664fb5b01 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 23 Apr 2021 22:17:32 -0700 Subject: [PATCH 11/26] Always convert client_signing_alg to be a symbol client_singing_alg could be passed in as a string, and we should make it possible to specify either a string or symbol. --- lib/omniauth/strategies/openid_connect.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index c39f584f..2475625d 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -304,7 +304,7 @@ def session def key_or_secret @key_or_secret ||= - case options.client_signing_alg + case options.client_signing_alg&.to_sym when :HS256, :HS384, :HS512 client_options.secret when :RS256, :RS384, :RS512 From faa8efa4936c91c91abb6dffc5d555edf7e80ba1 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Sun, 25 Apr 2021 11:42:08 -0700 Subject: [PATCH 12/26] SImplify error handling for decoding individual keys `JSON::JWK::Set::KidNotFound` should never be raised because we are checking individual keys within the set. --- lib/omniauth/strategies/openid_connect.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index c39f584f..4aa5c3be 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -259,7 +259,7 @@ def decode_with_each_key!(id_token) public_key.each do |key| begin decoded = decode!(id_token, key) - rescue JSON::JWK::Set::KidNotFound, JSON::JWS::VerificationFailed + rescue JSON::JWS::VerificationFailed next end From 41fa4757eefd8cc22f0226709811a134427e1dbd Mon Sep 17 00:00:00 2001 From: chandrn7 Date: Fri, 7 May 2021 20:57:46 +0000 Subject: [PATCH 13/26] Add email_verified field to info dict --- lib/omniauth/strategies/openid_connect.rb | 1 + test/lib/omniauth/strategies/openid_connect_test.rb | 1 + test/strategy_test_case.rb | 1 + 3 files changed, 3 insertions(+) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index f82c3003..9d07cf3c 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -65,6 +65,7 @@ def uid { name: user_info.name, email: user_info.email, + email_verified: user_info.email_verified, nickname: user_info.preferred_username, first_name: user_info.given_name, last_name: user_info.family_name, diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index b3f56caa..f26daf61 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -477,6 +477,7 @@ def test_info info = strategy.info assert_equal user_info.name, info[:name] assert_equal user_info.email, info[:email] + assert_equal user_info.email_verified, info[:email_verified] assert_equal user_info.preferred_username, info[:nickname] assert_equal user_info.given_name, info[:first_name] assert_equal user_info.family_name, info[:last_name] diff --git a/test/strategy_test_case.rb b/test/strategy_test_case.rb index 8bda6af2..480fb29f 100644 --- a/test/strategy_test_case.rb +++ b/test/strategy_test_case.rb @@ -32,6 +32,7 @@ def user_info sub: SecureRandom.hex(16), name: Faker::Name.name, email: Faker::Internet.email, + email_verified: Faker::Boolean.boolean, nickname: Faker::Name.first_name, preferred_username: Faker::Internet.user_name, given_name: Faker::Name.first_name, From 5a78d9b1fbe8d5b156c88e849a49b8c7e666e0d0 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 7 May 2021 13:59:33 -0700 Subject: [PATCH 14/26] Release v0.5.0 --- CHANGELOG.md | 6 ++++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 7 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f54a5db3..6c554482 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,9 @@ +# v0.5.0 (05.07.2021) + +- [Add email_verified field to info dict](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/7) +- [Simplify error handling for decoding individual keys](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/6) +- [Always convert client_signing_alg to be a symbol](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/5) + # v0.4.0 (04.23.2021) - [Fetch key from JWKS URI if available](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/3) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index ed83b8cf..a23721e9 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.4.0' + VERSION = '0.5.0' end end From 62adec295ad1ab3386087e8011f738294db4847a Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 8 Jul 2021 01:38:23 -0700 Subject: [PATCH 15/26] Support verification of HS256-signed JWTs When an OpenID provider such as Keycloak is configured to use the HS256 algorithm to signed JSON Web Tokens (JWTs), previously logins would fail with an obscure error: `JSON::JWS::UnexpectedAlgorithm (no implicit conversion of OpenSSL::PKey::RSA into String)`. The HS256 algorithm relies on a shared secret between the client and the server, and this secret is not in the list of JSON Web Key Set (JWKS) that is typically retrieved via a public endpoint during OpenID Discovery. The error was happening because decoding a HS256-signed JWT with public key RSA key will fail. We should only attempt to decode with the configured `client_options.secret`. To do this, we need to decode the JWT to examine the header to determine how it was signed. For example: ```json { "typ": "JWT", "alg": "RS256", "kid": "RrHQomcBaw2FJZ9Q5skkNaPC6sHJFLUAf4uoeaItDPE" } ``` This indicates that the payload was signed with `RS256`, a public key algorithm. Once we know the algorithm used to decode, we can determine which key to use. For public key algorithms such as `RS256`, we use the JWKS. For signature-based algoriths such as `HS256`, we use the configured client secret. This merge request also cleans up some technical debt. Previously we didn't peek at the unverified JWT to determine the key ID (`kid`) that the payload was signed with. If we got a `KidNotFound` exception from the decoding, previously we couldn't tell whether this was happening because we didn't have a matching key in JWKS, or whether the JWT didn't have a `kid` to begin with. Now that we decode the header, we can tell these cases apart. If the JWT has no `kid` and we get `KidNotFound` from decoding, we know the server didn't supply the right set of keys. Otherwise, we can just use try key until we find one that works. --- lib/omniauth/strategies/openid_connect.rb | 45 +++++-- test/fixtures/id_token.txt | 1 - .../strategies/openid_connect_test.rb | 117 ++++++++++++------ test/strategy_test_case.rb | 24 ++++ 4 files changed, 139 insertions(+), 48 deletions(-) delete mode 100644 test/fixtures/id_token.txt diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 9d07cf3c..6f62cf34 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -236,31 +236,55 @@ def access_token @access_token end + # Unlike ::OpenIDConnect::ResponseObject::IdToken.decode, this + # method splits the decoding and verification of JWT into two + # steps. First, we decode the JWT without verifying it to + # determine the algorithm used to sign. Then, we verify it using + # the appropriate public key (e.g. if algorithm is RS256) or + # shared secret (e.g. if algorithm is HS256). This works around a + # limitation in the openid_connect gem: + # https://github.com/nov/openid_connect/issues/61 def decode_id_token(id_token) - decode!(id_token, public_key) + decoded = JSON::JWT.decode(id_token, :skip_verification) + algorithm = decoded.algorithm.to_sym + + keyset = + case algorithm + when :RS256, :RS384, :RS512 + public_key + when :HS256, :HS384, :HS512 + client_options.secret + end + + decoded.verify!(keyset) + ::OpenIDConnect::ResponseObject::IdToken.new(decoded) rescue JSON::JWK::Set::KidNotFound - # Either the JWT doesn't have kid specified or the set of keys doesn't - # have a matching key. Since we can't tell the first case from the second, - # try each key individually to see if one works. + # If the JWT has a key ID (kid), then we know that the set of + # keys supplied doesn't contain the one we want, and we're + # done. However, if there is no kid, then we try each key + # individually to see if one works: # https://github.com/nov/json-jwt/pull/92#issuecomment-824654949 - decoded = decode_with_each_key!(id_token) + raise if decoded&.header&.key?('kid') + + decoded = decode_with_each_key!(id_token, keyset) raise unless decoded decoded + end def decode!(id_token, key) ::OpenIDConnect::ResponseObject::IdToken.decode(id_token, key) end - def decode_with_each_key!(id_token) - return unless public_key.is_a?(JSON::JWK::Set) + def decode_with_each_key!(id_token, keyset) + return unless keyset.is_a?(JSON::JWK::Set) - public_key.each do |key| + keyset.each do |key| begin decoded = decode!(id_token, key) - rescue JSON::JWS::VerificationFailed + rescue JSON::JWS::VerificationFailed, JSON::JWS::UnexpectedAlgorithm, JSON::JWS::UnknownAlgorithm next end @@ -304,7 +328,7 @@ def session end def key_or_secret - @key_or_secret ||= + @key_or_secret ||= begin case options.client_signing_alg&.to_sym when :HS256, :HS384, :HS512 client_options.secret @@ -315,6 +339,7 @@ def key_or_secret parse_x509_key(options.client_x509_signing_key) end end + end end def parse_x509_key(key) diff --git a/test/fixtures/id_token.txt b/test/fixtures/id_token.txt deleted file mode 100644 index 0afb3b9f..00000000 --- a/test/fixtures/id_token.txt +++ /dev/null @@ -1 +0,0 @@ -eyJhbGciOiJSUzI1NiIsImtpZCI6IjFlOWdkazcifQ.ewogImlzcyI6ICJodHRwOi8vc2VydmVyLmV4YW1wbGUuY29tIiwKICJzdWIiOiAiMjQ4Mjg5NzYxMDAxIiwKICJhdWQiOiAiczZCaGRSa3F0MyIsCiAibm9uY2UiOiAibi0wUzZfV3pBMk1qIiwKICJleHAiOiAxMzExMjgxOTcwLAogImlhdCI6IDEzMTEyODA5NzAKfQ.ggW8hZ1EuVLuxNuuIJKX_V8a_OMXzR0EHR9R6jgdqrOOF4daGU96Sr_P6qJp6IcmD3HP99Obi1PRs-cwh3LO-p146waJ8IhehcwL7F09JdijmBqkvPeB2T9CJNqeGpe-gccMg4vfKjkM8FcGvnzZUN4_KSP0aAp1tOJ1zZwgjxqGByKHiOtX7TpdQyHE5lcMiKPXfEIQILVq0pc_E2DzL7emopWoaoZTF_m0_N0YzFC6g6EJbOEoRoSK5hoDalrcvRYLSrQAZZKflyuVCyixEoV9GfNQC3_osjzw2PAithfubEEBLuVVk4XUVrWOLrLl0nx7RkKU8NXNHq-rvKMzqg diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index f26daf61..55c49d94 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -168,7 +168,7 @@ def test_callback_phase(session = {}, params = {}) strategy.options.issuer = 'example.com' strategy.options.client_signing_alg = :RS256 - strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json') + strategy.options.client_jwk_signing_key = jwks.to_s strategy.options.response_type = 'code' strategy.unstub(:user_info) @@ -177,7 +177,7 @@ def test_callback_phase(session = {}, params = {}) access_token.stubs(:refresh_token) access_token.stubs(:expires_in) access_token.stubs(:scope) - access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + access_token.stubs(:id_token).returns(jwt.to_s) client.expects(:access_token!).at_least_once.returns(access_token) access_token.expects(:userinfo!).returns(user_info) @@ -192,14 +192,13 @@ def test_callback_phase(session = {}, params = {}) end def test_callback_phase_with_id_token - code = SecureRandom.hex(16) state = SecureRandom.hex(16) - request.stubs(:params).returns('id_token' => code, 'state' => state) + request.stubs(:params).returns('id_token' => jwt.to_s, 'state' => state) request.stubs(:path_info).returns('') strategy.options.issuer = 'example.com' strategy.options.client_signing_alg = :RS256 - strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json') + strategy.options.client_jwk_signing_key = jwks.to_json strategy.options.response_type = 'id_token' strategy.unstub(:user_info) @@ -208,7 +207,7 @@ def test_callback_phase_with_id_token access_token.stubs(:refresh_token) access_token.stubs(:expires_in) access_token.stubs(:scope) - access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + access_token.stubs(:id_token).returns(jwt.to_s) id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email') @@ -221,14 +220,32 @@ def test_callback_phase_with_id_token end def test_callback_phase_with_id_token_no_kid - rsa_private = OpenSSL::PKey::RSA.generate(2048) other_rsa_private = OpenSSL::PKey::RSA.generate(2048) - key = JSON::JWK.new(rsa_private) + key = JSON::JWK.new(private_key) other_key = JSON::JWK.new(other_rsa_private) - token = JSON::JWT.new(payload).sign(rsa_private, :RS256).to_s state = SecureRandom.hex(16) - request.stubs(:params).returns('id_token' => token, 'state' => state) + request.stubs(:params).returns('id_token' => jwt.to_s, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.client_signing_alg = :RS256 + strategy.options.client_jwk_signing_key = { 'keys' => [other_key, key] }.to_json + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + + def test_callback_phase_with_id_token_with_kid + other_rsa_private = OpenSSL::PKey::RSA.generate(2048) + + key = JSON::JWK.new(private_key) + other_key = JSON::JWK.new(other_rsa_private) + state = SecureRandom.hex(16) + jwt_with_kid = JSON::JWT.new(payload).sign(key, :RS256) + request.stubs(:params).returns('id_token' => jwt_with_kid.to_s, 'state' => state) request.stubs(:path_info).returns('') strategy.options.issuer = issuer @@ -241,6 +258,45 @@ def test_callback_phase_with_id_token_no_kid strategy.callback_phase end + def test_callback_phase_with_id_token_with_kid_and_no_matching_kid + other_rsa_private = OpenSSL::PKey::RSA.generate(2048) + + key = JSON::JWK.new(private_key) + other_key = JSON::JWK.new(other_rsa_private) + state = SecureRandom.hex(16) + jwt_with_kid = JSON::JWT.new(payload).sign(key, :RS256) + request.stubs(:params).returns('id_token' => jwt_with_kid.to_s, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.client_signing_alg = :RS256 + # We use private_key here instead of the wrapped key, which contains a kid + strategy.options.client_jwk_signing_key = { 'keys' => [other_key, private_key] }.to_json + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + + assert_raises JSON::JWK::Set::KidNotFound do + strategy.callback_phase + end + end + + def test_callback_phase_with_id_token_with_hs256 + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => jwt_with_hs256.to_s, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.client_options.secret = hmac_secret + strategy.options.client_signing_alg = :HS256 + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + def test_callback_phase_with_id_token_no_matching_key rsa_private = OpenSSL::PKey::RSA.generate(2048) other_rsa_private = OpenSSL::PKey::RSA.generate(2048) @@ -266,11 +322,9 @@ def test_callback_phase_with_id_token_no_matching_key end def test_callback_phase_with_discovery - code = SecureRandom.hex(16) state = SecureRandom.hex(16) - jwks = JSON::JWK::Set.new(JSON.parse(File.read('test/fixtures/jwks.json'))['keys']) - request.stubs(:params).returns('code' => code, 'state' => state) + request.stubs(:params).returns('code' => jwt.to_s, 'state' => state) request.stubs(:path_info).returns('') strategy.options.client_options.host = 'example.com' @@ -285,7 +339,7 @@ def test_callback_phase_with_discovery config.stubs(:token_endpoint).returns('https://example.com/token') config.stubs(:userinfo_endpoint).returns('https://example.com/userinfo') config.stubs(:jwks_uri).returns('https://example.com/jwks') - config.stubs(:jwks).returns(jwks) + config.stubs(:jwks).returns(JSON::JWK::Set.new(jwks['keys'])) ::OpenIDConnect::Discovery::Provider::Config.stubs(:discover!).with('https://example.com/').returns(config) @@ -300,7 +354,7 @@ def test_callback_phase_with_discovery access_token.stubs(:refresh_token) access_token.stubs(:expires_in) access_token.stubs(:scope) - access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + access_token.stubs(:id_token).returns(jwt.to_s) client.expects(:access_token!).at_least_once.returns(access_token) access_token.expects(:userinfo!).returns(user_info) @@ -309,9 +363,9 @@ def test_callback_phase_with_discovery end def test_callback_phase_with_jwks_uri - code = SecureRandom.hex(16) + id_token = jwt.to_s state = SecureRandom.hex(16) - request.stubs(:params).returns('id_token' => code, 'state' => state) + request.stubs(:params).returns('id_token' => id_token, 'state' => state) request.stubs(:path_info).returns('') strategy.options.issuer = 'example.com' @@ -321,7 +375,7 @@ def test_callback_phase_with_jwks_uri HTTPClient .any_instance.stubs(:get_content) .with(strategy.options.client_options.jwks_uri) - .returns(File.read('test/fixtures/jwks.json')) + .returns(jwks.to_json) strategy.unstub(:user_info) access_token = stub('OpenIDConnect::AccessToken') @@ -329,7 +383,7 @@ def test_callback_phase_with_jwks_uri access_token.stubs(:refresh_token) access_token.stubs(:expires_in) access_token.stubs(:scope) - access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + access_token.stubs(:id_token).returns(id_token) id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:raw_attributes).returns('sub' => 'sub', 'name' => 'name', 'email' => 'email') @@ -494,7 +548,7 @@ def test_extra def test_credentials strategy.options.issuer = 'example.com' strategy.options.client_signing_alg = :RS256 - strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json') + strategy.options.client_jwk_signing_key = jwks.to_json id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:verify!).returns(true) @@ -505,7 +559,7 @@ def test_credentials access_token.stubs(:refresh_token).returns(SecureRandom.hex(16)) access_token.stubs(:expires_in).returns(Time.now) access_token.stubs(:scope).returns('openidconnect') - access_token.stubs(:id_token).returns(File.read('test/fixtures/id_token.txt')) + access_token.stubs(:id_token).returns(jwt.to_s) client.expects(:access_token!).returns(access_token) access_token.expects(:refresh_token).returns(access_token.refresh_token) @@ -592,11 +646,11 @@ def test_option_client_auth_method strategy.options.issuer = 'foobar.com' strategy.options.client_auth_method = :not_basic strategy.options.client_signing_alg = :RS256 - strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json') + strategy.options.client_jwk_signing_key = jwks.to_json json_response = { access_token: 'test_access_token', - id_token: File.read('test/fixtures/id_token.txt'), + id_token: jwt.to_s, token_type: 'Bearer', }.to_json success = Struct.new(:status, :body).new(200, json_response) @@ -619,16 +673,14 @@ def test_option_client_auth_method def test_public_key_with_jwks strategy.options.client_signing_alg = :RS256 - strategy.options.client_jwk_signing_key = File.read('./test/fixtures/jwks.json') + strategy.options.client_jwk_signing_key = jwks.to_json assert_equal JSON::JWK::Set, strategy.public_key.class end def test_public_key_with_jwk strategy.options.client_signing_alg = :RS256 - jwks_str = File.read('./test/fixtures/jwks.json') - jwks = JSON.parse(jwks_str) - jwk = jwks['keys'].first + jwk = jwks[:keys].first strategy.options.client_jwk_signing_key = jwk.to_json assert_equal JSON::JWK, strategy.public_key.class @@ -653,16 +705,7 @@ def test_id_token_auth_hash id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:verify!).returns(true) - id_token.stubs(:raw_attributes, :to_h).returns( - { - "iss": "http://server.example.com", - "sub": "248289761001", - "aud": "s6BhdRkqt3", - "nonce": "n-0S6_WzA2Mj", - "exp": 1311281970, - "iat": 1311280970, - } - ) + id_token.stubs(:raw_attributes, :to_h).returns(payload) request.stubs(:params).returns('state' => state, 'nounce' => nonce, 'id_token' => id_token) request.stubs(:path_info).returns('') diff --git a/test/strategy_test_case.rb b/test/strategy_test_case.rb index 480fb29f..f073f043 100644 --- a/test/strategy_test_case.rb +++ b/test/strategy_test_case.rb @@ -27,6 +27,30 @@ def payload } end + def private_key + @private_key ||= OpenSSL::PKey::RSA.generate(512) + end + + def jwt + @jwt ||= JSON::JWT.new(payload).sign(private_key, :RS256) + end + + def hmac_secret + @hmac_secret ||= SecureRandom.hex(16) + end + + def jwt_with_hs256 + @jwt_with_hs256 ||= JSON::JWT.new(payload).sign(hmac_secret, :HS256) + end + + def jwks + @jwks ||= begin + key = JSON::JWK.new(private_key) + keyset = JSON::JWK::Set.new(key) + { keys: keyset } + end + end + def user_info @user_info ||= OpenIDConnect::ResponseObject::UserInfo.new( sub: SecureRandom.hex(16), From cede014aa9a3cc739dfc050f8651eaea5c4bff4e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Thu, 8 Jul 2021 13:01:34 -0700 Subject: [PATCH 16/26] Release v0.6.0 --- CHANGELOG.md | 4 ++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 6c554482..f284d254 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# v0.6.0 (07.08.2021) + +- [Support verification of HS256-signed JWTs](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/8) + # v0.5.0 (05.07.2021) - [Add email_verified field to info dict](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/7) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index a23721e9..7ff83592 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.5.0' + VERSION = '0.6.0' end end From 5349818fb7ebe8596d229ad581d653525da04e0b Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 16 Jul 2021 11:16:05 -0700 Subject: [PATCH 17/26] Add option for specifying jwt_secret Keycloak doesn't use the OAuth2 client secret to encrypt JWT secrets. We need a separate secret to verify the JWT tokens. This commit also cleans up some code and eliminates the need for using `client_signing_alg`, since we peek at the JWT header to decide how to decode the token. --- README.md | 1 + lib/omniauth/strategies/openid_connect.rb | 32 ++++++++++--------- .../strategies/openid_connect_test.rb | 18 +++++++++-- 3 files changed, 34 insertions(+), 17 deletions(-) diff --git a/README.md b/README.md index 214e5e61..70ad9bc6 100644 --- a/README.md +++ b/README.md @@ -66,6 +66,7 @@ config.omniauth :openid_connect, { | post_logout_redirect_uri | The logout redirect uri to use per the [session management draft](https://openid.net/specs/openid-connect-session-1_0.html) | no | empty | https://myapp.com/logout/callback | | uid_field | The field of the user info response to be used as a unique id | no | 'sub' | "sub", "preferred_username" | | client_options | A hash of client options detailed in its own section | yes | | | +| jwt_secret | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. | ### Client Config Options diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 6f62cf34..354eefe5 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -36,7 +36,8 @@ class OpenIDConnect option :issuer option :discovery, false - option :client_signing_alg + option :client_signing_alg # Deprecated since we detect what is used to sign the JWT + option :jwt_secret option :client_jwk_signing_key option :client_x509_signing_key option :scope, [:openid] @@ -181,14 +182,20 @@ def public_key @public_key ||= begin if options.discovery config.jwks - elsif key_or_secret - key_or_secret + elsif configured_public_key + configured_public_key elsif client_options.jwks_uri fetch_key end end end + # Some OpenID providers use the OAuth2 client secret as the shared secret, but + # Keycloak uses a separate key that's stored inside the database. + def secret + options.jwt_secret || client_options.secret + end + private def fetch_key @@ -253,7 +260,7 @@ def decode_id_token(id_token) when :RS256, :RS384, :RS512 public_key when :HS256, :HS384, :HS512 - client_options.secret + secret end decoded.verify!(keyset) @@ -327,17 +334,12 @@ def session super end - def key_or_secret - @key_or_secret ||= begin - case options.client_signing_alg&.to_sym - when :HS256, :HS384, :HS512 - client_options.secret - when :RS256, :RS384, :RS512 - if options.client_jwk_signing_key - parse_jwk_key(options.client_jwk_signing_key) - elsif options.client_x509_signing_key - parse_x509_key(options.client_x509_signing_key) - end + def configured_public_key + @configured_public_key ||= begin + if options.client_jwk_signing_key + parse_jwk_key(options.client_jwk_signing_key) + elsif options.client_x509_signing_key + parse_x509_key(options.client_x509_signing_key) end end end diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index 55c49d94..f8204b12 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -297,6 +297,20 @@ def test_callback_phase_with_id_token_with_hs256 strategy.callback_phase end + def test_callback_phase_with_hs256_jwt_secret + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => jwt_with_hs256.to_s, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.jwt_secret = hmac_secret + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + def test_callback_phase_with_id_token_no_matching_key rsa_private = OpenSSL::PKey::RSA.generate(2048) other_rsa_private = OpenSSL::PKey::RSA.generate(2048) @@ -692,10 +706,10 @@ def test_public_key_with_x509 assert_equal OpenSSL::PKey::RSA, strategy.public_key.class end - def test_public_key_with_hmac + def test_secret_with_hmac strategy.options.client_options.secret = 'secret' strategy.options.client_signing_alg = :HS256 - assert_equal strategy.options.client_options.secret, strategy.public_key + assert_equal strategy.options.client_options.secret, strategy.secret end def test_id_token_auth_hash From 624b459e6c3c660206881e77bb6c58de97bbe801 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 16 Jul 2021 11:29:56 -0700 Subject: [PATCH 18/26] Release v0.7.0 --- CHANGELOG.md | 4 ++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f284d254..3bdd4623 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# v0.7.0 (07.16.2021) + +- [Add `jwt_secret` option to support Keycloak private key](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/10) + # v0.6.0 (07.08.2021) - [Support verification of HS256-signed JWTs](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/8) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 7ff83592..5d3ac85e 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.6.0' + VERSION = '0.7.0' end end From fb59cf0a3c64bc604360c7183290a527c211a960 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 16 Jul 2021 13:33:49 -0700 Subject: [PATCH 19/26] Add base64-encoded jwt_secret option For binary secrets as used by Keycloak, we need to encode the secret in base64. --- README.md | 3 ++- lib/omniauth/strategies/openid_connect.rb | 10 +++++++++- .../lib/omniauth/strategies/openid_connect_test.rb | 14 ++++++++++++++ 3 files changed, 25 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 70ad9bc6..1a6a8ce8 100644 --- a/README.md +++ b/README.md @@ -66,7 +66,8 @@ config.omniauth :openid_connect, { | post_logout_redirect_uri | The logout redirect uri to use per the [session management draft](https://openid.net/specs/openid-connect-session-1_0.html) | no | empty | https://myapp.com/logout/callback | | uid_field | The field of the user info response to be used as a unique id | no | 'sub' | "sub", "preferred_username" | | client_options | A hash of client options detailed in its own section | yes | | | -| jwt_secret | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. | +| jwt_secret | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. For secrets in binary, use `jwt_secret_base64`. | +| jwt_secret_base64 | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the base64-encoded secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. | ### Client Config Options diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 354eefe5..8c872a39 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -1,6 +1,7 @@ # frozen_string_literal: true require 'addressable/uri' +require 'base64' require 'timeout' require 'net/http' require 'open-uri' @@ -38,6 +39,7 @@ class OpenIDConnect option :discovery, false option :client_signing_alg # Deprecated since we detect what is used to sign the JWT option :jwt_secret + option :jwt_secret_base64 option :client_jwk_signing_key option :client_x509_signing_key option :scope, [:openid] @@ -193,11 +195,17 @@ def public_key # Some OpenID providers use the OAuth2 client secret as the shared secret, but # Keycloak uses a separate key that's stored inside the database. def secret - options.jwt_secret || client_options.secret + options.jwt_secret || base64_decoded_jwt_secret || client_options.secret end private + def base64_decoded_jwt_secret + return unless options.jwt_secret_base64 + + Base64.decode64(options.jwt_secret_base64) + end + def fetch_key @fetch_key ||= parse_jwk_key(::OpenIDConnect.http_client.get_content(client_options.jwks_uri)) end diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index f8204b12..483b31cb 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -311,6 +311,20 @@ def test_callback_phase_with_hs256_jwt_secret strategy.callback_phase end + def test_callback_phase_with_hs256_base64_jwt_secret + state = SecureRandom.hex(16) + request.stubs(:params).returns('id_token' => jwt_with_hs256.to_s, 'state' => state) + request.stubs(:path_info).returns('') + + strategy.options.issuer = issuer + strategy.options.jwt_secret_base64 = Base64.encode64(hmac_secret) + strategy.options.response_type = 'id_token' + + strategy.unstub(:user_info) + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + end + def test_callback_phase_with_id_token_no_matching_key rsa_private = OpenSSL::PKey::RSA.generate(2048) other_rsa_private = OpenSSL::PKey::RSA.generate(2048) From 74aa517fed3cc3e4d20448a94d935778f8fd3197 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 16 Jul 2021 14:13:46 -0700 Subject: [PATCH 20/26] Release v0.8.0 --- CHANGELOG.md | 4 ++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3bdd4623..b2a115e2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# v0.8.0 (07.16.2021) + +- [Add `jwt_secret_base64` option to support binary secrets](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/12) + # v0.7.0 (07.16.2021) - [Add `jwt_secret` option to support Keycloak private key](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/10) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 5d3ac85e..2305938d 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.7.0' + VERSION = '0.8.0' end end From 83542a1139be9ef04d82033c1df1bed7750c2f0f Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Fri, 16 Jul 2021 14:40:15 -0700 Subject: [PATCH 21/26] Fix jwt_secret documentation --- README.md | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/README.md b/README.md index 1a6a8ce8..64d2cc6c 100644 --- a/README.md +++ b/README.md @@ -66,8 +66,8 @@ config.omniauth :openid_connect, { | post_logout_redirect_uri | The logout redirect uri to use per the [session management draft](https://openid.net/specs/openid-connect-session-1_0.html) | no | empty | https://myapp.com/logout/callback | | uid_field | The field of the user info response to be used as a unique id | no | 'sub' | "sub", "preferred_username" | | client_options | A hash of client options detailed in its own section | yes | | | -| jwt_secret | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. For secrets in binary, use `jwt_secret_base64`. | -| jwt_secret_base64 | no | client_options.secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the base64-encoded secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. | +| jwt_secret | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. For secrets in binary, use `jwt_secret_base64`. | no | client_options.secret | "mysecret" | +| jwt_secret_base64 | For HMAC with SHA2 (e.g. HS256) signing algorithms, specify the base64-encoded secret used to sign the JWT token. Defaults to the OAuth2 client secret if not specified. `jwt_secret` takes precedence. | no | client_options.secret | "bXlzZWNyZXQ=\n" ### Client Config Options From d9fbe7dca261975004f37cb27ba4d9e3e3fe641e Mon Sep 17 00:00:00 2001 From: Bronwyn Barnett Date: Mon, 15 Nov 2021 19:44:11 +0000 Subject: [PATCH 22/26] Adding CONTRIBUTING.md after license audit review --- CONTRIBUTING.md | 40 ++++++++++++++++++++++++++++++++++++++++ 1 file changed, 40 insertions(+) create mode 100644 CONTRIBUTING.md diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md new file mode 100644 index 00000000..47ac4043 --- /dev/null +++ b/CONTRIBUTING.md @@ -0,0 +1,40 @@ +## Developer Certificate of Origin and License + +By contributing to GitLab B.V., you accept and agree to the following terms and +conditions for your present and future contributions submitted to GitLab B.V. +Except for the license granted herein to GitLab B.V. and recipients of software +distributed by GitLab B.V., you reserve all right, title, and interest in and to +your Contributions. + +All contributions are subject to the Developer Certificate of Origin and license set out at [docs.gitlab.com/ce/legal/developer_certificate_of_origin](https://docs.gitlab.com/ce/legal/developer_certificate_of_origin). + +_This notice should stay as the first item in the CONTRIBUTING.md file._ + +## Code of conduct + +As contributors and maintainers of this project, we pledge to respect all people +who contribute through reporting issues, posting feature requests, updating +documentation, submitting pull requests or patches, and other activities. + +We are committed to making participation in this project a harassment-free +experience for everyone, regardless of level of experience, gender, gender +identity and expression, sexual orientation, disability, personal appearance, +body size, race, ethnicity, age, or religion. + +Examples of unacceptable behavior by participants include the use of sexual +language or imagery, derogatory comments or personal attacks, trolling, public +or private harassment, insults, or other unprofessional conduct. + +Project maintainers have the right and responsibility to remove, edit, or reject +comments, commits, code, wiki edits, issues, and other contributions that are +not aligned to this Code of Conduct. Project maintainers who do not follow the +Code of Conduct may be removed from the project team. + +This code of conduct applies both within project spaces and in public spaces +when an individual is representing the project or its community. + +Instances of abusive, harassing, or otherwise unacceptable behavior can be +reported by emailing contact@gitlab.com. + +This Code of Conduct is adapted from the [Contributor Covenant](https://contributor-covenant.org), version 1.1.0, +available at [https://contributor-covenant.org/version/1/1/0/](https://contributor-covenant.org/version/1/1/0/). From 0de7c5b83a31387fad11df0968676640c364425d Mon Sep 17 00:00:00 2001 From: George Stockfisch Date: Mon, 3 Jan 2022 14:56:10 -0500 Subject: [PATCH 23/26] add support for ES[256|384|512|256K] algos --- lib/omniauth/strategies/openid_connect.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 8c872a39..1c6af686 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -265,7 +265,7 @@ def decode_id_token(id_token) keyset = case algorithm - when :RS256, :RS384, :RS512 + when :ES256, :ES384, :ES512, :ES256K, :RS256, :RS384, :RS512 public_key when :HS256, :HS384, :HS512 secret From 8072b5665367da0a7838db96794f4d005a7bd4c8 Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 3 Jan 2022 21:04:18 -0800 Subject: [PATCH 24/26] Release v0.9.0 --- CHANGELOG.md | 4 ++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index b2a115e2..03b8a447 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# v0.9.0 (01.03.2022) + +- [Add support for ES[256|384|512|256K] algorithms](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/17) + # v0.8.0 (07.16.2021) - [Add `jwt_secret_base64` option to support binary secrets](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/12) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 2305938d..2cb86a08 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.8.0' + VERSION = '0.9.0' end end From ff5417de84bbe43b61800b697c97899572cb5b4e Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 3 Jan 2022 21:16:53 -0800 Subject: [PATCH 25/26] Assume public key encryption unless HMAC is specified As described in https://github.com/nov/openid_connect/issues/61, `OpenIDConnect::ResponseObject::IdToken.decode` assumes public-key encryption and does not make it possible to verify a token signed with a HS256. The update to the gem to distinguish between RSA keys and HS256 keys created an issue where other public-key algorithms (e.g. ECSDA) weren't recognized. We simplify this code by using the private key only when we detect an HMAC type. --- lib/omniauth/strategies/openid_connect.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 1c6af686..74c43985 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -265,10 +265,10 @@ def decode_id_token(id_token) keyset = case algorithm - when :ES256, :ES384, :ES512, :ES256K, :RS256, :RS384, :RS512 - public_key when :HS256, :HS384, :HS512 secret + else + public_key end decoded.verify!(keyset) From bb6206a9b039b563cd4b758eef54c6b70051e0fa Mon Sep 17 00:00:00 2001 From: Stan Hu Date: Mon, 3 Jan 2022 21:21:30 -0800 Subject: [PATCH 26/26] Release v0.9.1 --- CHANGELOG.md | 4 ++++ lib/omniauth/openid_connect/version.rb | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 03b8a447..e435df64 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,3 +1,7 @@ +# v0.9.1 (01.03.2022) + +- [Assume public key encryption unless HMAC is specified](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/19) + # v0.9.0 (01.03.2022) - [Add support for ES[256|384|512|256K] algorithms](https://gitlab.com/gitlab-org/gitlab-omniauth-openid-connect/-/merge_requests/17) diff --git a/lib/omniauth/openid_connect/version.rb b/lib/omniauth/openid_connect/version.rb index 2cb86a08..ddd8f5e1 100644 --- a/lib/omniauth/openid_connect/version.rb +++ b/lib/omniauth/openid_connect/version.rb @@ -2,6 +2,6 @@ module OmniAuth module OpenIDConnect - VERSION = '0.9.0' + VERSION = '0.9.1'.freeze end end