Fix incorrect signature after HTTP redirect (mastodon#33757)

app/controllers/concerns/signature_verification.rb

@@ -117,7 +117,7 @@ def signed_headers
 
   def verify_signature_strength!
     raise SignatureVerificationError, 'Mastodon requires the Date header or (created) pseudo-header to be signed' unless signed_headers.include?('date') || signed_headers.include?('(created)')
-    raise SignatureVerificationError, 'Mastodon requires the Digest header or (request-target) pseudo-header to be signed' unless signed_headers.include?(Request::REQUEST_TARGET) || signed_headers.include?('digest')
+    raise SignatureVerificationError, 'Mastodon requires the Digest header or (request-target) pseudo-header to be signed' unless signed_headers.include?(HttpSignatureDraft::REQUEST_TARGET) || signed_headers.include?('digest')
     raise SignatureVerificationError, 'Mastodon requires the Host header to be signed when doing a GET request' if request.get? && !signed_headers.include?('host')
     raise SignatureVerificationError, 'Mastodon requires the Digest header to be signed when doing a POST request' if request.post? && !signed_headers.include?('digest')
   end
@@ -155,14 +155,14 @@ def verify_signature(actor, signature, compare_signed_string)
   def build_signed_string(include_query_string: true)
     signed_headers.map do |signed_header|
       case signed_header
-      when Request::REQUEST_TARGET
+      when HttpSignatureDraft::REQUEST_TARGET
         if include_query_string
-          "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.original_fullpath}"
+          "#{HttpSignatureDraft::REQUEST_TARGET}: #{request.method.downcase} #{request.original_fullpath}"
         else
           # Current versions of Mastodon incorrectly omit the query string from the (request-target) pseudo-header.
           # Therefore, temporarily support such incorrect signatures for compatibility.
           # TODO: remove eventually some time after release of the fixed version
-          "#{Request::REQUEST_TARGET}: #{request.method.downcase} #{request.path}"
+          "#{HttpSignatureDraft::REQUEST_TARGET}: #{request.method.downcase} #{request.path}"
         end
       when '(created)'
         raise SignatureVerificationError, 'Invalid pseudo-header (created) for rsa-sha256' unless signature_algorithm == 'hs2019'

app/lib/http_signature_draft.rb

@@ -0,0 +1,31 @@
+# frozen_string_literal: true
+
+# This implements an older draft of HTTP Signatures:
+# https://datatracker.ietf.org/doc/html/draft-cavage-http-signatures
+
+class HttpSignatureDraft
+  REQUEST_TARGET = '(request-target)'
+
+  def initialize(keypair, key_id, full_path: true)
+    @keypair = keypair
+    @key_id = key_id
+    @full_path = full_path
+  end
+
+  def request_target(verb, url)
+    if url.query.nil? || !@full_path
+      "#{verb} #{url.path}"
+    else
+      "#{verb} #{url.path}?#{url.query}"
+    end
+  end
+
+  def sign(signed_headers, verb, url)
+    signed_headers = signed_headers.merge(REQUEST_TARGET => request_target(verb, url))
+    signed_string = signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n")
+    algorithm = 'rsa-sha256'
+    signature = Base64.strict_encode64(@keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string))
+
+    "keyId=\"#{@key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\""
+  end
+end

app/lib/request.rb

@@ -61,8 +61,6 @@ def readpartial(size, buffer = nil)
 end
 
 class Request
-  REQUEST_TARGET = '(request-target)'
-
   # 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
   # about 15s in total
@@ -78,11 +76,18 @@ def initialize(verb, url, **options)
     @http_client = options.delete(:http_client)
     @allow_local = options.delete(:allow_local)
     @full_path   = !options.delete(:omit_query_string)
-    @options     = options.merge(socket_class: use_proxy? || @allow_local ? ProxySocket : Socket)
-    @options     = @options.merge(timeout_class: PerOperationWithDeadline, timeout_options: TIMEOUT)
+    @options     = {
+      follow: {
+        max_hops: 3,
+        on_redirect: ->(response, request) { re_sign_on_redirect(response, request) },
+      },
+      socket_class: use_proxy? || @allow_local ? ProxySocket : Socket,
+    }.merge(options)
     @options     = @options.merge(proxy_url) if use_proxy?
     @headers     = {}
 
+    @signing = nil
+
     raise Mastodon::HostValidationError, 'Instance does not support hidden service connections' if block_hidden_service?
 
     set_common_headers!
