<!-- Implementation issues are used break-up a large piece of work into small, discrete tasks that can move independently through the build workflow steps. They're typically used to populate a Feature Epic. Once created, an implementation issue is usually refined in order to populate and review the implementation plan and weight. Example workflow: https://about.gitlab.com/handbook/engineering/development/threat-management/planning/diagram.html#plan --> ## 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 https://gitlab.com/groups/gitlab-org/-/epics/16430+ / https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186120+ 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 <!-- Information that the developer might need to refer to when implementing the issue. - [Design Issue](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>) - [Design 1](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>/designs/<image>.png) - [Design 2](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>/designs/<image>.png) - [Similar implementation](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/<id>) --> ## Non-functional requirements <!-- Add details for required items and delete others. --> - [ ] 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 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186120. 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](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/ee/gitlab/ci/pipeline/chain/command.rb#L13) 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: - https://gitlab.com/gitlab-org/gitlab/-/issues/503788+ - https://gitlab.com/gitlab-org/gitlab/-/issues/517294+ 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 https://gitlab.com/gitlab-org/gitlab/-/merge_requests/186120+): ```diff 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 ``` <!-- Workflow and other relevant labels # ~"group::" ~"Category:" ~"GitLab Ultimate" Other settings you might want to include when creating the issue. # /assign @ # /epic & --> ## Verification steps <!-- Add verification steps to help GitLab team members test the implementation. This is particularly useful during the MR review and the ~"workflow::verification" step. You may not know exactly what the verification steps should be during issue refinement, so you can always come back later to add them. 1. Check-out the corresponding branch 1. ... 1. Profit! -->