From 28b482874ab4393639a77fdd895658096bcbfd57 Mon Sep 17 00:00:00 2001 From: ThibG Date: Mon, 7 Jan 2019 21:45:13 +0100 Subject: [PATCH] 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 --- .../concerns/signature_verification.rb | 45 +++++++++++++------ .../fetch_remote_account_service.rb | 8 ++-- .../activitypub/process_account_service.rb | 10 +++-- 3 files changed, 41 insertions(+), 22 deletions(-) diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 887096e8bd..91566c4fad 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/app/controllers/concerns/signature_verification.rb @@ -60,23 +60,26 @@ 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? - 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}" + 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? + + @signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}" + @signed_request_account = nil end def request_body @@ -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 diff --git a/app/services/activitypub/fetch_remote_account_service.rb b/app/services/activitypub/fetch_remote_account_service.rb index 8430d12d54..3c20449412 100644 --- a/app/services/activitypub/fetch_remote_account_service.rb +++ b/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 diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index d6480028f4..d6c791b449 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/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] - check_featured_collection! if @account.featured_collection_url.present? - check_links! unless @account.fields.empty? + 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