@@ -92,8 +97,9 @@ def initialize(verb, url, **options)
   def on_behalf_of(actor, sign_with: nil)
     raise ArgumentError, 'actor must not be nil' if actor.nil?
 
-    @actor         = actor
-    @keypair       = sign_with.present? ? OpenSSL::PKey::RSA.new(sign_with) : @actor.keypair
+    key_id = ActivityPub::TagManager.instance.key_uri_for(actor)
+    keypair = sign_with.present? ? OpenSSL::PKey::RSA.new(sign_with) : actor.keypair
+    @signing = HttpSignatureDraft.new(keypair, key_id, full_path: @full_path)
 
     self
   end
@@ -119,7 +125,7 @@ def perform
   end
 
   def headers
-    (@actor ? @headers.merge('Signature' => signature) : @headers).without(REQUEST_TARGET)
+    (@signing ? @headers.merge('Signature' => signature) : @headers)
   end
 
   class << self
@@ -134,14 +140,13 @@ def valid_url?(url)
     end
 
     def http_client
-      HTTP.use(:auto_inflate).follow(max_hops: 3)
+      HTTP.use(:auto_inflate)
     end
   end
 
   private
 
   def set_common_headers!
-    @headers[REQUEST_TARGET]    = request_target
     @headers['User-Agent']      = Mastodon::Version.user_agent
     @headers['Host']            = @url.host
     @headers['Date']            = Time.now.utc.httpdate
@@ -152,31 +157,28 @@ def set_digest!
     @headers['Digest'] = "SHA-256=#{Digest::SHA256.base64digest(@options[:body])}"
   end
 
-  def request_target
-    if @url.query.nil? || !@full_path
-      "#{@verb} #{@url.path}"
-    else
-      "#{@verb} #{@url.path}?#{@url.query}"
-    end
+  def signature
+    @signing.sign(@headers.without('User-Agent', 'Accept-Encoding'), @verb, @url)
   end
 
-  def signature
-    algorithm = 'rsa-sha256'
-    signature = Base64.strict_encode64(@keypair.sign(OpenSSL::Digest.new('SHA256'), signed_string))
+  def re_sign_on_redirect(_response, request)
+    # Delete existing signature if there is one, since it will be invalid
+    request.headers.delete('Signature')
 
-    "keyId=\"#{key_id}\",algorithm=\"#{algorithm}\",headers=\"#{signed_headers.keys.join(' ').downcase}\",signature=\"#{signature}\""
-  end
+    return unless @signing.present? && @verb == :get
 
-  def signed_string
-    signed_headers.map { |key, value| "#{key.downcase}: #{value}" }.join("\n")
-  end
+    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.
 
-  def signed_headers
-    @headers.without('User-Agent', 'Accept-Encoding')
-  end
+      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
 
-  def key_id
-    ActivityPub::TagManager.instance.key_uri_for(@actor)
+    signature_value = @signing.sign(signed_headers.without('User-Agent', 'Accept-Encoding'), @verb, Addressable::URI.parse(request.uri))
+    request.headers['Signature'] = signature_value
   end
 
   def http_client

spec/lib/request_spec.rb

@@ -60,16 +60,12 @@
         expect(a_request(:get, 'http://example.com')).to have_been_made.once
       end
 
-      it 'sets headers' do
-        expect { |block| subject.perform(&block) }.to yield_control
-        expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made
-      end
-
-      it 'closes underlying connection' do
+      it 'makes a request with expected headers, yields, and closes the underlying connection' do
         allow(subject.send(:http_client)).to receive(:close)
 
         expect { |block| subject.perform(&block) }.to yield_control
 
+        expect(a_request(:get, 'http://example.com').with(headers: subject.headers)).to have_been_made
         expect(subject.send(:http_client)).to have_received(:close)
       end
 
@@ -80,6 +76,29 @@
       end
     end
 
+    context 'with a redirect and HTTP signatures' do
+      let(:account) { Fabricate(:account) }
+
+      before do
+        stub_request(:get, 'http://example.com').to_return(status: 301, headers: { Location: 'http://redirected.example.com/foo' })
+        stub_request(:get, 'http://redirected.example.com/foo').to_return(body: 'lorem ipsum')
+      end
+
+      it 'makes a request with expected headers and follows redirects' 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
+
+        # 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
+      end
+    end
+
     context 'with private host' do
       around do |example|
         WebMock.disable!