From 4e41b8dc86dceaf43c154e718969e87de3c1a0f6 Mon Sep 17 00:00:00 2001 From: cmrd Senya Date: Wed, 2 Dec 2015 04:15:54 +0300 Subject: [PATCH 1/4] Allow Retraction to deal with Relayables --- lib/diaspora/federated/retraction.rb | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/lib/diaspora/federated/retraction.rb b/lib/diaspora/federated/retraction.rb index 5927dca85..61a5e254e 100644 --- a/lib/diaspora/federated/retraction.rb +++ b/lib/diaspora/federated/retraction.rb @@ -46,6 +46,14 @@ class Retraction logger.info "event=retraction status=complete type=#{type} guid=#{post_guid}" end + def correct_authorship? + if target.respond_to?(:relayable?) && target.relayable? + [target.author, target.parent.author].include?(person) + else + target.author == person + end + end + def receive(user, person) if self.type == 'Person' unless self.person.guid.to_s == self.post_guid.to_s @@ -55,7 +63,7 @@ class Retraction return end user.disconnected_by(self.target) - elsif self.target.nil? || self.target.author != self.person + elsif target.nil? || !correct_authorship? logger.warn "event=retraction status=abort reason='no post found authored by retractor' " \ "sender=#{person.diaspora_handle} post_guid=#{post_guid}" else From 08b910bd8851839745cdf41fb6d78c9fd8e3baef Mon Sep 17 00:00:00 2001 From: cmrd Senya Date: Mon, 30 Nov 2015 18:13:19 +0300 Subject: [PATCH 2/4] bump diaspora_federation --- Gemfile | 4 +++- Gemfile.lock | 16 ++++++++++------ 2 files changed, 13 insertions(+), 7 deletions(-) diff --git a/Gemfile b/Gemfile index 263e8c4a4..3ce11dc83 100644 --- a/Gemfile +++ b/Gemfile @@ -12,7 +12,7 @@ gem "unicorn", "4.9.0", require: false # Federation -gem "diaspora_federation-rails", "0.0.8" +gem "diaspora_federation-rails", "0.0.9" # API and JSON @@ -269,6 +269,8 @@ group :test do gem "timecop", "0.8.0" gem "webmock", "1.22.1", require: false gem "shoulda-matchers", "3.0.0" + + gem "diaspora_federation-test", "0.0.9" end group :development, :test do diff --git a/Gemfile.lock b/Gemfile.lock index a65ca2f33..238368349 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -152,15 +152,18 @@ GEM eventmachine (~> 1.0.8) http_parser.rb (~> 0.6) nokogiri (~> 1.6) - diaspora_federation (0.0.8) - faraday (~> 0.9.2) + diaspora_federation (0.0.9) + faraday (~> 0.9.0) faraday_middleware (~> 0.10.0) - nokogiri (~> 1.6, >= 1.6.6) + nokogiri (~> 1.6, >= 1.6.6.4) typhoeus (~> 0.7) valid (~> 1.0) - diaspora_federation-rails (0.0.8) - diaspora_federation (= 0.0.8) + diaspora_federation-rails (0.0.9) + diaspora_federation (= 0.0.9) rails (~> 4.2) + diaspora_federation-test (0.0.9) + diaspora_federation (= 0.0.9) + factory_girl (~> 4.5.0) diff-lcs (1.2.5) docile (1.1.5) domain_name (0.5.25) @@ -776,7 +779,8 @@ DEPENDENCIES devise-token_authenticatable (~> 0.4.0) devise_lastseenable (= 0.0.6) diaspora-vines (~> 0.2.0.develop) - diaspora_federation-rails (= 0.0.8) + diaspora_federation-rails (= 0.0.9) + diaspora_federation-test (= 0.0.9) entypo-rails (= 2.2.3) eye (= 0.7) facebox-rails (= 0.2.0) From 922d26f9768b206127b13642f476ca959e9e2dc9 Mon Sep 17 00:00:00 2001 From: cmrd Senya Date: Tue, 10 Nov 2015 21:34:25 +0300 Subject: [PATCH 3/4] Implement integration tests for the federation messages receive feature These are some initial tests, more to come. It tests some features of Request, StatusMessage, Comment, Like, Participation, Retraction, SignedRetraction, RelayableRetraction entities receive process. --- app/models/person.rb | 30 ++++ config/initializers/diaspora_federation.rb | 58 +++---- spec/federation_callbacks_spec.rb | 119 ++++++++++++++ .../federation_messages_generation.rb | 125 +++++++++++++++ .../federation/receive_federation_messages.rb | 146 ++++++++++++++++++ .../federation/shared_receive_relayable.rb | 31 ++++ .../federation/shared_receive_retraction.rb | 44 ++++++ 7 files changed, 525 insertions(+), 28 deletions(-) create mode 100644 spec/integration/federation/federation_messages_generation.rb create mode 100644 spec/integration/federation/receive_federation_messages.rb create mode 100644 spec/integration/federation/shared_receive_relayable.rb create mode 100644 spec/integration/federation/shared_receive_retraction.rb diff --git a/app/models/person.rb b/app/models/person.rb index f338daf3b..03c929330 100644 --- a/app/models/person.rb +++ b/app/models/person.rb @@ -317,6 +317,36 @@ class Person < ActiveRecord::Base self end + def webfinger + DiasporaFederation::Discovery::WebFinger.new( + acct_uri: "acct:#{diaspora_handle}", + alias_url: AppConfig.url_to("/people/#{guid}"), + hcard_url: AppConfig.url_to(DiasporaFederation::Engine.routes.url_helpers.hcard_path(guid)), + seed_url: AppConfig.pod_uri, + profile_url: profile_url, + atom_url: atom_url, + salmon_url: receive_url, + guid: guid, + public_key: serialized_public_key + ) + end + + def hcard + DiasporaFederation::Discovery::HCard.new( + guid: guid, + nickname: username, + full_name: "#{profile.first_name} #{profile.last_name}".strip, + url: AppConfig.pod_uri, + photo_large_url: image_url, + photo_medium_url: image_url(:thumb_medium), + photo_small_url: image_url(:thumb_small), + public_key: serialized_public_key, + searchable: searchable, + first_name: profile.first_name, + last_name: profile.last_name + ) + end + protected def clean_url diff --git a/config/initializers/diaspora_federation.rb b/config/initializers/diaspora_federation.rb index f15dcf5cf..b9f3670d0 100644 --- a/config/initializers/diaspora_federation.rb +++ b/config/initializers/diaspora_federation.rb @@ -8,38 +8,12 @@ DiasporaFederation.configure do |config| config.define_callbacks do on :fetch_person_for_webfinger do |handle| person = Person.find_local_by_diaspora_handle(handle) - if person - DiasporaFederation::Discovery::WebFinger.new( - acct_uri: "acct:#{person.diaspora_handle}", - alias_url: AppConfig.url_to("/people/#{person.guid}"), - hcard_url: AppConfig.url_to(DiasporaFederation::Engine.routes.url_helpers.hcard_path(person.guid)), - seed_url: AppConfig.pod_uri, - profile_url: person.profile_url, - atom_url: person.atom_url, - salmon_url: person.receive_url, - guid: person.guid, - public_key: person.serialized_public_key - ) - end + person.webfinger if person end on :fetch_person_for_hcard do |guid| person = Person.find_local_by_guid(guid) - if person - DiasporaFederation::Discovery::HCard.new( - guid: person.guid, - nickname: person.username, - full_name: "#{person.profile.first_name} #{person.profile.last_name}".strip, - url: AppConfig.pod_uri, - photo_large_url: person.image_url, - photo_medium_url: person.image_url(:thumb_medium), - photo_small_url: person.image_url(:thumb_small), - public_key: person.serialized_public_key, - searchable: person.searchable, - first_name: person.profile.first_name, - last_name: person.profile.last_name - ) - end + person.hcard if person end on :save_person_after_webfinger do |person| @@ -61,5 +35,33 @@ DiasporaFederation.configure do |config| person_entity.save! end + + on :fetch_private_key_by_diaspora_id do |diaspora_id| + key = Person.where(diaspora_handle: diaspora_id).joins(:owner).pluck(:serialized_private_key).first + OpenSSL::PKey::RSA.new key unless key.nil? + end + + on :fetch_author_private_key_by_entity_guid do |entity_type, guid| + key = entity_type.constantize.where(guid: guid).joins(author: :owner).pluck(:serialized_private_key).first + OpenSSL::PKey::RSA.new key unless key.nil? + end + + on :fetch_public_key_by_diaspora_id do |diaspora_id| + key = Person.where(diaspora_handle: diaspora_id).pluck(:serialized_public_key).first + OpenSSL::PKey::RSA.new key unless key.nil? + end + + on :fetch_author_public_key_by_entity_guid do |entity_type, guid| + key = entity_type.constantize.where(guid: guid).joins(:author).pluck(:serialized_public_key).first + OpenSSL::PKey::RSA.new key unless key.nil? + end + + on :entity_author_is_local? do |entity_type, guid| + entity_type.constantize.where(guid: guid).joins(author: :owner).exists? + end + + on :fetch_entity_author_id_by_guid do |entity_type, guid| + entity_type.constantize.where(guid: guid).joins(:author).pluck(:diaspora_handle).first + end end end diff --git a/spec/federation_callbacks_spec.rb b/spec/federation_callbacks_spec.rb index a2f3432b5..7c5cd9640 100644 --- a/spec/federation_callbacks_spec.rb +++ b/spec/federation_callbacks_spec.rb @@ -1,4 +1,5 @@ require "spec_helper" +require "diaspora_federation/test" describe "diaspora federation callbacks" do describe ":fetch_person_for_webfinger" do @@ -147,4 +148,122 @@ describe "diaspora federation callbacks" do end end end + + def create_a_local_person + FactoryGirl.create(:user).person + end + + def create_a_remote_person + FactoryGirl.create(:person) + end + + def create_post_by_a_local_person + FactoryGirl.create(:status_message, author: create_a_local_person).guid + end + + def create_post_by_a_remote_person + FactoryGirl.create(:status_message, author: create_a_remote_person).guid + end + + describe :fetch_private_key_by_diaspora_id do + it "returns a private key for a local user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, create_a_local_person.diaspora_handle) + ).not_to be_nil + end + + it "returns nil for a remote user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, create_a_remote_person.diaspora_handle) + ).to be_nil + end + + it "returns nil for an unknown id" do + expect( + DiasporaFederation.callbacks.trigger(described_class, FactoryGirl.generate(:diaspora_id)) + ).to be_nil + end + end + + describe :fetch_author_private_key_by_entity_guid do + it "returns a private key for a post by a local user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_local_person) + ).not_to be_nil + end + + it "returns nil for a post by a remote user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_remote_person) + ).to be_nil + end + + it "returns nil for an unknown post" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", FactoryGirl.generate(:guid)) + ).to be_nil + end + end + + describe :fetch_public_key_by_diaspora_id do + it "returns a public key for a person" do + expect( + DiasporaFederation.callbacks.trigger(described_class, create_a_remote_person.diaspora_handle) + ).not_to be_nil + end + + it "returns nil for an unknown person" do + expect( + DiasporaFederation.callbacks.trigger(described_class, FactoryGirl.generate(:diaspora_id)) + ).to be_nil + end + end + + describe :fetch_author_public_key_by_entity_guid do + it "returns a public key for a known post" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_remote_person) + ).not_to be_nil + end + + it "returns nil for an unknown post" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", FactoryGirl.generate(:guid)) + ).to be_nil + end + end + + describe :entity_author_is_local? do + it "returns true for a post by a local user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_local_person) + ).to be(true) + end + + it "returns false for a post by a remote user" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_remote_person) + ).to be(false) + end + + it "returns false for a unknown post" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", FactoryGirl.generate(:diaspora_id)) + ).to be(false) + end + end + + describe :fetch_entity_author_id_by_guid do + it "returns id for a existing guid" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", create_post_by_a_remote_person) + ).not_to be_nil + end + + it "returns nil for a non-existing guid" do + expect( + DiasporaFederation.callbacks.trigger(described_class, "Post", FactoryGirl.generate(:guid)) + ).to be_nil + end + end end diff --git a/spec/integration/federation/federation_messages_generation.rb b/spec/integration/federation/federation_messages_generation.rb new file mode 100644 index 000000000..6dff58e40 --- /dev/null +++ b/spec/integration/federation/federation_messages_generation.rb @@ -0,0 +1,125 @@ +def generate_xml(entity, remote_user, user) + DiasporaFederation::Salmon::EncryptedSlap.generate_xml( + remote_user.diaspora_handle, + OpenSSL::PKey::RSA.new(remote_user.encryption_key), + entity, + OpenSSL::PKey::RSA.new(user.encryption_key) + ) +end + +def generate_status_message + @entity = FactoryGirl.build( + :status_message_entity, + diaspora_id: @remote_user.diaspora_handle, + public: false + ) + + generate_xml(@entity, @remote_user, @user) +end + +def generate_forged_status_message + substitute_wrong_key(@remote_user, 1) + generate_status_message +end + +def mock_private_key_for_user(user) + expect(DiasporaFederation.callbacks).to receive(:trigger) + .with(:fetch_private_key_by_diaspora_id, user.person.diaspora_handle) + .once + .and_return(user.encryption_key) +end + +def retraction_mock_callbacks(entity, sender) + return unless [ + DiasporaFederation::Entities::SignedRetraction, + DiasporaFederation::Entities::RelayableRetraction + ].include?(entity.class) + + mock_private_key_for_user(sender) + + allow(DiasporaFederation.callbacks).to receive(:trigger) + .with( + :fetch_entity_author_id_by_guid, + entity.target_type, + entity.target_guid + ) + .once + .and_return(sender.encryption_key) +end + +def generate_retraction(entity_name, target_object, sender=@remote_user) + @entity = FactoryGirl.build( + entity_name, + diaspora_id: sender.diaspora_handle, + target_guid: target_object.guid, + target_type: target_object.class.to_s + ) + + retraction_mock_callbacks(@entity, sender) + + generate_xml(@entity, sender, @user) +end + +def generate_forged_retraction(entity_name, target_object, sender=@remote_user) + times = 1 + if %i(signed_retraction_entity relayable_retraction_entity).include?(entity_name) + times += 2 + end + + substitute_wrong_key(sender, times) + generate_retraction(entity_name, target_object, sender) +end + +def generate_relayable_local_parent(entity_name) + @entity = FactoryGirl.build( + entity_name, + parent_guid: @local_message.guid, + diaspora_id: @remote_user.person.diaspora_handle + ) + + mock_private_key_for_user(@remote_user) + + expect(DiasporaFederation.callbacks).to receive(:trigger) + .with(:fetch_author_private_key_by_entity_guid, "Post", kind_of(String)) + .and_return(nil) + generate_xml(@entity, @remote_user, @user) +end + +def generate_relayable_remote_parent(entity_name) + @entity = FactoryGirl.build( + entity_name, + parent_guid: @remote_message.guid, + diaspora_id: @remote_user2.person.diaspora_handle + ) + + mock_private_key_for_user(@remote_user2) + + expect(DiasporaFederation.callbacks).to receive(:trigger) + .with( + :fetch_author_private_key_by_entity_guid, + "Post", + @remote_message.guid + ) + .once + .and_return(@remote_user.encryption_key) + generate_xml(@entity, @remote_user, @user) +end + +def substitute_wrong_key(user, times_number) + expect(user).to receive(:encryption_key).exactly(times_number).times.and_return( + OpenSSL::PKey::RSA.new(1024) + ) +end + +# Checks when a remote pod wants to send us a relayable without having a key for declared diaspora ID +def generate_relayable_local_parent_wrong_author_key(entity_name) + substitute_wrong_key(@remote_user, 2) + generate_relayable_local_parent(entity_name) +end + +# Checks when a remote pod C wants to send us a relayable from its user, but bypassing the pod B where +# remote status came from. +def generate_relayable_remote_parent_wrong_parent_key(entity_name) + substitute_wrong_key(@remote_user, 2) + generate_relayable_remote_parent(entity_name) +end diff --git a/spec/integration/federation/receive_federation_messages.rb b/spec/integration/federation/receive_federation_messages.rb new file mode 100644 index 000000000..4436c109d --- /dev/null +++ b/spec/integration/federation/receive_federation_messages.rb @@ -0,0 +1,146 @@ +require "spec_helper" +require "diaspora_federation/test" +require "integration/federation/federation_messages_generation" +require "integration/federation/shared_receive_relayable" +require "integration/federation/shared_receive_retraction" + +describe Workers::ReceiveEncryptedSalmon do + before do + @user = alice + allow(User).to receive(:find) { |id| + @user if id == @user.id + } + + @remote_user = FactoryGirl.build(:user) # user on pod B + @remote_user2 = FactoryGirl.build(:user) # user on pod C + + allow_any_instance_of(DiasporaFederation::Discovery::Discovery) + .to receive(:webfinger) {|instance| + [@remote_user, @remote_user2].find {|user| user.diaspora_handle == instance.diaspora_id }.person.webfinger + } + allow_any_instance_of(DiasporaFederation::Discovery::Discovery) + .to receive(:hcard) {|instance| + [@remote_user, @remote_user2].find {|user| user.diaspora_handle == instance.diaspora_id }.person.hcard + } + + @remote_person = Person.find_or_fetch_by_identifier(@remote_user.diaspora_handle) + @remote_person2 = Person.find_or_fetch_by_identifier(@remote_user2.diaspora_handle) + end + + it "treats sharing request recive correctly" do + entity = FactoryGirl.build(:request_entity, recipient_id: @user.diaspora_handle) + + expect(Diaspora::Fetcher::Public).to receive(:queue_for).exactly(1).times + + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_xml(entity, @remote_user, @user)) + + expect(@user.contacts.count).to eq(2) + new_contact = @user.contacts.order(created_at: :asc).last + expect(new_contact).not_to be_nil + expect(new_contact.sharing).to eq(true) + expect(new_contact.person.diaspora_handle).to eq(@remote_user.diaspora_handle) + end + + it "doesn't save the status message if there is no sharing" do + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_status_message) + + expect(StatusMessage.exists?(guid: @entity.guid)).to be(false) + end + + describe "with messages which require sharing" do + before do + @remote_person = Person.find_or_fetch_by_identifier(@remote_user.diaspora_handle) + contact = @user.contacts.find_or_initialize_by(person_id: @remote_person.id) + contact.sharing = true + contact.save + end + + it "treats status message receive correctly" do + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_status_message) + + expect(StatusMessage.exists?(guid: @entity.guid)).to be(true) + end + + it "doesn't accept status message with wrong signature" do + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_forged_status_message) + + expect(StatusMessage.exists?(guid: @entity.guid)).to be(false) + end + + describe "retractions for non-relayable objects" do + %w( + retraction + signed_retraction + ).each do |retraction_entity_name| + context "with #{retraction_entity_name}" do + %w(status_message photo).each do |target| + context "with #{target}" do + it_behaves_like "it retracts non-relayable object" do + let(:target_object) { FactoryGirl.create(target.to_sym, author: @remote_person) } + let(:entity_name) { "#{retraction_entity_name}_entity".to_sym } + end + end + end + end + end + end + + describe "with messages which require a status to operate on" do + before do + @local_message = FactoryGirl.create(:status_message, author: @user.person) + @remote_message = FactoryGirl.create(:status_message, author: @remote_person) + end + + %w(comment like participation).each do |entity| + context "with #{entity}" do + it_behaves_like "it deals correctly with a relayable" do + let(:entity_name) { "#{entity}_entity".to_sym } + let(:klass) { entity.camelize.constantize } + end + end + end + + describe "retractions for relayable objects" do + %w( + retraction + signed_retraction + relayable_retraction + ).each do |retraction_entity_name| + context "with #{retraction_entity_name}" do + context "with comment" do + it_behaves_like "it retracts relayable object" do + # case for to-upstream federation + let(:entity_name) { "#{retraction_entity_name}_entity".to_sym } + let(:target_object) { FactoryGirl.create(:comment, author: @remote_person, post: @local_message) } + let(:sender) { @remote_user } + end + + it_behaves_like "it retracts relayable object" do + # case for to-downsteam federation + let(:target_object) { FactoryGirl.create(:comment, author: @remote_person2, post: @remote_message) } + let(:entity_name) { "#{retraction_entity_name}_entity".to_sym } + let(:sender) { @remote_user } + end + end + + context "with like" do + it_behaves_like "it retracts relayable object" do + # case for to-upstream federation + let(:entity_name) { "#{retraction_entity_name}_entity".to_sym } + let(:target_object) { FactoryGirl.create(:like, author: @remote_person, target: @local_message) } + let(:sender) { @remote_user } + end + + it_behaves_like "it retracts relayable object" do + # case for to-downsteam federation + let(:target_object) { FactoryGirl.create(:like, author: @remote_person2, target: @remote_message) } + let(:entity_name) { "#{retraction_entity_name}_entity".to_sym } + let(:sender) { @remote_user } + end + end + end + end + end + end + end +end diff --git a/spec/integration/federation/shared_receive_relayable.rb b/spec/integration/federation/shared_receive_relayable.rb new file mode 100644 index 000000000..b4ad41db3 --- /dev/null +++ b/spec/integration/federation/shared_receive_relayable.rb @@ -0,0 +1,31 @@ +shared_examples_for "it deals correctly with a relayable" do + it "treats upstream receive correctly" do + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_relayable_local_parent(entity_name)) + received_entity = klass.find_by(guid: @entity.guid) + expect(received_entity).not_to be_nil + expect(received_entity.author.diaspora_handle).to eq(@remote_person.diaspora_handle) + end + + it "rejects an upstream entity with a malformed author signature" do + Workers::ReceiveEncryptedSalmon.new.perform( + @user.id, + generate_relayable_local_parent_wrong_author_key(entity_name) + ) + expect(klass.exists?(guid: @entity.guid)).to be(false) + end + + it "treats downstream receive correctly" do + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_relayable_remote_parent(entity_name)) + received_entity = klass.find_by(guid: @entity.guid) + expect(received_entity).not_to be_nil + expect(received_entity.author.diaspora_handle).to eq(@remote_person2.diaspora_handle) + end + + it "declines downstream receive when sender signed with a wrong key" do + Workers::ReceiveEncryptedSalmon.new.perform( + @user.id, + generate_relayable_remote_parent_wrong_parent_key(entity_name) + ) + expect(klass.exists?(guid: @entity.guid)).to be(false) + end +end diff --git a/spec/integration/federation/shared_receive_retraction.rb b/spec/integration/federation/shared_receive_retraction.rb new file mode 100644 index 000000000..af02defab --- /dev/null +++ b/spec/integration/federation/shared_receive_retraction.rb @@ -0,0 +1,44 @@ +shared_examples_for "it retracts non-relayable object" do + it "retracts object by a correct retraction message" do + target_klass = target_object.class.to_s.constantize + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_retraction(entity_name, target_object)) + + expect(target_klass.exists?(guid: target_object.guid)).to be(false) + end + + it "doesn't retract object when retraction has wrong signatures" do + target_klass = target_object.class.to_s.constantize + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_forged_retraction(entity_name, target_object)) + + expect(target_klass.exists?(guid: target_object.guid)).to be(true) + end + + it "doesn't retract object when sender is different from target object" do + target_klass = target_object.class.to_s.constantize + Workers::ReceiveEncryptedSalmon.new.perform( + @user.id, + generate_retraction(entity_name, target_object, @remote_user2) + ) + + expect(target_klass.exists?(guid: target_object.guid)).to be(true) + end +end + +shared_examples_for "it retracts relayable object" do + it "retracts object by a correct message" do + target_klass = target_object.class.to_s.constantize + Workers::ReceiveEncryptedSalmon.new.perform(@user.id, generate_retraction(entity_name, target_object, sender)) + + expect(target_klass.exists?(guid: target_object.guid)).to be(false) + end + + it "doesn't retract object when retraction has wrong signatures" do + target_klass = target_object.class.to_s.constantize + Workers::ReceiveEncryptedSalmon.new.perform( + @user.id, + generate_forged_retraction(entity_name, target_object, sender) + ) + + expect(target_klass.exists?(guid: target_object.guid)).to be(true) + end +end From f0fc62e94d27d27e9fc6bc48455b415d555f5922 Mon Sep 17 00:00:00 2001 From: cmrd Senya Date: Wed, 2 Dec 2015 18:01:58 +0300 Subject: [PATCH 4/4] Fix a security issue that author_signature is not checked on the to-downstream receive of a federated relayable entity, allowing to forge relayables if you are an owner of the pod where a parent object is stored. closes #6539 --- Changelog.md | 2 ++ lib/diaspora/relayable.rb | 6 ++++++ .../federation/federation_messages_generation.rb | 7 +++++++ spec/integration/federation/shared_receive_relayable.rb | 8 ++++++++ 4 files changed, 23 insertions(+) diff --git a/Changelog.md b/Changelog.md index eece68961..51974a7db 100644 --- a/Changelog.md +++ b/Changelog.md @@ -1,10 +1,12 @@ # 0.5.6.0 ## Refactor +* Add more integration tests with the help of the new diaspora-federation gem [#6539](https://github.com/diaspora/diaspora/pull/6539) ## Bug fixes * Fix mention autocomplete when pasting the username [#6510](https://github.com/diaspora/diaspora/pull/6510) * Use and update updated\_at for notifications [#6573](https://github.com/diaspora/diaspora/pull/6573) +* Ensure the author signature is checked when receiving a relayable [#6539](https://github.com/diaspora/diaspora/pull/6539) ## Features diff --git a/lib/diaspora/relayable.rb b/lib/diaspora/relayable.rb index 87c19a2ff..3edd989c4 100644 --- a/lib/diaspora/relayable.rb +++ b/lib/diaspora/relayable.rb @@ -69,6 +69,12 @@ module Diaspora def receive(user, person=nil) comment_or_like = self.class.where(guid: self.guid).first || self + unless comment_or_like.signature_valid? + logger.warn "event=receive status=abort reason='object signature not valid' recipient=#{user.diaspora_handle} "\ + "sender=#{parent.author.diaspora_handle} payload_type=#{self.class} parent_id=#{parent.id}" + return + end + # Check to make sure the signature of the comment or like comes from the person claiming to author it unless comment_or_like.parent_author == user.person || comment_or_like.verify_parent_author_signature logger.warn "event=receive status=abort reason='object signature not valid' recipient=#{user.diaspora_handle} "\ diff --git a/spec/integration/federation/federation_messages_generation.rb b/spec/integration/federation/federation_messages_generation.rb index 6dff58e40..59bcc315e 100644 --- a/spec/integration/federation/federation_messages_generation.rb +++ b/spec/integration/federation/federation_messages_generation.rb @@ -117,6 +117,13 @@ def generate_relayable_local_parent_wrong_author_key(entity_name) generate_relayable_local_parent(entity_name) end +# Checks when a remote pod B wants to send us a relayable with authorship from a remote pod C user +# without having correct signature from him. +def generate_relayable_remote_parent_wrong_author_key(entity_name) + substitute_wrong_key(@remote_user2, 1) + generate_relayable_remote_parent(entity_name) +end + # Checks when a remote pod C wants to send us a relayable from its user, but bypassing the pod B where # remote status came from. def generate_relayable_remote_parent_wrong_parent_key(entity_name) diff --git a/spec/integration/federation/shared_receive_relayable.rb b/spec/integration/federation/shared_receive_relayable.rb index b4ad41db3..2d25cce46 100644 --- a/spec/integration/federation/shared_receive_relayable.rb +++ b/spec/integration/federation/shared_receive_relayable.rb @@ -21,6 +21,14 @@ shared_examples_for "it deals correctly with a relayable" do expect(received_entity.author.diaspora_handle).to eq(@remote_person2.diaspora_handle) end + it "rejects a downstream entity with a malformed author signature" do + Workers::ReceiveEncryptedSalmon.new.perform( + @user.id, + generate_relayable_remote_parent_wrong_author_key(entity_name) + ) + expect(klass.exists?(guid: @entity.guid)).to be(false) + end + it "declines downstream receive when sender signed with a wrong key" do Workers::ReceiveEncryptedSalmon.new.perform( @user.id,