diff --git a/app/models/account.rb b/app/models/account.rb index 4400b8807ad..9d5c4a78e09 100644 --- a/app/models/account.rb +++ b/app/models/account.rb @@ -490,7 +490,7 @@ class Account < ApplicationRecord before_destroy :clean_feed_manager def ensure_keys! - return unless local? && private_key.blank? && public_key.blank? + return unless local? && private_key.blank? && public_key.blank? && keypairs.empty? generate_keys save! @@ -510,11 +510,10 @@ class Account < ApplicationRecord end def generate_keys - return unless local? && private_key.blank? && public_key.blank? + return unless local? && private_key.blank? && public_key.blank? && keypairs.empty? keypair = OpenSSL::PKey::RSA.new(2048) - self.private_key = keypair.to_pem - self.public_key = keypair.public_key.to_pem + keypairs << keypairs.build(local_fragment: "#rsa-#{SecureRandom.hex(8)}", type: :rsa, public_key: keypair.public_key.to_pem, private_key: keypair.to_pem) end def normalize_domain diff --git a/spec/fabricators/account_fabricator.rb b/spec/fabricators/account_fabricator.rb index 81b92861338..e12c01dea53 100644 --- a/spec/fabricators/account_fabricator.rb +++ b/spec/fabricators/account_fabricator.rb @@ -1,17 +1,35 @@ # frozen_string_literal: true Fabricator(:account) do - transient :suspended, :silenced + transient :suspended, :silenced, :legacy_keypair username { sequence(:username) { |i| "#{Faker::Internet.user_name(separators: %w(_))}#{i}" } } last_webfingered_at { Time.now.utc } - public_key { SigningKeysHelpers::PUBLIC_RSA_TEST_KEY } - private_key { |attrs| attrs[:domain].present? ? nil : SigningKeysHelpers::PRIVATE_RSA_TEST_KEY } + public_key { |attrs| attrs[:legacy_keypair] ? SigningKeysHelpers::PUBLIC_RSA_TEST_KEY : '' } + private_key { |attrs| attrs[:legacy_keypair] && attrs[:domain].nil? ? SigningKeysHelpers::PRIVATE_RSA_TEST_KEY : nil } suspended_at { |attrs| attrs[:suspended] ? Time.now.utc : nil } silenced_at { |attrs| attrs[:silenced] ? Time.now.utc : nil } user { |attrs| attrs[:domain].nil? ? Fabricate.build(:user, account: nil) : nil } uri { |attrs| attrs[:domain].nil? ? '' : "https://#{attrs[:domain]}/users/#{attrs[:username]}" } discoverable true indexable true + + # This is not strictly needed but this avoids generating multiple keys + # and, when `store_private_key` is passed, stores private keys for use in request specs + keypairs do |attrs| + if attrs[:legacy_keypair] || attrs[:public_key].present? + [] + else + [ + Keypair.new( + type: :rsa, + public_key: SigningKeysHelpers::PUBLIC_RSA_TEST_KEY, + private_key: attrs[:domain].blank? ? SigningKeysHelpers::PRIVATE_RSA_TEST_KEY : nil, + uri: attrs[:domain].nil? ? nil : "https://#{attrs[:domain]}/users/#{attrs[:username]}#main-key", + local_fragment: attrs[:domain].nil? ? '#main-key' : nil + ), + ] + end + end end Fabricator(:remote_account, from: :account) do diff --git a/spec/fabricators/keypair_fabricator.rb b/spec/fabricators/keypair_fabricator.rb index 9a741a6651e..694e9b4e8f5 100644 --- a/spec/fabricators/keypair_fabricator.rb +++ b/spec/fabricators/keypair_fabricator.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true Fabricator(:keypair) do - account + account { Fabricate(:account, keypairs: []) } type :rsa public_key SigningKeysHelpers::PUBLIC_RSA_TEST_KEY expires_at nil @@ -12,7 +12,7 @@ Fabricator(:keypair) do keypair.private_key ||= SigningKeysHelpers::PRIVATE_RSA_TEST_KEY keypair.local_fragment ||= "##{Random.hex}" else - keypair.uri ||= ActivityPub::TagManager.instance.key_uri_for(keypair.account) + keypair.uri ||= "#{ActivityPub::TagManager.instance.uri_for(keypair.account)}##{Random.hex}" end end end diff --git a/spec/lib/activitypub/linked_data_signature_spec.rb b/spec/lib/activitypub/linked_data_signature_spec.rb index 4bdebfacefe..deb6cb43a1f 100644 --- a/spec/lib/activitypub/linked_data_signature_spec.rb +++ b/spec/lib/activitypub/linked_data_signature_spec.rb @@ -7,8 +7,8 @@ RSpec.describe ActivityPub::LinkedDataSignature do subject { described_class.new(json) } - let(:keyid) { 'http://example.com/alice#rsa-key' } let!(:sender) { Fabricate(:account, uri: 'http://example.com/alice', domain: 'example.com') } + let(:keyid) { sender.keypair.uri } let(:raw_json) do { @@ -55,8 +55,8 @@ RSpec.describe ActivityPub::LinkedDataSignature do signature # Unset key - old_key = sender.public_key - sender.update!(private_key: '', public_key: '') + old_key = sender.keypair.public_key + sender.keypairs.delete_all allow(ActivityPub::FetchRemoteKeyService).to receive(:new).and_return(service_stub) @@ -153,6 +153,9 @@ RSpec.describe ActivityPub::LinkedDataSignature do expect(subject['signature']).to be_a Hash expect(subject['signature']['signatureValue']).to be_present expect(Array(subject['@context'])).to include('https://w3id.org/security/v1') + + # Verification requires fetching the key, which we don't support for local keys + allow(Keypair).to receive(:from_keyid).with(sender.keypair(type: :rsa).full_uri).and_return(sender.keypair(type: :rsa)) expect(described_class.new(subject).verify_actor!).to eq sender end end diff --git a/spec/lib/mastodon/cli/accounts_spec.rb b/spec/lib/mastodon/cli/accounts_spec.rb index 06af275bf5a..73f5b83744c 100644 --- a/spec/lib/mastodon/cli/accounts_spec.rb +++ b/spec/lib/mastodon/cli/accounts_spec.rb @@ -655,7 +655,7 @@ RSpec.describe Mastodon::CLI::Accounts do context 'when there are duplicate URI accounts' do before do - Fabricate.times(2, :account, domain: 'host.example', uri: uri) + Fabricate.times(2, :account, domain: 'host.example', uri: uri, legacy_keypair: true) allow(ActivityPub::FetchRemoteAccountService).to receive(:new).and_return(service_double) end diff --git a/spec/lib/mastodon/cli/maintenance_spec.rb b/spec/lib/mastodon/cli/maintenance_spec.rb index 3e8eb9c3604..6e1fb3cb6d1 100644 --- a/spec/lib/mastodon/cli/maintenance_spec.rb +++ b/spec/lib/mastodon/cli/maintenance_spec.rb @@ -89,7 +89,7 @@ RSpec.describe Mastodon::CLI::Maintenance do def prepare_duplicate_data ActiveRecord::Base.connection.remove_index :accounts, name: :index_accounts_on_username_and_domain_lower - duplicate_record(:account, username: duplicate_account_username, domain: duplicate_account_domain) + duplicate_record(:account, username: duplicate_account_username, domain: duplicate_account_domain, legacy_keypair: true) duplicate_record(:account, username: duplicate_account_username, domain: nil) end diff --git a/spec/models/account_spec.rb b/spec/models/account_spec.rb index 69842edf509..0ae71d54162 100644 --- a/spec/models/account_spec.rb +++ b/spec/models/account_spec.rb @@ -736,9 +736,9 @@ RSpec.describe Account do it 'generates keys' do account = described_class.create!(domain: nil, username: 'user_without_keys') - expect(account) - .to be_private_key - .and be_public_key + expect(account.private_key).to be_nil + expect(account.public_key).to eq '' + expect(account.keypair.keypair) .to be_private .and be_public diff --git a/spec/models/keypair_spec.rb b/spec/models/keypair_spec.rb index 59482d3bcad..8f308adc093 100644 --- a/spec/models/keypair_spec.rb +++ b/spec/models/keypair_spec.rb @@ -45,8 +45,8 @@ RSpec.describe Keypair do end end - context 'when no key with the expected key ID exists but there is an account with the same ID and a key' do - let(:account) { Fabricate(:account, domain: 'example.com') } + context 'when no key with the expected key ID exists but there is an account with the same ID and a legacy key' do + let(:account) { Fabricate(:account, domain: 'example.com', legacy_keypair: true) } let(:keyid) { "#{ActivityPub::TagManager.instance.uri_for(account)}#main-rsa-key" } it 'returns the expected Keypair' do @@ -60,7 +60,7 @@ RSpec.describe Keypair do end context 'when no key with the expected key ID exists but there is an account with the same ID and no key' do - let(:account) { Fabricate(:account, domain: 'example.com', public_key: '', private_key: nil) } + let(:account) { Fabricate(:account, domain: 'example.com') } let(:keyid) { "#{ActivityPub::TagManager.instance.uri_for(account)}#main-rsa-key" } it 'returns nil' do diff --git a/spec/requests/instance_actor_spec.rb b/spec/requests/instance_actor_spec.rb index ce0bbe3b820..88fbcd89189 100644 --- a/spec/requests/instance_actor_spec.rb +++ b/spec/requests/instance_actor_spec.rb @@ -23,7 +23,7 @@ RSpec.describe 'Instance actor endpoint' do inbox: instance_actor_inbox_url, outbox: instance_actor_outbox_url, publicKey: include( - id: instance_actor_url(anchor: 'main-key') + id: start_with(instance_actor_url) ), url: about_more_url(instance_actor: true) ) diff --git a/spec/requests/signature_verification_spec.rb b/spec/requests/signature_verification_spec.rb index b3146830e2c..95569119019 100644 --- a/spec/requests/signature_verification_spec.rb +++ b/spec/requests/signature_verification_spec.rb @@ -704,7 +704,7 @@ RSpec.describe 'signature verification concern' do end context 'with a known account' do - let!(:actor) { Fabricate(:account, domain: 'remote.domain', uri: 'https://remote.domain/users/bob', private_key: nil, public_key: '') } + let!(:actor) { Fabricate(:account, domain: 'remote.domain', uri: 'https://remote.domain/users/bob') } before do Fabricate(:keypair, account: actor, type: :ed25519, public_key: actor_keypair.public_to_pem, uri: 'https://remote.domain/users/bob#main-key') diff --git a/spec/services/activitypub/process_account_service_spec.rb b/spec/services/activitypub/process_account_service_spec.rb index 3392531765f..9536809e457 100644 --- a/spec/services/activitypub/process_account_service_spec.rb +++ b/spec/services/activitypub/process_account_service_spec.rb @@ -125,7 +125,7 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with a legacy key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', legacy_keypair: true) } it 'invalidates the legacy key and stores the new key' do expect { subject.call('alice', 'example.com', payload) } @@ -135,13 +135,14 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with an old key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', public_key: '') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } before do + alice.keypairs.delete_all Fabricate(:keypair, account: alice, uri: 'https://foo.test/actor#old-key', type: :rsa) end - it 'invalidates the legacy key and stores the new key' do + it 'invalidates the old key and stores the new key' do expect { subject.call('alice', 'example.com', payload) } .to change { alice.reload.keypairs.to_a }.from(contain_exactly(have_attributes({ uri: 'https://foo.test/actor#old-key' }))).to(contain_exactly(have_attributes({ uri: 'https://foo.test/actor#key1', type: 'rsa', public_key: }))) @@ -222,7 +223,7 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with a legacy key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', legacy_keypair: true) } it 'invalidates the legacy key and stores the new key' do expect { subject.call('alice', 'example.com', payload) } @@ -232,9 +233,10 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with an old key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', public_key: '') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } before do + alice.keypairs.delete_all Fabricate(:keypair, account: alice, uri: 'https://foo.test/actor#old-key', type: :rsa) end @@ -289,7 +291,7 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with a legacy key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', legacy_keypair: true) } it 'invalidates the legacy key and stores the new keys' do expect { subject.call('alice', 'example.com', payload) } @@ -345,7 +347,7 @@ RSpec.describe ActivityPub::ProcessAccountService do end context 'when the account was known with a legacy key' do - let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice') } + let!(:alice) { Fabricate(:account, uri: 'https://foo.test/actor', domain: 'example.com', username: 'alice', legacy_keypair: true) } it 'invalidates the legacy key and stores the new keys' do expect { subject.call('alice', 'example.com', payload) } diff --git a/spec/services/fetch_resource_service_spec.rb b/spec/services/fetch_resource_service_spec.rb index ee4810571be..c7e032b80a4 100644 --- a/spec/services/fetch_resource_service_spec.rb +++ b/spec/services/fetch_resource_service_spec.rb @@ -70,7 +70,7 @@ RSpec.describe FetchResourceService do it 'signs request' do subject - expect(a_request(:get, url).with(headers: { 'Signature' => /keyId="#{Regexp.escape(ActivityPub::TagManager.instance.key_uri_for(Account.representative))}"/ })).to have_been_made + expect(a_request(:get, url).with(headers: { 'Signature' => /keyId="#{Regexp.escape(Account.representative.keypair(type: :rsa).full_uri)}"/ })).to have_been_made end context 'when content type is application/atom+xml' do