Fix refreshing keys from stale actors on signature verification error (#38515)

app/controllers/concerns/signature_verification.rb

@@ -133,11 +133,22 @@ module SignatureVerification
   end
 
   def keypair_refresh_key!(keypair)
-    # TODO: this currently only is concerned with refreshing the actor and returning the legacy key, this needs to be reworked
     return if keypair.actor.local? || !keypair.actor.activitypub?
-    return keypair.actor.refresh! if keypair.actor.respond_to?(:refresh!) && keypair.actor.possibly_stale?
 
-    Keypair.from_legacy_account(ActivityPub::FetchRemoteActorService.new.call(keypair.actor.uri, only_key: true, suppress_errors: false))
+    actor = if keypair.actor.possibly_stale?
+              # Doing a full profile refresh
+              keypair.actor.refresh!
+            else
+              # Only refreshing keys, skipping potentially more expensive requests
+              ActivityPub::FetchRemoteActorService.new.call(keypair.actor.uri, only_key: true, suppress_errors: false)
+            end
+
+    keypair_uri = keypair.uri
+
+    keypair = actor.keypairs.find_by(uri: keypair_uri)
+    return keypair if keypair.present?
+
+    Keypair.from_legacy_account(actor, uri: keypair_uri) if actor.public_key.present?
   rescue Mastodon::PrivateNetworkAddressError => e
     raise Mastodon::SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})"
   rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteActorService::Error, Webfinger::Error => e

app/services/activitypub/fetch_remote_key_service.rb

@@ -6,6 +6,8 @@ class ActivityPub::FetchRemoteKeyService < BaseService
   class Error < StandardError; end
 
   # Returns actor that owns the key
+  # This is to be used when we know a key URI but don't know the associated account URI,
+  # otherwise use `ActivityPub::FetchRemoteActorService`.
   def call(uri, suppress_errors: true)
     raise Error, 'No key URI given' if uri.blank?
 
@@ -44,9 +46,14 @@ class ActivityPub::FetchRemoteKeyService < BaseService
   end
 
   def find_actor(uri, prefetched_body)
-    actor   = ActivityPub::TagManager.instance.uri_to_actor(uri)
-    actor ||= ActivityPub::FetchRemoteActorService.new.call(uri, prefetched_body: prefetched_body, suppress_errors: @suppress_errors)
-    actor
+    # `FetchRemoteKeyService` is called when we don't know of a key.
+    # This is most likely because we don't know of an account yet, but it could also be because we have stale data.
+    # Return the actor if it is known and has been updated recently, otherwise, process it in full.
+
+    actor = ActivityPub::TagManager.instance.uri_to_actor(uri)
+    return actor if actor.present? && !actor.possibly_stale?
+
+    ActivityPub::FetchRemoteActorService.new.call(uri, prefetched_body: prefetched_body, suppress_errors: @suppress_errors)
   end
 
   def expected_type?

app/services/activitypub/process_account_service.rb

@@ -32,8 +32,15 @@ class ActivityPub::ProcessAccountService < BaseService
     @options[:request_id] ||= "#{Time.now.utc.to_i}-#{username}@#{domain}"
 
     with_redis_lock("process_account:#{@uri}") do
-      @account            = Account.remote.find_by(uri: @uri) if @options[:only_key]
-      @account          ||= Account.find_remote(@username, @domain)
+      if @options[:only_key]
+        # `only_key` is used to update an existing account known by its `uri`.
+        # Lookup by handle and new account creation do not make sense in this case.
+        @account = Account.remote.find_by(uri: @uri)
+        return if @account.nil?
+      else
+        @account = Account.find_remote(@username, @domain)
+      end
+
       @old_public_keys = @account.present? ? (@account.keypairs.pluck(:public_key) + [@account.public_key.presence].compact) : []
       @old_protocol = @account&.protocol
       @suspension_changed = false

spec/requests/signature_verification_spec.rb

@@ -71,7 +71,25 @@ RSpec.describe 'signature verification concern' do
 
   context 'with an HTTP Signature (draft version)' do
     context 'with a known account' do
