Improvements to signature verification (#9667)

* Refactor signature verification a bit

* Rescue signature verification if recorded public key is invalid

Fixes #8822

* Always re-fetch AP signing key when HTTP Signature verification fails

But when the account is not marked as stale, avoid fetching collections and
media, and avoid webfinger round-trip.

* Apply stoplight to key/account update as well as initial key retrieval

app/controllers/concerns/signature_verification.rb

@@ -60,24 +60,27 @@ module SignatureVerification
     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
-    elsif account.possibly_stale?
-      account = account.refresh!
+    return account unless verify_signature(account, signature, compare_signed_string).nil?
+
+    account_stoplight = Stoplight("source:#{request.ip}") { account.possibly_stale? ? account.refresh! : account_refresh_key(account) }
+      .with_fallback { nil }
+      .with_threshold(1)
+      .with_cool_off_time(5.minutes.seconds)
+      .with_error_handler { |error, handle| error.is_a?(HTTP::Error) ? handle.call(error) : raise(error) }
+
+    account = account_stoplight.run
+
+    if account.nil?
+      @signature_verification_failure_reason = "Public key not found for key #{signature_params['keyId']}"
+      @signed_request_account = nil
+      return
+    end
+
+    return account unless verify_signature(account, signature, compare_signed_string).nil?
 
-      if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string)
-        @signed_request_account = account
-        @signed_request_account
-      else
     @signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}"
     @signed_request_account = nil
   end
-    else
-      @signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}"
-      @signed_request_account = nil
-    end
-  end
 
   def request_body
     @request_body ||= request.raw_post
@@ -85,6 +88,15 @@ module SignatureVerification
 
   private
 
+  def verify_signature(account, signature, compare_signed_string)
+    if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string)
+      @signed_request_account = account
+      @signed_request_account
+    end
+  rescue OpenSSL::PKey::RSAError
+    nil
+  end
+
   def build_signed_string(signed_headers)
     signed_headers = 'date' if signed_headers.blank?
 
@@ -131,4 +143,9 @@ module SignatureVerification
       account
     end
   end
+
+  def account_refresh_key(account)
+    return if account.local? || !account.activitypub?
+    ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true)
+  end
 end

app/services/activitypub/fetch_remote_account_service.rb

@@ -5,8 +5,8 @@ class ActivityPub::FetchRemoteAccountService < BaseService
 
   SUPPORTED_TYPES = %w(Application Group Organization Person Service).freeze
 
-  # Does a WebFinger roundtrip on each call
-  def call(uri, id: true, prefetched_body: nil, break_on_redirect: false)
+  # Does a WebFinger roundtrip on each call, unless `only_key` is true
+  def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false)
     return ActivityPub::TagManager.instance.uri_to_resource(uri, Account) if ActivityPub::TagManager.instance.local_uri?(uri)
 
     @json = if prefetched_body.nil?
@@ -21,9 +21,9 @@ class ActivityPub::FetchRemoteAccountService < BaseService
     @username = @json['preferredUsername']
     @domain   = Addressable::URI.parse(@uri).normalized_host
 
-    return unless verified_webfinger?
+    return unless only_key || verified_webfinger?
 
-    ActivityPub::ProcessAccountService.new.call(@username, @domain, @json)
+    ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key)
   rescue Oj::ParseError
     nil
   end

app/services/activitypub/process_account_service.rb

@@ -33,8 +33,10 @@ class ActivityPub::ProcessAccountService < BaseService
 
     after_protocol_change! if protocol_changed?
     after_key_change! if key_changed? && !@options[:signed_with_known_key]
+    unless @options[:only_key]
       check_featured_collection! if @account.featured_collection_url.present?
       check_links! unless @account.fields.empty?
+    end
 
     @account
   rescue Oj::ParseError
@@ -54,11 +56,11 @@ class ActivityPub::ProcessAccountService < BaseService
   end
 
   def update_account
-    @account.last_webfingered_at = Time.now.utc
+    @account.last_webfingered_at = Time.now.utc unless @options[:only_key]
     @account.protocol            = :activitypub
 
     set_immediate_attributes!
-    set_fetchable_attributes!
+    set_fetchable_attributes! unless @options[:only_keys]
 
     @account.save_with_optional_media!
   end