From f32cc873fac3b730af86bb8095862190770174ff Mon Sep 17 00:00:00 2001 From: Tan Le Date: Mon, 12 Jul 2021 22:58:17 +1000 Subject: [PATCH] Audit successful GPG key creation and removal Create an audit event when GPG key has been added or removed. This commit covers actions from both the App and API. Changelog: added EE: true --- .../profiles/gpg_keys_controller.rb | 2 +- app/services/gpg_keys/create_service.rb | 10 +++- app/services/gpg_keys/destroy_service.rb | 2 + doc/administration/audit_events.md | 1 + ee/app/models/concerns/auditable.rb | 4 ++ ee/app/models/ee/gpg_key.rb | 6 +++ ee/app/models/ee/key.rb | 6 +++ ee/app/services/ee/gpg_keys/create_service.rb | 26 ++++++++++ .../services/ee/gpg_keys/destroy_service.rb | 26 ++++++++++ ee/lib/gitlab/audit/target.rb | 2 +- ee/spec/lib/gitlab/audit/target_spec.rb | 4 +- ee/spec/models/concerns/auditable_spec.rb | 6 +++ ee/spec/models/ee/gpg_key_spec.rb | 6 +++ ee/spec/models/ee/key_spec.rb | 7 +++ .../ee/gpg_keys/create_service_spec.rb | 43 +++++++++++++++++ .../ee/gpg_keys/destroy_service_spec.rb | 47 +++++++++++++++++++ 16 files changed, 193 insertions(+), 5 deletions(-) create mode 100644 ee/app/services/ee/gpg_keys/create_service.rb create mode 100644 ee/app/services/ee/gpg_keys/destroy_service.rb create mode 100644 ee/spec/services/ee/gpg_keys/create_service_spec.rb create mode 100644 ee/spec/services/ee/gpg_keys/destroy_service_spec.rb diff --git a/app/controllers/profiles/gpg_keys_controller.rb b/app/controllers/profiles/gpg_keys_controller.rb index 7f04927f517d27..9e16d195b00ed2 100644 --- a/app/controllers/profiles/gpg_keys_controller.rb +++ b/app/controllers/profiles/gpg_keys_controller.rb @@ -22,7 +22,7 @@ def create end def destroy - @gpg_key.destroy + GpgKeys::DestroyService.new(current_user).execute(@gpg_key) respond_to do |format| format.html { redirect_to profile_gpg_keys_url, status: :found } diff --git a/app/services/gpg_keys/create_service.rb b/app/services/gpg_keys/create_service.rb index e41444b2a82c55..ab8b12732d7755 100644 --- a/app/services/gpg_keys/create_service.rb +++ b/app/services/gpg_keys/create_service.rb @@ -3,9 +3,17 @@ module GpgKeys class CreateService < Keys::BaseService def execute - key = user.gpg_keys.create(params) + key = create(params) notification_service.new_gpg_key(key) if key.persisted? key end + + private + + def create(params) + user.gpg_keys.create(params) + end end end + +GpgKeys::CreateService.prepend_mod diff --git a/app/services/gpg_keys/destroy_service.rb b/app/services/gpg_keys/destroy_service.rb index cecbfe266112b0..2e82509897e235 100644 --- a/app/services/gpg_keys/destroy_service.rb +++ b/app/services/gpg_keys/destroy_service.rb @@ -7,3 +7,5 @@ def execute(key) end end end + +GpgKeys::DestroyService.prepend_mod diff --git a/doc/administration/audit_events.md b/doc/administration/audit_events.md index ae921d4822a44f..7a871caf658b4d 100644 --- a/doc/administration/audit_events.md +++ b/doc/administration/audit_events.md @@ -166,6 +166,7 @@ The following user actions are recorded: - A failed attempt to create or revoke a user's personal access token ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/276921) in GitLab 13.6) - Administrator added or removed ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/323905) in GitLab 14.1) - Removed SSH key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) +- Added or removed GPG key ([introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/220127) in GitLab 14.1) Instance events can also be accessed via the [Instance Audit Events API](../api/audit_events.md#instance-audit-events). diff --git a/ee/app/models/concerns/auditable.rb b/ee/app/models/concerns/auditable.rb index d5e66565a40ff3..187dd0a097d086 100644 --- a/ee/app/models/concerns/auditable.rb +++ b/ee/app/models/concerns/auditable.rb @@ -6,4 +6,8 @@ def push_audit_event(event) ::Gitlab::Audit::EventQueue.push(event) end + + def audit_details + raise NotImplementedError, "#{self.class} does not implement #{__method__}" + end end diff --git a/ee/app/models/ee/gpg_key.rb b/ee/app/models/ee/gpg_key.rb index c2edd6b7fb2238..c48adecac5101b 100644 --- a/ee/app/models/ee/gpg_key.rb +++ b/ee/app/models/ee/gpg_key.rb @@ -4,9 +4,15 @@ module EE module GpgKey extend ActiveSupport::Concern + include Auditable + prepended do scope :preload_users, -> { preload(:user) } scope :for_user, -> (user) { where(user: user) } end + + def audit_details + user.name + end end end diff --git a/ee/app/models/ee/key.rb b/ee/app/models/ee/key.rb index c3ba5f47c20bd7..c03943ba9a9b84 100644 --- a/ee/app/models/ee/key.rb +++ b/ee/app/models/ee/key.rb @@ -4,6 +4,8 @@ module EE module Key extend ActiveSupport::Concern + include Auditable + prepended do include UsageStatistics @@ -41,5 +43,9 @@ def enforce_ssh_key_expiration_feature_available? License.feature_available?(:enforce_ssh_key_expiration) end end + + def audit_details + title + end end end diff --git a/ee/app/services/ee/gpg_keys/create_service.rb b/ee/app/services/ee/gpg_keys/create_service.rb new file mode 100644 index 00000000000000..c7ce188f3d0103 --- /dev/null +++ b/ee/app/services/ee/gpg_keys/create_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module EE + module GpgKeys + module CreateService + extend ::Gitlab::Utils::Override + + override :create + def create(params) + super.tap do |key| + next unless key.persisted? + + audit_context = { + name: 'add_gpg_key', + author: user, + scope: key.user, + target: key, + message: 'Added GPG key' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/app/services/ee/gpg_keys/destroy_service.rb b/ee/app/services/ee/gpg_keys/destroy_service.rb new file mode 100644 index 00000000000000..12789d055ca10e --- /dev/null +++ b/ee/app/services/ee/gpg_keys/destroy_service.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +module EE + module GpgKeys + module DestroyService + extend ::Gitlab::Utils::Override + + override :execute + def execute(key) + super.tap do |destroyed| + next unless destroyed + + audit_context = { + name: 'remove_gpg_key', + author: user, + scope: key.user, + target: key, + message: 'Removed GPG key' + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + end + end + end +end diff --git a/ee/lib/gitlab/audit/target.rb b/ee/lib/gitlab/audit/target.rb index 7bfd2994915a44..b9cb54aece8c48 100644 --- a/ee/lib/gitlab/audit/target.rb +++ b/ee/lib/gitlab/audit/target.rb @@ -14,7 +14,7 @@ def type end def details - @object.try(:name) || @object.try(:title) || 'unknown' + @object.try(:name) || @object.try(:audit_details) || 'unknown' end end end diff --git a/ee/spec/lib/gitlab/audit/target_spec.rb b/ee/spec/lib/gitlab/audit/target_spec.rb index d4441d11576d07..67b814cd10ce40 100644 --- a/ee/spec/lib/gitlab/audit/target_spec.rb +++ b/ee/spec/lib/gitlab/audit/target_spec.rb @@ -26,7 +26,7 @@ describe '#details' do using RSpec::Parameterized::TableSyntax - where(:name, :title, :details) do + where(:name, :audit_details, :details) do 'jackie' | 'wanderer' | 'jackie' 'jackie' | nil | 'jackie' nil | 'wanderer' | 'wanderer' @@ -35,7 +35,7 @@ before do allow(object).to receive(:name).and_return(name) if name - allow(object).to receive(:title).and_return(title) if title + allow(object).to receive(:audit_details).and_return(audit_details) if audit_details end with_them do diff --git a/ee/spec/models/concerns/auditable_spec.rb b/ee/spec/models/concerns/auditable_spec.rb index ea31e3ccfbdd0b..d56f1001908125 100644 --- a/ee/spec/models/concerns/auditable_spec.rb +++ b/ee/spec/models/concerns/auditable_spec.rb @@ -34,4 +34,10 @@ end end end + + describe '#audit_details' do + it 'raises error to prompt for implementation' do + expect { instance.audit_details }.to raise_error(/does not implement audit_details/) + end + end end diff --git a/ee/spec/models/ee/gpg_key_spec.rb b/ee/spec/models/ee/gpg_key_spec.rb index 19e1c827227b56..b182e44bdd3e4c 100644 --- a/ee/spec/models/ee/gpg_key_spec.rb +++ b/ee/spec/models/ee/gpg_key_spec.rb @@ -12,4 +12,10 @@ it { is_expected.to contain_exactly(gpg_key) } end + + describe '#audit_details' do + it "equals to the user's name" do + expect(gpg_key.audit_details).to eq(user.name) + end + end end diff --git a/ee/spec/models/ee/key_spec.rb b/ee/spec/models/ee/key_spec.rb index 09150e7fae10ac..d7ff2dc95f6b83 100644 --- a/ee/spec/models/ee/key_spec.rb +++ b/ee/spec/models/ee/key_spec.rb @@ -72,4 +72,11 @@ end end end + + describe '#audit_details' do + it 'equals to the title' do + key = build(:personal_key) + expect(key.audit_details).to eq(key.title) + end + end end diff --git a/ee/spec/services/ee/gpg_keys/create_service_spec.rb b/ee/spec/services/ee/gpg_keys/create_service_spec.rb new file mode 100644 index 00000000000000..67d9aaec19cf30 --- /dev/null +++ b/ee/spec/services/ee/gpg_keys/create_service_spec.rb @@ -0,0 +1,43 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GpgKeys::CreateService do + let_it_be(:user) { create(:user) } + + let(:params) { attributes_for(:gpg_key) } + + subject { described_class.new(user, params) } + + it 'creates an audit event', :aggregate_failures do + expect { subject.execute }.to change(GpgKey, :count).by(1) + .and change(AuditEvent, :count).by(1) + + key = user.gpg_keys.last + + expect(AuditEvent.last).to have_attributes( + author: user, + entity_id: key.user.id, + target_id: key.id, + target_type: key.class.name, + target_details: key.user.name, + details: include(custom_message: 'Added GPG key') + ) + end + + it 'returns the correct value' do + expect(subject.execute).to eq(GpgKey.last) + end + + context 'when create operation fails' do + let(:params) { attributes_for(:gpg_key).except(:key) } + + it 'does not create an audit event' do + expect { subject.execute }.not_to change(AuditEvent, :count) + end + + it 'returns the correct value' do + expect(subject.execute).to be_a_new(GpgKey) + end + end +end diff --git a/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb b/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb new file mode 100644 index 00000000000000..52b23cdfe1583e --- /dev/null +++ b/ee/spec/services/ee/gpg_keys/destroy_service_spec.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe GpgKeys::DestroyService do + let_it_be(:user) { create(:user) } + + subject { described_class.new(user) } + + it 'creates an audit event', :aggregate_failures do + key = create(:personal_key) + + expect { subject.execute(key) }.to change(AuditEvent, :count).by(1) + + expect(AuditEvent.last).to have_attributes( + author: user, + entity_id: key.user.id, + target_id: key.id, + target_type: key.class.name, + target_details: key.title, + details: include(custom_message: 'Removed GPG key') + ) + end + + it 'returns the correct value' do + key = build(:gpg_key) + allow(key).to receive(:destroy).and_return(true) + + expect(subject.execute(key)).to eq(true) + end + + context 'when destroy operation fails' do + let(:key) { build(:gpg_key) } + + before do + allow(key).to receive(:destroy).and_return(false) + end + + it 'does not create an audit event' do + expect { subject.execute(key) }.not_to change(AuditEvent, :count) + end + + it 'returns the correct value' do + expect(subject.execute(key)).to eq(false) + end + end +end -- GitLab