From 1b6ced588715276a838a5286b47948ebb80df11c Mon Sep 17 00:00:00 2001 From: Eugen Rochko Date: Wed, 9 Aug 2017 23:54:14 +0200 Subject: [PATCH] Add Digest header to requests with body, handle acct and URI keyId (#4565) --- .../concerns/signature_verification.rb | 23 +++++- app/lib/request.rb | 24 +++++- .../concerns/signature_verification_spec.rb | 78 ++++++++++++++----- 3 files changed, 100 insertions(+), 25 deletions(-) diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index abe845d937..aeb8da879e 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -31,7 +31,7 @@ module SignatureVerification return end - account = ResolveRemoteAccountService.new.call(signature_params['keyId'].gsub(/\Aacct:/, '')) + account = account_from_key_id(signature_params['keyId']) if account.nil? @signed_request_account = nil @@ -49,6 +49,10 @@ module SignatureVerification end end + def request_body + @request_body ||= request.raw_post + end + private def build_signed_string(signed_headers) @@ -57,6 +61,8 @@ module SignatureVerification signed_headers.split(' ').map do |signed_header| if signed_header == Request::REQUEST_TARGET "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" + elsif signed_header == 'digest' + "digest: #{body_digest}" else "#{signed_header}: #{request.headers[to_header_name(signed_header)]}" end @@ -73,6 +79,10 @@ module SignatureVerification (Time.now.utc - time_sent).abs <= 30 end + def body_digest + "SHA-256=#{Digest::SHA256.base64digest(request_body)}" + end + def to_header_name(name) name.split(/-/).map(&:capitalize).join('-') end @@ -81,7 +91,14 @@ module SignatureVerification signature_params['keyId'].blank? || signature_params['signature'].blank? || signature_params['algorithm'].blank? || - signature_params['algorithm'] != 'rsa-sha256' || - !signature_params['keyId'].start_with?('acct:') + signature_params['algorithm'] != 'rsa-sha256' + end + + def account_from_key_id(key_id) + if key_id.start_with?('acct:') + ResolveRemoteAccountService.new.call(key_id.gsub(/\Aacct:/, '')) + elsif !ActivityPub::TagManager.instance.local_uri?(key_id) + ActivityPub::FetchRemoteAccountService.new.call(key_id) + end end end diff --git a/app/lib/request.rb b/app/lib/request.rb index e73c5ac20b..c01e079259 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -12,15 +12,21 @@ class Request @headers = {} set_common_headers! + set_digest! if options.key?(:body) end - def on_behalf_of(account) + def on_behalf_of(account, key_id_format = :acct) raise ArgumentError unless account.local? - @account = account + + @account = account + @key_id_format = key_id_format + + self end def add_headers(new_headers) @headers.merge!(new_headers) + self end def perform @@ -40,8 +46,11 @@ class Request @headers['Date'] = Time.now.utc.httpdate end + def set_digest! + @headers['Digest'] = "SHA-256=#{Digest::SHA256.base64digest(@options[:body])}" + end + def signature - key_id = @account.to_webfinger_s algorithm = 'rsa-sha256' signature = Base64.strict_encode64(@account.keypair.sign(OpenSSL::Digest::SHA256.new, signed_string)) @@ -60,6 +69,15 @@ class Request @user_agent ||= "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +#{root_url})" end + def key_id + case @key_id_format + when :acct + @account.to_webfinger_s + when :uri + [ActivityPub::TagManager.instance.uri_for(@account), '#main-key'].join + end + end + def timeout { write: 10, connect: 10, read: 10 } end diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb index b371795ab0..64648621e0 100644 --- a/spec/controllers/concerns/signature_verification_spec.rb +++ b/spec/controllers/concerns/signature_verification_spec.rb @@ -16,7 +16,7 @@ describe ApplicationController, type: :controller do end before do - routes.draw { get 'success' => 'anonymous#success' } + routes.draw { match via: [:get, :post], 'success' => 'anonymous#success' } end context 'without signature header' do @@ -40,34 +40,74 @@ describe ApplicationController, type: :controller do context 'with signature header' do let!(:author) { Fabricate(:account) } - before do - get :success + context 'without body' do + before do + get :success - fake_request = Request.new(:get, request.url) - fake_request.on_behalf_of(author) + fake_request = Request.new(:get, request.url) + fake_request.on_behalf_of(author) - request.headers.merge!(fake_request.headers) - end + request.headers.merge!(fake_request.headers) + end - describe '#signed_request?' do - it 'returns true' do - expect(controller.signed_request?).to be true + describe '#signed_request?' do + it 'returns true' do + expect(controller.signed_request?).to be true + end + end + + describe '#signed_request_account' do + it 'returns an account' do + expect(controller.signed_request_account).to eq author + end + + it 'returns nil when path does not match' do + request.path = '/alternative-path' + expect(controller.signed_request_account).to be_nil + end + + it 'returns nil when method does not match' do + post :success + expect(controller.signed_request_account).to be_nil + end end end - describe '#signed_request_account' do - it 'returns an account' do - expect(controller.signed_request_account).to eq author + context 'with body' do + before do + post :success, body: 'Hello world' + + fake_request = Request.new(:post, request.url, body: 'Hello world') + fake_request.on_behalf_of(author) + + request.headers.merge!(fake_request.headers) end - it 'returns nil when path does not match' do - request.path = '/alternative-path' - expect(controller.signed_request_account).to be_nil + describe '#signed_request?' do + it 'returns true' do + expect(controller.signed_request?).to be true + end end - it 'returns nil when method does not match' do - post :success - expect(controller.signed_request_account).to be_nil + describe '#signed_request_account' do + it 'returns an account' do + expect(controller.signed_request_account).to eq author + end + + it 'returns nil when path does not match' do + request.path = '/alternative-path' + expect(controller.signed_request_account).to be_nil + end + + it 'returns nil when method does not match' do + get :success + expect(controller.signed_request_account).to be_nil + end + + it 'returns nil when body has been tampered' do + request.headers['RAW_POST_DATA'] = 'doo doo doo' + expect(controller.signed_request_account).to be_nil + end end end end