commit: 1618b68bfa740ed655ac45d7d5f4f46fed6c8c62 parent: c1f201c49a007e5c0740c00651e549a7b0416b05 Author: Eugen Rochko <eugen@zeonfederated.com> Date: Fri, 14 Jul 2017 20:41:49 +0200 HTTP signatures (#4146) * Add Request class with HTTP signature generator Spec: https://tools.ietf.org/html/draft-cavage-http-signatures-06 * Add HTTP signature verification concern * Add test for SignatureVerification concern * Add basic test for Request class * Make PuSH subscribe/unsubscribe requests use new Request class Accidentally fix lease_seconds not being set and sent properly, and change the new minimum subscription duration to 1 day * Make all PuSH workers use new Request class * Make Salmon sender use new Request class * Make FetchLinkService use new Request class * Make FetchAtomService use the new Request class * Make Remotable use the new Request class * Make ResolveRemoteAccountService use the new Request class * Add more tests * Allow +-30 seconds window for signed request to remain valid * Disable time window validation for signed requests, restore 7 days as PuSH subscription duration (which was previous default due to a bug)Diffstat:
23 files changed, 379 insertions(+), 90 deletions(-)
diff --git a/app/controllers/accounts_controller.rb b/app/controllers/accounts_controller.rb @@ -2,6 +2,7 @@ class AccountsController < ApplicationController include AccountControllerConcern + include SignatureVerification def show respond_to do |format| diff --git a/app/controllers/api/subscriptions_controller.rb b/app/controllers/api/subscriptions_controller.rb @@ -42,7 +42,7 @@ class Api::SubscriptionsController < Api::BaseController end def lease_seconds_or_default - (params['hub.lease_seconds'] || 86_400).to_i.seconds + (params['hub.lease_seconds'] || 1.day).to_i.seconds end def set_account diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +# Implemented according to HTTP signatures (Draft 6) +# <https://tools.ietf.org/html/draft-cavage-http-signatures-06> +module SignatureVerification + extend ActiveSupport::Concern + + def signed_request? + request.headers['Signature'].present? + end + + def signed_request_account + return @signed_request_account if defined?(@signed_request_account) + + unless signed_request? + @signed_request_account = nil + return + end + + raw_signature = request.headers['Signature'] + signature_params = {} + + raw_signature.split(',').each do |part| + parsed_parts = part.match(/([a-z]+)="([^"]+)"/i) + next if parsed_parts.nil? || parsed_parts.size != 3 + signature_params[parsed_parts[1]] = parsed_parts[2] + end + + if incompatible_signature?(signature_params) + @signed_request_account = nil + return + end + + account = ResolveRemoteAccountService.new.call(signature_params['keyId'].gsub(/\Aacct:/, '')) + + if account.nil? + @signed_request_account = nil + return + end + + signature = Base64.decode64(signature_params['signature']) + compare_signed_string = build_signed_string(signature_params['headers']) + + if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string) + @signed_request_account = account + @signed_request_account + else + @signed_request_account = nil + end + end + + private + + def build_signed_string(signed_headers) + signed_headers = 'date' if signed_headers.blank? + + signed_headers.split(' ').map do |signed_header| + if signed_header == Request::REQUEST_TARGET + "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}" + else + "#{signed_header}: #{request.headers[to_header_name(signed_header)]}" + end + end.join("\n") + end + + def matches_time_window? + begin + time_sent = DateTime.httpdate(request.headers['Date']) + rescue ArgumentError + return false + end + + (Time.now.utc - time_sent).abs <= 30 + end + + def to_header_name(name) + name.split(/-/).map(&:capitalize).join('-') + end + + def incompatible_signature?(signature_params) + signature_params['keyId'].blank? || + signature_params['signature'].blank? || + signature_params['algorithm'].blank? || + signature_params['algorithm'] != 'rsa-sha256' || + !signature_params['keyId'].start_with?('acct:') + end +end diff --git a/app/controllers/stream_entries_controller.rb b/app/controllers/stream_entries_controller.rb @@ -2,6 +2,7 @@ class StreamEntriesController < ApplicationController include Authorization + include SignatureVerification layout 'public' diff --git a/app/helpers/http_helper.rb b/app/helpers/http_helper.rb @@ -1,17 +0,0 @@ -# frozen_string_literal: true - -module HttpHelper - def http_client(options = {}) - timeout = { write: 10, connect: 10, read: 10 }.merge(options) - - HTTP.headers(user_agent: user_agent) - .timeout(:per_operation, timeout) - .follow - end - - private - - def user_agent - @user_agent ||= "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +http://#{Rails.configuration.x.local_domain}/)" - end -end diff --git a/app/lib/provider_discovery.rb b/app/lib/provider_discovery.rb @@ -1,11 +1,9 @@ # frozen_string_literal: true class ProviderDiscovery < OEmbed::ProviderDiscovery - extend HttpHelper - class << self def discover_provider(url, options = {}) - res = http_client.get(url) + res = Request.new(:get, url).perform format = options[:format] raise OEmbed::NotFound, url if res.code != 200 || res.mime_type != 'text/html' diff --git a/app/lib/request.rb b/app/lib/request.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +class Request + REQUEST_TARGET = '(request-target)' + + include RoutingHelper + + def initialize(verb, url, options = {}) + @verb = verb + @url = Addressable::URI.parse(url).normalize + @options = options + @headers = {} + + set_common_headers! + end + + def on_behalf_of(account) + raise ArgumentError unless account.local? + @account = account + end + + def add_headers(new_headers) + @headers.merge!(new_headers) + end + + def perform + http_client.headers(headers).public_send(@verb, @url.to_s, @options) + end + + def headers + (@account ? @headers.merge('Signature' => signature) : @headers).without(REQUEST_TARGET) + end + + private + + def set_common_headers! + @headers[REQUEST_TARGET] = "#{@verb} #{@url.path}" + @headers['User-Agent'] = user_agent + @headers['Host'] = @url.host + @headers['Date'] = Time.now.utc.httpdate + 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)) + + "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers}\",signature=\"#{signature}\"" + end + + def signed_string + @headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n") + end + + def signed_headers + @headers.keys.join(' ').downcase + end + + def user_agent + @user_agent ||= "#{HTTP::Request::USER_AGENT} (Mastodon/#{Mastodon::Version}; +#{root_url})" + end + + def timeout + { write: 10, connect: 10, read: 10 } + end + + def http_client + HTTP.timeout(:per_operation, timeout).follow + end +end diff --git a/app/models/account.rb b/app/models/account.rb @@ -130,7 +130,7 @@ class Account < ApplicationRecord end def subscription(webhook_url) - OStatus2::Subscription.new(remote_url, secret: secret, lease_seconds: 86_400 * 30, webhook: webhook_url, hub: hub_url) + OStatus2::Subscription.new(remote_url, secret: secret, lease_seconds: 30.days.seconds, webhook: webhook_url, hub: hub_url) end def save_with_optional_media! diff --git a/app/models/concerns/remotable.rb b/app/models/concerns/remotable.rb @@ -1,7 +1,6 @@ # frozen_string_literal: true module Remotable - include HttpHelper extend ActiveSupport::Concern included do @@ -20,7 +19,7 @@ module Remotable return if !%w(http https).include?(parsed_url.scheme) || parsed_url.host.empty? || self[attribute_name] == url begin - response = http_client.get(url) + response = Request.new(:get, url).perform return if response.code != 200 diff --git a/app/models/subscription.rb b/app/models/subscription.rb @@ -16,8 +16,8 @@ # class Subscription < ApplicationRecord - MIN_EXPIRATION = 7.days.seconds.to_i - MAX_EXPIRATION = 30.days.seconds.to_i + MIN_EXPIRATION = 1.day.to_i + MAX_EXPIRATION = 30.days.to_i belongs_to :account, required: true diff --git a/app/services/fetch_atom_service.rb b/app/services/fetch_atom_service.rb @@ -1,16 +1,14 @@ # frozen_string_literal: true class FetchAtomService < BaseService - include HttpHelper - def call(url) return if url.blank? - response = http_client.head(url) + response = Request.new(:head, url).perform Rails.logger.debug "Remote status HEAD request returned code #{response.code}" - response = http_client.get(url) if response.code == 405 + response = Request.new(:get, url).perform if response.code == 405 Rails.logger.debug "Remote status GET request returned code #{response.code}" @@ -49,6 +47,6 @@ class FetchAtomService < BaseService end def fetch(url) - http_client.get(url).to_s + Request.new(:get, url).perform.to_s end end diff --git a/app/services/fetch_link_card_service.rb b/app/services/fetch_link_card_service.rb @@ -1,8 +1,6 @@ # frozen_string_literal: true class FetchLinkCardService < BaseService - include HttpHelper - URL_PATTERN = %r{https?://\S+} def call(status) @@ -13,7 +11,7 @@ class FetchLinkCardService < BaseService url = url.to_s card = PreviewCard.where(status: status).first_or_initialize(status: status, url: url) - res = http_client.head(url) + res = Request.new(:head, url).perform return if res.code != 200 || res.mime_type != 'text/html' @@ -80,7 +78,7 @@ class FetchLinkCardService < BaseService end def attempt_opengraph(card, url) - response = http_client.get(url) + response = Request.new(:get, url).perform return if response.code != 200 || response.mime_type != 'text/html' diff --git a/app/services/resolve_remote_account_service.rb b/app/services/resolve_remote_account_service.rb @@ -2,7 +2,6 @@ class ResolveRemoteAccountService < BaseService include OStatus2::MagicKey - include HttpHelper DFRN_NS = 'http://purl.org/macgirvin/dfrn/1.0' @@ -79,7 +78,7 @@ class ResolveRemoteAccountService < BaseService end def get_feed(url) - response = http_client(write: 20, connect: 20, read: 50).get(Addressable::URI.parse(url).normalize) + response = Request.new(:get, url).perform raise Goldfinger::Error, "Feed attempt failed for #{url}: HTTP #{response.code}" unless response.code == 200 [response.to_s, Nokogiri::XML(response)] end diff --git a/app/services/send_interaction_service.rb b/app/services/send_interaction_service.rb @@ -12,13 +12,23 @@ class SendInteractionService < BaseService return if block_notification? - envelope = salmon.pack(@xml, @source_account.keypair) - delivery = salmon.post(@target_account.salmon_url, envelope) + delivery = build_request.perform + raise "Delivery failed for #{target_account.salmon_url}: HTTP #{delivery.code}" unless delivery.code > 199 && delivery.code < 300 end private + def build_request + request = Request.new(:post, @target_account.salmon_url, body: envelope) + request.add_headers('Content-Type' => 'application/magic-envelope+xml') + request + end + + def envelope + salmon.pack(@xml, @source_account.keypair) + end + def block_notification? DomainBlock.blocked?(@target_account.domain) end diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb @@ -2,34 +2,54 @@ class SubscribeService < BaseService def call(account) - account.secret = SecureRandom.hex + @account = account + @account.secret = SecureRandom.hex + @response = build_request.perform - subscription = account.subscription(api_subscription_url(account.id)) - response = subscription.subscribe - - if response_failed_permanently?(response) + if response_failed_permanently? # We're not allowed to subscribe. Fail and move on. - account.secret = '' - account.save! - elsif response_successful?(response) + @account.secret = '' + @account.save! + elsif response_successful? # The subscription will be confirmed asynchronously. - account.save! + @account.save! else # The response was either a 429 rate limit, or a 5xx error. # We need to retry at a later time. Fail loudly! - raise "Subscription attempt failed for #{account.acct} (#{account.hub_url}): HTTP #{response.code}" + raise "Subscription attempt failed for #{@account.acct} (#{@account.hub_url}): HTTP #{@response.code}" end end private + def build_request + request = Request.new(:post, @account.hub_url, form: subscription_params) + request.on_behalf_of(some_local_account) if some_local_account + request + end + + def subscription_params + { + 'hub.topic': @account.remote_url, + 'hub.mode': 'subscribe', + 'hub.callback': api_subscription_url(@account.id), + 'hub.verify': 'async', + 'hub.secret': @account.secret, + 'hub.lease_seconds': 7.days.seconds, + } + end + + def some_local_account + @some_local_account ||= Account.local.first + end + # Any response in the 3xx or 4xx range, except for 429 (rate limit) - def response_failed_permanently?(response) - (response.status.redirect? || response.status.client_error?) && !response.status.too_many_requests? + def response_failed_permanently? + (@response.status.redirect? || @response.status.client_error?) && !@response.status.too_many_requests? end # Any response in the 2xx range - def response_successful?(response) - response.status.success? + def response_successful? + @response.status.success? end end diff --git a/app/services/unsubscribe_service.rb b/app/services/unsubscribe_service.rb @@ -2,17 +2,30 @@ class UnsubscribeService < BaseService def call(account) - subscription = account.subscription(api_subscription_url(account.id)) - response = subscription.unsubscribe + @account = account + @response = build_request.perform - unless response.status.success? - Rails.logger.debug "PuSH unsubscribe for #{account.acct} failed: #{response.status}" - end + Rails.logger.debug "PuSH unsubscribe for #{@account.acct} failed: #{@response.status}" unless @response.status.success? - account.secret = '' - account.subscription_expires_at = nil - account.save! + @account.secret = '' + @account.subscription_expires_at = nil + @account.save! rescue HTTP::Error, OpenSSL::SSL::SSLError - Rails.logger.debug "PuSH subscription request for #{account.acct} could not be made due to HTTP or SSL error" + Rails.logger.debug "PuSH subscription request for #{@account.acct} could not be made due to HTTP or SSL error" + end + + private + + def build_request + Request.new(:post, @account.hub_url, form: subscription_params) + end + + def subscription_params + { + 'hub.topic': @account.remote_url, + 'hub.mode': 'unsubscribe', + 'hub.callback': api_subscription_url(@account.id), + 'hub.verify': 'async', + } end end diff --git a/app/workers/pubsubhubbub/confirmation_worker.rb b/app/workers/pubsubhubbub/confirmation_worker.rb @@ -60,9 +60,7 @@ class Pubsubhubbub::ConfirmationWorker end def callback_get_with_params - HTTP.headers(user_agent: 'Mastodon/PubSubHubbub') - .timeout(:per_operation, write: 20, connect: 20, read: 50) - .get(subscription.callback_url, params: callback_params) + Request.new(:get, subscription.callback_url, params: callback_params).perform end def callback_response_body @@ -71,10 +69,10 @@ class Pubsubhubbub::ConfirmationWorker def callback_params { - 'hub.topic' => account_url(subscription.account, format: :atom), - 'hub.mode' => mode, - 'hub.challenge' => challenge, - 'hub.lease_seconds' => subscription.lease_seconds, + 'hub.topic': account_url(subscription.account, format: :atom), + 'hub.mode': mode, + 'hub.challenge': challenge, + 'hub.lease_seconds': subscription.lease_seconds, } end diff --git a/app/workers/pubsubhubbub/delivery_worker.rb b/app/workers/pubsubhubbub/delivery_worker.rb @@ -33,9 +33,9 @@ class Pubsubhubbub::DeliveryWorker end def callback_post_payload - HTTP.timeout(:per_operation, write: 50, connect: 20, read: 50) - .headers(headers) - .post(subscription.callback_url, body: payload) + request = Request.new(:post, subscription.callback_url, body: payload) + request.add_headers(headers) + request.perform end def blocked_domain? @@ -48,13 +48,12 @@ class Pubsubhubbub::DeliveryWorker def headers { - 'User-Agent' => 'Mastodon/PubSubHubbub', 'Content-Type' => 'application/atom+xml', - 'Link' => link_headers, + 'Link' => link_header, }.merge(signature_headers.to_h) end - def link_headers + def link_header LinkHeader.new([hub_link_header, self_link_header]).to_s end diff --git a/spec/controllers/concerns/signature_verification_spec.rb b/spec/controllers/concerns/signature_verification_spec.rb @@ -0,0 +1,74 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe ApplicationController, type: :controller do + controller do + include SignatureVerification + + def success + head 200 + end + + def alternative_success + head 200 + end + end + + before do + routes.draw { get 'success' => 'anonymous#success' } + end + + context 'without signature header' do + before do + get :success + end + + describe '#signed_request?' do + it 'returns false' do + expect(controller.signed_request?).to be false + end + end + + describe '#signed_request_account' do + it 'returns nil' do + expect(controller.signed_request_account).to be_nil + end + end + end + + context 'with signature header' do + let!(:author) { Fabricate(:account) } + + before do + get :success + + fake_request = Request.new(:get, request.url) + fake_request.on_behalf_of(author) + + request.headers.merge!(fake_request.headers) + end + + 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 +end diff --git a/spec/helpers/http_helper_spec.rb b/spec/helpers/http_helper_spec.rb @@ -1,13 +0,0 @@ -# frozen_string_literal: true - -require 'rails_helper' - -describe HttpHelper do - describe 'http_client' do - it 'returns HTTP::Client with default options' do - options = helper.http_client.default_options - expect(options.headers['User-Agent']).to match /.+ \(Mastodon\/.+;\ \+http:\/\/cb6e6126\.ngrok\.io\/\)/ - expect(options.timeout_options).to eq read_timeout: 10, write_timeout: 10, connect_timeout: 10 - end - end -end diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb @@ -0,0 +1,54 @@ +# frozen_string_literal: true + +require 'rails_helper' + +describe Request do + subject { Request.new(:get, 'http://example.com') } + + describe '#headers' do + it 'returns user agent' do + expect(subject.headers['User-Agent']).to be_present + end + + it 'returns the date header' do + expect(subject.headers['Date']).to be_present + end + + it 'returns the host header' do + expect(subject.headers['Host']).to be_present + end + + it 'does not return virtual request-target header' do + expect(subject.headers['(request-target)']).to be_nil + end + end + + describe '#on_behalf_of' do + it 'when used, adds signature header' do + subject.on_behalf_of(Fabricate(:account)) + expect(subject.headers['Signature']).to be_present + end + end + + describe '#add_headers' do + it 'adds headers to the request' do + subject.add_headers('Test' => 'Foo') + expect(subject.headers['Test']).to eq 'Foo' + end + end + + describe '#perform' do + before do + stub_request(:get, 'http://example.com') + subject.perform + end + + it 'executes a HTTP request' do + expect(a_request(:get, 'http://example.com')).to have_been_made.once + end + + it 'sets headers' do + expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made + end + end +end diff --git a/spec/workers/pubsubhubbub/confirmation_worker_spec.rb b/spec/workers/pubsubhubbub/confirmation_worker_spec.rb @@ -83,6 +83,6 @@ describe Pubsubhubbub::ConfirmationWorker do end def http_headers - { 'Connection' => 'close', 'Host' => 'example.com', 'User-Agent' => 'Mastodon/PubSubHubbub' } + { 'Connection' => 'close', 'Host' => 'example.com', 'User-Agent' => 'http.rb/2.2.2 (Mastodon/1.4.7; +https://cb6e6126.ngrok.io/)' } end end diff --git a/spec/workers/pubsubhubbub/delivery_worker_spec.rb b/spec/workers/pubsubhubbub/delivery_worker_spec.rb @@ -59,7 +59,7 @@ describe Pubsubhubbub::DeliveryWorker do 'Content-Type' => 'application/atom+xml', 'Host' => 'example.com', 'Link' => "<https://#{Rails.configuration.x.local_domain}/api/push>; rel=\"hub\", <https://#{Rails.configuration.x.local_domain}/users/#{subscription.account.username}.atom>; rel=\"self\"", - 'User-Agent' => 'Mastodon/PubSubHubbub', + 'User-Agent' => 'http.rb/2.2.2 (Mastodon/1.4.7; +https://cb6e6126.ngrok.io/)', }.tap do |basic| known_digest = OpenSSL::HMAC.hexdigest(OpenSSL::Digest.new('sha1'), subscription.secret.to_s, payload) basic.merge('X-Hub-Signature' => "sha1=#{known_digest}") if subscription.secret?