From fa7856ac640bf05f34bd891f43580c4da564e4d2 Mon Sep 17 00:00:00 2001 From: Will Green Date: Sun, 25 Sep 2016 01:09:31 -0400 Subject: [PATCH 1/4] verify the UserInfo response --- lib/omniauth/strategies/openid_connect.rb | 17 +++++- .../strategies/openid_connect_test.rb | 57 +++++++++++++++++++ 2 files changed, 71 insertions(+), 3 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index e4705c90..f2677fd3 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -151,7 +151,14 @@ def discover! end def user_info - @user_info ||= access_token.userinfo! + @user_info ||= begin + _user_info = access_token.userinfo! + if id_token.sub.eql? _user_info.sub + _user_info + else + ::OpenIDConnect::ResponseObject::UserInfo.new(sub: '😶', warning: 'UserInfo subject does not match ID Token subject. Discard UserInfo response.') + end + end end def access_token @@ -160,8 +167,8 @@ def access_token scope: (options.scope if options.send_scope_to_token_endpoint), client_auth_method: options.client_auth_method ) - _id_token = decode_id_token _access_token.id_token - _id_token.verify!( + @id_token = decode_id_token _access_token.id_token + @id_token.verify!( issuer: options.issuer, client_id: client_options.identifier, nonce: stored_nonce @@ -170,6 +177,10 @@ def access_token }.call() end + def id_token + @id_token + end + def decode_id_token(id_token) ::OpenIDConnect::ResponseObject::IdToken.decode(id_token, public_key) end diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index 0deda94d..c992e5c0 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -59,6 +59,7 @@ def test_callback_phase(session = {}, params = {}) id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:verify!).with({:issuer => strategy.options.issuer, :client_id => @identifier, :nonce => nonce}).returns(true) + id_token.stubs(:sub).returns(user_info.sub) ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token) strategy.unstub(:user_info) @@ -102,6 +103,7 @@ def test_callback_phase_with_discovery id_token = stub('OpenIDConnect::ResponseObject::IdToken') id_token.stubs(:verify!).with({:issuer => 'https://example.com/', :client_id => @identifier, :nonce => nonce}).returns(true) + id_token.stubs(:sub).returns(user_info.sub) ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(id_token) strategy.unstub(:user_info) @@ -189,6 +191,7 @@ def test_callback_phase_with_socket_error end def test_info + user_info.sub info = strategy.info assert_equal user_info.name, info[:name] assert_equal user_info.email, info[:email] @@ -201,6 +204,60 @@ def test_info assert_equal({ website: user_info.website }, info[:urls]) end + def test_info_when_subject_does_not_match_id_token_subject + 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('') + + strategy.options.issuer = 'example.com' + strategy.options.client_signing_alg = :RS256 + strategy.options.client_jwk_signing_key = File.read('test/fixtures/jwks.json') + + id_token = stub('OpenIDConnect::ResponseObject::IdToken') + id_token.stubs(:verify!).with({:issuer => strategy.options.issuer, :client_id => @identifier, :nonce => nonce}).returns(true) + id_token.stubs(:sub).returns(user_info.sub) + ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(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')) + client.expects(:access_token!).at_least_once.returns(access_token) + _user_info = OpenIDConnect::ResponseObject::UserInfo.new( + sub: SecureRandom.hex(16), + name: Faker::Name.name, + email: Faker::Internet.email, + nickname: Faker::Name.first_name, + preferred_username: Faker::Internet.user_name, + given_name: Faker::Name.first_name, + family_name: Faker::Name.last_name, + gender: 'female', + picture: Faker::Internet.url + ".png", + phone_number: Faker::PhoneNumber.phone_number, + website: Faker::Internet.url, + ) + access_token.expects(:userinfo!).returns(_user_info) + + strategy.call!({'rack.session' => {'omniauth.state' => state, 'omniauth.nonce' => nonce}}) + + info = strategy.info + assert_equal nil, info[:name] + assert_equal nil, info[:email] + assert_equal nil, info[:nickname] + assert_equal nil, info[:first_name] + assert_equal nil, info[:last_name] + assert_equal nil, info[:gender] + assert_equal nil, info[:image] + assert_equal nil, info[:phone] + assert_equal({ website: nil }, info[:urls]) + assert_equal({ raw_info: {sub: "😶", warning: "UserInfo subject does not match ID Token subject. Discard UserInfo response."}}, strategy.extra) + end + def test_extra assert_equal({ raw_info: user_info.as_json }, strategy.extra) end From 26be358d42259c9d5e8ced187466174d34bd8d1a Mon Sep 17 00:00:00 2001 From: Will Green Date: Sun, 25 Sep 2016 01:10:20 -0400 Subject: [PATCH 2/4] lambda to begin...end --- 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 f2677fd3..01f17995 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -162,7 +162,7 @@ def user_info end def access_token - @access_token ||= lambda { + @access_token ||= begin _access_token = client.access_token!( scope: (options.scope if options.send_scope_to_token_endpoint), client_auth_method: options.client_auth_method @@ -174,7 +174,7 @@ def access_token nonce: stored_nonce ) _access_token - }.call() + end end def id_token From 982f03202f82487719706c1d4cf12a976496ef08 Mon Sep 17 00:00:00 2001 From: Will Green Date: Sun, 25 Sep 2016 01:18:53 -0400 Subject: [PATCH 3/4] refactor to expose id_token without code duplication --- lib/omniauth/strategies/openid_connect.rb | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 01f17995..16999cc9 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -161,24 +161,28 @@ def user_info end end - def access_token - @access_token ||= begin + def access_and_id_tokens + @tokens ||= begin _access_token = client.access_token!( scope: (options.scope if options.send_scope_to_token_endpoint), client_auth_method: options.client_auth_method ) - @id_token = decode_id_token _access_token.id_token - @id_token.verify!( + _id_token = decode_id_token _access_token.id_token + _id_token.verify!( issuer: options.issuer, client_id: client_options.identifier, nonce: stored_nonce ) - _access_token + [_access_token, _id_token] end end + def access_token + @access_token ||= access_and_id_tokens[0] + end + def id_token - @id_token + @id_token ||= access_and_id_tokens[1] end def decode_id_token(id_token) From c6588d3651041741ce0972df4b11d99a4ba7dcb1 Mon Sep 17 00:00:00 2001 From: Will Green Date: Sun, 25 Sep 2016 01:23:11 -0400 Subject: [PATCH 4/4] ensure sub claim is pulled from id token ID Token is authoritative over the UserInfo response --- lib/omniauth/strategies/openid_connect.rb | 2 +- .../strategies/openid_connect_test.rb | 29 ++++++++++++++++++- 2 files changed, 29 insertions(+), 2 deletions(-) diff --git a/lib/omniauth/strategies/openid_connect.rb b/lib/omniauth/strategies/openid_connect.rb index 16999cc9..a3383ba5 100644 --- a/lib/omniauth/strategies/openid_connect.rb +++ b/lib/omniauth/strategies/openid_connect.rb @@ -43,7 +43,7 @@ class OpenIDConnect option :send_scope_to_token_endpoint, true option :client_auth_method - uid { user_info.sub } + uid { id_token.sub } info do { diff --git a/test/lib/omniauth/strategies/openid_connect_test.rb b/test/lib/omniauth/strategies/openid_connect_test.rb index c992e5c0..2934918a 100644 --- a/test/lib/omniauth/strategies/openid_connect_test.rb +++ b/test/lib/omniauth/strategies/openid_connect_test.rb @@ -43,7 +43,34 @@ def test_request_phase_with_discovery end def test_uid - assert_equal user_info.sub, strategy.uid + code = SecureRandom.hex(16) + state = SecureRandom.hex(16) + nonce = SecureRandom.hex(16) + sub = SecureRandom.hex(16) + request.stubs(:params).returns({'code' => code,'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') + + id_token = stub('OpenIDConnect::ResponseObject::IdToken') + id_token.stubs(:verify!).with({:issuer => strategy.options.issuer, :client_id => @identifier, :nonce => nonce}).returns(true) + id_token.stubs(:sub).returns(sub) + ::OpenIDConnect::ResponseObject::IdToken.stubs(:decode).returns(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')) + client.expects(:access_token!).at_least_once.returns(access_token) + + strategy.call!({'rack.session' => {'omniauth.state' => state, 'omniauth.nonce' => nonce}}) + + assert_equal sub, strategy.uid end def test_callback_phase(session = {}, params = {})