diff --git a/.gitignore b/.gitignore index a93278e8..00e91217 100644 --- a/.gitignore +++ b/.gitignore @@ -18,3 +18,6 @@ tmp .ruby-version .ruby-gemset Gemfile.lock + +# Backup files +*~ diff --git a/README.md b/README.md index de34f212..4fbaef7e 100644 --- a/README.md +++ b/README.md @@ -54,7 +54,7 @@ config.omniauth :openid_connect, { | discovery | Should OpenID discovery be used. This is recommended if the IDP provides a discovery endpoint. See client config for how to manually enter discovered values. | no | false | one of: true, false | | client_auth_method | Which authentication method to use to authenticate your app with the authorization server | no | Sym: basic | "basic", "jwks" | | scope | Which OpenID scopes to include (:openid is always required) | no | Array [:openid] | [:openid, :profile, :email] | -| response_type | Which OAuth2 response type to use with the authorization request | no | String: code | one of: 'code', 'id_token' | +| `response_type` | Which OAuth2 response type to use with the authorization request | no | String: code | one of: 'code', 'id_token token'
**Security note:** Do not use 'token' or 'id_token'. The 'token' (raw OAuth 2.0) MUST NOT be used for the authentication purpose. The 'id_token' is used only for the Self-Issued OpenID Providers. Instead, use 'id_token token' (the Implicit Flow). | | state | A value to be used for the OAuth2 state parameter on the authorization request. Can be a proc that generates a string. | no | Random 16 character string | Proc.new { SecureRandom.hex(32) } | | response_mode | The response mode per [spec](https://openid.net/specs/oauth-v2-form-post-response-mode-1_0.html) | no | nil | one of: :query, :fragment, :form_post, :web_message | | display | An optional parameter to the authorization request to determine how the authorization and consent page | no | nil | one of: :page, :popup, :touch, :wap | diff --git a/lib/omniauth/openid_connect.rb b/lib/omniauth/openid_connect.rb index 6158e0f0..da712fc1 100644 --- a/lib/omniauth/openid_connect.rb +++ b/lib/omniauth/openid_connect.rb @@ -2,4 +2,4 @@ require 'omniauth/openid_connect/errors' require 'omniauth/openid_connect/version' -require 'omniauth/strategies/openid_connect' +require 'omniauth/openid_connect/util' diff --git a/lib/omniauth/openid_connect/util.rb b/lib/omniauth/openid_connect/util.rb new file mode 100644 index 00000000..ff4fa1fa --- /dev/null +++ b/lib/omniauth/openid_connect/util.rb @@ -0,0 +1,46 @@ +# frozen_string_literal: true + +module OmniAuth + module OpenIDConnect + module Util + # @param [String or Hash] key_or_hash Certificate data in PEM format + # @raise [OpenSSL::X509::CertificateError] Certificate format is incorrect + def self.parse_x509_key(key_or_hash, kid) + raise TypeError if !key_or_hash.is_a?(String) && !key_or_hash.is_a?(Hash) + + if key_or_hash.is_a?(Hash) + # https://www.googleapis.com/oauth2/v1/certs format + raise TypeError unless kid + + key_or_hash.each do |key, pem| + return OpenSSL::X509::Certificate.new(pem).public_key if key == kid + end + raise ArgumentError, "missing kid: #{kid}" + else + OpenSSL::X509::Certificate.new(key_or_hash).public_key + end + end + + # Decode JSON Web Key (JWK) or JWK Set format. + # See RFC 7517 + # @param [String or Hash] key_or_hash JSON-formatted string, or a hash. + # Sample: https://www.googleapis.com/oauth2/v3/certs + def self.parse_jwk_key(key_or_hash, _kid) + case key_or_hash + when String + json = JSON.parse(key_or_hash) + when Hash + json = key_or_hash + else + raise TypeError, "key was #{key_or_hash.class}, #{key_or_hash.inspect}" + end + + if json.key?('keys') + JSON::JWK::Set.new json['keys'] + else + JSON::JWK.new json + end + end + end + end +end diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 9a530250..8a19cf5a 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -37,10 +37,13 @@ class OpenIDConnect option :issuer option :discovery, false option :client_signing_alg + # Required if you set 'discovery:false'. + # IdP's public keys. NOT client's. option :client_jwk_signing_key option :client_x509_signing_key + option :scope, [:openid] - option :response_type, 'code' # ['code', 'id_token'] + option :response_type, 'code' # 'code', ['id_token', 'token'] option :state option :response_mode # [:query, :fragment, :form_post, :web_message] option :display, nil # [:page, :popup, :touch, :wap] @@ -98,6 +101,23 @@ def config @config ||= ::OpenIDConnect::Discovery::Provider::Config.discover!(options.issuer) end + # @override + # Called before both of request_phase and callback_phase + def setup_phase + super + + if configured_response_type != 'code' && + configured_response_type != 'id_token token' + raise ArgumentError, 'Not supported response_type' + end + + if configured_response_type == 'id_token token' && + client_options.secret + raise ArgumentError, 'MUST NOT set client_secret on the Implicit Flow' + end + end + + # @override def request_phase options.issuer = issuer if options.issuer.to_s.empty? discover! @@ -112,15 +132,15 @@ def callback_phase raise CallbackError, error: params['error'], reason: error_description, uri: params['error_uri'] if error raise CallbackError, error: :csrf_detected, reason: "Invalid 'state' parameter" if invalid_state - return unless valid_response_type? + return if configured_response_type == 'code' && !valid_response_type? options.issuer = issuer if options.issuer.nil? || options.issuer.empty? - verify_id_token!(params['id_token']) if configured_response_type == 'id_token' + verify_id_token!(params['id_token']) if configured_response_type == 'id_token token' discover! client.redirect_uri = redirect_uri - return id_token_callback_phase if configured_response_type == 'id_token' + return implicit_flow_callback_phase if configured_response_type == 'id_token token' client.authorization_code = authorization_code access_token @@ -181,10 +201,19 @@ def authorize_uri client.authorization_uri(opts.reject { |_k, v| v.nil? }) end - def public_key + # @return [JSON::JWK::Set or JSON::JWK] IdP's RSA public keys. NOT client's. + def public_key(kid = nil) + # [Security issue] Do not call key_or_secret() here. + return config.jwks if options.discovery - key_or_secret || config.jwks + return OmniAuth::OpenIDConnect::Util.parse_jwk_key(options.client_jwk_signing_key, kid) if options.client_jwk_signing_key + + if options.client_x509_signing_key + return OmniAuth::OpenIDConnect::Util.parse_x509_key(options.client_x509_signing_key, kid) + end + + raise 'internal error: missing RSA public key' end private @@ -273,34 +302,22 @@ def session super end - def key_or_secret - case options.client_signing_alg + # For HMAC-SHA256, return client_secret as the common key. + # For RSA, return the public key of the authentication server + def key_or_secret(header = nil) + raise TypeError if header && !header.respond_to?(:[]) + + case header ? header['alg'].to_sym : 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 + public_key(header['kid']) + else + # ES256 : ECDSA using P-256 curve and SHA-256 hash + raise ArgumentError, "unsupported alg: #{header['alg']}" end end - def parse_x509_key(key) - OpenSSL::X509::Certificate.new(key).public_key - end - - def parse_jwk_key(key) - json = JSON.parse(key) - return JSON::JWK::Set.new(json['keys']) if json.key?('keys') - - JSON::JWK.new(json) - end - - def decode(str) - UrlSafeBase64.decode64(str).unpack1('B*').to_i(2).to_s - end - def redirect_uri return client_options.redirect_uri unless params['redirect_uri'] @@ -324,7 +341,21 @@ def logout_path_pattern @logout_path_pattern ||= %r{\A#{Regexp.quote(request_path)}(/logout)} end - def id_token_callback_phase + # The Implicit Flow: + # Get an access token at the same time as id_token. There is a risk that + # one of them has been tampered with. (Token Hijacking) + # For that reason, + # (1) You MUST verify the signature of the id_token by the public key of + # IdP. Instead of choosing the key with the response header, you have + # to use always the public key. + # (2) The access token must be validated by the id_token. + def implicit_flow_callback_phase + if !params['access_token'] || !params['id_token'] + fail! :missing_id_token, + OmniAuth::OpenIDConnect::MissingIdTokenError.new(params['error']) + return + end + user_data = decode_id_token(params['id_token']).raw_attributes env['omniauth.auth'] = AuthHash.new( provider: name, @@ -335,6 +366,7 @@ def id_token_callback_phase call_app! end + # Called only from callback_phase() def valid_response_type? return true if params.key?(configured_response_type) @@ -344,8 +376,18 @@ def valid_response_type? false end + # Normalize options.response_type. + # @return [String] 'code' or 'id_token token' def configured_response_type - @configured_response_type ||= options.response_type.to_s + unless @configured_response_type + ary = case options.response_type + when Array then options.response_type + when Symbol then [options.response_type] + else options.response_type.split(/[ \t]+/) + end + @configured_response_type = ary.sort.join(' ') + end + @configured_response_type end def verify_id_token!(id_token) diff --git a/lib/omniauth_openid_connect.rb b/lib/omniauth_openid_connect.rb index e04c2d37..0575a3dc 100644 --- a/lib/omniauth_openid_connect.rb +++ b/lib/omniauth_openid_connect.rb @@ -1,3 +1,4 @@ # frozen_string_literal: true require 'omniauth/openid_connect' +require 'omniauth/strategies/openid_connect' diff --git a/test/lib/omniauth/strategies/implicit_flow_test.rb b/test/lib/omniauth/strategies/implicit_flow_test.rb new file mode 100644 index 00000000..ea8c0363 --- /dev/null +++ b/test/lib/omniauth/strategies/implicit_flow_test.rb @@ -0,0 +1,127 @@ +# frozen_string_literal: true + +require_relative '../../../test_helper' + +module OmniAuth + module Strategies + class NotYet + # Implicit Flow + def test_request_phase_with_response_mode + expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$} + strategy.options.issuer = 'example.com' + strategy.options.response_mode = 'form_post' + strategy.options.response_type = 'id_token' + strategy.options.client_options.host = 'example.com' + + strategy.expects(:redirect).with(regexp_matches(expected_redirect)) + strategy.request_phase + end + + def test_request_phase_with_response_mode_symbol + expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$} + strategy.options.issuer = 'example.com' + strategy.options.response_mode = 'form_post' + strategy.options.response_type = :id_token + strategy.options.client_options.host = 'example.com' + + strategy.expects(:redirect).with(regexp_matches(expected_redirect)) + strategy.request_phase + end + + 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).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.response_type = 'id_token' + + 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 + + # Implicit Flow + def test_callback_phase_without_id_token + state = SecureRandom.hex(16) + nonce = SecureRandom.hex(16) + request.stubs(:params).returns('state' => state) + request.stubs(:path).returns('') + strategy.options.response_type = 'id_token' + + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + + strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError)) + strategy.callback_phase + end + + # Implicit Flow + 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).returns('') + strategy.options.response_type = :id_token + + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + + strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError)) + strategy.callback_phase + end + + # Implicit Flow + def test_id_token_auth_hash + state = SecureRandom.hex(16) + nonce = SecureRandom.hex(16) + strategy.options.response_type = 'token id_token' + strategy.options.issuer = 'example.com' + + 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": 1_311_281_970, + "iat": 1_311_280_970, + } + ) + + request.stubs(:params).returns('state' => state, 'nounce' => nonce, 'id_token' => id_token) + request.stubs(:path).returns('') + + strategy.stubs(:decode_id_token).returns(id_token) + strategy.stubs(:stored_state).returns(state) + + strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) + strategy.callback_phase + + auth_hash = strategy.send(:env)['omniauth.auth'] + assert auth_hash.key?('provider') + assert auth_hash.key?('uid') + assert auth_hash.key?('info') + assert auth_hash.key?('extra') + assert auth_hash['extra'].key?('raw_info') + end + end + end +end diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index fbf5dd44..79dee96c 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -116,28 +116,6 @@ def test_request_phase_with_discovery assert_nil strategy.options.client_options.end_session_endpoint end - def test_request_phase_with_response_mode - expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$} - strategy.options.issuer = 'example.com' - strategy.options.response_mode = 'form_post' - strategy.options.response_type = 'id_token' - strategy.options.client_options.host = 'example.com' - - strategy.expects(:redirect).with(regexp_matches(expected_redirect)) - strategy.request_phase - end - - def test_request_phase_with_response_mode_symbol - expected_redirect = %r{^https://example\.com/authorize\?client_id=1234&nonce=\w{32}&response_mode=form_post&response_type=id_token&scope=openid&state=\w{32}$} - strategy.options.issuer = 'example.com' - strategy.options.response_mode = 'form_post' - strategy.options.response_type = :id_token - strategy.options.client_options.host = 'example.com' - - strategy.expects(:redirect).with(regexp_matches(expected_redirect)) - strategy.request_phase - end - def test_option_acr_values strategy.options.client_options[:host] = 'foobar.com' @@ -159,7 +137,9 @@ def test_request_phase_with_allowed_params strategy.options.allow_authorize_params = %i[name logo resource] strategy.options.extra_authorize_params = { resource: 'xyz' } strategy.options.client_options.host = 'example.com' - request.stubs(:params).returns('name' => 'example', 'logo' => 'example_logo', 'resource' => 'abc', + request.stubs(:params).returns('name' => 'example', + 'logo' => 'example_logo', + 'resource' => 'abc', 'not_allowed' => 'filter_me') assert(strategy.authorize_uri =~ /resource=xyz/, 'URI must contain fixed param resource') @@ -210,36 +190,6 @@ def test_callback_phase(_session = {}, _params = {}) strategy.callback_phase end - 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).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.response_type = 'id_token' - - 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_discovery # rubocop:disable Metrics/AbcSize code = SecureRandom.hex(16) state = SecureRandom.hex(16) @@ -319,32 +269,6 @@ def test_callback_phase_without_code strategy.callback_phase end - def test_callback_phase_without_id_token - state = SecureRandom.hex(16) - nonce = SecureRandom.hex(16) - request.stubs(:params).returns('state' => state) - request.stubs(:path).returns('') - strategy.options.response_type = 'id_token' - - strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) - - strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError)) - strategy.callback_phase - end - - 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).returns('') - strategy.options.response_type = :id_token - - strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) - - strategy.expects(:fail!).with(:missing_id_token, is_a(OmniAuth::OpenIDConnect::MissingIdTokenError)) - strategy.callback_phase - end - def test_callback_phase_with_timeout code = SecureRandom.hex(16) state = SecureRandom.hex(16) @@ -591,46 +515,64 @@ def test_public_key_with_x509 assert_equal OpenSSL::PKey::RSA, strategy.public_key.class end - def test_public_key_with_hmac - strategy.options.client_options.secret = 'secret' - strategy.options.client_signing_alg = :HS256 - assert_equal strategy.options.client_options.secret, strategy.public_key + def test_setup_phase + strategy.options.response_type = 'code' + strategy.setup_phase end - 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' + # Not supported yet: https://openid.net/specs/openid-financial-api-part-2-1_0.html + def test_setup_phase_not_supported_response_type + strategy.options.response_type = 'code id_token' + assert_raises ArgumentError do + strategy.setup_phase + end + end - 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": 1_311_281_970, - "iat": 1_311_280_970, - } - ) + def test_setup_phase_unnecessary_secret + strategy.options.response_type = 'token id_token' + strategy.options.client_options.secret = 'hoge' + assert_raises ArgumentError do + strategy.setup_phase + end + end - request.stubs(:params).returns('state' => state, 'nounce' => nonce, 'id_token' => id_token) - request.stubs(:path).returns('') + def test_public_key_missing + strategy.options.discovery = false + assert_raises RuntimeError do + strategy.public_key + end + end - strategy.stubs(:decode_id_token).returns(id_token) - strategy.stubs(:stored_state).returns(state) + def test_script_name + assert_equal '', strategy.send(:script_name) + end - strategy.call!('rack.session' => { 'omniauth.state' => state, 'omniauth.nonce' => nonce }) - strategy.callback_phase + def test_key_or_secret + assert_raises TypeError do + strategy.send(:key_or_secret, 123) + end + assert_raises ArgumentError do + strategy.send(:key_or_secret, { 'alg' => 'hoge' }) + end + + strategy.options.client_options.secret = 'hoge' + h = { 'alg' => :HS256 } + assert_equal 'hoge', strategy.send(:key_or_secret, h) + + h = { 'alg' => :RS256, 'kid' => 0 } + strategy.options.client_jwk_signing_key = File.read('./test/fixtures/jwks.json') + assert_equal JSON::JWK::Set, strategy.send(:key_or_secret, h).class + end + + def test_prepare_implicit_flow_callback_phase + request.stubs(:params).returns('access_token' => nil) + strategy.expects(:fail!) + strategy.send(:implicit_flow_callback_phase) + end - auth_hash = strategy.send(:env)['omniauth.auth'] - assert auth_hash.key?('provider') - assert auth_hash.key?('uid') - assert auth_hash.key?('info') - assert auth_hash.key?('extra') - assert auth_hash['extra'].key?('raw_info') + def test_configured_response_type + strategy.options.response_type = %i[token id_token] + assert_equal 'id_token token', strategy.send(:configured_response_type) end end end diff --git a/test/test_helper.rb b/test/test_helper.rb index 908ea450..bc2111bd 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -4,7 +4,7 @@ require 'minitest/autorun' require 'mocha/minitest' require 'faker' -require 'active_support' +require 'omniauth' SimpleCov.start do if ENV['CI']