From 6ca89cad13b174d4cc294f9bd7886c2be13bce14 Mon Sep 17 00:00:00 2001 From: Sam Figueroa Date: Fri, 15 Sep 2023 12:54:07 +0200 Subject: [PATCH] Audit CI variable changes - Adds audit events (with types) for: ci_instance_variable_created ci_instance_variable_updated ci_instance_variable_deleted - :value changes on Ci::InstanceVariable are skipped since it's a virtual attribute that doesn't track changes - Refs: https://gitlab.com/gitlab-org/gitlab/-/issues/8070 Changelog: changed EE: true --- app/models/ci/instance_variable.rb | 4 + .../audit_event_types.md | 3 + .../ci/audit_variable_change_service.rb | 47 +++++- .../types/ci_instance_variable_created.yml | 9 ++ .../types/ci_instance_variable_deleted.yml | 9 ++ .../types/ci_instance_variable_updated.yml | 9 ++ .../ci/audit_variable_change_service_spec.rb | 137 +++++++++++++----- 7 files changed, 173 insertions(+), 45 deletions(-) create mode 100644 ee/config/audit_events/types/ci_instance_variable_created.yml create mode 100644 ee/config/audit_events/types/ci_instance_variable_deleted.yml create mode 100644 ee/config/audit_events/types/ci_instance_variable_updated.yml diff --git a/app/models/ci/instance_variable.rb b/app/models/ci/instance_variable.rb index 3e572dbe18f29d..179befb84690bc 100644 --- a/app/models/ci/instance_variable.rb +++ b/app/models/ci/instance_variable.rb @@ -47,5 +47,9 @@ def cached_data end end end + + def audit_details + key + end end end diff --git a/doc/administration/audit_event_streaming/audit_event_types.md b/doc/administration/audit_event_streaming/audit_event_types.md index fa89074ad53faf..2dd5d99f168ffa 100644 --- a/doc/administration/audit_event_streaming/audit_event_types.md +++ b/doc/administration/audit_event_streaming/audit_event_types.md @@ -178,6 +178,9 @@ Audit event types belong to the following product categories. | [`ci_group_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a CI variable is created at a group level| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | | [`ci_group_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a group's CI variable is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | | [`ci_group_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a group's CI variable is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | +| [`ci_instance_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI variable is created| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) | +| [`ci_instance_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI varialbe is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) | +| [`ci_instance_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882) | When an instance level CI variable is changed| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [16.5](https://gitlab.com/gitlab-org/gitlab/-/issues/8070) | | [`ci_variable_created`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a CI variable is created at a project level| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | | [`ci_variable_deleted`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a project's CI variable is deleted| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | | [`ci_variable_updated`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/91983) | Triggered when a project's CI variable is updated| **{check-circle}** Yes | **{check-circle}** Yes | GitLab [15.2](https://gitlab.com/gitlab-org/gitlab/-/issues/363090) | diff --git a/ee/app/services/ci/audit_variable_change_service.rb b/ee/app/services/ci/audit_variable_change_service.rb index 3d72b170806b57..7523ec6175f393 100644 --- a/ee/app/services/ci/audit_variable_change_service.rb +++ b/ee/app/services/ci/audit_variable_change_service.rb @@ -4,7 +4,12 @@ module Ci class AuditVariableChangeService < ::BaseContainerService include ::Audit::Changes - AUDITABLE_VARIABLE_CLASSES = [::Ci::Variable, ::Ci::GroupVariable].freeze + AUDITABLE_VARIABLE_CLASSES = [::Ci::Variable, ::Ci::GroupVariable, ::Ci::InstanceVariable].freeze + TEXT_COLUMNS = %i[key value].freeze + SECURITY_COLUMNS = { + protected: 'variable protection', + masked: 'variable masking' + }.freeze def execute return unless container.feature_available?(:audit_events) @@ -14,16 +19,46 @@ def execute when :create, :destroy log_audit_event(params[:action], params[:variable]) when :update + audit_update(params[:variable]) + end + end + + private + + def audit_update(variable) + SECURITY_COLUMNS.each do |column, as_text| audit_changes( - :protected, - as: 'variable protection', entity: container, - model: params[:variable], target_details: params[:variable].key, - event_type: event_type_name(params[:variable], params[:action]) + column, + as: as_text, + entity: container, + model: variable, + target_details: variable.key, + event_type: event_type_name(variable, :update) + ) + end + + TEXT_COLUMNS.compact.each do |column| + # instance variables don't have a :value column + next if column == :value && variable.is_a?(::Ci::InstanceVariable) + + audit_changes( + column, + entity: container, + model: variable, + target_details: variable.key, + event_type: event_type_name(variable, :update), + skip_changes: skip_changes?(variable, column) ) end end - private + def skip_changes?(variable, column) + return false unless column == :value + # do not include masked values in audit, if masking or unmasking + return true if variable.masked? + + variable.masked_changed? && variable.masked_change.any? + end def log_audit_event(action, variable) audit_context = { diff --git a/ee/config/audit_events/types/ci_instance_variable_created.yml b/ee/config/audit_events/types/ci_instance_variable_created.yml new file mode 100644 index 00000000000000..767b474cbd1f2b --- /dev/null +++ b/ee/config/audit_events/types/ci_instance_variable_created.yml @@ -0,0 +1,9 @@ +--- +name: ci_instance_variable_created +description: When an instance level CI variable is created +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882 +feature_category: continuous_integration +milestone: '16.5' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/ci_instance_variable_deleted.yml b/ee/config/audit_events/types/ci_instance_variable_deleted.yml new file mode 100644 index 00000000000000..6539944fb66017 --- /dev/null +++ b/ee/config/audit_events/types/ci_instance_variable_deleted.yml @@ -0,0 +1,9 @@ +--- +name: ci_instance_variable_deleted +description: When an instance level CI varialbe is deleted +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882 +feature_category: continuous_integration +milestone: '16.5' +saved_to_database: true +streamed: true diff --git a/ee/config/audit_events/types/ci_instance_variable_updated.yml b/ee/config/audit_events/types/ci_instance_variable_updated.yml new file mode 100644 index 00000000000000..85596ac1bf5c20 --- /dev/null +++ b/ee/config/audit_events/types/ci_instance_variable_updated.yml @@ -0,0 +1,9 @@ +--- +name: ci_instance_variable_updated +description: When an instance level CI variable is changed +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/8070 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/131882 +feature_category: continuous_integration +milestone: '16.5' +saved_to_database: true +streamed: true diff --git a/ee/spec/services/ci/audit_variable_change_service_spec.rb b/ee/spec/services/ci/audit_variable_change_service_spec.rb index 1f8b8093fdb06e..4c118970d9d2c9 100644 --- a/ee/spec/services/ci/audit_variable_change_service_spec.rb +++ b/ee/spec/services/ci/audit_variable_change_service_spec.rb @@ -8,6 +8,7 @@ let_it_be(:user) { create(:user) } let(:group) { create(:group) } + let(:instance_variable) { create(:ci_instance_variable, key: 'CI_DEBUG_TRACE', value: true) } let(:group_variable) { create(:ci_group_variable, group: group) } let(:destination) { create(:external_audit_event_destination, group: group) } let(:project) { create(:project, group: group) } @@ -27,13 +28,13 @@ expect { execute }.to change(AuditEvent, :count).from(0).to(1) end - it 'logs variable group creation' do + it 'logs (project/group/instance) variable creation' do execute - audit_event = AuditEvent.last.present + audit_event = AuditEvent.last.presence - expect(audit_event.action).to eq(message) - expect(audit_event.target).to eq(variable.key) + expect(audit_event.details[:custom_message]).to eq(message) + expect(audit_event.details[:target_details]).to eq(variable.key) end it_behaves_like 'sends correct event type in audit event stream' do @@ -55,10 +56,10 @@ it 'logs variable protection update' do execute - audit_event = AuditEvent.last.present + audit_event = AuditEvent.last.presence - expect(audit_event.action).to eq('Changed variable protection from false to true') - expect(audit_event.target).to eq(variable.key) + expect(audit_event.details[:custom_message]).to eq('Changed variable protection from false to true') + expect(audit_event.details[:target_details]).to eq(variable.key) end it_behaves_like 'sends correct event type in audit event stream' do @@ -66,6 +67,47 @@ end end + shared_examples 'audit value change' do + let(:action) { :update } + + context 'when updated masked' do + before do + variable.masked = true + variable.save! + end + + it 'logs audit event' do + expect { execute }.to change(AuditEvent, :count).from(0).to(1) + end + + it 'logs variable masked update' do + execute + + audit_event = AuditEvent.last.presence + + expect(audit_event.details[:custom_message]).to eq('Changed variable masking from false to true') + expect(audit_event.details[:target_details]).to eq(variable.key) + end + + it_behaves_like 'sends correct event type in audit event stream' do + let_it_be(:event_type) { event_type } + end + end + + context 'when masked is and was false' do + it 'audit with from and to of the value' do + variable.masked = false + variable.value = 'A' + variable.save! + variable.reload + + variable.value = 'B' + + expect { execute }.not_to change(AuditEvent, :count) + end + end + end + shared_examples 'no audit events are created' do context 'when creating variable' do let(:action) { :create } @@ -106,7 +148,7 @@ it 'logs variable destruction' do execute - audit_event = AuditEvent.last.present + audit_event = AuditEvent.last.presence expect(audit_event.action).to eq(message) expect(audit_event.target).to eq(variable.key) @@ -124,55 +166,72 @@ group.external_audit_event_destinations.create!(destination_url: 'http://example.com') end - context 'when creating group variable' do - it_behaves_like 'audit creation' do - let(:variable) { group_variable } + context 'with instance variables' do + let(:variable) { instance_variable } - let_it_be(:message) { 'Added ci group variable' } - let_it_be(:event_type) { "ci_group_variable_created" } + context 'when creating instance variable' do + it_behaves_like 'audit creation' do + let_it_be(:message) { 'Added ci instance variable' } + let_it_be(:event_type) { "ci_instance_variable_created" } + end end - end - - context 'when updating group variable protection' do - it_behaves_like 'audit when updating variable protection' do - let(:variable) { group_variable } - let_it_be(:event_type) { "ci_group_variable_updated" } + context 'when updating instance variable protection' do + it_behaves_like 'audit when updating variable protection' do + let_it_be(:event_type) { "ci_instance_variable_updated" } + end end end - context 'when deleting group variable' do - it_behaves_like 'audit when updating variable protection' do - let(:variable) { group_variable } + context 'with group variables' do + let(:variable) { group_variable } - let_it_be(:message) { 'Removed ci group variable' } + it_behaves_like 'audit value change' do let_it_be(:event_type) { "ci_group_variable_updated" } end - end - context 'when creating project variable' do - it_behaves_like 'audit creation' do - let(:variable) { project_variable } + context 'when creating group variable' do + it_behaves_like 'audit creation' do + let_it_be(:message) { 'Added ci group variable' } + let_it_be(:event_type) { "ci_group_variable_created" } + end + end + + context 'when updating group variable protection' do + it_behaves_like 'audit when updating variable protection' do + let_it_be(:event_type) { "ci_group_variable_updated" } + end + end - let_it_be(:message) { 'Added ci variable' } - let_it_be(:event_type) { "ci_variable_created" } + context 'when deleting group variable' do + it_behaves_like 'audit when updating variable protection' do + let_it_be(:message) { 'Removed ci group variable' } + let_it_be(:event_type) { "ci_group_variable_updated" } + end end end - context 'when updating project variable protection' do - it_behaves_like 'audit when updating variable protection' do - let(:variable) { project_variable } + context 'with project variables' do + let(:variable) { project_variable } - let_it_be(:event_type) { "ci_variable_updated" } + context 'when creating project variable' do + it_behaves_like 'audit creation' do + let_it_be(:message) { 'Added ci variable' } + let_it_be(:event_type) { "ci_variable_created" } + end end - end - context 'when deleting project variable' do - it_behaves_like 'audit when updating variable protection' do - let(:variable) { project_variable } + context 'when updating project variable protection' do + it_behaves_like 'audit when updating variable protection' do + let_it_be(:event_type) { "ci_variable_updated" } + end + end - let_it_be(:message) { 'Removed ci variable' } - let_it_be(:event_type) { "ci_variable_updated" } + context 'when deleting project variable' do + it_behaves_like 'audit when updating variable protection' do + let_it_be(:message) { 'Removed ci variable' } + let_it_be(:event_type) { "ci_variable_updated" } + end end end end -- GitLab