From 198bfd73e56bbd04bb18a3cfcd8888a9c232c661 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Wed, 20 Apr 2022 18:21:00 +0200 Subject: [PATCH 1/2] Add auditing of runner token reset operations Changelog: added EE: true --- .../reset_registration_token_service.rb | 2 + .../runners_token_audit_event_service.rb | 55 ++++++++++ .../reset_registration_token_service.rb | 40 +++++++ .../runners_token_audit_event_service_spec.rb | 101 ++++++++++++++++++ .../reset_registration_token_service_spec.rb | 87 +++++++++++++++ 5 files changed, 285 insertions(+) create mode 100644 ee/app/services/audit_events/runners_token_audit_event_service.rb create mode 100644 ee/app/services/ee/ci/runners/reset_registration_token_service.rb create mode 100644 ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb create mode 100644 ee/spec/services/ci/runners/reset_registration_token_service_spec.rb diff --git a/app/services/ci/runners/reset_registration_token_service.rb b/app/services/ci/runners/reset_registration_token_service.rb index bbe49c0464487a..2a3fb08c5e1fd9 100644 --- a/app/services/ci/runners/reset_registration_token_service.rb +++ b/app/services/ci/runners/reset_registration_token_service.rb @@ -29,3 +29,5 @@ def execute end end end + +Ci::Runners::ResetRegistrationTokenService.prepend_mod diff --git a/ee/app/services/audit_events/runners_token_audit_event_service.rb b/ee/app/services/audit_events/runners_token_audit_event_service.rb new file mode 100644 index 00000000000000..d3b5a2bee43a22 --- /dev/null +++ b/ee/app/services/audit_events/runners_token_audit_event_service.rb @@ -0,0 +1,55 @@ +# frozen_string_literal: true + +module AuditEvents + class RunnersTokenAuditEventService < ::AuditEventService + SAFE_TOKEN_LENGTH = 8 + + # Logs an audit event related to a runner event + # + # @param [User] author the user initiating the operation + # @param [Group, Project, ApplicationSetting] scope the scope that the operation applies to + # @param [String] previous_registration_token the previous registration token associated with scope + # @param [String] new_registration_token the new registration token associated with scope + def initialize(author, scope, previous_registration_token, new_registration_token) + @scope = scope + + raise ArgumentError, 'Missing scope' unless scope + + safe_scope = scope.is_a?(::ApplicationSetting) ? author : scope + + super(author, safe_scope, details(author, previous_registration_token, new_registration_token)) + end + + private + + def details(author, previous_registration_token, new_registration_token) + details = { + action: :custom, + custom_message: message, + from: safe_token(previous_registration_token), + to: safe_token(new_registration_token) + } + + details[:errors] = @scope.errors.full_messages unless @scope.errors.empty? + + details + end + + def message + return 'Reset instance runner registration token' if @scope.is_a?(::ApplicationSetting) + + "Reset #{@scope.class.name.downcase} runner registration token" + end + + def safe_token(token) + return token unless token.is_a?(String) + + safe_token_length = SAFE_TOKEN_LENGTH + if token.start_with?(::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX) + safe_token_length += ::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX.length + end + + token[0...safe_token_length] + end + end +end diff --git a/ee/app/services/ee/ci/runners/reset_registration_token_service.rb b/ee/app/services/ee/ci/runners/reset_registration_token_service.rb new file mode 100644 index 00000000000000..36a89b708c34c0 --- /dev/null +++ b/ee/app/services/ee/ci/runners/reset_registration_token_service.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +module EE + module Ci + module Runners + module ResetRegistrationTokenService + extend ::Gitlab::Utils::Override + include ::Audit::Changes + + override :execute + def execute + previous_registration_token = runners_token + new_registration_token = super + + audit_log_event(previous_registration_token, new_registration_token) if new_registration_token + + new_registration_token + end + + private + + def runners_token + if scope.is_a?(::ApplicationSetting) + ::ApplicationSetting.current_without_cache.runners_registration_token + else + scope.runners_token + end + end + + def audit_log_event(previous_registration_token, new_registration_token) + ::AuditEvents::RunnersTokenAuditEventService.new( + user, + scope, + previous_registration_token, + new_registration_token).security_event + end + end + end + end +end diff --git a/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb b/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb new file mode 100644 index 00000000000000..f7c74b936e4756 --- /dev/null +++ b/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb @@ -0,0 +1,101 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe AuditEvents::RunnersTokenAuditEventService do + describe '#security_event' do + let(:logger) { instance_double(Gitlab::AuditJsonLogger) } + + let(:user) { create(:user) } + let(:runners_token_prefix) { '' } + let(:old_token) { "#{runners_token_prefix}old-token" } + let(:new_token) { "#{runners_token_prefix}new-token" } + let(:logged_old_token) { "#{runners_token_prefix}old-toke" } + let(:logged_new_token) { "#{runners_token_prefix}new-toke" } + let(:service) { described_class.new(user, entity, old_token, new_token) } + let(:entity_details) do + { + entity_id: entity.id, + entity_type: entity_type + } + end + + shared_examples 'logs the event to file' do + it 'logs the event to file' do + freeze_time do + expected_logger_details = { + action: :custom, + author_id: user.id, + author_name: user.name, + from: logged_old_token, + to: logged_new_token, + custom_message: message, + created_at: DateTime.current + }.merge(entity_details) + + expect(service).to receive(:file_logger).and_return(logger) + expect(logger).to receive(:info).with(expected_logger_details) + + stub_licensed_features(audit_events: true, extended_audit_events: true) + + expect { service.security_event }.to change(AuditEvent, :count).by(1) + + security_event = AuditEvent.last + + expect(security_event.details).to include( + { + author_name: user.name, + custom_message: message, + from: logged_old_token, + to: logged_new_token + } + ) + + expect(security_event.author_id).to eq(user.id) + expect(security_event.entity_type).to eq(entity_details[:entity_type]) + + if entity.respond_to?(:full_path) + expect(security_event.entity_id).to eq(entity.id) + else + expect(security_event.entity_id).to eq(user.id) + end + end + end + end + + context 'for instance' do + let(:entity) { create(:application_setting) } + let(:message) { 'Reset instance runner registration token' } + let(:entity_details) do + { + entity_id: user.id, + entity_type: 'User' + } + end + + it_behaves_like 'logs the event to file' + end + + context 'for group' do + let(:entity) { create(:group) } + let(:entity_type) { 'Group' } + let(:message) { 'Reset group runner registration token' } + + it_behaves_like 'logs the event to file' + end + + context 'for project' do + let(:entity) { create(:project) } + let(:entity_type) { 'Project' } + let(:message) { 'Reset project runner registration token' } + + it_behaves_like 'logs the event to file' + + context 'with runners_token_prefix set to RUNNERS_TOKEN_PREFIX' do + let(:runners_token_prefix) { ::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX } + + it_behaves_like 'logs the event to file' + end + end + end +end diff --git a/ee/spec/services/ci/runners/reset_registration_token_service_spec.rb b/ee/spec/services/ci/runners/reset_registration_token_service_spec.rb new file mode 100644 index 00000000000000..789aa4239568cf --- /dev/null +++ b/ee/spec/services/ci/runners/reset_registration_token_service_spec.rb @@ -0,0 +1,87 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ::Ci::Runners::ResetRegistrationTokenService, '#execute' do + subject { described_class.new(scope, current_user).execute } + + let_it_be(:user) { build(:user) } + let_it_be(:admin_user) { create(:user, :admin) } + + shared_examples 'a registration token reset operation' do + context 'without user' do + let(:current_user) { nil } + + it 'does not audit and returns false', :aggregate_failures do + expect(scope).not_to receive(token_reset_method_name) + expect(::AuditEvents::RunnersTokenAuditEventService).not_to receive(:new) + + is_expected.to be_falsey + end + end + + context 'with unauthorized user' do + let(:current_user) { user } + + it 'does not audit and returns false', :aggregate_failures do + expect(scope).not_to receive(token_reset_method_name) + expect(::AuditEvents::RunnersTokenAuditEventService).not_to receive(:new) + + is_expected.to be_falsey + end + end + + context 'with admin user', :enable_admin_mode do + let(:current_user) { admin_user } + let(:audit_service) { instance_double(::AuditEvents::RunnersTokenAuditEventService) } + + before do + expect(scope).to receive(token_reset_method_name) do + expect(scope).to receive(token_method_name).and_return("new #{scope.class.name} token value") + true + end.once + + expect(::AuditEvents::RunnersTokenAuditEventService).to receive(:new) + .with(current_user, scope, scope.public_send(token_method_name), "new #{scope.class.name} token value") + .once.and_return(audit_service) + expect(audit_service).to receive(:security_event).once.and_return('track_event_return_value') + end + + it 'calls security_event on RunnersTokenAuditEventService and returns the new token', :aggregate_failures do + is_expected.to eq("new #{scope.class.name} token value") + end + end + end + + context 'with instance scope' do + let_it_be(:scope) { create(:application_setting) } + + before do + allow(ApplicationSetting).to receive(:current).and_return(scope) + allow(ApplicationSetting).to receive(:current_without_cache).and_return(scope) + end + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_registration_token } + let(:token_reset_method_name) { :reset_runners_registration_token! } + end + end + + context 'with group scope' do + let_it_be(:scope) { create(:group) } + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end + end + + context 'with project scope' do + let_it_be(:scope) { create(:project) } + + it_behaves_like 'a registration token reset operation' do + let(:token_method_name) { :runners_token } + let(:token_reset_method_name) { :reset_runners_token! } + end + end +end -- GitLab From f5a252de66bd0b8d185c102b2e25a7567299f6e2 Mon Sep 17 00:00:00 2001 From: Pedro Pombeiro Date: Tue, 26 Apr 2022 11:25:37 +0200 Subject: [PATCH 2/2] Address MR review comments --- .../audit_events/runner_audit_event_service.rb | 11 +---------- .../runners_token_audit_event_service.rb | 17 +++-------------- .../services/audit_events/safe_runner_token.rb | 18 ++++++++++++++++++ .../runners_token_audit_event_service_spec.rb | 8 +++++--- 4 files changed, 27 insertions(+), 27 deletions(-) create mode 100644 ee/app/services/audit_events/safe_runner_token.rb diff --git a/ee/app/services/audit_events/runner_audit_event_service.rb b/ee/app/services/audit_events/runner_audit_event_service.rb index e2b3649d9b9b15..1b4119d7af6c6a 100644 --- a/ee/app/services/audit_events/runner_audit_event_service.rb +++ b/ee/app/services/audit_events/runner_audit_event_service.rb @@ -2,7 +2,7 @@ module AuditEvents class RunnerAuditEventService < ::AuditEventService - SAFE_TOKEN_LENGTH = 8 + include SafeRunnerToken # Logs an audit event related to a runner event # @@ -63,14 +63,5 @@ def runner_path url_helpers.admin_runner_path(@runner) end end - - def safe_author(author) - return author unless author.is_a?(String) - - safe_token_length = SAFE_TOKEN_LENGTH - safe_token_length += ::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX.length if author.start_with?(::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX) - - author[0...safe_token_length] - end end end diff --git a/ee/app/services/audit_events/runners_token_audit_event_service.rb b/ee/app/services/audit_events/runners_token_audit_event_service.rb index d3b5a2bee43a22..a70ce75cabb941 100644 --- a/ee/app/services/audit_events/runners_token_audit_event_service.rb +++ b/ee/app/services/audit_events/runners_token_audit_event_service.rb @@ -2,7 +2,7 @@ module AuditEvents class RunnersTokenAuditEventService < ::AuditEventService - SAFE_TOKEN_LENGTH = 8 + include SafeRunnerToken # Logs an audit event related to a runner event # @@ -26,8 +26,8 @@ def details(author, previous_registration_token, new_registration_token) details = { action: :custom, custom_message: message, - from: safe_token(previous_registration_token), - to: safe_token(new_registration_token) + from: safe_author(previous_registration_token), + to: safe_author(new_registration_token) } details[:errors] = @scope.errors.full_messages unless @scope.errors.empty? @@ -40,16 +40,5 @@ def message "Reset #{@scope.class.name.downcase} runner registration token" end - - def safe_token(token) - return token unless token.is_a?(String) - - safe_token_length = SAFE_TOKEN_LENGTH - if token.start_with?(::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX) - safe_token_length += ::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX.length - end - - token[0...safe_token_length] - end end end diff --git a/ee/app/services/audit_events/safe_runner_token.rb b/ee/app/services/audit_events/safe_runner_token.rb new file mode 100644 index 00000000000000..483f105d37e247 --- /dev/null +++ b/ee/app/services/audit_events/safe_runner_token.rb @@ -0,0 +1,18 @@ +# frozen_string_literal: true + +module AuditEvents + module SafeRunnerToken + SAFE_TOKEN_LENGTH = 8 + + def safe_author(author) + return author unless author.is_a?(String) + + safe_token_length = SAFE_TOKEN_LENGTH + if author.start_with?(::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX) + safe_token_length += ::RunnersTokenPrefixable::RUNNERS_TOKEN_PREFIX.length + end + + author[0...safe_token_length] + end + end +end diff --git a/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb b/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb index f7c74b936e4756..10665cb8570190 100644 --- a/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb +++ b/ee/spec/services/audit_events/runners_token_audit_event_service_spec.rb @@ -21,7 +21,11 @@ end shared_examples 'logs the event to file' do - it 'logs the event to file' do + before do + stub_licensed_features(audit_events: true, extended_audit_events: true) + end + + it 'logs the event to file', :aggregate_failures do freeze_time do expected_logger_details = { action: :custom, @@ -36,8 +40,6 @@ expect(service).to receive(:file_logger).and_return(logger) expect(logger).to receive(:info).with(expected_logger_details) - stub_licensed_features(audit_events: true, extended_audit_events: true) - expect { service.security_event }.to change(AuditEvent, :count).by(1) security_event = AuditEvent.last -- GitLab