diff --git a/config/audit_events/types/merge_request_branch_bypassed_by_security_policy.yml b/config/audit_events/types/merge_request_branch_bypassed_by_security_policy.yml new file mode 100644 index 0000000000000000000000000000000000000000..df611a29d8395c040103fc76871ff25617c22a9e --- /dev/null +++ b/config/audit_events/types/merge_request_branch_bypassed_by_security_policy.yml @@ -0,0 +1,11 @@ +--- +name: merge_request_branch_bypassed_by_security_policy +description: The merge request's approval is bypassed by the branches configured in + the security policy +introduced_by_issue: https://gitlab.com/gitlab-org/gitlab/-/issues/549646 +introduced_by_mr: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/195942 +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 78d9af2078d320b928a031dc2b283196f45e61ce..97e1271bda277850868c3793e70b26e220f54258 100644 --- a/doc/user/compliance/audit_event_types.md +++ b/doc/user/compliance/audit_event_types.md @@ -537,6 +537,7 @@ Audit event types belong to the following product categories. | [`security_policy_create`](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/192797) | A security policy is created | {{< icon name="check-circle" >}} Yes | GitLab [18.1](https://gitlab.com/gitlab-org/gitlab/-/issues/539230) | Project | | [`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 | | [`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_merge_request_rule.rb b/ee/app/models/approval_merge_request_rule.rb index b3d699a0c7d30fbfbee55238293b276ce750ce08..f23ce6be4d392a9ee2dd6fb4d87c868c3f936d70 100644 --- a/ee/app/models/approval_merge_request_rule.rb +++ b/ee/app/models/approval_merge_request_rule.rb @@ -139,6 +139,11 @@ def applicable_to_branch?(branch) self.approval_project_rule.applies_to_branch?(branch) end + def branches_exempted_by_policy? + approval_policy_rule.present? && + approval_policy_rule.branches_exempted_by_policy?(merge_request.source_branch, merge_request.target_branch) + end + def sync_approved_approvers # Before being merged, approved_approvers are dynamically calculated in # ApprovalWrappedRule instead of being persisted. @@ -237,9 +242,4 @@ def policy_applies_to_target_branch?(branch) approval_policy_rule.policy_applies_to_target_branch?(branch, project.default_branch) end - - def branches_exempted_by_policy? - approval_policy_rule.present? && - approval_policy_rule.branches_exempted_by_policy?(merge_request.source_branch, merge_request.target_branch) - end end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index dbdee243d58394fc4af9283bcf9709c9b8bc0cd6..1c92e8b9c17b7154dd8deabf6b02f2e130a79bf1 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -485,6 +485,16 @@ def schedule_policy_synchronization end end + def security_policies_with_branch_exceptions + approval_rules + .includes(approval_policy_rule: :security_policy) + .select(&:branches_exempted_by_policy?) + .map(&:approval_policy_rule) + .map(&:security_policy) + .uniq + end + strong_memoize_attr :security_policies_with_branch_exceptions + # TODO: Will be removed with https://gitlab.com/gitlab-org/gitlab/-/issues/504296 def sync_project_approval_rules_for_policy_configuration(configuration_id) return if merged? diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index c5b804eaabd56e6670b4e9abbe9431950127d842..fc61c728f94b9407e1a95bd19e8f8b9334c4b4db 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -19,6 +19,7 @@ class Policy < ApplicationRecord APPROVAL_MERGE_REQUEST_RULES_BATCH_SIZE = 5000 belongs_to :security_orchestration_policy_configuration, class_name: 'Security::OrchestrationPolicyConfiguration' + belongs_to :security_policy_management_project, class_name: 'Project' has_many :approval_policy_rules, class_name: 'Security::ApprovalPolicyRule', foreign_key: 'security_policy_id', inverse_of: :security_policy has_many :scan_execution_policy_rules, class_name: 'Security::ScanExecutionPolicyRule', diff --git a/ee/app/services/ee/merge_requests/after_create_service.rb b/ee/app/services/ee/merge_requests/after_create_service.rb index e3d9e1edf095d415b90742aa7abaf4b91e04d49e..efdd2a926fd89cc87e6235f440e577a2cfc7af26 100644 --- a/ee/app/services/ee/merge_requests/after_create_service.rb +++ b/ee/app/services/ee/merge_requests/after_create_service.rb @@ -22,6 +22,7 @@ def prepare_merge_request(merge_request) schedule_approval_notifications(merge_request) schedule_duo_code_review(merge_request) track_usage_event if merge_request.project.scan_result_policy_reads.any? + audit_security_policy_branch_bypass(merge_request) publish_event(merge_request) end diff --git a/ee/app/services/ee/merge_requests/base_service.rb b/ee/app/services/ee/merge_requests/base_service.rb index 850c040de37e2d57e9bdff0b3f573c983b8a7166..a26607257c8042a1fa0b2f4ae747f855d302015f 100644 --- a/ee/app/services/ee/merge_requests/base_service.rb +++ b/ee/app/services/ee/merge_requests/base_service.rb @@ -139,6 +139,22 @@ def capture_suggested_reviewers_accepted(merge_request) .perform_async(merge_request.id, suggested_reviewer_ids) end + def audit_security_policy_branch_bypass(merge_request) + matching_policies = merge_request.security_policies_with_branch_exceptions + + return if matching_policies.empty? + + matching_policies.each do |policy| + next unless ::Feature.enabled?(:approval_policy_branch_exceptions, policy.security_policy_management_project) + + log_audit_event( + merge_request, + 'merge_request_branch_bypassed_by_security_policy', + "Approvals bypassed by security policy #{policy.name}" + ) + end + end + def log_audit_event(merge_request, event_name, message) audit_context = { name: event_name, diff --git a/ee/app/services/ee/merge_requests/reopen_service.rb b/ee/app/services/ee/merge_requests/reopen_service.rb index 1ac7c561a6d600fa76493726c803bb2023113fd4..5f68e88b3551e1550029fa032079ece53fa8cc49 100644 --- a/ee/app/services/ee/merge_requests/reopen_service.rb +++ b/ee/app/services/ee/merge_requests/reopen_service.rb @@ -10,6 +10,7 @@ def execute(merge_request) super.tap do delete_approvals(merge_request) resync_policies(merge_request) + audit_security_policy_branch_bypass(merge_request) if current_user.project_bot? log_audit_event(merge_request, 'merge_request_reopened_by_project_bot', diff --git a/ee/app/services/ee/merge_requests/update_service.rb b/ee/app/services/ee/merge_requests/update_service.rb index 117eccac64b43ffd7cc959ad7c617511425488e4..4233b6d0248ab4b78f22bd9ca6614136bb9c84ad 100644 --- a/ee/app/services/ee/merge_requests/update_service.rb +++ b/ee/app/services/ee/merge_requests/update_service.rb @@ -63,6 +63,7 @@ def delete_approvals_on_target_branch_change(merge_request) delete_approvals(merge_request) if reset_approvals?(merge_request, nil) sync_any_merge_request_approval_rules(merge_request) notify_for_policy_violations(merge_request) + audit_security_policy_branch_bypass(merge_request) end def reset_approval_rules(merge_request) diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index 596d68a8f7c3eb7cce8a4f5cd34e7b37bf8f7999..193d0ede28e03984487c0ef4ea08ca75c4910931 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -3468,4 +3468,43 @@ def stub_foss_conditions_met it_behaves_like 'synchronizes policies for a merge request' end + + describe '#security_policies_with_branch_exceptions' do + let_it_be(:security_policy) do + create(:security_policy, content: { + bypass_settings: { + branches: [ + { source: { name: 'feature' }, target: { name: 'main' } } + ] + } + }) + end + + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, security_policy: security_policy) + end + + let_it_be_with_refind(:merge_request) { create(:merge_request, source_branch: 'feature', target_branch: 'main') } + + context 'when merge_request has approval rule with security policy' do + let_it_be(:approval_rule) do + create(:approval_merge_request_rule, merge_request: merge_request, approval_policy_rule: approval_policy_rule) + end + + it 'returns the security policy that exempts the branch' do + expect(merge_request.security_policies_with_branch_exceptions).to contain_exactly(security_policy) + end + + it 'returns empty if no rules exempt the branch' do + merge_request.update!(source_branch: 'other', target_branch: 'main') + expect(merge_request.security_policies_with_branch_exceptions).to be_empty + end + end + + context 'when merge_request does not have approval rule with security policy' do + it 'returns empty' do + expect(merge_request.security_policies_with_branch_exceptions).to be_empty + end + end + end end diff --git a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb index c25ce4c9a2aa8927036121ee959cab86590ec4dc..8afa93b8960a5fae08661d9171329e720627c2ac 100644 --- a/ee/spec/services/ee/merge_requests/after_create_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/after_create_service_spec.rb @@ -36,6 +36,8 @@ it_behaves_like 'synchronizes policies for a merge request' end + it_behaves_like 'audits security policy branch bypass' + describe 'suggested reviewers' do before do allow(MergeRequests::FetchSuggestedReviewersWorker).to receive(:perform_async) diff --git a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb index 5f48ea38365d3312a2f1dcf53be90b33af7c0a86..0e18986d6f5c5ee2161554d9b3f0684afcad63f0 100644 --- a/ee/spec/services/ee/merge_requests/reopen_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/reopen_service_spec.rb @@ -65,6 +65,10 @@ end end + it_behaves_like 'audits security policy branch bypass' do + let(:execute) { merge_request_reopen_service } + end + describe '#resync_policies' do let(:feature_licensed) { true } let_it_be(:scan_result_policy_read) { create(:scan_result_policy_read, project: project) } diff --git a/ee/spec/services/ee/merge_requests/update_service_spec.rb b/ee/spec/services/ee/merge_requests/update_service_spec.rb index d90970e5c537d288a6eeb5e9e7402e470c39552c..ac99363b0b5995de7d25651c8f0ed9c9c3664056 100644 --- a/ee/spec/services/ee/merge_requests/update_service_spec.rb +++ b/ee/spec/services/ee/merge_requests/update_service_spec.rb @@ -286,6 +286,14 @@ def update_merge_request(opts) it_behaves_like 'reset all approvals' end + + it_behaves_like 'audits security policy branch bypass' do + before do + merge_request.update!(target_branch: 'master') + end + + let(:execute) { update_merge_request(target_branch: 'main') } + end end context 'updating blocking merge requests' do diff --git a/ee/spec/support/shared_examples/services/merge_requests/audit_security_policy_branch_bypass_shared_examples.rb b/ee/spec/support/shared_examples/services/merge_requests/audit_security_policy_branch_bypass_shared_examples.rb new file mode 100644 index 0000000000000000000000000000000000000000..abd89fcf75d0ea8bbd21f6ba0cfd44d86ef740a7 --- /dev/null +++ b/ee/spec/support/shared_examples/services/merge_requests/audit_security_policy_branch_bypass_shared_examples.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.shared_examples 'audits security policy branch bypass' do + let_it_be_with_refind(:merge_request) do + create(:merge_request, source_branch: 'feature', target_branch: 'main') + end + + context 'when security policy with branch bypass is present' do + let_it_be(:security_policy) do + create(:security_policy, content: { + bypass_settings: { + branches: [ + { source: { name: 'feature' }, target: { name: 'main' } } + ] + } + }) + end + + let_it_be(:approval_policy_rule) do + create(:approval_policy_rule, security_policy: security_policy) + end + + let_it_be(:approval_rule) do + create(:approval_merge_request_rule, merge_request: merge_request, approval_policy_rule: approval_policy_rule) + end + + it 'creates an audit event' do + expect { execute }.to change { AuditEvent.count }.by(1) + + event = AuditEvent.last + expect(event.details[:custom_message]).to include('Approvals bypassed by security policy') + expect(event.entity).to eq(merge_request.target_project) + end + + context 'when approval_policy_branch_exceptions is disabled' do + before do + stub_feature_flags(approval_policy_branch_exceptions: false) + end + + it 'does not create an audit event' do + expect { execute }.not_to change { AuditEvent.count } + end + end + end + + context 'when security policy does not exist with branch bypass' do + it 'does not create an audit event' do + expect { execute }.not_to change { AuditEvent.count } + end + end +end