Improve error reporting and logging when processing remote accounts (#15605)

* Add a more descriptive PrivateNetworkAddressError exception class

* Remove unnecessary exception class to rescue clause

* Remove unnecessary include to JsonLdHelper

* Give more neutral error message when too many webfinger redirects

* Remove unnecessary guard condition

* Rework how “ActivityPub::FetchRemoteAccountService” handles errors

Add “suppress_errors” keyword argument to avoid raising errors in
ActivityPub::FetchRemoteAccountService#call (default/previous behavior).

* Rework how “ActivityPub::FetchRemoteKeyService” handles errors

Add “suppress_errors” keyword argument to avoid raising errors in
ActivityPub::FetchRemoteKeyService#call (default/previous behavior).

* Fix Webfinger::RedirectError not being a subclass of Webfinger::Error

* Add suppress_errors option to ResolveAccountService

Defaults to true (to preserve previous behavior). If set to false,
errors will be raised instead of caught, allowing the caller to be
informed of what went wrong.

* Return more precise error when failing to fetch account signing AP payloads

* Add tests

* Fixes

* Refactor error handling a bit

* Fix various issues

* Add specific error when provided Digest is not 256 bits of base64-encoded data

* Please CodeClimate

* Improve webfinger error reporting
This commit is contained in:
Claire 2022-09-20 23:30:26 +02:00 committed by GitHub
parent 14c7c9e40e
commit 1145dbd327
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
10 changed files with 158 additions and 47 deletions

View file

@ -93,11 +93,15 @@ module SignatureVerification
return account unless verify_signature(account, signature, compare_signed_string).nil? return account unless verify_signature(account, signature, compare_signed_string).nil?
@signature_verification_failure_reason = "Verification failed for #{account.username}@#{account.domain} #{account.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)" fail_with! "Verification failed for #{account.username}@#{account.domain} #{account.uri} using rsa-sha256 (RSASSA-PKCS1-v1_5 with SHA-256)"
@signed_request_account = nil
rescue SignatureVerificationError => e rescue SignatureVerificationError => e
@signature_verification_failure_reason = e.message fail_with! e.message
@signed_request_account = nil rescue HTTP::Error, OpenSSL::SSL::SSLError => e
fail_with! "Failed to fetch remote data: #{e.message}"
rescue Mastodon::UnexptectedResponseError
fail_with! 'Failed to fetch remote data (got unexpected reply from server)'
rescue Stoplight::Error::RedLight
fail_with! 'Fetching attempt skipped because of recent connection failure'
end end
def request_body def request_body
@ -106,6 +110,11 @@ module SignatureVerification
private private
def fail_with!(message)
@signature_verification_failure_reason = message
@signed_request_account = nil
end
def signature_params def signature_params
@signature_params ||= begin @signature_params ||= begin
raw_signature = request.headers['Signature'] raw_signature = request.headers['Signature']
@ -138,7 +147,17 @@ module SignatureVerification
digests = request.headers['Digest'].split(',').map { |digest| digest.split('=', 2) }.map { |key, value| [key.downcase, value] } digests = request.headers['Digest'].split(',').map { |digest| digest.split('=', 2) }.map { |key, value| [key.downcase, value] }
sha256 = digests.assoc('sha-256') sha256 = digests.assoc('sha-256')
raise SignatureVerificationError, "Mastodon only supports SHA-256 in Digest header. Offered algorithms: #{digests.map(&:first).join(', ')}" if sha256.nil? raise SignatureVerificationError, "Mastodon only supports SHA-256 in Digest header. Offered algorithms: #{digests.map(&:first).join(', ')}" if sha256.nil?
raise SignatureVerificationError, "Invalid Digest value. Computed SHA-256 digest: #{body_digest}; given: #{sha256[1]}" if body_digest != sha256[1]
return if body_digest == sha256[1]
digest_size = begin
Base64.strict_decode64(sha256[1].strip).length
rescue ArgumentError
raise SignatureVerificationError, "Invalid Digest value. The provided Digest value is not a valid base64 string. Given digest: #{sha256[1]}"
end
raise SignatureVerificationError, "Invalid Digest value. The provided Digest value is not a SHA-256 digest. Given digest: #{sha256[1]}" if digest_size != 32
raise SignatureVerificationError, "Invalid Digest value. Computed SHA-256 digest: #{body_digest}; given: #{sha256[1]}"
end end
def verify_signature(account, signature, compare_signed_string) def verify_signature(account, signature, compare_signed_string)
@ -216,19 +235,20 @@ module SignatureVerification
end end
if key_id.start_with?('acct:') if key_id.start_with?('acct:')
stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, '')) } stoplight_wrap_request { ResolveAccountService.new.call(key_id.gsub(/\Aacct:/, ''), suppress_errors: false) }
elsif !ActivityPub::TagManager.instance.local_uri?(key_id) elsif !ActivityPub::TagManager.instance.local_uri?(key_id)
account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account) account = ActivityPub::TagManager.instance.uri_to_resource(key_id, Account)
account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false) } account ||= stoplight_wrap_request { ActivityPub::FetchRemoteKeyService.new.call(key_id, id: false, suppress_errors: false) }
account account
end end
rescue Mastodon::HostValidationError rescue Mastodon::PrivateNetworkAddressError => e
nil raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})"
rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteAccountService::Error, ActivityPub::FetchRemoteKeyService::Error, Webfinger::Error => e
raise SignatureVerificationError, e.message
end end
def stoplight_wrap_request(&block) def stoplight_wrap_request(&block)
Stoplight("source:#{request.remote_ip}", &block) Stoplight("source:#{request.remote_ip}", &block)
.with_fallback { nil }
.with_threshold(1) .with_threshold(1)
.with_cool_off_time(5.minutes.seconds) .with_cool_off_time(5.minutes.seconds)
.with_error_handler { |error, handle| error.is_a?(HTTP::Error) || error.is_a?(OpenSSL::SSL::SSLError) ? handle.call(error) : raise(error) } .with_error_handler { |error, handle| error.is_a?(HTTP::Error) || error.is_a?(OpenSSL::SSL::SSLError) ? handle.call(error) : raise(error) }
@ -237,6 +257,10 @@ module SignatureVerification
def account_refresh_key(account) def account_refresh_key(account)
return if account.local? || !account.activitypub? return if account.local? || !account.activitypub?
ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true) ActivityPub::FetchRemoteAccountService.new.call(account.uri, only_key: true, suppress_errors: false)
rescue Mastodon::PrivateNetworkAddressError => e
raise SignatureVerificationError, "Requests to private network addresses are disallowed (tried to query #{e.host})"
rescue Mastodon::HostValidationError, ActivityPub::FetchRemoteAccountService::Error, Webfinger::Error => e
raise SignatureVerificationError, e.message
end end
end end

