From 0634c80ebd94f63097eedba53f21441d2106d224 Mon Sep 17 00:00:00 2001 From: Sashi Kumar Kumaresan Date: Mon, 19 May 2025 09:42:36 +0200 Subject: [PATCH] Add security report time window to policy_tuning This MR adds security_report_time_window to policy_tuning section of MR approval policy. The time window value will be used to choose the target branch pipeline when the latest pipeline is not complete or does not produce security reports. EE: true Changelog: added --- .../merge_request_approval_policies.md | 22 +++- .../scan_result/edge_case_settings.vue | 10 +- .../advanced_settings/constants.js | 10 ++ .../advanced_settings/edge_cases_section.vue | 76 +++++++++++- .../groups/security/policies_controller.rb | 1 + .../projects/security/policies_controller.rb | 1 + ee/app/models/concerns/approval_rule_like.rb | 6 + ee/app/models/ee/merge_request.rb | 59 +++++++-- ee/app/models/security/policy.rb | 4 + .../scan_result_policies/related_pipelines.rb | 14 +-- .../update_approvals_service.rb | 37 +++--- .../validate_policy_service.rb | 9 ++ ...eable_policy_rules_notification_service.rb | 6 +- .../json_schemas/approval_policy_content.json | 6 + .../security_orchestration_policy.json | 6 + .../approval_policy_time_window.yml | 10 ++ .../scan_result/edge_case_settings_spec.js | 22 ++++ .../edge_cases_section_spec.js | 36 +++++- ee/spec/models/merge_request_spec.rb | 103 ++++++++++++++++ ...orchestration_policy_configuration_spec.rb | 87 +++++++++++++ .../related_pipelines_spec.rb | 10 +- .../update_approvals_service_spec.rb | 2 - .../validate_policy_service_spec.rb | 115 ++++++++++++++++++ locale/gitlab.pot | 18 +++ 24 files changed, 617 insertions(+), 53 deletions(-) create mode 100644 ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml diff --git a/doc/user/application_security/policies/merge_request_approval_policies.md b/doc/user/application_security/policies/merge_request_approval_policies.md index 2a6570a8805274..936c336f3732c0 100644 --- a/doc/user/application_security/policies/merge_request_approval_policies.md +++ b/doc/user/application_security/policies/merge_request_approval_policies.md @@ -430,6 +430,8 @@ On GitLab Self-Managed, by default the `fallback_behavior` field is available. T ## `policy_tuning` +### `unblock_rules_using_execution_policies` + {{< history >}} - [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/498624) support for use in pipeline execution policies in GitLab 17.10 [with a flag](../../../administration/feature_flags/_index.md) named `unblock_rules_using_pipeline_execution_policies`. Enabled by default. @@ -447,9 +449,9 @@ The availability of support for pipeline execution policies is controlled by a f |--------|----------|----------|--------------------|----------------------------------------------------------------------------------------------------------------------| | `unblock_rules_using_execution_policies` | `boolean` | false | `true`, `false` | When enabled, approval rules do not block merge requests when a scan is required by a scan execution policy or a pipeline execution policy but a required scan artifact is missing from the target branch. This option only works when the project or group has an existing scan execution policy or pipeline execution policy with matching scanners. | -### Examples +#### Examples -#### Example of `policy_tuning` with a scan execution policy +##### Example of `policy_tuning` with a scan execution policy You can use this example in a `.gitlab/security-policies/policy.yml` file stored in a [security policy project](enforcement/security_policy_projects.md): @@ -495,7 +497,7 @@ approval_policy: unblock_rules_using_execution_policies: true ``` -#### Example of `policy_tuning` with a pipeline execution policy +##### Example of `policy_tuning` with a pipeline execution policy {{< alert type="warning" >}} @@ -529,7 +531,7 @@ include: - template: Jobs/Dependency-Scanning.gitlab-ci.yml ``` -##### Recreate pipeline execution policies created before GitLab 17.10 +###### Recreate pipeline execution policies created before GitLab 17.10 Pipeline execution policies created before GitLab 17.10 do not contain the data required to use the `policy_tuning` feature. To use this feature with older pipeline execution policies, @@ -552,6 +554,18 @@ To recreate a pipeline execution policy: 1. In the **.YAML mode**, paste the contents of the old policy. 1. Select **Update via merge request** and merge the generated merge request. +### `security_report_time_window` + +{{< history >}} + +- [Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/525509) in GitLab 18.4 [with a flag](../../../administration/feature_flags/_index.md) named `approval_policy_time_window`. + +{{< /history >}} + +| Field | Type | Required | Possible values | Description | +|--------|----------|----------|--------------------|----------------------------------------------------------------------------------------------------------------------| +| `security_report_time_window` | `integer` | false | 1 to 10080 (7 days) | Specifies the time window in minutes for choosing the target pipeline for the security report comparison. | + ## Policy scope schema To customize policy enforcement, you can define a policy's scope to either include or exclude diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue b/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue index c33227fbbc99ea..fab9943040fc9f 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue @@ -3,6 +3,8 @@ import { s__ } from '~/locale'; import { UNBLOCK_RULES_KEY, UNBLOCK_RULES_TEXT, + TIME_WINDOW_KEY, + TIME_WINDOW_TEXT, } from '../../policy_editor/scan_result/advanced_settings/constants'; export default { @@ -10,6 +12,7 @@ export default { i18n: { title: s__('SecurityOrchestration|Edge case settings'), [UNBLOCK_RULES_KEY]: UNBLOCK_RULES_TEXT, + [TIME_WINDOW_KEY]: TIME_WINDOW_TEXT, }, props: { settings: { @@ -20,7 +23,12 @@ export default { }, computed: { settingsList() { - return Object.entries(this.settings).map(([key]) => this.$options.i18n[key] || key); + return Object.entries(this.settings).map(([key, value]) => { + if (key === TIME_WINDOW_KEY && typeof value === 'number') { + return `${this.$options.i18n[key]}: ${value} ${value === 1 ? s__('ScanResultPolicy|minute') : s__('ScanResultPolicy|minutes')}`; + } + return this.$options.i18n[key] || key; + }); }, }, }; diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js index 76033508433451..79f82525603e13 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants.js @@ -10,6 +10,16 @@ export const UNBLOCK_RULES_TEXT = s__( 'ScanResultPolicy|Make approval rules optional using execution policies', ); +export const TIME_WINDOW_KEY = 'security_report_time_window'; + +export const TIME_WINDOW_TEXT = s__( + 'ScanResultPolicy|Use time window for target pipeline comparison', +); + +export const TIME_WINDOW_POPOVER_DESC = s__( + 'ScanResultPolicy|This addresses issues in busy projects where the target branch always has a pipeline running. Instead of only using the latest pipeline (which might be pending), the policy will consider security reports from successful pipelines completed within the specified time window.', +); + export const NAME = 'name'; export const PATTERN = 'pattern'; export const SOURCE = 'source'; diff --git a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue index 7092830bb54bbe..5908b5a81b3ea4 100644 --- a/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue +++ b/ee/app/assets/javascripts/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue @@ -1,26 +1,53 @@ @@ -52,5 +87,40 @@ export default { {{ $options.i18n.popoverDesc }} + +
+ + {{ $options.i18n.TIME_WINDOW_TEXT }} + + + + + {{ $options.i18n.TIME_WINDOW_POPOVER_DESC }} + + +
+ +
+ + {{ $options.i18n.timeWindowUnit }} +
+ +
+
diff --git a/ee/app/controllers/groups/security/policies_controller.rb b/ee/app/controllers/groups/security/policies_controller.rb index a4bb6d798c1305..30a78cfc6486c4 100644 --- a/ee/app/controllers/groups/security/policies_controller.rb +++ b/ee/app/controllers/groups/security/policies_controller.rb @@ -18,6 +18,7 @@ class PoliciesController < Groups::ApplicationController push_frontend_feature_flag(:scheduled_pipeline_execution_policies, group) push_frontend_feature_flag(:flexible_scan_execution_policy, group) push_frontend_feature_flag(:security_policies_combined_list, group) + push_frontend_feature_flag(:approval_policy_time_window, group) end feature_category :security_policy_management diff --git a/ee/app/controllers/projects/security/policies_controller.rb b/ee/app/controllers/projects/security/policies_controller.rb index 24f4398e1d3700..d3ad117773a3a7 100644 --- a/ee/app/controllers/projects/security/policies_controller.rb +++ b/ee/app/controllers/projects/security/policies_controller.rb @@ -18,6 +18,7 @@ class PoliciesController < Projects::ApplicationController push_frontend_feature_flag(:security_policy_approval_warn_mode, project.group) push_frontend_feature_flag(:flexible_scan_execution_policy, project.group) push_frontend_feature_flag(:security_policies_combined_list, project) + push_frontend_feature_flag(:approval_policy_time_window, project) end feature_category :security_policy_management diff --git a/ee/app/models/concerns/approval_rule_like.rb b/ee/app/models/concerns/approval_rule_like.rb index b549fce927c8f4..4739dd5662d311 100644 --- a/ee/app/models/concerns/approval_rule_like.rb +++ b/ee/app/models/concerns/approval_rule_like.rb @@ -83,6 +83,12 @@ module ApprovalRuleLike scope :by_report_types, ->(report_types) { where(report_type: report_types) } end + def security_report_time_window + return unless approval_policy_rule + + approval_policy_rule.security_policy.security_report_time_window + end + def vulnerability_attribute_false_positive nil end diff --git a/ee/app/models/ee/merge_request.rb b/ee/app/models/ee/merge_request.rb index c4a2027f4e9ce8..80ab6956855b39 100644 --- a/ee/app/models/ee/merge_request.rb +++ b/ee/app/models/ee/merge_request.rb @@ -10,6 +10,7 @@ module MergeRequest include FromUnion MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON = 10 + MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON_TIME_WINDOW = 1000 prepended do include Elastic::ApplicationVersionedSearch @@ -576,6 +577,23 @@ def latest_scan_finding_comparison_pipeline find_target_branch_pipeline_by_sha_in_order_of_preference(target_shas, :has_security_reports?) end + def target_pipeline_for_approval_policy(approval_rule, report_type) + predicate = report_type == :scan_finding ? :has_security_reports? : :has_sbom_reports? + target_shas = [diff_head_pipeline&.target_sha, diff_base_sha, diff_start_sha] + pipelines = find_target_branch_pipelines_for(target_shas) + + comparison_pipeline = find_pipeline_with_report(pipelines, predicate) + time_window = approval_rule.security_report_time_window + + return comparison_pipeline if comparison_pipeline.present? + + if ::Feature.enabled?(:approval_policy_time_window, project) && time_window.present? + return find_pipeline_with_time_window(pipelines, time_window, report_type) + end + + comparison_pipeline || latest_pipeline_for_target_branch + end + def diff_head_pipeline?(pipeline) pipeline.source_sha == diff_head_sha || pipeline.sha == diff_head_sha end @@ -719,18 +737,43 @@ def security_comparison?(service_class) end def find_target_branch_pipeline_by_sha_in_order_of_preference(shas, predicate) - pipelines = shas - .compact - .lazy - .flat_map do |sha| - target_branch_pipelines_for(sha: sha) - .order(id: :desc) - .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON) - end + pipelines = find_target_branch_pipelines_for(shas) + find_pipeline_with_report(pipelines, predicate) + end + def find_pipeline_with_report(pipelines, predicate) pipelines.find { |pipeline| pipeline.self_and_project_descendants.any?(&predicate) } end + def find_target_branch_pipelines_for(shas) + shas.compact.lazy.flat_map do |sha| + target_branch_pipelines_for(sha: sha) + .order(id: :desc) + .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON) + end + end + + def find_pipeline_with_time_window(pipelines, time_window, report_type) + reports_scope = if report_type == :scan_finding + ::Ci::JobArtifact.security_reports + else + ::Ci::JobArtifact.of_report_type(:sbom) + end + + filtered_pipelines = project + .ci_pipelines + .with_reports(reports_scope) + .created_after(Time.current - (time_window * 60)) + .where(ref: target_branch) + + filtered_pipelines = filtered_pipelines.created_before_id(pipelines.first.id) if pipelines.present? + + filtered_pipelines + .limit(MAX_CHECKED_PIPELINES_FOR_SECURITY_REPORT_COMPARISON_TIME_WINDOW) + .order(id: :desc) + .take(1) + end + def has_approved_license_check? if rule = approval_rules.license_compliance.last ApprovalWrappedRule.wrap(self, rule).approved? diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb index 67406a33b63370..9185fe7cc2aef0 100644 --- a/ee/app/models/security/policy.rb +++ b/ee/app/models/security/policy.rb @@ -344,6 +344,10 @@ def supports_policy_rules? Security::PolicyRule::SUPPORTED_POLICY_TYPES.include?(type.to_sym) end + def security_report_time_window + policy_content.dig(:policy_tuning, :security_report_time_window) + end + private def content_by_type diff --git a/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb b/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb index d0c1948a43b05f..ef5fa7e8052343 100644 --- a/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb +++ b/ee/app/services/concerns/security/scan_result_policies/related_pipelines.rb @@ -23,8 +23,8 @@ def related_pipelines(pipeline) end end - def related_target_pipeline_ids_for_merge_request(merge_request, report_type) - target_pipeline = target_pipeline_for_merge_request(merge_request, report_type) + def related_target_pipeline_ids_for_merge_request(merge_request, report_type, approval_rule) + target_pipeline = target_pipeline_for_merge_request(merge_request, report_type, approval_rule) return [] unless target_pipeline Security::RelatedPipelinesFinder.new(target_pipeline, { @@ -37,14 +37,8 @@ def related_pipeline_sources Enums::Ci::Pipeline.ci_and_security_orchestration_sources.values end - def target_pipeline_for_merge_request(merge_request, report_type) - target_pipeline = if report_type == :scan_finding - merge_request.latest_scan_finding_comparison_pipeline - elsif report_type == :license_scanning - merge_request.latest_comparison_pipeline_with_sbom_reports - end - - target_pipeline || merge_request.latest_pipeline_for_target_branch + def target_pipeline_for_merge_request(merge_request, report_type, approval_rule) + merge_request.target_pipeline_for_approval_policy(approval_rule, report_type) end end end diff --git a/ee/app/services/security/scan_result_policies/update_approvals_service.rb b/ee/app/services/security/scan_result_policies/update_approvals_service.rb index ac338f8f20ecc1..1b846456294c5b 100644 --- a/ee/app/services/security/scan_result_policies/update_approvals_service.rb +++ b/ee/app/services/security/scan_result_policies/update_approvals_service.rb @@ -48,8 +48,12 @@ def related_pipeline_with_security_reports_exists? related_pipelines(pipeline).with_reports(::Ci::JobArtifact.security_reports).exists? end - def validation_context - { pipeline_ids: related_source_pipeline_ids, target_pipeline_ids: related_target_pipeline_ids } + def validation_context(approval_rule = nil) + if approval_rule + { pipeline_ids: related_source_pipeline_ids, target_pipeline_ids: related_target_pipeline_ids(approval_rule) } + else + { pipeline_ids: related_source_pipeline_ids } + end end delegate :project, to: :pipeline @@ -73,7 +77,7 @@ def evaluate_rules(approval_rules) evaluation.error!( merge_request_approval_rule, :scan_removed, - context: validation_context, missing_scans: missing_scans(approval_rule) + context: validation_context(approval_rule), missing_scans: missing_scans(approval_rule) ) next true end @@ -140,9 +144,9 @@ def violates_vulnerabilities_allowed?(approval_rule, pipeline_uuids) def missing_scans(approval_rule) scanners = approval_rule.scanners - return (scanners - pipeline_security_scan_types) unless target_pipeline + return (scanners - pipeline_security_scan_types) unless target_pipeline(approval_rule) - scan_types_diff = target_pipeline_security_scan_types - pipeline_security_scan_types + scan_types_diff = target_pipeline_security_scan_types(approval_rule) - pipeline_security_scan_types return scan_types_diff if scanners.empty? @@ -154,15 +158,9 @@ def pipeline_security_scan_types end strong_memoize_attr :pipeline_security_scan_types - def target_pipeline_security_scan_types - security_scan_types(related_target_pipeline_ids) - end - strong_memoize_attr :target_pipeline_security_scan_types - - def target_pipeline - target_pipeline_for_merge_request(merge_request, :scan_finding) + def target_pipeline_security_scan_types(approval_rule) + security_scan_types(related_target_pipeline_ids(approval_rule)) end - strong_memoize_attr :target_pipeline def fail_evaluation_with_data!(rule, newly_detected: nil, previously_existing: nil) evaluation.fail!( @@ -173,7 +171,7 @@ def fail_evaluation_with_data!(rule, newly_detected: nil, previously_existing: n previously_existing: Security::ScanResultPolicyViolation.trim_violations(previously_existing) }.compact_blank }, - context: validation_context + context: validation_context(rule) ) end @@ -185,10 +183,13 @@ def security_scan_types(pipeline_ids) Security::Scan.by_pipeline_ids(pipeline_ids).distinct_scan_types end - def related_target_pipeline_ids - related_target_pipeline_ids_for_merge_request(merge_request, :scan_finding) + def target_pipeline(approval_rule) + target_pipeline_for_merge_request(merge_request, :scan_finding, approval_rule) + end + + def related_target_pipeline_ids(approval_rule) + related_target_pipeline_ids_for_merge_request(merge_request, :scan_finding, approval_rule) end - strong_memoize_attr :related_target_pipeline_ids def related_source_pipeline_ids related_pipeline_ids(pipeline) @@ -196,7 +197,7 @@ def related_source_pipeline_ids strong_memoize_attr :related_source_pipeline_ids def target_pipeline_findings_uuids(approval_rule) - findings_uuids(target_pipeline, approval_rule, related_target_pipeline_ids) + findings_uuids(target_pipeline(approval_rule), approval_rule, related_target_pipeline_ids(approval_rule)) end def pipeline_findings_uuids(approval_rule) diff --git a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb index aaed727b2471df..fa2fb7ee1c80e4 100644 --- a/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb +++ b/ee/app/services/security/security_orchestration_policies/validate_policy_service.rb @@ -31,6 +31,7 @@ def execute return error_with_title(s_('SecurityOrchestration|Timezone is invalid'), field: :timezone) if invalid_timezone? return error_with_title(s_('SecurityOrchestration|Vulnerability age requires previously existing vulnerability states (detected, confirmed, resolved, or dismissed)'), field: :vulnerability_age) if invalid_vulnerability_age? return error_with_title(s_('SecurityOrchestration|Invalid Compliance Framework ID(s)'), field: :compliance_frameworks) if invalid_compliance_framework_ids? + return error_with_title(s_('SecurityOrchestration|Invalid security report time window'), field: :security_report_time_window) if invalid_security_report_time_window? if required_approvals_exceed_eligible_approvers? return errors_with_title(s_('SecurityOrchestration|Required approvals exceed eligible approvers.'), title: s_('SecurityOrchestration|Logic error'), field: :actions, indices: multiple_approvals_failed_action_indices) @@ -293,6 +294,14 @@ def invalid_vulnerability_age? end end + def invalid_security_report_time_window? + return false unless approval_policy? + return false if ::Feature.disabled?(:approval_policy_time_window, container) + + security_report_time_window = policy.dig(:policy_tuning, :security_report_time_window) + security_report_time_window.present? && (security_report_time_window < 1 || security_report_time_window > 10080) + end + def policy_type policy[:type].to_sym end diff --git a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb index 36d23aba587ba1..ff68286e08611b 100644 --- a/ee/app/services/security/unenforceable_policy_rules_notification_service.rb +++ b/ee/app/services/security/unenforceable_policy_rules_notification_service.rb @@ -44,7 +44,7 @@ def update_for_report_type(merge_request, report_type, approval_rules) policy_evaluation = Security::SecurityOrchestrationPolicies::PolicyRuleEvaluationService.new(merge_request) applicable_rules.each do |rule| - policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type)) + policy_evaluation.error!(rule, pipeline_error, context: validation_context(report_type, rule)) end policy_evaluation.save @@ -74,11 +74,11 @@ def pipeline_error pipeline&.failed? ? :pipeline_failed : :artifacts_missing end - def validation_context(report_type) + def validation_context(report_type, rule) return if pipeline.nil? { pipeline_ids: related_pipeline_ids(pipeline), - target_pipeline_ids: related_target_pipeline_ids_for_merge_request(merge_request, report_type) } + target_pipeline_ids: related_target_pipeline_ids_for_merge_request(merge_request, report_type, rule) } end def log_message(report_type, message, **attributes) diff --git a/ee/app/validators/json_schemas/approval_policy_content.json b/ee/app/validators/json_schemas/approval_policy_content.json index 04253cbae39a68..490648fedf0f32 100644 --- a/ee/app/validators/json_schemas/approval_policy_content.json +++ b/ee/app/validators/json_schemas/approval_policy_content.json @@ -30,6 +30,12 @@ "unblock_rules_using_execution_policies": { "type": "boolean", "description": "Experimental: Unblock policy approval rules with missing scans of scanners which are enforced by Scan execution policies. If scan does not produce artifacts because it is not applicable to the given project, approval rules are automatically unblocked and do not require approvals." + }, + "security_report_time_window": { + "type": "integer", + "minimum": 1, + "maximum": 10080, + "description": "Specifies the time window in minutes for choosing the target pipeline for the security report comparison." } } }, diff --git a/ee/app/validators/json_schemas/security_orchestration_policy.json b/ee/app/validators/json_schemas/security_orchestration_policy.json index 3d4798c61d481f..e81c01332ae8c9 100644 --- a/ee/app/validators/json_schemas/security_orchestration_policy.json +++ b/ee/app/validators/json_schemas/security_orchestration_policy.json @@ -1097,6 +1097,12 @@ "unblock_rules_using_execution_policies": { "type": "boolean", "description": "Experimental: Unblock policy approval rules with missing scans of scanners which are enforced by Scan execution policies. If scan does not produce artifacts because it is not applicable to the given project, approval rules are automatically unblocked and do not require approvals." + }, + "security_report_time_window": { + "type": "integer", + "minimum": 1, + "maximum": 10080, + "description": "Specifies the time window in minutes for choosing the target pipeline for the security report comparison." } } }, diff --git a/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml b/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml new file mode 100644 index 00000000000000..1b5b5126bd7720 --- /dev/null +++ b/ee/config/feature_flags/gitlab_com_derisk/approval_policy_time_window.yml @@ -0,0 +1,10 @@ +--- +name: approval_policy_time_window +description: This feature flag will enable MR approval policies to select pipelines with security reports from a time window. +feature_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/525509 +introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/191739 +rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/543027 +milestone: '18.4' +group: group::security policies +type: gitlab_com_derisk +default_enabled: false diff --git a/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js b/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js index f4a8dcf3faf744..f58d51256ca4df 100644 --- a/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js +++ b/ee/spec/frontend/security_orchestration/components/policy_drawer/scan_result/edge_case_settings_spec.js @@ -2,6 +2,8 @@ import { shallowMount } from '@vue/test-utils'; import { UNBLOCK_RULES_KEY, UNBLOCK_RULES_TEXT, + TIME_WINDOW_KEY, + TIME_WINDOW_TEXT, } from 'ee/security_orchestration/components/policy_editor/scan_result/advanced_settings/constants'; import EdgeCaseSettings from 'ee/security_orchestration/components/policy_drawer/scan_result/edge_case_settings.vue'; @@ -28,4 +30,24 @@ describe('EdgeCaseSettings', () => { expect(paragraphs.at(0).text()).toBe(UNBLOCK_RULES_TEXT); expect(paragraphs.at(1).text()).toBe('customKey'); }); + + describe('time window formatting', () => { + it('formats time window in minutes', () => { + createComponent({ [TIME_WINDOW_KEY]: 60 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 60 minutes`); + }); + + it('handles plural forms correctly', () => { + createComponent({ [TIME_WINDOW_KEY]: 120 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 120 minutes`); + }); + + it('handles single unit forms correctly', () => { + createComponent({ [TIME_WINDOW_KEY]: 1 }); + const paragraphs = wrapper.findAll('p'); + expect(paragraphs.at(0).text()).toContain(`${TIME_WINDOW_TEXT}: 1 minute`); + }); + }); }); diff --git a/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js b/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js index c376063f857d16..1e5251f86be531 100644 --- a/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js +++ b/ee/spec/frontend/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section_spec.js @@ -1,19 +1,25 @@ import { shallowMount } from '@vue/test-utils'; -import { GlFormCheckbox, GlPopover } from '@gitlab/ui'; +import { GlFormCheckbox, GlPopover, GlFormInput, GlFormGroup } from '@gitlab/ui'; import EdgeCasesSection from 'ee/security_orchestration/components/policy_editor/scan_result/advanced_settings/edge_cases_section.vue'; describe('EdgeCasesSection', () => { let wrapper; - const createComponent = (propsData = {}) => { + const createComponent = (propsData = {}, featureFlags = {}) => { wrapper = shallowMount(EdgeCasesSection, { propsData: { policyTuning: { unblock_rules_using_execution_policies: false }, ...propsData }, + mocks: { + glFeatures: { approvalPolicyTimeWindow: false, ...featureFlags }, + }, }); }; const findCheckbox = () => wrapper.findComponent(GlFormCheckbox); const findPopover = () => wrapper.findComponent(GlPopover); + const findTimeWindowInput = () => wrapper.findComponent(GlFormInput); + const findTimeWindowGroup = () => wrapper.findComponent(GlFormGroup); + describe('default state when edge cases setting is not selected', () => { beforeEach(() => { createComponent(); @@ -44,4 +50,30 @@ describe('EdgeCasesSection', () => { expect(findCheckbox().attributes('checked')).toBe('true'); }); }); + + describe('time window feature', () => { + it('renders time window input when time window is set', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + expect(findTimeWindowInput().exists()).toBe(true); + expect(findTimeWindowGroup().exists()).toBe(true); + expect(findTimeWindowInput().attributes('value')).toBe('60'); + }); + + it('emits when time window input value changes', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + findTimeWindowInput().vm.$emit('input', '120'); + expect(wrapper.emitted('changed')).toEqual([ + ['policy_tuning', { security_report_time_window: 120 }], + ]); + }); + + it('validates time window input range', () => { + createComponent({ policyTuning: { security_report_time_window: 60 } }); + findTimeWindowInput().vm.$emit('input', '0'); + expect(wrapper.emitted('changed')).toBeUndefined(); + + findTimeWindowInput().vm.$emit('input', '10081'); + expect(wrapper.emitted('changed')).toBeUndefined(); + }); + }); }); diff --git a/ee/spec/models/merge_request_spec.rb b/ee/spec/models/merge_request_spec.rb index d3d9ee7cc5972b..05d09da96ab851 100644 --- a/ee/spec/models/merge_request_spec.rb +++ b/ee/spec/models/merge_request_spec.rb @@ -3611,4 +3611,107 @@ def stub_foss_conditions_met it_behaves_like 'not editable by user' end end + + describe '#target_pipeline_for_approval_policy' do + let_it_be(:project) { create(:project, :repository) } + let_it_be(:target_branch) { 'main' } + let_it_be(:merge_request) { create(:merge_request, source_project: project, target_project: project, target_branch: target_branch) } + let_it_be(:merge_request_diff) { create(:merge_request_diff, merge_request: merge_request) } + + let_it_be(:old_pipeline) do + create(:ee_ci_pipeline, + project: project, + ref: target_branch, + created_at: 5.days.ago + ) + end + + let_it_be(:license_scanning_pipeline) do + create(:ee_ci_pipeline, :with_cyclonedx_report, + project: project, + ref: target_branch, + created_at: 2.days.ago + ) + end + + let_it_be(:scan_finding_pipeline) do + create(:ee_ci_pipeline, :with_sast_report, + project: project, + ref: target_branch, + created_at: 1.day.ago + ) + end + + let_it_be(:latest_pipeline) do + create(:ee_ci_pipeline, project: project, ref: target_branch, sha: merge_request.diff_base_sha) + end + + let_it_be(:security_policy) { create(:security_policy, :approval_policy) } + let_it_be(:approval_policy_rule) { create(:approval_policy_rule, security_policy: security_policy) } + let_it_be(:scan_finding_rule) { create(:report_approver_rule, :scan_finding, merge_request: merge_request, approval_policy_rule: approval_policy_rule) } + let_it_be(:license_scanning_rule) { create(:report_approver_rule, :license_scanning, merge_request: merge_request, approval_policy_rule: approval_policy_rule) } + + before do + security_policy.clear_memoization(:policy_content) + end + + shared_examples 'returns correct pipeline for report type' do |report_type, rule| + context "when feature flag is disabled" do + before do + stub_feature_flags(approval_policy_time_window: false) + end + + context 'when there is no pipeline with the correct report type' do + it 'returns the latest pipeline' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(latest_pipeline) + end + end + + context 'when there is a pipeline with the correct report type' do + let_it_be(:pipeline_with_report) do + create(:ee_ci_pipeline, :with_cyclonedx_report, project: project, ref: target_branch, sha: merge_request.diff_base_sha) + end + + it 'returns the pipeline with the correct report type' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(pipeline_with_report) + end + end + end + + context "when time window is set" do + before do + security_policy.update!(content: { policy_tuning: { security_report_time_window: 3 * 24 * 60 } }) + end + + it "returns the first pipeline within the time window with the correct report type" do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(report_type == :scan_finding ? scan_finding_pipeline : license_scanning_pipeline) + end + + context 'when there is a pipeline with the correct report type' do + let_it_be(:pipeline_with_report) do + create(:ee_ci_pipeline, :with_cyclonedx_report, project: project, ref: target_branch, sha: merge_request.diff_base_sha, created_at: 1.hour.ago) + end + + before do + security_policy.update!(content: { policy_tuning: { security_report_time_window: 2 * 60 } }) + end + + it 'returns the pipeline with the correct report type' do + pipeline = merge_request.target_pipeline_for_approval_policy(send(rule), report_type) + + expect(pipeline).to eq(pipeline_with_report) + end + end + end + end + + include_examples 'returns correct pipeline for report type', :scan_finding, :scan_finding_rule + include_examples 'returns correct pipeline for report type', :license_scanning, :license_scanning_rule + end end diff --git a/ee/spec/models/security/orchestration_policy_configuration_spec.rb b/ee/spec/models/security/orchestration_policy_configuration_spec.rb index 61f97eec160d11..e687622cdce1b2 100644 --- a/ee/spec/models/security/orchestration_policy_configuration_spec.rb +++ b/ee/spec/models/security/orchestration_policy_configuration_spec.rb @@ -2523,6 +2523,93 @@ specify { expect(errors).to be_empty } end end + + describe "policy_tuning" do + let(:rule) do + { + type: "scan_finding", + branches: %w[master], + scanners: %w[container_scanning], + vulnerabilities_allowed: 0, + severity_levels: %w[critical], + vulnerability_states: %w[detected] + } + end + + let(:action) do + { + type: "require_approval", + approvals_required: 1, + user_approvers_ids: [42] + } + end + + let(:approval_policy) do + build(:approval_policy, rules: rules, actions: actions, policy_tuning: policy_tuning) + end + + describe "security_report_time_window" do + context 'with valid value' do + let(:policy_tuning) { { security_report_time_window: 60 } } + + specify { expect(errors).to be_empty } + end + + context 'with minimum valid value' do + let(:policy_tuning) { { security_report_time_window: 1 } } + + specify { expect(errors).to be_empty } + end + + context 'with maximum valid value' do + let(:policy_tuning) { { security_report_time_window: 10080 } } + + specify { expect(errors).to be_empty } + end + + context 'when value is below minimum' do + let(:policy_tuning) { { security_report_time_window: 0 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=minimum") + end + end + + context 'when value is above maximum' do + let(:policy_tuning) { { security_report_time_window: 10081 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=maximum") + end + end + + context 'when value is not an integer' do + let(:policy_tuning) { { security_report_time_window: "60" } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is not of type: integer") + end + end + + context 'when value is negative' do + let(:policy_tuning) { { security_report_time_window: -1 } } + + specify do + expect(errors).to contain_exactly( + "property '/approval_policy/0/policy_tuning/security_report_time_window' is invalid: error_type=minimum") + end + end + + context 'with empty policy_tuning object' do + let(:policy_tuning) { {} } + + specify { expect(errors).to be_empty } + end + end + end end shared_examples_for "pipeline_execution_policy_content" do |policy_type| diff --git a/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb b/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb index 4ce73a338ba35b..e485b5891a146d 100644 --- a/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb +++ b/ee/spec/services/concerns/security/scan_result_policies/related_pipelines_spec.rb @@ -9,6 +9,10 @@ create(:merge_request, :with_merge_request_pipeline, source_project: project) end + let_it_be(:approval_rule) do + create(:approval_project_rule, :scan_finding) + end + let(:class_instance) { subject_class.new } let(:subject_class) do @@ -28,7 +32,9 @@ end describe '#target_pipeline_for_merge_request' do - subject(:target_pipeline) { subject_class.new.target_pipeline_for_merge_request(merge_request, report_type) } + subject(:target_pipeline) do + subject_class.new.target_pipeline_for_merge_request(merge_request, report_type, approval_rule) + end before do merge_request.update_head_pipeline @@ -108,7 +114,7 @@ let(:report_type) { :scan_finding } subject(:related_target_pipeline_ids) do - subject_class.new.related_target_pipeline_ids_for_merge_request(merge_request, report_type) + subject_class.new.related_target_pipeline_ids_for_merge_request(merge_request, report_type, approval_rule) end context 'when there is no pipeline on target branch' do diff --git a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb index e95fb1f0eca615..116db847b74283 100644 --- a/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb +++ b/ee/spec/services/security/scan_result_policies/update_approvals_service_spec.rb @@ -193,7 +193,6 @@ merge_request_iid: merge_request.iid, message: 'Evaluating scan_finding rules from approval policies', pipeline_ids: [pipeline.id], - target_pipeline_ids: [target_pipeline.id], project_path: project.full_path ).and_call_original @@ -632,7 +631,6 @@ merge_request_iid: merge_request.iid, message: 'Evaluating scan_finding rules from approval policies', pipeline_ids: [pipeline.id], - target_pipeline_ids: [target_pipeline.id], project_path: project.full_path ).and_call_original diff --git a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb index 03e253975e6c03..10ea80e9e0d04c 100644 --- a/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb +++ b/ee/spec/services/security/security_orchestration_policies/validate_policy_service_spec.rb @@ -676,6 +676,117 @@ end end + shared_examples 'checks if security_report_time_window is valid' do + context "when policy_type is approval_policy" do + let(:policy_type) { 'approval_policy' } + + before do + stub_feature_flags(approval_policy_time_window: true) + end + + context 'when security_report_time_window is not provided' do + it { expect(result[:status]).to eq(:success) } + end + + context 'when security_report_time_window is provided' do + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: security_report_time_window + } + } + end + + context 'when security_report_time_window is valid' do + where(:security_report_time_window, :status) do + 1 | :success + 60 | :success + 1440 | :success + 10080 | :success + end + + with_them do + it { expect(result[:status]).to eq(status) } + end + end + + context 'when security_report_time_window is invalid' do + where(:security_report_time_window, :status) do + 0 | :error + -1 | :error + 10081 | :error + 20000 | :error + end + + with_them do + it { expect(result[:status]).to eq(status) } + it { expect(result[:message]).to eq('Invalid policy') } + it { expect(result[:details]).to match_array(['Invalid security report time window']) } + + it_behaves_like 'sets validation errors', field: :security_report_time_window, message: 'Invalid security report time window' + end + end + + context 'when security_report_time_window is nil' do + let(:security_report_time_window) { nil } + + it { expect(result[:status]).to eq(:success) } + end + + context 'when security_report_time_window is empty string' do + let(:security_report_time_window) { '' } + + it { expect(result[:status]).to eq(:success) } + end + end + + context 'when feature flag is disabled' do + before do + stub_feature_flags(approval_policy_time_window: false) + end + + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: 0 + } + } + end + + it { expect(result[:status]).to eq(:success) } + end + end + + context "when policy_type is not approval_policy" do + let(:policy_type) { 'scan_execution_policy' } + let(:policy) do + { + type: policy_type, + name: name, + enabled: enabled, + rules: rules, + policy_tuning: { + security_report_time_window: 0 + } + } + end + + before do + stub_feature_flags(approval_policy_time_window: true) + end + + it { expect(result[:status]).to eq(:success) } + end + end + shared_examples 'checks scan execution policy action limit' do let(:action) { { scan: 'container_scanning' } } let(:action_limit) { 2 } @@ -879,6 +990,7 @@ it_behaves_like 'checks if branches are provided in rule' it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' @@ -938,6 +1050,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' it_behaves_like 'checks pipeline execution schedule policy schedule limit' @@ -973,6 +1086,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks if branches exist for the provided branch_type' do where(:policy_type, :branch_type, :status, :expected_error_message) do :scan_execution_policy | 'all' | :success | nil @@ -1039,6 +1153,7 @@ def setup_repository(project, branches) it_behaves_like 'checks if timezone is valid' it_behaves_like 'checks if cadence is valid' it_behaves_like 'checks if vulnerability_age is valid' + it_behaves_like 'checks if security_report_time_window is valid' it_behaves_like 'checks scan execution policy action limit' it_behaves_like 'checks scan execution policy schedule limit' it_behaves_like 'checks pipeline execution schedule policy schedule limit' diff --git a/locale/gitlab.pot b/locale/gitlab.pot index acebe4296602a4..f59559d9cb299e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -56781,9 +56781,18 @@ msgstr "" msgid "ScanResultPolicy|There are no access tokens created" msgstr "" +msgid "ScanResultPolicy|This addresses issues in busy projects where the target branch always has a pipeline running. Instead of only using the latest pipeline (which might be pending), the policy will consider security reports from successful pipelines completed within the specified time window." +msgstr "" + msgid "ScanResultPolicy|Unknown" msgstr "" +msgid "ScanResultPolicy|Use security reports from pipelines completed within the last" +msgstr "" + +msgid "ScanResultPolicy|Use time window for target pipeline comparison" +msgstr "" + msgid "ScanResultPolicy|Users" msgstr "" @@ -56865,6 +56874,12 @@ msgstr "" msgid "ScanResultPolicy|matching type" msgstr "" +msgid "ScanResultPolicy|minute" +msgstr "" + +msgid "ScanResultPolicy|minutes" +msgstr "" + msgid "ScanResultPolicy|scanners" msgstr "" @@ -58699,6 +58714,9 @@ msgstr "" msgid "SecurityOrchestration|Invalid policy type" msgstr "" +msgid "SecurityOrchestration|Invalid security report time window" +msgstr "" + msgid "SecurityOrchestration|Invalid syntax" msgstr "" -- GitLab