Refactor outgoing HTTP Signature code as HTTP.rb feature/middleware (#39766)

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

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