Enforce variable precedence from scheduled PEP

Why are we doing this work

Currently, the variable precedence is not handled in the same way for scheduled pipeline execution policies as for the regular PEP.

The reason is that the job option execution_policy_job is not set when creating a scheduled PEP.

If the variable precedence is not handled for scheduled PEPs, the behavior of the pipeline can be altered by setting project CI/CD variables and can lead to bypass of the enforced compliance checks.

Furthermore, with Variable precedence controls in pipeline execut... (&16430 - closed) / Add options to control variables override behav... (!186120 - merged) we're introducing a new option variables_override that's not being handled by scheduled PEPs.

We should have feature parity for the variable precedence handling between scheduled and regular pipeline execution policies.

Relevant links

Non-functional requirements

  • Documentation:
  • Feature flag:
  • Performance:
  • Testing:

Implementation plan

Without variables_override option

  • Set execution_policy_job option when creating a scheduled PEP (easy part) to ensure the highest precedence

With variables_override option

Introduced by !186120 (merged).

To support variables_override option, we are currently missing some abstractions. We are not able to pass the currently built scheduled pipeline execution policy into the pipeline_policy_context to enforce the variable behavior based on the variables_override configuration.

For regular PEP, we have current_policy to read the policy configuration. However, we cannot simply pass the scheduled PEP as current_policy because it is used as a flag to treat it as a "virtual" pipeline, i.e. it's not going to be persisted (e.g. StopDryRun uses it to ensure we don't persist the policy pipeline). It would cause other unexpected issues. This is the part where we're missing the abstractions.


We should consider prioritizing the following issues:

Here's a (very) rough patch that would fix the precedence, but refactoring is needed to address the missing abstractions (applied on top of the branch from Add options to control variables override behav... (!186120 - merged)):

diff --git a/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb
index 4eda9649c5e8..baaef706b04c 100644
--- a/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb
+++ b/ee/app/workers/security/pipeline_execution_policies/run_schedule_worker.rb
@@ -34,7 +34,27 @@ def execute(schedule)
           schedule.project,
           schedule.project.security_policy_bot,
           ref: schedule.project.default_branch_or_main
-        ).execute(PIPELINE_SOURCE, content: ci_content, ignore_skip_ci: true)
+        ).execute(PIPELINE_SOURCE,
+          content: ci_content,
+          ignore_skip_ci: true,
+          pipeline_policy_context: policy_context(schedule))
+      end
+
+      def policy_context(schedule)
+        # TODO: Create a factory for the context so that it can be created with a specific inner PipelineExecutionPolicy
+        # context where we can pass the scheduled policy.
+        Gitlab::Ci::Pipeline::ExecutionPolicies::PipelineContext.new(
+          project: schedule.project,
+          source: PIPELINE_SOURCE,
+          current_user: schedule.project.security_policy_bot,
+          # TODO: Extract a base class and use a different class for scheduled config
+          current_scheduled_policy: Security::PipelineExecutionPolicy::Config.new(
+            policy: schedule.security_policy.content.symbolize_keys
+                            .merge(name: schedule.security_policy.name, pipeline_config_strategy: 'schedule'),
+            policy_project_id: schedule.security_policy.security_policy_management_project_id,
+            policy_index: schedule.security_policy.policy_index
+          )
+        )
       end
 
       def log_pipeline_creation_failure(result, schedule)
diff --git a/ee/lib/gitlab/ci/pipeline/execution_policies/pipeline_context.rb b/ee/lib/gitlab/ci/pipeline/execution_policies/pipeline_context.rb
index ca86184fd5df..3cf60bd01c61 100644
--- a/ee/lib/gitlab/ci/pipeline/execution_policies/pipeline_context.rb
+++ b/ee/lib/gitlab/ci/pipeline/execution_policies/pipeline_context.rb
@@ -8,9 +8,12 @@ module ExecutionPolicies
         class PipelineContext
           include ::Gitlab::Utils::StrongMemoize
 
-          def initialize(project:, command: nil)
+          def initialize(project:, command: nil, source: nil, current_user: nil, current_scheduled_policy: nil)
             @project = project
+            @source = source
+            @current_user = current_user
             @command = command # TODO: decouple from this (https://gitlab.com/gitlab-org/gitlab/-/issues/503788)
