From 804d1a55fcf9b3c2851de3ff1d3de1d6e92b59a0 Mon Sep 17 00:00:00 2001
From: Brian Williams <bwilliams@gitlab.com>
Date: Wed, 20 Nov 2024 10:25:14 -0600
Subject: [PATCH] Auto-resolve vulnerabilities based on vulnerability
management policy
Automatically resolve vulnerabilities if a vulnerability management
policy is configured.
---
.../vulnerability_management_policy.rb | 10 +++
ee/app/models/security/policy.rb | 1 +
.../vulnerability_management_policy_rule.rb | 20 +++++
.../ingestion/mark_as_resolved_service.rb | 19 ++++-
.../vulnerabilities/auto_resolve_service.rb | 74 +++++++++++--------
.../beta/auto_resolve_vulnerabilities.yml | 9 +++
ee/spec/factories/security/policies.rb | 10 +++
...lnerability_management_policy_rule_spec.rb | 54 ++++++++++++++
.../mark_as_resolved_service_spec.rb | 42 ++++++++---
.../auto_resolve_service_spec.rb | 21 +++++-
10 files changed, 215 insertions(+), 45 deletions(-)
create mode 100644 ee/config/feature_flags/beta/auto_resolve_vulnerabilities.yml
diff --git a/ee/app/models/concerns/security/vulnerability_management_policy.rb b/ee/app/models/concerns/security/vulnerability_management_policy.rb
index 0e23907a03be9..8e9d31ad25965 100644
--- a/ee/app/models/concerns/security/vulnerability_management_policy.rb
+++ b/ee/app/models/concerns/security/vulnerability_management_policy.rb
@@ -11,5 +11,15 @@ def active_vulnerability_management_policies
def vulnerability_management_policy
policy_by_type(:vulnerability_management_policy)
end
+
+ def match?(vulnerability)
+ no_longer_detected_rules.any? { |rule| rule.match?(vulnerability) }
+ end
+
+ def no_longer_detected_rules
+ vulnerability_management_policy_rules
+ .undeleted
+ .no_longer_detected
+ end
end
end
diff --git a/ee/app/models/security/policy.rb b/ee/app/models/security/policy.rb
index c1943dc8dcc01..a8e251c38b6d1 100644
--- a/ee/app/models/security/policy.rb
+++ b/ee/app/models/security/policy.rb
@@ -3,6 +3,7 @@
module Security
class Policy < ApplicationRecord
include EachBatch
+ include Security::VulnerabilityManagementPolicy
self.table_name = 'security_policies'
self.inheritance_column = :_type_disabled
diff --git a/ee/app/models/security/vulnerability_management_policy_rule.rb b/ee/app/models/security/vulnerability_management_policy_rule.rb
index 71d45b1cce2e7..74ef4f2a1e5e9 100644
--- a/ee/app/models/security/vulnerability_management_policy_rule.rb
+++ b/ee/app/models/security/vulnerability_management_policy_rule.rb
@@ -11,5 +11,25 @@ class VulnerabilityManagementPolicyRule < ApplicationRecord
belongs_to :security_policy, class_name: 'Security::Policy', inverse_of: :vulnerability_management_policy_rules
validates :typed_content, json_schema: { filename: "vulnerability_management_policy_rule_content" }
+
+ scope :no_longer_detected, -> { where(type: :no_longer_detected) }
+
+ def match?(vulnerability)
+ scanners.include?(vulnerability.report_type) && severity_levels.include?(vulnerability.severity)
+ end
+
+ private
+
+ def scanners
+ return ::Enums::Vulnerability.report_types.keys if content['scanners'].blank?
+
+ content['scanners']
+ end
+
+ def severity_levels
+ return ::Enums::Vulnerability.severity_levels.keys if content['severity_levels'].blank?
+
+ content['severity_levels']
+ end
end
end
diff --git a/ee/app/services/security/ingestion/mark_as_resolved_service.rb b/ee/app/services/security/ingestion/mark_as_resolved_service.rb
index e9db98d46e2e5..44091f3f69dec 100644
--- a/ee/app/services/security/ingestion/mark_as_resolved_service.rb
+++ b/ee/app/services/security/ingestion/mark_as_resolved_service.rb
@@ -8,6 +8,7 @@ module Ingestion
# on the default branch if they were not detected again.
class MarkAsResolvedService
include Gitlab::InternalEventsTracking
+ include Gitlab::Utils::StrongMemoize
CVS_SCANNER_EXTERNAL_ID = 'gitlab-sbom-vulnerability-scanner'
CS_SCANNERS_EXTERNAL_IDS = %w[trivy].freeze
@@ -44,10 +45,13 @@ def execute
delegate :vulnerability_reads, to: :project, private: true
def process_batch(batch)
- (batch.pluck_primary_key - ingested_ids).then { |missing_ids| mark_as_resolved(missing_ids) }
+ (batch.pluck_primary_key - ingested_ids).then do |missing_ids|
+ mark_as_no_longer_detected(missing_ids)
+ auto_resolve(missing_ids)
+ end
end
- def mark_as_resolved(missing_ids)
+ def mark_as_no_longer_detected(missing_ids)
return if missing_ids.blank?
resolved_count = Vulnerability.id_in(missing_ids)
@@ -58,6 +62,17 @@ def mark_as_resolved(missing_ids)
track_no_longer_detected_vulnerabilities(resolved_count)
end
+ def auto_resolve(missing_ids)
+ return unless auto_resolve_enabled?
+
+ Vulnerabilities::AutoResolveService.new(project, missing_ids).execute
+ end
+
+ def auto_resolve_enabled?
+ ::Feature.enabled?(:auto_resolve_vulnerabilities, project)
+ end
+ strong_memoize_attr :auto_resolve_enabled?
+
def process_existing_cvs_vulnerabilities_for_container_scanning
vulnerability_reads
.by_scanner_ids(cvs_scanner_id)
diff --git a/ee/app/services/vulnerabilities/auto_resolve_service.rb b/ee/app/services/vulnerabilities/auto_resolve_service.rb
index 4b146c6d58860..4a776517ff9c6 100644
--- a/ee/app/services/vulnerabilities/auto_resolve_service.rb
+++ b/ee/app/services/vulnerabilities/auto_resolve_service.rb
@@ -2,20 +2,20 @@
module Vulnerabilities
class AutoResolveService
+ include Gitlab::Utils::StrongMemoize
+
MAX_BATCH = 100
- def initialize(project, vulnerability_ids, security_policy_name)
+ def initialize(project, vulnerability_ids)
@project = project
- @vulnerability_ids = vulnerability_ids
- @security_policy_name = security_policy_name
+ @vulnerabilities = Vulnerability.id_in(vulnerability_ids.first(MAX_BATCH))
end
def execute
+ return ServiceResponse.success if policies.blank?
return error_response unless can_create_state_transitions?
- vulnerability_ids.each_slice(MAX_BATCH).each do |ids|
- resolve(Vulnerability.id_in(ids))
- end
+ resolve_vulnerabilities
refresh_statistics
ServiceResponse.success
@@ -25,24 +25,37 @@ def execute
private
- attr_reader :project, :vulnerability_ids, :security_policy_name
+ attr_reader :project, :vulnerabilities
- def resolve(vulnerabilities)
- # rubocop:disable CodeReuse/ActiveRecord -- context specific
- # rubocop:disable Database/AvoidUsingPluckWithoutLimit -- Caller limits to 100 records
- vulnerability_attrs = vulnerabilities.pluck(:id, :state)
- # rubocop:enable CodeReuse/ActiveRecord
- # rubocop:enable Database/AvoidUsingPluckWithoutLimit
+ def vulnerabilities_to_resolve
+ policies_by_vulnerability.keys
+ end
- return if vulnerability_attrs.empty?
+ def policies_by_vulnerability
+ policies.each_with_object({}) do |policy, memo|
+ vulnerabilities.each do |vulnerability|
+ if policy.match?(vulnerability)
+ memo[vulnerability] ||= []
+ memo[vulnerability].push(policy)
+ end
+ end
+ end
+ end
+ strong_memoize_attr :policies_by_vulnerability
+
+ def policies
+ # TODO: This should only include policies that have a `no_longer_detected` rule
+ # and an `auto_resolve` action
+ project.security_policies.type_vulnerability_management_policy
+ end
- state_transitions = transition_attributes_for(vulnerability_attrs)
- system_notes = system_note_attributes_for(vulnerability_attrs)
+ def resolve_vulnerabilities
+ return if vulnerabilities_to_resolve.empty?
Vulnerability.transaction do
- Vulnerabilities::StateTransition.insert_all!(state_transitions)
+ Vulnerabilities::StateTransition.insert_all!(state_transition_attrs)
- vulnerabilities.update_all(
+ Vulnerability.id_in(vulnerabilities_to_resolve.map(&:id)).update_all(
state: :resolved,
auto_resolved: true,
resolved_by_id: user.id,
@@ -50,28 +63,28 @@ def resolve(vulnerabilities)
updated_at: now
)
end
- Note.insert_all!(system_notes)
+ Note.insert_all!(system_note_attrs)
end
- def transition_attributes_for(attrs)
- attrs.map do |id, state|
+ def state_transition_attrs
+ vulnerabilities_to_resolve.map do |vulnerability|
{
- vulnerability_id: id,
- from_state: state,
+ vulnerability_id: vulnerability.id,
+ from_state: vulnerability.state,
to_state: :resolved,
author_id: user.id,
- comment: comment,
+ comment: comment(vulnerability),
created_at: now,
updated_at: now
}
end
end
- def system_note_attributes_for(attrs)
- attrs.map do |id, _|
+ def system_note_attrs
+ vulnerabilities_to_resolve.map do |vulnerability|
{
noteable_type: "Vulnerability",
- noteable_id: id,
+ noteable_id: vulnerability.id,
project_id: project.id,
namespace_id: project.project_namespace_id,
system: true,
@@ -79,7 +92,7 @@ def system_note_attributes_for(attrs)
'changed',
:resolved,
nil,
- comment
+ comment(vulnerability)
),
author_id: user.id,
created_at: now,
@@ -88,8 +101,9 @@ def system_note_attributes_for(attrs)
end
end
- def comment
- _("Auto-resolved by vulnerability management policy") + " #{security_policy_name}"
+ def comment(vulnerability)
+ policy_names = policies_by_vulnerability[vulnerability].map(&:name)
+ _("Auto-resolved by vulnerability management policy") + " #{policy_names.join(', ')}"
end
def user
diff --git a/ee/config/feature_flags/beta/auto_resolve_vulnerabilities.yml b/ee/config/feature_flags/beta/auto_resolve_vulnerabilities.yml
new file mode 100644
index 0000000000000..7f3d40dfe8038
--- /dev/null
+++ b/ee/config/feature_flags/beta/auto_resolve_vulnerabilities.yml
@@ -0,0 +1,9 @@
+---
+name: auto_resolve_vulnerabilities
+feature_issue_url: https://gitlab.com/groups/gitlab-org/-/epics/5708
+introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/173437
+rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/505711
+milestone: '17.7'
+group: group::security insights
+type: beta
+default_enabled: false
diff --git a/ee/spec/factories/security/policies.rb b/ee/spec/factories/security/policies.rb
index 5ff9e12c603c7..f0b49e6c148d6 100644
--- a/ee/spec/factories/security/policies.rb
+++ b/ee/spec/factories/security/policies.rb
@@ -37,6 +37,16 @@
end
require_approval
+ transient do
+ linked_projects { [] }
+ end
+
+ after(:create) do |policy, evaluator|
+ evaluator.linked_projects.each do |project|
+ create(:security_policy_project_link, project: project, security_policy: policy)
+ end
+ end
+
trait :deleted do
policy_index { -1 }
end
diff --git a/ee/spec/models/security/vulnerability_management_policy_rule_spec.rb b/ee/spec/models/security/vulnerability_management_policy_rule_spec.rb
index 673d20ec65ebb..568ebe0a97e23 100644
--- a/ee/spec/models/security/vulnerability_management_policy_rule_spec.rb
+++ b/ee/spec/models/security/vulnerability_management_policy_rule_spec.rb
@@ -23,4 +23,58 @@
end
end
end
+
+ describe '#match?' do
+ def severity_wildcard_should_match_all_severities
+ ::Enums::Vulnerability.severity_levels.keys.map do |severity|
+ [['sast'], [], :sast, severity, true]
+ end
+ end
+
+ def scanner_wildcard_should_match_all_scanners
+ ::Enums::Vulnerability.report_types.keys.map do |report_type|
+ [[], ['critical'], report_type, 'critical', true]
+ end
+ end
+
+ def should_match_if_partials_match
+ [
+ [%w[dependency_scanning container_scanning], %w[info low], :dependency_scanning, :info, true],
+ [%w[dependency_scanning container_scanning], %w[info low], :dependency_scanning, :low, true],
+ [%w[dependency_scanning container_scanning], %w[info low], :container_scanning, :info, true],
+ [%w[dependency_scanning container_scanning], %w[info low], :container_scanning, :low, true]
+ ]
+ end
+
+ def should_not_match_if_partials_dont_match
+ [
+ [%w[dependency_scanning container_scanning], %w[info low], :sast, :info, false],
+ [%w[dependency_scanning container_scanning], %w[info low], :dependency_scanning, :critical, false],
+ [%w[dependency_scanning container_scanning], %w[info low], :sast, :critical, false]
+ ]
+ end
+
+ where(:policy_scanners, :policy_severities, :vulnerability_report_type, :vulnerability_severity, :expected) do
+ severity_wildcard_should_match_all_severities +
+ scanner_wildcard_should_match_all_scanners +
+ should_match_if_partials_match +
+ should_not_match_if_partials_dont_match
+ end
+
+ with_them do
+ specify do
+ rule = build_stubbed(:vulnerability_management_policy_rule,
+ type: :no_longer_detected,
+ content: { 'scanners' => policy_scanners, 'severity_levels' => policy_severities }
+ )
+
+ vulnerability = build_stubbed(:vulnerability,
+ report_type: vulnerability_report_type,
+ severity: vulnerability_severity
+ )
+
+ expect(rule.match?(vulnerability)).to eq(expected)
+ end
+ end
+ end
end
diff --git a/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb b/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb
index 6701417eae13f..cb38746c3241a 100644
--- a/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb
+++ b/ee/spec/services/security/ingestion/mark_as_resolved_service_spec.rb
@@ -11,17 +11,41 @@
let(:ingested_ids) { [] }
let_it_be(:scanner) { create(:vulnerabilities_scanner, project: project) }
- it 'resolves non-generic vulnerabilities detected by the scanner' do
- vulnerability = create(:vulnerability, :sast,
- project: project,
- present_on_default_branch: true,
- resolved_on_default_branch: false,
- findings: [create(:vulnerabilities_finding, project: project, scanner: scanner)]
- )
+ context 'when there is a vulnerability to be resolved' do
+ let_it_be(:vulnerability) do
+ create(:vulnerability, :sast,
+ project: project,
+ present_on_default_branch: true,
+ resolved_on_default_branch: false,
+ findings: [create(:vulnerabilities_finding, project: project, scanner: scanner)]
+ )
+ end
- command.execute
+ it 'resolves non-generic vulnerabilities detected by the scanner' do
+ command.execute
+
+ expect(vulnerability.reload).to be_resolved_on_default_branch
+ end
+
+ it 'calls AutoResolveService on missing_ids' do
+ expect_next_instance_of(Vulnerabilities::AutoResolveService, project, [vulnerability.id]) do |service|
+ expect(service).to receive(:execute)
+ end
- expect(vulnerability.reload).to be_resolved_on_default_branch
+ command.execute
+ end
+
+ context 'when auto_resolve_vulnerabilities feature flag is disabled' do
+ before do
+ stub_feature_flags(auto_resolve_vulnerabilities: false)
+ end
+
+ it 'does not call AutoResolveService' do
+ expect(Vulnerabilities::AutoResolveService).not_to receive(:new)
+
+ command.execute
+ end
+ end
end
context 'with multiple vulnerabilities' do
diff --git a/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb b/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb
index 6d8aa8cb7edb3..2295635785ad9 100644
--- a/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb
+++ b/ee/spec/services/vulnerabilities/auto_resolve_service_spec.rb
@@ -7,11 +7,24 @@
let_it_be(:namespace) { create(:namespace) }
let_it_be_with_reload(:project) { create(:project, namespace: namespace) }
let_it_be(:vulnerability) { create(:vulnerability, :with_findings, :detected, :high_severity, project: project) }
+ let_it_be(:policy) { create(:security_policy, :vulnerability_management_policy, linked_projects: [project]) }
+ let_it_be(:policy_rule) do
+ create(:vulnerability_management_policy_rule,
+ security_policy: policy,
+ content: {
+ type: 'no_longer_detected',
+ scanners: [],
+ severity_levels: []
+ }
+ )
+ end
+
let(:vulnerability_ids) { [vulnerability.id] }
+
let(:comment) { _("Auto-resolved by vulnerability management policy") + " #{security_policy_name}" }
- let(:security_policy_name) { 'resolve_low_severity_vulnerabilities' }
+ let(:security_policy_name) { policy.name }
- subject(:service) { described_class.new(project, vulnerability_ids, security_policy_name) }
+ subject(:service) { described_class.new(project, vulnerability_ids) }
before_all do
project.add_guest(user)
@@ -128,14 +141,14 @@
it 'does not introduce N+1 queries' do
control = ActiveRecord::QueryRecorder.new do
- described_class.new(project, vulnerability_ids, security_policy_name).execute
+ described_class.new(project, vulnerability_ids).execute
end
new_vulnerability = create(:vulnerability, :with_findings, project: project)
vulnerability_ids << new_vulnerability.id
expect do
- described_class.new(project, vulnerability_ids, security_policy_name).execute
+ described_class.new(project, vulnerability_ids).execute
end.not_to exceed_query_limit(control)
end
end
--
GitLab