From 72705c3ffba27003741a1fd8f7b3f430713b259a Mon Sep 17 00:00:00 2001 From: nov Date: Tue, 27 Dec 2022 14:48:58 +0900 Subject: [PATCH 1/6] replace ruby-jwt with json-jwt for jwks caching --- lib/omniauth/strategies/apple.rb | 110 +++++++++++++------------ omniauth-apple.gemspec | 2 +- spec/omniauth/strategies/apple_spec.rb | 34 +++++--- 3 files changed, 79 insertions(+), 67 deletions(-) diff --git a/lib/omniauth/strategies/apple.rb b/lib/omniauth/strategies/apple.rb index 7c6ca86..781cf15 100644 --- a/lib/omniauth/strategies/apple.rb +++ b/lib/omniauth/strategies/apple.rb @@ -1,21 +1,17 @@ # frozen_string_literal: true require 'omniauth-oauth2' -require 'net/https' +require 'json/jwt' module OmniAuth module Strategies class Apple < OmniAuth::Strategies::OAuth2 - class JWTFetchingFailed < CallbackError - def initialize(error_reason = nil, error_uri = nil) - super :jwks_fetching_failed, error_reason, error_uri - end - end + ISSUER = 'https://appleid.apple.com' option :name, 'apple' option :client_options, - site: 'https://appleid.apple.com', + site: ISSUER, authorize_url: '/auth/authorize', token_url: '/auth/token', auth_scheme: :request_body @@ -24,13 +20,13 @@ def initialize(error_reason = nil, error_uri = nil) scope: 'email name' option :authorized_client_ids, [] - uid { id_info['sub'] } + uid { id_info[:sub] } # Documentation on parameters # https://developer.apple.com/documentation/sign_in_with_apple/sign_in_with_apple_rest_api/authenticating_users_with_sign_in_with_apple info do prune!( - sub: id_info['sub'], + sub: id_info[:sub], email: email, first_name: first_name, last_name: last_name, @@ -41,8 +37,8 @@ def initialize(error_reason = nil, error_uri = nil) end extra do - id_token = request.params['id_token'] || access_token&.params&.dig('id_token') - prune!(raw_info: {id_info: id_info, user_info: user_info, id_token: id_token}) + id_token_str = request.params['id_token'] || access_token&.params&.dig('id_token') + prune!(raw_info: {id_info: id_info, user_info: user_info, id_token: id_token_str}) end def client @@ -50,12 +46,12 @@ def client end def email_verified - value = id_info['email_verified'] + value = id_info[:email_verified] value == true || value == "true" end def is_private_email - value = id_info['is_private_email'] + value = id_info[:is_private_email] value == true || value == "true" end @@ -79,54 +75,63 @@ def stored_nonce def id_info @id_info ||= if request.params&.key?('id_token') || access_token&.params&.key?('id_token') - id_token = request.params['id_token'] || access_token.params['id_token'] - if (verification_key = fetch_jwks) - jwt_options = { - verify_iss: true, - iss: 'https://appleid.apple.com', - verify_iat: true, - verify_aud: true, - aud: [options.client_id].concat(options.authorized_client_ids), - algorithms: ['RS256'], - jwks: verification_key - } - payload, _header = ::JWT.decode(id_token, nil, true, jwt_options) - verify_nonce!(payload) - payload + id_token_str = request.params['id_token'] || access_token.params['id_token'] + id_token = JSON::JWT.decode(id_token_str, :skip_verification) + if (jwk = fetch_jwk(id_token.kid)) + id_token.verify! jwk + verify_claims!(id_token) + id_token else {} end end end - def fetch_jwks - conn = Faraday.new(headers: {user_agent: 'ruby/omniauth-apple'}) do |c| - c.response :json, parser_options: { symbolize_names: true } - c.adapter Faraday.default_adapter - end - res = conn.get 'https://appleid.apple.com/auth/keys' - if res.success? - res.body - else - raise JWTFetchingFailed.new('HTTP Error when fetching JWKs') - end - rescue JWTFetchingFailed, Faraday::Error => e + def fetch_jwk(kid) + JSON::JWK::Set::Fetcher.fetch File.join(ISSUER, 'auth/keys'), kid: kid + rescue JSON::ParserError, JSON::JWT::Exception, Faraday::Error => e fail!(:jwks_fetching_failed, e) and nil end - def verify_nonce!(payload) - return unless payload['nonce_supported'] + def verify_claims!(id_token) + verify_iss!(id_token) + verify_aud!(id_token) + verify_iat!(id_token) + verify_exp!(id_token) + verify_nonce!(id_token) if id_token[:nonce_supported] + end + + def verify_iss!(id_token) + invalid_claim! :iss unless id_token[:iss] == ISSUER + end + + def verify_aud!(id_token) + invalid_claim! :aud unless [options.client_id].concat(options.authorized_client_ids).include?(id_token[:aud]) + end + + def verify_iat!(id_token) + invalid_claim! :iat unless id_token[:iat] <= Time.now.to_i + end + + def verify_exp!(id_token) + invalid_claim! :exp unless id_token[:exp] >= Time.now.to_i + end - return if payload['nonce'] && payload['nonce'] == stored_nonce + def verify_nonce!(id_token) + invalid_claim! :nonce unless id_token[:nonce] && id_token[:nonce] == stored_nonce + end - fail!(:nonce_mismatch, CallbackError.new(:nonce_mismatch, 'nonce mismatch')) + def invalid_claim!(claim) + key = :"#{claim}_invalid" + message = "#{claim} invalid" + fail! key, CallbackError.new(key, message) end def client_id @client_id ||= if id_info.nil? options.client_id else - id_info['aud'] if options.authorized_client_ids.include? id_info['aud'] + id_info[:aud] if options.authorized_client_ids.include? id_info[:aud] end end @@ -138,7 +143,7 @@ def user_info end def email - id_info['email'] + id_info[:email] end def first_name @@ -157,16 +162,15 @@ def prune!(hash) end def client_secret - payload = { + jwt = JSON::JWT.new( iss: options.team_id, - aud: 'https://appleid.apple.com', + aud: ISSUER, sub: client_id, - iat: Time.now.to_i, - exp: Time.now.to_i + 60 - } - headers = { kid: options.key_id } - - ::JWT.encode(payload, private_key, 'ES256', headers) + iat: Time.now, + exp: Time.now + 60 + ) + jwt.kid = options.key_id + jwt.sign(private_key).to_s end def private_key diff --git a/omniauth-apple.gemspec b/omniauth-apple.gemspec index d97186b..e6ec42f 100644 --- a/omniauth-apple.gemspec +++ b/omniauth-apple.gemspec @@ -37,7 +37,7 @@ Gem::Specification.new do |spec| spec.require_paths = ["lib"] spec.add_dependency 'omniauth-oauth2' - spec.add_dependency 'jwt' + spec.add_dependency 'json-jwt' spec.add_development_dependency "bundler", "~> 2.0" spec.add_development_dependency "rake", "~> 13.0" spec.add_development_dependency "rspec", "~> 3.9" diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb index f2fcdd9..ee7dd7b 100644 --- a/spec/omniauth/strategies/apple_spec.rb +++ b/spec/omniauth/strategies/apple_spec.rb @@ -31,8 +31,8 @@ ] } end - let(:id_token_header) do - { kid: '1' } + let(:kid) do + '1' end let(:id_token_payload) do { @@ -46,7 +46,11 @@ 'email_verified' => true, } end - let(:id_token) { JWT.encode(id_token_payload, apple_key, 'RS256', id_token_header) } + let(:id_token) do + jwt = JSON::JWT.new(id_token_payload) + jwt.kid = kid + jwt.sign(apple_key).to_s + end let(:access_token) { OAuth2::AccessToken.from_hash(subject.client, 'id_token' => id_token) } subject do @@ -228,11 +232,11 @@ describe '#info' do let(:user_info_payload) do { - name: { - firstName: 'first', - lastName: 'last', - }, - email: 'something@privatrerelay.appleid.com', + name: { + firstName: 'first', + lastName: 'last', + }, + email: 'something@privatrerelay.appleid.com', } end before(:each) do @@ -266,7 +270,7 @@ context 'fails nonce' do before(:each) do expect(subject).to receive(:fail!).with( - :nonce_mismatch, + :nonce_invalid, instance_of(OmniAuth::Strategies::OAuth2::CallbackError) ).and_return([302, {}, '']) end @@ -320,10 +324,14 @@ end context 'issued by invalid issuer' do - it 'raises JWT::InvalidIssuerError' do + it 'should fail with :iss_invalid' do id_token_payload['iss'] = 'https://appleid.badguy.com' request.params.merge!('id_token' => id_token) - expect { subject.extra }.to raise_error(JWT::InvalidIssuerError) + expect(subject).to receive(:fail!).with( + :iss_invalid, + instance_of(OmniAuth::Strategies::OAuth2::CallbackError) + ) + subject.extra end end @@ -359,7 +367,7 @@ it do expect(subject).to receive(:fail!).with( :jwks_fetching_failed, - instance_of(OmniAuth::Strategies::Apple::JWTFetchingFailed) + instance_of(Faraday::ServerError) ).and_return([302, {}, '']) subject.info end @@ -378,7 +386,7 @@ it do expect(subject).to receive(:fail!).with( :jwks_fetching_failed, - instance_of(Faraday::ParsingError) + instance_of(JSON::ParserError) ).and_return([302, {}, '']) subject.info end From bd9e734070df9673c561fe1070bf011d648111ed Mon Sep 17 00:00:00 2001 From: nov Date: Tue, 27 Dec 2022 15:11:56 +0900 Subject: [PATCH 2/6] v1.3.0 --- lib/omniauth/apple/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/apple/version.rb b/lib/omniauth/apple/version.rb index 87eb014..dc3346c 100644 --- a/lib/omniauth/apple/version.rb +++ b/lib/omniauth/apple/version.rb @@ -1,5 +1,5 @@ module OmniAuth module Apple - VERSION = "1.2.2" + VERSION = "1.3.0" end end From f43c471a29ee8a82c18dfa751c419ecc51085e71 Mon Sep 17 00:00:00 2001 From: nov Date: Wed, 28 Dec 2022 11:59:42 +0900 Subject: [PATCH 3/6] specs for invalid id_token claims --- spec/omniauth/strategies/apple_spec.rb | 123 ++++++++++++++++--------- spec/spec_helper.rb | 1 + 2 files changed, 83 insertions(+), 41 deletions(-) diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb index ee7dd7b..e368a90 100644 --- a/spec/omniauth/strategies/apple_spec.rb +++ b/spec/omniauth/strategies/apple_spec.rb @@ -34,32 +34,35 @@ let(:kid) do '1' end - let(:id_token_payload) do + let(:valid_id_token_payload) do { - 'iss' => 'https://appleid.apple.com', - 'sub' => 'sub-1', - 'aud' => 'appid', - 'exp' => Time.now.to_i + 3600, - 'iat' => Time.now.to_i, - 'nonce_supported' => true, - 'email' => 'something@privatrerelay.appleid.com', - 'email_verified' => true, + iss: 'https://appleid.apple.com', + sub: 'sub-1', + aud: 'appid', + exp: Time.now + 3600, + iat: Time.now, + nonce_supported: true, + email: 'something@privatrerelay.appleid.com', + email_verified: true, } end + let(:id_token_payload) do + valid_id_token_payload + end let(:id_token) do jwt = JSON::JWT.new(id_token_payload) jwt.kid = kid jwt.sign(apple_key).to_s end let(:access_token) { OAuth2::AccessToken.from_hash(subject.client, 'id_token' => id_token) } - - subject do + let(:strategy) do OmniAuth::Strategies::Apple.new(app, 'appid', 'secret', options ).tap do |strategy| allow(strategy).to receive(:request) do request end end end + subject { strategy } before do OmniAuth.config.test_mode = true @@ -303,49 +306,87 @@ describe '#extra' do before(:each) do - subject.authorize_params # initializes session / populates 'nonce', 'state', etc - id_token_payload['nonce'] = subject.session['omniauth.nonce'] + strategy.authorize_params # initializes session / populates 'nonce', 'state', etc + id_token_payload[:nonce] ||= strategy.session['omniauth.nonce'] end - describe 'id_token' do - context 'issued by valid issuer' do + describe 'extra[:raw_info]' do + subject { strategy.extra[:raw_info] } + + context 'when the id_token is given' do before(:each) do request.params.merge!('id_token' => id_token) end - context 'when the id_token is passed into the access token' do - it 'should include id_token when set on the access_token' do - expect(subject.extra[:raw_info]).to include(id_token: id_token) - end - it 'should include id_info when id_token is set on the access_token' do - expect(subject.extra[:raw_info]).to include(id_info: id_token_payload) - end + shared_examples :return_raw_info do + it { is_expected.to include(id_token: id_token) } + it { is_expected.to include(id_info: id_token_payload) } end - end - context 'issued by invalid issuer' do - it 'should fail with :iss_invalid' do - id_token_payload['iss'] = 'https://appleid.badguy.com' - request.params.merge!('id_token' => id_token) - expect(subject).to receive(:fail!).with( - :iss_invalid, - instance_of(OmniAuth::Strategies::OAuth2::CallbackError) - ) - subject.extra + context 'when valid' do + it_behaves_like :return_raw_info + it do + expect(strategy).not_to receive(:fail!) + subject + end end - end - context 'when the id_token is missing' do - it 'should not include id_token' do - allow(subject).to receive(:access_token).and_return(nil) - expect(subject.extra).not_to have_key(:raw_info) - end + context 'when invalid' do + let(:id_token_payload) do + valid_id_token_payload.merge(invalid_claims) + end + + shared_examples :invalid_at do |claim| + it_behaves_like :return_raw_info + it do + expect(strategy).to receive(:fail!).with( + :"#{claim}_invalid", + instance_of(OmniAuth::Strategies::OAuth2::CallbackError) + ) + subject + end + end + + context 'on iss' do + let(:invalid_claims) do + { iss: 'https://invalid.example.com' } + end + it_behaves_like :invalid_at, :iss + end + + context 'on aud' do + let(:invalid_claims) do + { aud: 'invalid_client' } + end + it_behaves_like :invalid_at, :aud + end - it 'should not include id_info' do - allow(subject).to receive(:access_token).and_return(nil) - expect(subject.extra).not_to have_key(:raw_info) + context 'on iat' do + let(:invalid_claims) do + { iat: Time.now + 30 } + end + it_behaves_like :invalid_at, :iat + end + + context 'on exp' do + let(:invalid_claims) do + { exp: Time.now - 30 } + end + it_behaves_like :invalid_at, :exp + end + + context 'on nonce' do + let(:invalid_claims) do + { nonce: 'invalid' } + end + it_behaves_like :invalid_at, :nonce + end end end + + context 'otherwise' do + it { is_expected.to be_nil } + end end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 9097835..b869d3c 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -6,6 +6,7 @@ SimpleCov.start('test_frameworks') require 'omniauth-apple' +OmniAuth.config.logger = Logger.new(nil) require 'webmock/rspec' WebMock.disable_net_connect! From 4ba73319ea7159353dbcbb0a6c991c585e7d93be Mon Sep 17 00:00:00 2001 From: nov Date: Wed, 28 Dec 2022 12:00:45 +0900 Subject: [PATCH 4/6] v1.3.0.alpha --- lib/omniauth/apple/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/apple/version.rb b/lib/omniauth/apple/version.rb index dc3346c..4f02de7 100644 --- a/lib/omniauth/apple/version.rb +++ b/lib/omniauth/apple/version.rb @@ -1,5 +1,5 @@ module OmniAuth module Apple - VERSION = "1.3.0" + VERSION = '1.3.0.alpha' end end From 92f33205f54c25d155f12b09874a4415e47f20d3 Mon Sep 17 00:00:00 2001 From: nov Date: Wed, 28 Dec 2022 13:42:20 +0900 Subject: [PATCH 5/6] raise CallbackError instead of calling fail! --- lib/omniauth/apple/version.rb | 2 +- lib/omniauth/strategies/apple.rb | 31 ++++++----- spec/omniauth/strategies/apple_spec.rb | 72 ++++++++++++++------------ 3 files changed, 58 insertions(+), 47 deletions(-) diff --git a/lib/omniauth/apple/version.rb b/lib/omniauth/apple/version.rb index 4f02de7..012eaa3 100644 --- a/lib/omniauth/apple/version.rb +++ b/lib/omniauth/apple/version.rb @@ -1,5 +1,5 @@ module OmniAuth module Apple - VERSION = '1.3.0.alpha' + VERSION = '1.3.0.alpha2' end end diff --git a/lib/omniauth/strategies/apple.rb b/lib/omniauth/strategies/apple.rb index 781cf15..5ad3a40 100644 --- a/lib/omniauth/strategies/apple.rb +++ b/lib/omniauth/strategies/apple.rb @@ -77,20 +77,27 @@ def id_info @id_info ||= if request.params&.key?('id_token') || access_token&.params&.key?('id_token') id_token_str = request.params['id_token'] || access_token.params['id_token'] id_token = JSON::JWT.decode(id_token_str, :skip_verification) - if (jwk = fetch_jwk(id_token.kid)) - id_token.verify! jwk - verify_claims!(id_token) - id_token - else - {} - end + verify_id_token! id_token + id_token end end - def fetch_jwk(kid) + def verify_id_token!(id_token) + jwk = fetch_jwk! id_token.kid + verify_signature! id_token, jwk + verify_claims! id_token + end + + def fetch_jwk!(kid) JSON::JWK::Set::Fetcher.fetch File.join(ISSUER, 'auth/keys'), kid: kid - rescue JSON::ParserError, JSON::JWT::Exception, Faraday::Error => e - fail!(:jwks_fetching_failed, e) and nil + rescue => e + raise CallbackError.new(:jwks_fetching_failed, e) + end + + def verify_signature!(id_token, jwk) + id_token.verify! jwk + rescue => e + raise CallbackError.new(:id_token_signature_invalid, e) end def verify_claims!(id_token) @@ -122,9 +129,7 @@ def verify_nonce!(id_token) end def invalid_claim!(claim) - key = :"#{claim}_invalid" - message = "#{claim} invalid" - fail! key, CallbackError.new(key, message) + raise CallbackError.new(:id_token_claims_invalid, "#{claim} invalid") end def client_id diff --git a/spec/omniauth/strategies/apple_spec.rb b/spec/omniauth/strategies/apple_spec.rb index e368a90..2709d1b 100644 --- a/spec/omniauth/strategies/apple_spec.rb +++ b/spec/omniauth/strategies/apple_spec.rb @@ -271,19 +271,22 @@ end context 'fails nonce' do - before(:each) do - expect(subject).to receive(:fail!).with( - :nonce_invalid, - instance_of(OmniAuth::Strategies::OAuth2::CallbackError) - ).and_return([302, {}, '']) - end - it 'when differs from session' do - subject.session['omniauth.nonce'] = 'abc' - subject.info + context 'when differs from session' do + before { subject.session['omniauth.nonce'] = 'abc' } + it do + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, 'id_token_claims_invalid | nonce invalid' + ) + end end - it 'when missing from session' do - subject.session.delete('omniauth.nonce') - subject.info + + context 'when missing from session' do + before { subject.session.delete('omniauth.nonce') } + it do + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, 'id_token_claims_invalid | nonce invalid' + ) + end end end @@ -318,32 +321,39 @@ request.params.merge!('id_token' => id_token) end - shared_examples :return_raw_info do + context 'when valid' do it { is_expected.to include(id_token: id_token) } it { is_expected.to include(id_info: id_token_payload) } - end - - context 'when valid' do - it_behaves_like :return_raw_info it do expect(strategy).not_to receive(:fail!) subject end end - context 'when invalid' do + context 'when signature invalid' do + let(:id_token) do + jwt = JSON::JWT.new(id_token_payload) + jwt.kid = kid + jwt.to_s + end + + it do + expect { subject }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, /id_token_signature_invalid/ + ) + end + end + + context 'when claims invalid' do let(:id_token_payload) do valid_id_token_payload.merge(invalid_claims) end shared_examples :invalid_at do |claim| - it_behaves_like :return_raw_info it do - expect(strategy).to receive(:fail!).with( - :"#{claim}_invalid", - instance_of(OmniAuth::Strategies::OAuth2::CallbackError) + expect { subject }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, "id_token_claims_invalid | #{claim} invalid" ) - subject end end @@ -406,11 +416,9 @@ end it do - expect(subject).to receive(:fail!).with( - :jwks_fetching_failed, - instance_of(Faraday::ServerError) - ).and_return([302, {}, '']) - subject.info + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, /jwks_fetching_failed/ + ) end end @@ -425,11 +433,9 @@ end it do - expect(subject).to receive(:fail!).with( - :jwks_fetching_failed, - instance_of(JSON::ParserError) - ).and_return([302, {}, '']) - subject.info + expect { subject.info }.to raise_error( + OmniAuth::Strategies::OAuth2::CallbackError, /jwks_fetching_failed/ + ) end end end From b32669f16ce37f1af66ecfdc08b178ceafd82b17 Mon Sep 17 00:00:00 2001 From: nov Date: Tue, 17 Jan 2023 15:24:14 +0900 Subject: [PATCH 6/6] v1.3.0 --- lib/omniauth/apple/version.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/omniauth/apple/version.rb b/lib/omniauth/apple/version.rb index 012eaa3..684fad7 100644 --- a/lib/omniauth/apple/version.rb +++ b/lib/omniauth/apple/version.rb @@ -1,5 +1,5 @@ module OmniAuth module Apple - VERSION = '1.3.0.alpha2' + VERSION = '1.3.0' end end