From acd0a161e5d957c4206304e856458dd7aef94661 Mon Sep 17 00:00:00 2001 From: Claire Date: Thu, 9 Jul 2026 16:11:51 +0200 Subject: [PATCH] Refactor outgoing HTTP Signature code as HTTP.rb feature/middleware (#39766) --- app/lib/request.rb | 80 +++++++++++++++++++++------------------- spec/lib/request_spec.rb | 35 +++++++++++++----- 2 files changed, 68 insertions(+), 47 deletions(-) diff --git a/app/lib/request.rb b/app/lib/request.rb index f7bbd10446..ee4c486dfc 100644 --- a/app/lib/request.rb +++ b/app/lib/request.rb @@ -60,6 +60,25 @@ class PerOperationWithDeadline < HTTP::Timeout::PerOperation end end +# Implement HTTP Signature cavage draft as a HTTP.rb middleware +class CavageSignatureFeature < HTTP::Feature + def initialize(keypair: nil, covered_headers: []) + super + + @signing = HttpSignatureDraft.new(keypair.keypair, keypair.full_uri) if keypair.present? + @covered_headers = covered_headers + end + + def wrap_request(request) + signed_headers = request.headers.to_h.slice(*@covered_headers) + request.headers['Signature'] = @signing.sign(signed_headers, request.verb, Addressable::URI.parse(request.uri)) + request + end + + # Register this feature with http.rb + ::HTTP::Options.register_feature(:http_cavage_signature, self) +end + class Request # We enforce a 5s timeout on DNS resolving, 5s timeout on socket opening # and 5s timeout on the TLS handshake, meaning the worst case should take @@ -69,6 +88,8 @@ class Request include RoutingHelper + attr_reader :headers, :signing_keypair + def initialize(verb, url, **options) raise ArgumentError if url.blank? @@ -79,7 +100,6 @@ class Request @options = { follow: { max_hops: 3, - on_redirect: ->(response, request) { re_sign_on_redirect(response, request) }, }, }.merge(options).merge( socket_class: use_proxy? || @allow_local ? ProxySocket : Socket, @@ -89,19 +109,17 @@ class Request @options = @options.merge(proxy_url) if use_proxy? @headers = {} - @signing = nil + @signing_keypair = nil raise Mastodon::HostValidationError, 'Instance does not support hidden service connections' if block_hidden_service? set_common_headers! - set_digest! if options.key?(:body) end def on_behalf_of(actor, sign_with: nil) raise ArgumentError, 'actor must not be nil' if actor.nil? - keypair = sign_with.presence || actor.keypair(type: :rsa) - @signing = HttpSignatureDraft.new(keypair.keypair, keypair.full_uri) + @signing_keypair = sign_with.presence || actor.keypair(type: :rsa) self end @@ -113,7 +131,7 @@ class Request def perform begin - response = http_client.request(@verb, @url.to_s, @options.merge(headers: headers)) + response = perform_cavage_signed_request rescue => e raise e.class, "#{e.message} on #{@url}", e.backtrace[0] end @@ -126,10 +144,6 @@ class Request end end - def headers - (@signing ? @headers.merge('Signature' => signature) : @headers) - end - class << self def valid_url?(url) begin @@ -148,6 +162,24 @@ class Request private + def perform_cavage_signed_request + headers = @headers + headers = headers.merge('Digest' => "SHA-256=#{Digest::SHA256.base64digest(@options[:body])}") if @options[:body] + + client = http_client + + if @signing_keypair.present? + client = client.use( + http_cavage_signature: { + keypair: @signing_keypair, + covered_headers: headers.keys - %w(User-Agent Accept-Encoding Accept), + } + ) + end + + client.request(@verb, @url.to_s, @options.merge(headers:)) + end + # Using code from https://github.com/sporkmonger/addressable/blob/3450895887d0a1770660d8831d1b6fcfed9bd9d6/lib/addressable/uri.rb#L1609-L1635 # to preserve some URL Encodings while normalizing def normalize_preserving_url_encodings(url, preserved_chars = SAFE_PRESERVED_CHARS, *flags) @@ -183,34 +215,6 @@ class Request @headers['Accept-Encoding'] = 'gzip' if @verb != :head end - def set_digest! - @headers['Digest'] = "SHA-256=#{Digest::SHA256.base64digest(@options[:body])}" - end - - def signature - @signing.sign(@headers.without('User-Agent', 'Accept-Encoding'), @verb, @url) - end - - def re_sign_on_redirect(_response, request) - # Delete existing signature if there is one, since it will be invalid - request.headers.delete('Signature') - - return unless @signing.present? && @verb == :get - - signed_headers = request.headers.to_h.slice(*@headers.keys) - unless @headers.keys.all? { |key| signed_headers.key?(key) } - # We have lost some headers in the process, so don't sign the new - # request, in order to avoid issuing a valid signature with fewer - # conditions than expected. - - Rails.logger.warn { "Some headers (#{@headers.keys - signed_headers.keys}) have been lost on redirect from {@uri} to #{request.uri}, this should not happen. Skipping signatures" } - return - end - - signature_value = @signing.sign(signed_headers.without('User-Agent', 'Accept-Encoding', 'Accept'), @verb, Addressable::URI.parse(request.uri)) - request.headers['Signature'] = signature_value - end - def http_client @http_client ||= Request.http_client end diff --git a/spec/lib/request_spec.rb b/spec/lib/request_spec.rb index 228c8bbb67..f8c2de203a 100644 --- a/spec/lib/request_spec.rb +++ b/spec/lib/request_spec.rb @@ -27,9 +27,9 @@ RSpec.describe Request do end describe '#on_behalf_of' do - it 'when used, adds signature header' do + it 'when used, sets signing keypair' do subject.on_behalf_of(Fabricate(:account)) - expect(subject.headers['Signature']).to be_present + expect(subject.signing_keypair).to be_present end end @@ -110,14 +110,12 @@ RSpec.describe Request do expect { |block| subject.on_behalf_of(account).perform(&block) }.to yield_control # request.headers includes the `Signature` sent for the first request - expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made.once + expect(a_request(:get, 'http://example.com').with(headers: subject.headers.merge('Signature' => /.*/))).to have_been_made.once + expect(a_request(:get, 'http://example.com').with { |request| verified_signed_mocked_request?(request, account.keypair) }).to have_been_made.once - # request.headers includes the `Signature`, but it has changed - expect(a_request(:get, 'http://redirected.example.com/foo').with(headers: subject.headers.merge({ 'Host' => 'redirected.example.com' }))).to_not have_been_made - - # `with(headers: )` matching tests for inclusion, so strip `Signature` - # This doesn't actually test that there is a signature, but it tests that the original signature is not passed - expect(a_request(:get, 'http://redirected.example.com/foo').with(headers: subject.headers.without('Signature').merge({ 'Host' => 'redirected.example.com' }))).to have_been_made.once + # This doesn't actually test that the signature has changed, but I verified this manually + expect(a_request(:get, 'http://redirected.example.com/foo').with(headers: subject.headers.merge({ 'Host' => 'redirected.example.com', 'Signature' => /.*/ }))).to have_been_made.once + expect(a_request(:get, 'http://redirected.example.com/foo').with { |request| verified_signed_mocked_request?(request, account.keypair) }).to have_been_made.once end end @@ -225,4 +223,23 @@ RSpec.describe Request do expect(subject.perform { |response| response.body_with_limit.encoding }).to eq Encoding::UTF_8 end end + + def verified_signed_mocked_request?(webmock_request, keypair) + # Webmock requests are rather barebones and our signature verification + # code works with `ActionDispatch::Request` objects. + + # This method builds a test request from the webmock request first, + # but this currently does not cover requests with bodies. + # A better way would probably be to build a whole Rack env, but + # that code was a lot more straightforward, and sufficient for + # the added test. + + request = ActionDispatch::TestRequest.create + request.request_uri = webmock_request.uri + request.path = webmock_request.uri.path + request.request_method = webmock_request.method + request.headers.merge!(webmock_request.headers) + + SignedRequest.new(request).verified?(keypair) + end end