View file

@ -208,7 +208,7 @@ class Request
addresses.each do |address| addresses.each do |address|
begin begin
check_private_address(address) check_private_address(address, host)
sock = ::Socket.new(address.is_a?(Resolv::IPv6) ? ::Socket::AF_INET6 : ::Socket::AF_INET, ::Socket::SOCK_STREAM, 0) sock = ::Socket.new(address.is_a?(Resolv::IPv6) ? ::Socket::AF_INET6 : ::Socket::AF_INET, ::Socket::SOCK_STREAM, 0)
sockaddr = ::Socket.pack_sockaddr_in(port, address.to_s) sockaddr = ::Socket.pack_sockaddr_in(port, address.to_s)
@ -264,10 +264,10 @@ class Request
alias new open alias new open
def check_private_address(address) def check_private_address(address, host)
addr = IPAddr.new(address.to_s) addr = IPAddr.new(address.to_s)
return if private_address_exceptions.any? { |range| range.include?(addr) } return if private_address_exceptions.any? { |range| range.include?(addr) }
raise Mastodon::HostValidationError if PrivateAddressCheck.private_address?(addr) raise Mastodon::PrivateNetworkAddressError, host if PrivateAddressCheck.private_address?(addr)
end end
def private_address_exceptions def private_address_exceptions

View file

@ -3,7 +3,7 @@
class Webfinger class Webfinger
class Error < StandardError; end class Error < StandardError; end
class GoneError < Error; end class GoneError < Error; end
class RedirectError < StandardError; end class RedirectError < Error; end
class Response class Response
attr_reader :uri attr_reader :uri

View file

