diff --git a/config/audit_events/types/merge_request_merged_with_policy_violations.yml b/config/audit_events/types/merge_request_merged_with_policy_violations.yml new file mode 100644 index 0000000000000000000000000000000000000000..be73d13cd42c5f994401b80a9833cb265d30c8f4 --- /dev/null +++ b/config/audit_events/types/merge_request_merged_with_policy_violations.yml @@ -0,0 +1,10 @@ +--- +name: merge_request_merged_with_policy_violations +description: A merge request merged with security policy violations +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/work_items/549813 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775 +feature_category: security_policy_management +milestone: '18.2' +saved_to_database: true +streamed: true +scope: [Project] diff --git a/doc/user/compliance/audit_event_types.md b/doc/user/compliance/audit_event_types.md index 3c9fbdd6aef471c36168e2ecd8c2363e14230adc..796f9798bbf288566959cb2623d6ad33bb0282d2 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -539,6 +539,7 @@ Audit event types belong to the following product categories. | [`security_policy_delete`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is deleted | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`security_policy_update`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is updated | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`merge_request_branch_bypassed_by_security_policy`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195942) | The merge request's approval is bypassed by the branches configured in the security policy | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/issues/549646) | Project | +| [`merge_request_merged_with_policy_violations`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775) | A merge request merged with security policy violations | {{< icon name="check-circle" >}} Yes | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/549813) | Project | | [`policy_violations_detected`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193482) | Security policy violation is detected in the merge request | {{< icon name="dotted-circle" >}} No | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/work_items/549811) | Project | | [`policy_violations_resolved`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/193482) | Security policy violations are resolved in the merge request | {{< icon name="dotted-circle" >}} No | GitLab [18.2](https://gitlab.com/gitlab-org/gitlab/-/issues/549812) | Project | diff --git a/ee/app/models/approval_wrapped_rule.rb b/ee/app/models/approval_wrapped_rule.rb index dcfc7c4ac9164a3ec65ea11a306bdcdb3da4e9e9..14040684dae3cf7d2b80ef7c98faacd0969a1337 100644 --- a/ee/app/models/approval_wrapped_rule.rb +++ b/ee/app/models/approval_wrapped_rule.rb @@ -24,7 +24,7 @@ class ApprovalWrappedRule def_delegators( :@approval_rule, :regular?, :any_approver?, :code_owner?, :report_approver?, - :overridden?, :id, :users, :groups, :code_owner, + :overridden?, :id, :users, :groups, :code_owner, :from_scan_result_policy?, :source_rule, :rule_type, :report_type, :approvals_required, :section, :to_global_id ) @@ -99,7 +99,7 @@ def invalid_rule? def allow_merge_when_invalid? return true if fail_open? - !approval_rule.from_scan_result_policy? || + !from_scan_result_policy? || Security::OrchestrationPolicyConfiguration.policy_management_project?(project) end @@ -143,7 +143,7 @@ def unactioned_approvers end def name - return approval_rule.name unless approval_rule.from_scan_result_policy? + return approval_rule.name unless from_scan_result_policy? if policy_has_multiple_actions? "#{approval_rule.policy_name} - Action #{approval_rule.approval_policy_action_idx + 1}" @@ -152,12 +152,12 @@ def name end end - private - def fail_open? approval_rule.scan_result_policy_read&.fail_open? || false end + private + def filter_approvers(approvers) filtered_approvers = ApprovalState.filter_author(approvers, merge_request) diff --git a/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb new file mode 100644 index 0000000000000000000000000000000000000000..75d2769680b5b12ded5d717a0610dd1dda0d8a3c --- /dev/null +++ b/ee/app/services/merge_requests/merged_with_policy_violations_audit_event_service.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +module MergeRequests + class MergedWithPolicyViolationsAuditEventService + include Gitlab::Utils::StrongMemoize + + attr_reader :merge_request + + def initialize(merge_request) + @merge_request = merge_request + end + + def execute + return unless merge_request.merged? + return if violations.empty? + + audit_context = { + name: 'merge_request_merged_with_policy_violations', + author: merged_by, + scope: target_project, + target: merge_request, + message: "Merge request with title '#{merge_request.title}' was merged with " \ + "#{violations.count} security policy violation(s)", + additional_details: audit_details + } + + ::Gitlab::Audit::Auditor.audit(audit_context) + end + + private + + def merged_by + merge_request.metrics.merged_by || Gitlab::Audit::DeletedAuthor.new(id: -4, name: 'Unknown User') + end + + def audit_details + { + merge_request_title: merge_request.title, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + merged_at: merge_request.merged_at, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + project_id: target_project.id, + project_name: target_project.name, + project_full_path: target_project.full_path, + violated_policies: violated_policies, + security_policy_approval_rules: policy_approval_rules, + violation_details: violation_details + } + end + + # There can be violated policies which were skipped because of invalid rules + # We collect both types here for visibility + def wrapped_approval_rules + approval_state.wrapped_approval_rules + approval_state.invalid_approvers_rules + end + + def policy_approval_rules + wrapped_approval_rules.filter_map do |wrapped_rule| + next unless wrapped_rule.from_scan_result_policy? + + { + name: wrapped_rule.name, + report_type: wrapped_rule.report_type, + approvals_required: wrapped_rule.approvals_required, + approved: wrapped_rule.approved?, + approved_approvers: wrapped_rule.approved_approvers.map(&:username).sort, + invalid_rule: wrapped_rule.invalid_rule?, + fail_open: wrapped_rule.fail_open? + } + end + end + + def violation_details + { + fail_open_policies: details.fail_open_policies.as_json, + fail_closed_policies: details.fail_closed_policies.as_json, + warn_mode_policies: details.warn_mode_policies.as_json, + new_scan_finding_violations: details.new_scan_finding_violations.as_json, + previous_scan_finding_violations: details.previous_scan_finding_violations.as_json, + license_scanning_violations: details.license_scanning_violations.as_json, + any_merge_request_violations: details.any_merge_request_violations.as_json, + errors: details.errors.as_json, + comparison_pipelines: details.comparison_pipelines.as_json + } + end + + def violated_policies + violations.filter_map do |violation| + security_policy = violation.security_policy + next unless security_policy + + policy_configuration = security_policy.security_orchestration_policy_configuration + + { + policy_id: security_policy.id, + policy_name: security_policy.name, + policy_type: security_policy.type, + security_orchestration_policy_configuration_id: policy_configuration.id, + security_policy_management_project_id: policy_configuration.security_policy_management_project_id + } + end + end + + def violations + merge_request.scan_result_policy_violations + end + strong_memoize_attr :violations + + def target_project + merge_request.project + end + strong_memoize_attr :target_project + + def approval_state + ApprovalState.new(merge_request) + end + strong_memoize_attr :approval_state + + def details + Security::ScanResultPolicies::PolicyViolationDetails.new(merge_request) + end + strong_memoize_attr :details + end +end diff --git a/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb b/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb index cdcbb94fd8a72993c9df695463389ae49278d19a..7924aa813cf280af094e2aa0cbddba815db30ec7 100644 --- a/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb +++ b/ee/app/workers/security/scan_result_policies/cleanup_merge_request_violations_worker.rb @@ -15,7 +15,12 @@ def handle_event(event) project = merge_request.project return unless project.licensed_feature_available?(:security_orchestration_policies) - log_running_violations_after_merge(merge_request) if event.is_a?(::MergeRequests::MergedEvent) + return if merge_request.scan_result_policy_violations.none? + + if event.is_a?(::MergeRequests::MergedEvent) + log_running_violations_after_merge(merge_request) + audit_merged_with_policy_violations(merge_request) + end merge_request.scan_result_policy_violations.delete_all(:delete_all) end @@ -36,6 +41,13 @@ def log_running_violations_after_merge(merge_request) approval_policy_rule_ids: running_violations.filter_map(&:approval_policy_rule_id) ) end + + def audit_merged_with_policy_violations(merge_request) + return unless Feature.enabled?(:collect_merge_request_merged_with_policy_violations_audit_events, + merge_request.project) + + MergeRequests::MergedWithPolicyViolationsAuditEventService.new(merge_request).execute + end end end end diff --git a/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml b/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml new file mode 100644 index 0000000000000000000000000000000000000000..c78d2eb5f3831f9ae302f97ac01de0c82c04bdc4 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/collect_merge_request_merged_with_policy_violations_audit_events.yml @@ -0,0 +1,10 @@ +--- +name: collect_merge_request_merged_with_policy_violations_audit_events +description: +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/work_items/549813 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195775 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/551784 +milestone: '18.2' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/models/approval_wrapped_rule_spec.rb b/ee/spec/models/approval_wrapped_rule_spec.rb index 480177a25af519356338880a74d0a3aeaa5e7ae6..909b8a19b9c9fda3ab91bba91e2cd61c9f0b6a5a 100644 --- a/ee/spec/models/approval_wrapped_rule_spec.rb +++ b/ee/spec/models/approval_wrapped_rule_spec.rb @@ -507,4 +507,56 @@ def approved_approvers_for_rule_id(rule_id) end end end + + describe '#fail_open?' do + subject { approval_wrapped_rule.fail_open? } + + context 'when rule has a linked scan_result_policy_read' do + before do + rule.update!(scan_result_policy_read: scan_result_policy_read) + end + + context 'when rule is fail open' do + let(:scan_result_policy_read) { build(:scan_result_policy_read, :fail_open) } + + it { is_expected.to eq(true) } + end + + context 'when rule is not fail closed' do + let(:scan_result_policy_read) { build(:scan_result_policy_read, :fail_closed) } + + it { is_expected.to eq(false) } + end + end + + context 'when rule has no linked scan_result_policy_read' do + it { is_expected.to eq(false) } + end + end + + describe "#from_scan_result_policy?" do + subject { approval_wrapped_rule.from_scan_result_policy? } + + it 'delegates from_scan_result_policy to approval_rule' do + expect(rule).to receive(:from_scan_result_policy?) + + subject + end + + context 'when the approval rule is from a scan result policy' do + before do + rule.update!(report_type: :scan_finding) + end + + it { is_expected.to eq(true) } + end + + context 'when the approval rule is not from a scan result policy' do + before do + rule.update!(report_type: :code_coverage) + end + + it { is_expected.to eq(false) } + end + end end diff --git a/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb new file mode 100644 index 0000000000000000000000000000000000000000..9762f7f711b9269feb8b404f64a75d24055693fe --- /dev/null +++ b/ee/spec/services/merge_requests/merged_with_policy_violations_audit_event_service_spec.rb @@ -0,0 +1,223 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe MergeRequests::MergedWithPolicyViolationsAuditEventService, feature_category: :security_policy_management do + let_it_be(:merger) { create :user } + let_it_be(:approver) { create :user, username: 'approver one' } + let_it_be(:mr_author) { create :user, username: 'author one' } + let_it_be(:project) { create :project, :repository } + let_it_be(:merge_time) { Time.now.utc } + let_it_be_with_reload(:merge_request) do + create :merge_request, + :opened, + title: 'MR One', + description: 'This was a triumph', + author: mr_author, + source_project: project, + target_project: project + end + + let_it_be(:policy_project) { create(:project, :repository) } + let_it_be(:policy_configuration) do + create(:security_orchestration_policy_configuration, project: project, + security_policy_management_project: policy_project) + end + + let_it_be(:security_policy_name) { 'Test Policy' } + let_it_be(:security_policy) do + create(:security_policy, :approval_policy, + name: security_policy_name, + security_orchestration_policy_configuration: policy_configuration) + end + + let(:service) { described_class.new(merge_request) } + + def build_violation_details(policy, data, status = :failed) + create(:scan_result_policy_violation, status, project: project, merge_request: merge_request, + scan_result_policy_read: policy, approval_policy_rule: approval_policy_rule, violation_data: data) + end + + describe '#execute' do + let(:execute_service) { service.execute } + + let(:audit_context) do + { + name: 'merge_request_merged_with_policy_violations', + author: merger, + scope: project, + target: merge_request, + message: "Merge request with title 'MR One' was merged with 1 security policy violation(s)", + additional_details: { + merge_request_title: merge_request.title, + merge_request_id: merge_request.id, + merge_request_iid: merge_request.iid, + merged_at: merge_request.merged_at, + source_branch: merge_request.source_branch, + target_branch: merge_request.target_branch, + project_id: project.id, + project_name: project.name, + project_full_path: project.full_path, + violated_policies: [ + { + policy_id: security_policy.id, + policy_name: security_policy_name, + policy_type: 'approval_policy', + security_policy_management_project_id: policy_project.id, + security_orchestration_policy_configuration_id: policy_configuration.id + } + ], + security_policy_approval_rules: policy_approval_rules, + violation_details: violation_details + } + } + end + + shared_examples 'recording the audit event' do + it 'records a merge_request_merged_with_policy_violations audit event' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with(audit_context) + + execute_service + end + end + + shared_examples 'not recording the audit event' do + it 'does not record a merge_request_merged_with_policy_violations audit event' do + expect(::Gitlab::Audit::Auditor).not_to receive(:audit).with(anything) + + execute_service + end + end + + context 'when merge request is merged' do + context 'with scan result policy violations' do + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, :license_finding, security_policy: security_policy) + end + + let_it_be_with_reload(:approval_rule) do + create( + :report_approver_rule, + :license_scanning, + merge_request: merge_request.reload, + approval_policy_rule: approval_policy_rule, + approvals_required: 1, + user_ids: [approver.id] + ) + end + + let_it_be(:approval_merge_request_rules_approved_approver) do + create(:approval_merge_request_rules_approved_approver, + approval_merge_request_rule: approval_rule, user: approver) + end + + let(:policy_approval_rules) do + [ + { + name: approval_rule.name, + report_type: 'license_scanning', + approved: true, + approvals_required: 1, + approved_approvers: ['approver one'], + invalid_rule: false, + fail_open: false + } + ] + end + + let(:license_scanning_violations_json) do + { + "dependencies" => %w[A B], + "license" => "MIT License", + "url" => "https://spdx.org/licenses/MIT.html" + } + end + + let(:violations_count) { 1 } + let(:violation_details) do + { + fail_open_policies: [], + fail_closed_policies: [security_policy_name], + warn_mode_policies: [], + new_scan_finding_violations: [], + previous_scan_finding_violations: [], + license_scanning_violations: [license_scanning_violations_json], + any_merge_request_violations: [], + errors: [], + comparison_pipelines: [] + } + end + + before do + policy = create(:scan_result_policy_read, project: project, + security_orchestration_policy_configuration: policy_configuration) + + create(:report_approver_rule, :license_scanning, merge_request: merge_request, + scan_result_policy_read: policy, name: security_policy_name) + + build_violation_details(policy, + violations: { license_scanning: { 'MIT License' => %w[A B] } } + ) + merge_request.update!(state_id: MergeRequest.available_states[:merged]) + merge_request.metrics.update!(merged_at: merge_time, merged_by: merger) + end + + it_behaves_like 'recording the audit event' + + context 'with invalid rules' do + before do + approval_rule.update_columns(approvals_required: 2) + end + + let(:policy_approval_rules) do + [ + { + name: approval_rule.name, + report_type: 'license_scanning', + approved: false, + approvals_required: 2, + approved_approvers: ['approver one'], + invalid_rule: true, + fail_open: false + } + ] + end + + it_behaves_like 'recording the audit event' + end + + context 'when merged by author is not available' do + before do + merge_request.metrics.update!(merged_by: nil) + end + + it 'audits with a deleted author' do + expect(::Gitlab::Audit::Auditor).to receive(:audit).with( + a_hash_including( + author: an_instance_of(Gitlab::Audit::DeletedAuthor) + ) + ) + + execute_service + end + end + end + + context 'without scan result policy violations' do + before do + merge_request.update!(state_id: MergeRequest.available_states[:merged]) + end + + it_behaves_like 'not recording the audit event' + end + end + + context 'when merge request is not merged' do + before do + merge_request.update!(state_id: MergeRequest.available_states[:closed]) + end + + it_behaves_like 'not recording the audit event' + end + end +end diff --git a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb index 153aff2a3f5646d2161c30fa0f7abbf346549555..5c8089262e3f555c6101a943328213a250cb3cd0 100644 --- a/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb +++ b/ee/spec/workers/security/scan_result_policies/cleanup_merge_request_violations_worker_spec.rb @@ -59,6 +59,22 @@ end end + shared_examples_for 'not logging running violations' do + it 'does not log running violations' do + expect(Gitlab::AppJsonLogger).not_to receive(:info) + + perform + end + end + + shared_examples_for 'not recording audit event for violations' do + it 'does not record merged with policy violations audit event' do + expect(MergeRequests::MergedWithPolicyViolationsAuditEventService).not_to receive(:new) + + perform + end + end + context 'with existing merge request' do context 'when event is MergedEvent' do let(:event) { merge_request_merged_event } @@ -77,16 +93,37 @@ perform end - context 'when there are no running violations' do + it 'records merged with policy violations audit event' do + allow_next_instance_of(::MergeRequests::MergedWithPolicyViolationsAuditEventService) do |audit_event_service| + expect(audit_event_service).to receive(:execute) + end + + perform + end + + context 'when there is no merge_request scan result policy violations' do before do - merge_request_violation.update!(status: :failed) + merge_request_violation.update!(merge_request_id: unrelated_merge_request_violation.merge_request_id) end - it 'does not log running violations' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) + it_behaves_like 'not logging running violations' + it_behaves_like 'not recording audit event for violations' + end + + context 'when the collect merged with policy violations audit event feature is disabled' do + before do + stub_feature_flags(collect_merge_request_merged_with_policy_violations_audit_events: false) + end + + it_behaves_like 'not recording audit event for violations' + end - perform + context 'when there are no running violations' do + before do + merge_request_violation.update!(status: :failed) end + + it_behaves_like 'not logging running violations' end end @@ -96,11 +133,7 @@ it_behaves_like 'an idempotent worker' it_behaves_like 'deletes approval policy violations' - it 'does not log running violations' do - expect(Gitlab::AppJsonLogger).not_to receive(:info) - - perform - end + it_behaves_like 'not logging running violations' end context 'when feature is not licensed' do