+            @current_scheduled_policy = current_scheduled_policy # For passing the scheduled PEP policy
           end
 
           delegate :policy_pipelines, :override_policy_stages, :build_policy_pipelines!,
@@ -22,7 +25,8 @@ def initialize(project:, command: nil)
 
           def pipeline_execution_context
             ::Gitlab::Ci::Pipeline::PipelineExecutionPolicies::PipelineContext
-              .new(context: self, project: project, command: command)
+              .new(context: self, project: project, command: command, source: source, current_user: current_user,
+                current_scheduled_policy: current_scheduled_policy)
           end
           strong_memoize_attr :pipeline_execution_context
 
@@ -42,7 +46,7 @@ def skip_ci_allowed?(ref:)
 
           private
 
-          attr_reader :project, :command
+          attr_reader :project, :command, :current_scheduled_policy, :source, :current_user
         end
       end
     end
diff --git a/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb b/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb
index 287ec267be4b..92569c73b1d4 100644
--- a/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb
+++ b/ee/lib/gitlab/ci/pipeline/pipeline_execution_policies/pipeline_context.rb
@@ -12,13 +12,19 @@ class PipelineContext
 
           attr_reader :policy_pipelines, :override_policy_stages, :injected_policy_stages
 
-          def initialize(context:, project:, command: nil)
+          # TODO: We probably don't want to pass current_scheduled_policy explicitly, but use `current_policy`.
+          # However, `creating_policy_pipeline?` is used in many places and it's assumed that we're creating a "virtual" pipeline that's not persisted.
+          # We're missing some abstractions (e.g. `dry_run?`) to be able to keep the scheduled PEP in `current_policy` for the "real" pipeline.
+          def initialize(context:, project:, command: nil, source: nil, current_user: nil, current_scheduled_policy: nil)
             @context = context
             @project = project
             @command = command # TODO: decouple from this (https://gitlab.com/gitlab-org/gitlab/-/issues/503788)
             @policy_pipelines = []
             @override_policy_stages = []
             @injected_policy_stages = []
+            @source = source || command&.source
+            @current_user = current_user || command&.current_user
+            @current_scheduled_policy = current_scheduled_policy
           end
 
           def build_policy_pipelines!(partition_id)
@@ -59,13 +65,13 @@ def has_execution_policy_pipelines?
           def scheduled_execution_policy_pipeline?
             return false if Feature.disabled?(:scheduled_pipeline_execution_policies, project)
 
-            command&.source == ::Security::PipelineExecutionPolicies::RunScheduleWorker::PIPELINE_SOURCE
+            source == ::Security::PipelineExecutionPolicies::RunScheduleWorker::PIPELINE_SOURCE
           end
 
           def skip_ci_allowed?
             return true unless has_execution_policy_pipelines?
 
-            policy_pipelines.all? { |policy_pipeline| policy_pipeline.skip_ci_allowed?(command.current_user&.id) }
+            policy_pipelines.all? { |policy_pipeline| policy_pipeline.skip_ci_allowed?(current_user&.id) }
           end
 
           def has_overriding_execution_policy_pipelines?
@@ -114,22 +120,24 @@ def valid_stage?(stage)
           end
 
           def job_options
-            return {} unless creating_policy_pipeline?
+            return {} unless creating_policy_pipeline? || scheduled_execution_policy_pipeline?
 
+            # Setting this option is what enforces the variable precedence.
+            policy = current_policy || current_scheduled_policy
             { execution_policy_job: true }.tap do |options|
               if Feature.enabled?(:security_policies_optional_variables_control, project)
-                options[:execution_policy_variables_override] = current_policy.variables_override_strategy
+                options[:execution_policy_variables_override] = policy.variables_override_strategy
               end
             end
           end
 
           private
 
-          attr_reader :project, :command, :current_policy
+          attr_reader :project, :command, :current_policy, :current_scheduled_policy, :source, :current_user
 
           def policies
-            return [] if command&.source.blank?
-            return [] if Enums::Ci::Pipeline.dangling_sources.key?(command.source&.to_sym)
+            return [] if source.blank?
+            return [] if Enums::Ci::Pipeline.dangling_sources.key?(source.to_sym)
 
             ::Gitlab::Security::Orchestration::ProjectPipelineExecutionPolicies.new(project).configs
           end

Verification steps

Edited by 🤖 GitLab Bot 🤖