@fabiopitino @furkanayhan I tried the approach above and gathered some thoughts:
- The mapping is passed to each Ci::CreatePipelineService for PEP evaluation.
The issue here is that the policies are evaluated before we process the project config, so we don't have the normalized_jobs yet that we could pass for the PEP evaluation.
- If mapping doesn't exist (typical project pipeline), lazily initialize an empty mapping.
I believe this is what we already do with normalized_jobs in Ci::Config here and this MR passes it along to the YamlProcessor::Result.
- If mapping exists use that to reference
needs.
This is what this MR implements
- When jobs are added to a pipeline, they are also added to the mapping object. This allows other policies to reference an up-to-date mapping as the pipeline changes.
The jobs are added to a project pipeline towards the end, at the point when we can collect all the mappings.
We can combine all the mappings from the policy pipelines and pass them to JobInjector, so that the mappings are not dependent on the policy pipeline order.
I added tests to ensure that the needs can be evaluated also across policies.
The MR should resolve some issues with needs that customers have today, but maybe we could explore a different architectural approach of building policy pipelines where we break the EvaluatePolicies step into two:
Gitlab::Ci::Pipeline::Chain::AssignPartition,
Gitlab::Ci::Pipeline::Chain::PipelineExecutionPolicies::EvaluatePolicies, # Instead of building full policy pipelines at this point, maybe it's enough to build until `Config::Process` so that we can extract the declared stages for the project pipeline to merge
Gitlab::Ci::Pipeline::Chain::Skip,
Gitlab::Ci::Pipeline::Chain::Validate::Config,
Gitlab::Ci::Pipeline::Chain::Config::Content,
Gitlab::Ci::Pipeline::Chain::Config::Process, # Here we could extend the static validation and use the policies' `normalized_jobs` to support references from project to policies
Gitlab::Ci::Pipeline::Chain::PipelineExecutionPolicies::BuildPolicies # Up until this point we stay in the "declared pipeline space". Here we would continue the policy pipeline build based on the processed configs from before, but also feeding the processed project config for extra context.
Gitlab::Ci::Pipeline::Chain::StopLinting,
...
Gitlab::Ci::Pipeline::Chain::PipelineExecutionPolicies::ApplyPolicies, # Same as today, pipelines get merged
Gitlab::Ci::Pipeline::Chain::StopDryRun,
Or possibly using a slightly different SEQUENCE of steps for the policy pipeline.
WDYT?
Martin Cavoj (c5f7a371) at 17 Mar 18:51
Combine all job mappings to support cross-policy needs references
... and 7578 more commits
@mc_rocha could you please set to merge? I pushed a small change to the migration which removed your approval. Thanks!
@arfedoro I've just noticed - this is not a user-facing change so we shouldn't add a changelog for it.
Adds prevent_editing_approval_rules boolean to the policy approval settings JSON schemas and the ApprovalSettings value object.
This is MR 1 of 5 — a data-layer only change with no enforcement behavior. It unblocks all other MRs.
| File | Change |
|---|---|
ee/app/validators/json_schemas/approval_policy_content.json |
Add prevent_editing_approval_rules: boolean to approval_settings.properties
|
app/validators/json_schemas/scan_result_policy_project_approval_settings.json |
Add prevent_editing_approval_rules: boolean to properties
|
ee/lib/security/scan_result_policies/approval_settings.rb |
Add prevent_editing_approval_rules accessor method |
Create a project
Go to Secure Policies
In the project, create an approval policy:
approval_policy:
- name: Prevent editing approval rules
description: ''
enabled: true
actions:
- type: require_approval
approvals_required: 1
role_approvers:
- owner
- maintainer
- type: send_bot_message
enabled: true
rules:
- type: any_merge_request
branch_type: protected
commits: any
approval_settings:
block_branch_modification: false
prevent_pushing_and_force_pushing: false
prevent_approval_by_author: false
prevent_approval_by_commit_author: false
remove_approvals_with_new_commit: false
require_password_to_approve: false
prevent_editing_approval_rules: true
fallback_behavior:
fail: closed
enforcement_type: enforce
In rails console, verify that prevent_editing_approval_rules is persisted correctly:
Security::Policy.last.approval_policy.approval_settings.prevent_editing_approval_rules # => true
Part of #593362 Related to #588291
Changelog: added EE: true
@arfedoro thanks, looks good! At this point, there's nothing much to control with a feature flag, but it would be useful in a follow-up MR for the enforcement.
Can we add verification steps in the MR description? Something like:
approval_policy:
- name: Prevent editing approval rules
description: ''
enabled: true
actions:
- type: require_approval
approvals_required: 1
role_approvers:
- owner
- maintainer
- type: send_bot_message
enabled: true
rules:
- type: any_merge_request
branch_type: protected
commits: any
approval_settings:
block_branch_modification: false
prevent_pushing_and_force_pushing: false
prevent_approval_by_author: false
prevent_approval_by_commit_author: false
remove_approvals_with_new_commit: false
require_password_to_approve: false
prevent_editing_approval_rules: true
fallback_behavior:
fail: closed
enforcement_type: enforce
prevent_editing_approval_rules is persisted correctly:
Security::Policy.last.approval_policy.approval_settings.prevent_editing_approval_rules # => true
@dblessing could you please take over the maintainer review?
@bauerdominic done in !227096 (3b2b66b0).
Martin Cavoj (3b2b66b0) at 17 Mar 14:18
Add not null default 0
Security approval policies can be configured with branch rules to target specific branches (e.g., branches: ['feature/*']). Currently, when a merge request is created, approval rules and violations are registered for all policies, regardless of whether the policy's branch rules match the MR's target branch. Filtering only occurs later during evaluation in ApprovalState#wrapped_rules.
This causes several issues:
This MR introduces pre-filtering based on branch rules before registering policy violations and creating merge request rules.
When the security_policy_approval_rule_branch_filtering feature flag is enabled:
ApprovalProjectRule checks applies_to_branch? before creating the merge request approval rule and violation.This is a backend change and the merge request approval rules as well as the security policies feature are expected to work as before. However, there is one side affect of this change.
Approval overrides are applied based on persisted violations. Previously, overrides were applied regardless of the target branch because violations were registered even when policies did not apply.
This MR corrects that behavior. Approval overrides are now enforced only when the policy applies to the merge request’s target branch according to its branch rules.
See the discussion here for more on this: #553189 (comment 2903710412)
The ApprovalProjectRule#apply_report_approver_rules_to method now checks whether the approval rule applies to the merge request's target branch before creating or updating the MR approval rule.
If the branch is not protected or the rule doesn't apply to that branch, the method deletes any existing rules/violations and returns nil instead of a rule object.
This change impacts all services that eventually call apply_report_approver_rules_to:
UpdateApprovalsService → PolicyRuleEvaluationService#save → MergeRequest#reset_required_approvals → apply_report_approver_rules_to
GroupProjectTransferWorker → LinkProjectService → SyncMergeRequestsService → apply_report_approver_rules_to
MergeRequests::ReopenService → resync_policies → MergeRequest#reset_required_approvals → apply_report_approver_rules_to
MergeRequests::UpdateService → schedule_policy_synchronization → MergeRequest#sync_project_approval_rules_for_* → apply_report_approver_rules_to
SyncReportApproverApprovalRules → MergeRequest#synchronize_approval_rules_from_target_project → apply_report_approver_rules_to
SyncMergeRequestsService → sync_merge_request → MergeRequest#sync_project_approval_rules_for_approval_policy_rules → apply_report_approver_rules_to
All these services share a common downstream dependency on apply_report_approver_rules_to. To ensure tests pass, each affected spec was updated to:
protected_branch matching the merge request's target branch:for_all_protected_branches trait to approval_project_rule factories| Before | After |
|---|---|
![]() |
![]() |
New queries:
Check if any existing violations:
SELECT 1 AS one FROM "scan_result_policy_violations" INNER JOIN "scan_result_policies" ON "scan_result_policy_violations"."scan_result_policy_id" = "scan_result_policies"."id" WHERE "scan_result_policies"."id" = 23 AND "scan_result_policy_violations"."merge_request_id" = 140 LIMIT 1
explain: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/49749/commands/148126
Deleting Violations:
DELETE FROM "scan_result_policy_violations"
WHERE "scan_result_policy_violations"."id" IN (
SELECT "scan_result_policy_violations"."id"
FROM "scan_result_policy_violations"
INNER JOIN "scan_result_policies"
ON "scan_result_policy_violations"."scan_result_policy_id" = "scan_result_policies"."id"
WHERE "scan_result_policies"."id" = 23
AND "scan_result_policy_violations"."merge_request_id" = 140
);
Explain: https://console.postgres.ai/gitlab/gitlab-production-main/sessions/49749/commands/148125 (It returns 0 rows, as we can't know the scan_result_policies.id since it is not exposed through APIs)
Delete Approval Rules
DELETE FROM "approval_merge_request_rules"
WHERE ("approval_merge_request_rules"."id")
IN (
SELECT
"approval_merge_request_rules"."id"
FROM
"approval_merge_request_rules"
INNER JOIN "approval_merge_request_rule_sources" ON "approval_merge_request_rule_sources"."approval_merge_request_rule_id" = "approval_merge_request_rules"."id"
WHERE
"approval_merge_request_rules"."merge_request_id" = 16248692
AND "approval_merge_request_rules"."rule_type" = 3
AND "approval_merge_request_rule_sources"."approval_project_rule_id" = 47696360)
Query plan: https://console.postgres.aiundefined/shared/322fbb4b-07b8-459e-a879-ce6536d7dea6
Spike issue #553189
bin/rails runner "Feature.enable(:security_policy_approval_rule_branch_filtering)"
.gitlab/security-policies/policy.yml
---
approval_policy:
- name: SAST scan - feature branch
description: ''
enabled: true
rules:
- type: scan_finding
scanners:
- sast
vulnerabilities_allowed: 0
severity_levels: []
vulnerability_states: []
branches:
- feature/*
actions:
- type: require_approval
approvals_required: 1
role_approvers:
- developer
- maintainer
- owner
- type: send_bot_message
enabled: true
approval_settings:
block_branch_modification: false
prevent_pushing_and_force_pushing: false
prevent_approval_by_author: true
prevent_approval_by_commit_author: false
remove_approvals_with_new_commit: false
require_password_to_approve: false
fallback_behavior:
fail: closed
- name: Secret Detection - Protected Branch
description: ''
enabled: true
enforcement_type: enforce
rules:
- type: scan_finding
scanners:
- type: secret_detection
vulnerabilities_allowed: 0
severity_levels:
- critical
- high
vulnerability_states:
- new_needs_triage
vulnerabilities_allowed: 0
severity_levels: []
vulnerability_states: []
branch_type: protected
actions:
- type: require_approval
approvals_required: 1
group_approvers_ids:
- 24
- type: send_bot_message
enabled: true
approval_settings:
block_branch_modification: false
prevent_pushing_and_force_pushing: false
prevent_approval_by_author: false
prevent_approval_by_commit_author: false
remove_approvals_with_new_commit: false
require_password_to_approve: false
fallback_behavior:
fail: closed
.gitlab-ci.yml
image: busybox:latest
include:
- template: 'Jobs/Secret-Detection.gitlab-ci.yml'
- template: 'Jobs/SAST.gitlab-ci.yml'
stages:
- test
test_job:
stage: test
script:
- echo "Running tests..."
variables:
AST_ENABLE_MR_PIPELINES: "true"
Navigate to project -> settings -> Protected Branches and add a protected branch feature/*
Add the following 2 files in a branch and create MR targeting main branch
env.sample
AWS_TOKEN=AKIAZYONPI3G4JNCCWGQ
test.rb
job = params[:job]
eval(job)
Observation: only the Secret detection - protected branch is violated. Approval rules and policy bot comment is recorded accordingly.
feature/test-branch) in the repository and Change the MR target branch to itObservation: Both (Secret detection - protected branch and SAST scan - feature branch ) policy is violated. Approval rules and policy bot comment is updated accordingly
project -\> settings -\> Merge requests -\> Merge request approvals : Uncheck all boxesSecret detection - protected branch is violated since the policy has prevent_approval_by_author: true overwrite.main , now the MR is allowed to be approved by merge request author.Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Related to #583612
Adds a new PEP/SEP-scoped mergeability check (security_policy_pipeline_check) that ensures all pipelines for the latest commit must succeed before a merge request can be merged when security policies are enforced.
When Pipeline Execution Policies (PEP) or Scan Execution Policies (SEP) inject security scans into MR pipelines, projects with existing branch pipelines end up with duplicate pipelines. The existing mergeability check only evaluates the MR pipeline (which includes the security scans), completely ignoring the branch pipeline where functional tests may be failing. This allows broken code to be merged even when branch pipelines are red.
Introduces a new CheckSecurityPolicyPipelineStatusService mergeability check that:
security_orchestration_policies license, has PEP/SEP policy configurations (scoped to type_scan_execution_policy or type_pipeline_execution_policy to avoid activating for projects with only approval or vulnerability management policies), and at least one internal pipeline exists for the latest MR commitmerge_request_event source) and the latest branch pipeline (all other sources) for the HEAD SHAfor_sha_or_source_sha to match pipelines where the SHA is a merge commit but source_sha is the branch HEADallow_merge_on_skipped_pipeline), consistent with CheckCiStatusService
checking) if any evaluated pipeline is still activeskip_security_policy_pipeline_check
security_policy_pipeline_check (disabled by default)ee/app/services/merge_requests/mergeability/check_security_policy_pipeline_status_service.rb - core mergeability check logicwith_security_policy_jobs on Ci::Pipeline to efficiently query pipelines that have policy-injected buildsmergeable_state_checks and adds skip_security_policy_pipeline_check to auto-merge skip optionsSECURITY_POLICY_PIPELINE_CHECK enum value in DetailedMergeStatusEnum and MergeabilityCheckIdentifier
security_policy_pipeline_check (gitlab_com_derisk, disabled by default)Note: Frontend changes (merge check component, contextual pipeline note, CSS, and frontend constants) have been removed from this MR and will be handled separately.
Closes #589650
security_orchestration_policies license featuresecurity_policy_pipeline_check feature flagSECURITY_POLICY_PIPELINE_CHECK mergeability check identifier via the GraphQL APIEvaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
@bauerdominic good call, I mirrored this from PEP where we couldn't do it, as we need to differentiate between self-healed records and the old ones that need to use the old scheduling, and I moved the NOT NULL DEFAULT 0 into a cleanup issue.
However, for SEP project schedules, we can add that right away
CI catalog component usage statistics were not being tracked when components were included via pipeline execution policy pipelines. Since policy pipelines run as separate child pipelines, their included_components were invisible to the ComponentUsage chain step running on the parent pipeline.
This MR fixes the gap by:
included_components attribute to Security::PipelineExecutionPolicy::Pipeline to store which catalog components were used by each policy pipelineincluded_components from yaml_processor_result after each policy pipeline is created in PipelineContext#build_policy_pipelines!
policy_pipelines_included_components to aggregate and deduplicate components across all policy pipelinesComponentUsage when running inside a policy pipeline (creating_policy_pipeline?) to avoid double-counting, since each policy pipeline runs its own full CreatePipelineService chain — the project pipeline's ComponentUsage step (via EE override) then merges in all policy pipeline components and tracks them in a single call to TrackComponentUsageWorker
Note: This change was proposed by GitLab Duo based on the observed undercounting problem. While I understand the approach taken here, I'm aware there may be alternative solutions — reviewer input on the overall approach is very welcome.
The CI/CD Catalog currently shows only 6 projects using the component, while the AI hackathon participants group has 100+ projects that used that CI component via a pipeline execution policy — none of which are being counted in the usage statistics. Note that this fix only corrects the undercounting for future pipeline runs — past runs are unaffected.
N/A - backend only change
Ci::Catalog::Resources::TrackComponentUsageWorker is enqueued with both the project's own components and the policy pipeline's componentsee/spec/services/ci/create_pipeline_service/pipeline_execution_policy_spec.rb that exercises the full flow (policy enforces a CI file using a catalog component → TrackComponentUsageWorker is enqueued with the correct data)? I did not add one to keep the scope focused, but happy to if reviewers feel it is needed.Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
@furkanayhan looks great to me too, thanks @mmichaux-ext!
@alan I found it - it was in the MR !220685 (comment 3057874752) and this is the issue: #589398.
@alan I think that's a good approach
Thanks @alan for working on this and apologies for the delay! I left a few comments
suggestion: we should add a test for two competing policies to cover the fact that the highest severity wins.
suggestion: this doesn't appear to be used.
question: we match this as "override already applied", although the applied override was for medium severity and our finding is high. Is that an issue? Should we match the severity? I don't know if finding's severity can change over time.
suggestion(non-blocking): this is redundant, as it's the same as the content defined above