-      let!(:actor) { Fabricate(:account, username: 'bob', domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, public_key: actor_keypair.public_key.to_pem) }
+      let!(:actor) { Fabricate(:account, username: 'bob', domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, public_key: actor_keypair.public_key.to_pem, protocol: :activitypub) }
+
+      let(:actor_json) do
+        {
+          '@context': [
+            'https://www.w3.org/ns/activitystreams',
+            'https://w3id.org/security/v1',
+          ],
+          id: ActivityPub::TagManager.instance.uri_for(actor),
+          type: 'Person',
+          preferredUsername: 'bob',
+          inbox: 'https://remote.domain/inbox',
+          publicKey: {
+            id: 'https://remote.domain/users/bob#main-key',
+            owner: 'https://remote.domain/users/bob',
+            publicKeyPem: actor_keypair.public_key.to_pem,
+          },
+        }
+      end
 
       context 'with an acct key ID' do
         let(:signature_header) do
@@ -115,6 +133,30 @@ RSpec.describe 'signature verification concern' do
             signature_actor_id: actor.id.to_s
           )
         end
+
+        context 'when the key material has changed' do
+          # Let the user be known with the default test keys
+          let!(:actor) { Fabricate(:account, username: 'bob', domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, protocol: :activitypub) }
+
+          # Needed to update the keypair
+          before { stub_key_requests }
+
+          it 'successfuly verifies signature after refreshing the account', :aggregate_failures do
+            expect(signature_header).to eq build_signature_string(actor_keypair, 'https://remote.domain/users/bob#main-key', 'get /activitypub/success', { 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', 'Host' => 'www.example.com' })
+
+            get '/activitypub/success', headers: {
+              'Host' => 'www.example.com',
+              'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT',
+              'Signature' => signature_header,
+            }
+
+            expect(response).to have_http_status(200)
+            expect(response.parsed_body).to match(
+              signed_request: true,
+              signature_actor_id: actor.id.to_s
+            )
+          end
+        end
       end
 
       context 'with a valid signature on a GET request that has a query string' do
@@ -166,6 +208,9 @@ RSpec.describe 'signature verification concern' do
           'keyId="https://remote.domain/users/bob#main-key",algorithm="rsa-sha256",headers="date host (request-target)",signature="SDMa4r/DQYMXYxVgYO2yEqGWWUXugKjVuz0I8dniQAk+aunzBaF2aPu+4grBfawAshlx1Xytl8lhb0H2MllEz16/tKY7rUrb70MK0w8ohXgpb0qs3YvQgdj4X24L1x2MnkFfKHR/J+7TBlnivq0HZqXm8EIkPWLv+eQxu8fbowLwHIVvRd/3t6FzvcfsE0UZKkoMEX02542MhwSif6cu7Ec/clsY9qgKahb9JVGOGS1op9Lvg/9y1mc8KCgD83U5IxVygYeYXaVQ6gixA9NgZiTCwEWzHM5ELm7w5hpdLFYxYOHg/3G3fiqJzpzNQAcCD4S4JxfE7hMI0IzVlNLT6A=="' # rubocop:disable Layout/LineLength
         end
 
+        # Signature verification will fail, so the key will be refetched
+        before { stub_key_requests }
+
         it 'fails to verify signature', :aggregate_failures do
           expect(signature_header).to eq build_signature_string(actor_keypair, 'https://remote.domain/users/bob#main-key', 'get /activitypub/success?foo=42', { 'Date' => 'Wed, 20 Dec 2023 10:00:00 GMT', 'Host' => 'www.example.com' })
 
@@ -184,6 +229,9 @@ RSpec.describe 'signature verification concern' do
       end
 
       context 'with a mismatching path' do
+        # Signature verification will fail, so the key will be refetched
+        before { stub_key_requests }
+
         it 'fails to verify signature', :aggregate_failures do
           get '/activitypub/alternative-path', headers: {
             'Host' => 'www.example.com',
@@ -335,6 +383,9 @@ RSpec.describe 'signature verification concern' do
       end
 
       context 'with a tampered path in a POST request' do
+        # Signature verification will fail, so the key will be refetched
+        before { stub_key_requests }
+
         it 'fails to verify signature', :aggregate_failures do
           post '/activitypub/alternative-path', params: 'Hello world', headers: {
             'Host' => 'www.example.com',
@@ -685,6 +736,11 @@ RSpec.describe 'signature verification concern' do
     end
   end
 
+  def stub_key_requests
+    stub_request(:get, 'https://remote.domain/users/bob').to_return(status: 200, body: actor_json.to_json, headers: { 'Content-Type': 'application/activity+json' })
+    stub_request(:get, 'https://remote.domain/users/bob#main-key').to_return(status: 200, body: actor_json.to_json, headers: { 'Content-Type': 'application/activity+json' })
+  end
+
   def activitypub_tests_controller
     Class.new(ApplicationController) do
       include SignatureVerification