diff --git a/app/controllers/concerns/signature_verification.rb b/app/controllers/concerns/signature_verification.rb index 49b7287239..6f3b93dfcd 100644 --- a/app/controllers/concerns/signature_verification.rb +++ b/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 diff --git a/app/services/activitypub/fetch_remote_key_service.rb b/app/services/activitypub/fetch_remote_key_service.rb index e6f691b288..9b61ac6086 100644 --- a/app/services/activitypub/fetch_remote_key_service.rb +++ b/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? diff --git a/app/services/activitypub/process_account_service.rb b/app/services/activitypub/process_account_service.rb index 65b1d03897..6254539327 100644 --- a/app/services/activitypub/process_account_service.rb +++ b/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 diff --git a/spec/requests/signature_verification_spec.rb b/spec/requests/signature_verification_spec.rb index 1391aeb750..7b863e6897 100644 --- a/spec/requests/signature_verification_spec.rb +++ b/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