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
This commit is contained in:
ThibG 2019-01-07 21:45:13 +01:00 committed by Eugen Rochko
parent cf3c0fc38c
commit 28b482874a
3 changed files with 41 additions and 22 deletions

View File

@ -60,24 +60,27 @@ module SignatureVerification
signature = Base64.decode64(signature_params['signature']) signature = Base64.decode64(signature_params['signature'])
compare_signed_string = build_signed_string(signature_params['headers']) compare_signed_string = build_signed_string(signature_params['headers'])
if account.keypair.public_key.verify(OpenSSL::Digest::SHA256.new, signature, compare_signed_string) return account unless verify_signature(account, signature, compare_signed_string).nil?
@signed_request_account = account
@signed_request_account account_stoplight = Stoplight("source:#{request.ip}") { account.possibly_stale? ? account.refresh! : account_refresh_key(account) }
elsif account.possibly_stale? .with_fallback { nil }
account = account.refresh! .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}" @signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}"
@signed_request_account = nil @signed_request_account = nil
end end
else
@signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri}"
@signed_request_account = nil
end
end
def request_body def request_body
@request_body ||= request.raw_post @request_body ||= request.raw_post
@ -85,6 +88,15 @@ module SignatureVerification
private 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) def build_signed_string(signed_headers)
signed_headers = 'date' if signed_headers.blank? signed_headers = 'date' if signed_headers.blank?
@ -131,4 +143,9 @@ module SignatureVerification
account account
end end
end end
def account_refresh_key(account)
return if account.local? || !account.activitypub?
ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true)
end
end end

View File

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

View File

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