@ -5,10 +5,12 @@ class ActivityPub::FetchRemoteAccountService < BaseService
include DomainControlHelper include DomainControlHelper
include WebfingerHelper include WebfingerHelper
class Error < StandardError; end
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, unless `only_key` is true # 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) def call(uri, id: true, prefetched_body: nil, break_on_redirect: false, only_key: false, suppress_errors: true)
return if domain_not_allowed?(uri) return if domain_not_allowed?(uri)
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)
@ -18,38 +20,50 @@ class ActivityPub::FetchRemoteAccountService < BaseService
else else
body_to_json(prefetched_body, compare_id: id ? uri : nil) body_to_json(prefetched_body, compare_id: id ? uri : nil)
end end
rescue Oj::ParseError
raise Error, "Error parsing JSON-LD document #{uri}"
end end
return if !supported_context? || !expected_type? || (break_on_redirect && @json['movedTo'].present?) raise Error, "Error fetching actor JSON at #{uri}" if @json.nil?
raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?
raise Error, "Unexpected object type for actor #{uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_type?
raise Error, "Actor #{uri} has moved to #{@json['movedTo']}" if break_on_redirect && @json['movedTo'].present?
@uri = @json['id'] @uri = @json['id']
@username = @json['preferredUsername'] @username = @json['preferredUsername']
@domain = Addressable::URI.parse(@uri).normalized_host @domain = Addressable::URI.parse(@uri).normalized_host
return unless only_key || verified_webfinger? check_webfinger! unless only_key
ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key) ActivityPub::ProcessAccountService.new.call(@username, @domain, @json, only_key: only_key, verified_webfinger: !only_key)
rescue Oj::ParseError rescue Error => e
nil Rails.logger.debug "Fetching account #{uri} failed: #{e.message}"
raise unless suppress_errors
end end
private private
def verified_webfinger? def check_webfinger!
webfinger = webfinger!("acct:#{@username}@#{@domain}") webfinger = webfinger!("acct:#{@username}@#{@domain}")
confirmed_username, confirmed_domain = split_acct(webfinger.subject) confirmed_username, confirmed_domain = split_acct(webfinger.subject)
return webfinger.link('self', 'href') == @uri if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? if @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero?
raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
return
end
webfinger = webfinger!("acct:#{confirmed_username}@#{confirmed_domain}") webfinger = webfinger!("acct:#{confirmed_username}@#{confirmed_domain}")
@username, @domain = split_acct(webfinger.subject) @username, @domain = split_acct(webfinger.subject)
return false unless @username.casecmp(confirmed_username).zero? && @domain.casecmp(confirmed_domain).zero? unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
return false if webfinger.link('self', 'href') != @uri raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{uri} (stopped at #{@username}@#{@domain})"
end
true raise Error, "Webfinger response for #{@username}@#{@domain} does not loop back to #{@uri}" if webfinger.link('self', 'href') != @uri
rescue Webfinger::Error rescue Webfinger::RedirectError => e
false raise Error, e.message
rescue Webfinger::Error => e
raise Error, "Webfinger error when resolving #{@username}@#{@domain}: #{e.message}"
end end
def split_acct(acct) def split_acct(acct)

View file

@ -3,9 +3,11 @@
class ActivityPub::FetchRemoteKeyService < BaseService class ActivityPub::FetchRemoteKeyService < BaseService
include JsonLdHelper include JsonLdHelper
class Error < StandardError; end
# Returns account that owns the key # Returns account that owns the key
def call(uri, id: true, prefetched_body: nil) def call(uri, id: true, prefetched_body: nil, suppress_errors: true)
return if uri.blank? raise Error, 'No key URI given' if uri.blank?
if prefetched_body.nil? if prefetched_body.nil?
if id if id
@ -13,7 +15,7 @@ class ActivityPub::FetchRemoteKeyService < BaseService
if person? if person?
@json = fetch_resource(@json['id'], true) @json = fetch_resource(@json['id'], true)
elsif uri != @json['id'] elsif uri != @json['id']
return raise Error, "Fetched URI #{uri} has wrong id #{@json['id']}"
end end
else else
@json = fetch_resource(uri, id) @json = fetch_resource(uri, id)
@ -22,21 +24,29 @@ class ActivityPub::FetchRemoteKeyService < BaseService
@json = body_to_json(prefetched_body, compare_id: id ? uri : nil) @json = body_to_json(prefetched_body, compare_id: id ? uri : nil)
end end
return unless supported_context?(@json) && expected_type? raise Error, "Unable to fetch key JSON at #{uri}" if @json.nil?
return find_account(@json['id'], @json) if person? raise Error, "Unsupported JSON-LD context for document #{uri}" unless supported_context?(@json)
raise Error, "Unexpected object type for key #{uri}" unless expected_type?
return find_account(@json['id'], @json, suppress_errors) if person?
@owner = fetch_resource(owner_uri, true) @owner = fetch_resource(owner_uri, true)
return unless supported_context?(@owner) && confirmed_owner? raise Error, "Unable to fetch actor JSON #{owner_uri}" if @owner.nil?
raise Error, "Unsupported JSON-LD context for document #{owner_uri}" unless supported_context?(@owner)
raise Error, "Unexpected object type for actor #{owner_uri} (expected any of: #{SUPPORTED_TYPES})" unless expected_owner_type?
raise Error, "publicKey id for #{owner_uri} does not correspond to #{@json['id']}" unless confirmed_owner?
find_account(owner_uri, @owner) find_account(owner_uri, @owner, suppress_errors)
rescue Error => e
Rails.logger.debug "Fetching key #{uri} failed: #{e.message}"
raise unless suppress_errors
end end
private private
def find_account(uri, prefetched_body) def find_account(uri, prefetched_body, suppress_errors)
account = ActivityPub::TagManager.instance.uri_to_resource(uri, Account) account = ActivityPub::TagManager.instance.uri_to_resource(uri, Account)
account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body) account ||= ActivityPub::FetchRemoteAccountService.new.call(uri, prefetched_body: prefetched_body, suppress_errors: suppress_errors)
account account
end end
@ -56,7 +66,11 @@ class ActivityPub::FetchRemoteKeyService < BaseService
@owner_uri ||= value_or_id(@json['owner']) @owner_uri ||= value_or_id(@json['owner'])
end end
def expected_owner_type?
equals_or_includes_any?(@owner['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES)
end
def confirmed_owner? def confirmed_owner?
equals_or_includes_any?(@owner['type'], ActivityPub::FetchRemoteAccountService::SUPPORTED_TYPES) && value_or_id(@owner['publicKey']) == @json['id'] value_or_id(@owner['publicKey']) == @json['id']
end end
end end

View file

@ -32,8 +32,6 @@ class ActivityPub::ProcessAccountService < BaseService
process_duplicate_accounts! if @options[:verified_webfinger] process_duplicate_accounts! if @options[:verified_webfinger]
end end
return if @account.nil?
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]
clear_tombstones! if key_changed? clear_tombstones! if key_changed?

View file

@ -1,7 +1,6 @@
# frozen_string_literal: true # frozen_string_literal: true
class ResolveAccountService < BaseService class ResolveAccountService < BaseService
include JsonLdHelper
include DomainControlHelper include DomainControlHelper
include WebfingerHelper include WebfingerHelper
include Redisable include Redisable
@ -13,6 +12,7 @@ class ResolveAccountService < BaseService
# @param [Hash] options # @param [Hash] options
# @option options [Boolean] :redirected Do not follow further Webfinger redirects # @option options [Boolean] :redirected Do not follow further Webfinger redirects
# @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data # @option options [Boolean] :skip_webfinger Do not attempt any webfinger query or refreshing account data
# @option options [Boolean] :suppress_errors When failing, return nil instead of raising an error
# @return [Account] # @return [Account]
def call(uri, options = {}) def call(uri, options = {})
return if uri.blank? return if uri.blank?
@ -52,15 +52,15 @@ class ResolveAccountService < BaseService
# either needs to be created, or updated from fresh data # either needs to be created, or updated from fresh data
fetch_account! fetch_account!
rescue Webfinger::Error, Oj::ParseError => e rescue Webfinger::Error => e
Rails.logger.debug "Webfinger query for #{@uri} failed: #{e}" Rails.logger.debug "Webfinger query for #{@uri} failed: #{e}"
nil raise unless @options[:suppress_errors]
end end
private private
def process_options!(uri, options) def process_options!(uri, options)
@options = options @options = { suppress_errors: true }.merge(options)
if uri.is_a?(Account) if uri.is_a?(Account)
@account = uri @account = uri
@ -96,7 +96,7 @@ class ResolveAccountService < BaseService
@username, @domain = split_acct(@webfinger.subject) @username, @domain = split_acct(@webfinger.subject)
unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero? unless confirmed_username.casecmp(@username).zero? && confirmed_domain.casecmp(@domain).zero?
raise Webfinger::RedirectError, "The URI #{uri} tries to hijack #{@username}@#{@domain}" raise Webfinger::RedirectError, "Too many webfinger redirects for URI #{uri} (stopped at #{@username}@#{@domain})"
end end
rescue Webfinger::GoneError rescue Webfinger::GoneError
@gone = true @gone = true
@ -110,7 +110,7 @@ class ResolveAccountService < BaseService
return unless activitypub_ready? return unless activitypub_ready?
with_lock("resolve:#{@username}@#{@domain}") do with_lock("resolve:#{@username}@#{@domain}") do
@account = ActivityPub::FetchRemoteAccountService.new.call(actor_url) @account = ActivityPub::FetchRemoteAccountService.new.call(actor_url, suppress_errors: @options[:suppress_errors])
end end
@account @account

View file

@ -25,4 +25,13 @@ module Mastodon
end end
end end
end end
class PrivateNetworkAddressError < HostValidationError
attr_reader :host
def initialize(host)
@host = host
super()
end
end
end end

View file

@ -119,6 +119,58 @@ RSpec.describe ActivityPub::FetchRemoteAccountService, type: :service do
include_examples 'sets profile data' include_examples 'sets profile data'
end end
context 'when WebFinger returns a different URI' do
let!(:webfinger) { { subject: 'acct:alice@example.com', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
before do
stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor))
stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
end
it 'fetches resource' do
account
expect(a_request(:get, 'https://example.com/alice')).to have_been_made.once
end
it 'looks up webfinger' do
account
expect(a_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com')).to have_been_made.once
end
it 'does not create account' do
expect(account).to be_nil
end
end
context 'when WebFinger returns a different URI after a redirection' do
let!(:webfinger) { { subject: 'acct:alice@iscool.af', links: [{ rel: 'self', href: 'https://example.com/bob' }] } }
before do
stub_request(:get, 'https://example.com/alice').to_return(body: Oj.dump(actor))
stub_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
stub_request(:get, 'https://iscool.af/.well-known/webfinger?resource=acct:alice@iscool.af').to_return(body: Oj.dump(webfinger), headers: { 'Content-Type': 'application/jrd+json' })
end
it 'fetches resource' do
account
expect(a_request(:get, 'https://example.com/alice')).to have_been_made.once
end
it 'looks up webfinger' do
account
expect(a_request(:get, 'https://example.com/.well-known/webfinger?resource=acct:alice@example.com')).to have_been_made.once
end
it 'looks up "redirected" webfinger' do
account
expect(a_request(:get, 'https://iscool.af/.well-known/webfinger?resource=acct:alice@iscool.af')).to have_been_made.once
end
it 'does not create account' do
expect(account).to be_nil
end
end
context 'with wrong id' do context 'with wrong id' do
it 'does not create account' do it 'does not create account' do
expect(subject.call('https://fake.address/@foo', prefetched_body: Oj.dump(actor))).to be_nil expect(subject.call('https://fake.address/@foo', prefetched_body: Oj.dump(actor))).to be_nil

View file

@ -137,8 +137,8 @@ RSpec.describe ResolveAccountService, type: :service do
stub_request(:get, 'https://evil.example.com/.well-known/webfinger?resource=acct:foo@evil.example.com').to_return(body: Oj.dump(webfinger2), headers: { 'Content-Type': 'application/jrd+json' }) stub_request(:get, 'https://evil.example.com/.well-known/webfinger?resource=acct:foo@evil.example.com').to_return(body: Oj.dump(webfinger2), headers: { 'Content-Type': 'application/jrd+json' })
end end
it 'returns new remote account' do it 'does not return a new remote account' do
expect { subject.call('Foo@redirected.example.com') }.to raise_error Webfinger::RedirectError expect(subject.call('Foo@redirected.example.com')).to be_nil
end end
end end