From 5e4e90950f52b93a5c4e6359d65c1ce2d772bc8c Mon Sep 17 00:00:00 2001
From: mo khan <mo@mokhan.ca>
Date: Tue, 24 Jan 2023 16:33:10 -0700
Subject: [PATCH] Resolve vulnerabilities from successful scans
Instead of automatically resolving all vulnerabilities we automatically
resolve vulnerabilities when the scanner that detected it completes a
successful scan.
Changelog: changed
MR: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/109923
EE: true
---
ee/app/models/security/scan.rb | 8 ++
ee/app/models/vulnerabilities/read.rb | 1 +
.../ingestion/ingest_report_service.rb | 6 +-
.../ingestion/ingest_reports_service.rb | 5 --
.../ingestion/mark_as_resolved_service.rb | 24 +++---
ee/spec/models/security/scan_spec.rb | 9 +++
ee/spec/models/vulnerabilities/read_spec.rb | 15 ++++
.../ingestion/ingest_report_service_spec.rb | 17 +++-
.../ingestion/ingest_reports_service_spec.rb | 11 +--
.../mark_as_resolved_service_spec.rb | 78 ++++++++++++++-----
10 files changed, 129 insertions(+), 45 deletions(-)
diff --git a/ee/app/models/security/scan.rb b/ee/app/models/security/scan.rb
index ece5d709323f..d290cb99ba14 100644
--- a/ee/app/models/security/scan.rb
+++ b/ee/app/models/security/scan.rb
@@ -112,12 +112,20 @@ def remediations_proxy
@remediations_proxy ||= RemediationsProxy.new(job_artifact&.file)
end
+ def scanner
+ project.vulnerability_scanners.find_by_external_id(primary_scanner&.external_id)
+ end
+
private
def security_report
job_artifact&.security_report
end
+ def primary_scanner
+ security_report&.primary_scanner
+ end
+
def job_artifact
build.job_artifacts.find_by_file_type(scan_type)
end
diff --git a/ee/app/models/vulnerabilities/read.rb b/ee/app/models/vulnerabilities/read.rb
index f4299fcb56d4..c55eec58dbdd 100644
--- a/ee/app/models/vulnerabilities/read.rb
+++ b/ee/app/models/vulnerabilities/read.rb
@@ -41,6 +41,7 @@ class Read < ApplicationRecord
scope :order_detected_at_asc, -> { reorder(vulnerability_id: :asc) }
scope :order_detected_at_desc, -> { reorder(vulnerability_id: :desc) }
+ scope :by_scanner, -> (scanner) { where(scanner: scanner) }
scope :by_scanner_ids, -> (scanner_ids) { where(scanner_id: scanner_ids) }
scope :for_projects, -> (project_ids) { where(project_id: project_ids) }
scope :grouped_by_severity, -> { reorder(severity: :desc).group(:severity) }
diff --git a/ee/app/services/security/ingestion/ingest_report_service.rb b/ee/app/services/security/ingestion/ingest_report_service.rb
index 25350d589de8..360ade9b024b 100644
--- a/ee/app/services/security/ingestion/ingest_report_service.rb
+++ b/ee/app/services/security/ingestion/ingest_report_service.rb
@@ -23,7 +23,9 @@ def initialize(security_scan)
end
def execute
- finding_map_collection.each_slice(BATCH_SIZE).flat_map { |slice| ingest_slice(slice) }
+ finding_map_collection.each_slice(BATCH_SIZE)
+ .flat_map { |slice| ingest_slice(slice) }
+ .tap { |ids| MarkAsResolvedService.execute(scanner, ids) }
end
private
@@ -31,7 +33,7 @@ def execute
attr_reader :security_scan
attr_accessor :errored
- delegate :pipeline, to: :security_scan, private: true
+ delegate :pipeline, :scanner, to: :security_scan, private: true
def finding_map_collection
@finding_map_collection ||= FindingMapCollection.new(security_scan)
diff --git a/ee/app/services/security/ingestion/ingest_reports_service.rb b/ee/app/services/security/ingestion/ingest_reports_service.rb
index 97b7aea7d863..7f6cdd600ecc 100644
--- a/ee/app/services/security/ingestion/ingest_reports_service.rb
+++ b/ee/app/services/security/ingestion/ingest_reports_service.rb
@@ -30,7 +30,6 @@ def execute
def store_reports
latest_security_scans
.flat_map { |scan| ingest(scan) }
- .then { |ids| mark_resolved_vulnerabilities(ids) }
end
def latest_security_scans
@@ -41,10 +40,6 @@ def ingest(security_scan)
IngestReportService.execute(security_scan)
end
- def mark_resolved_vulnerabilities(existing_ids)
- MarkAsResolvedService.execute(project, existing_ids)
- end
-
def mark_project_as_vulnerable!
project.project_setting.update!(has_vulnerabilities: true)
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 4bfc3a144e82..21fdb4c8324f 100644
--- a/ee/app/services/security/ingestion/mark_as_resolved_service.rb
+++ b/ee/app/services/security/ingestion/mark_as_resolved_service.rb
@@ -3,27 +3,33 @@
module Security
module Ingestion
# This service class takes the IDs of recently ingested
- # vulnerabilities for a project and marks the missing
- # vulnerabilities as resolved on default branch.
+ # vulnerabilities for a project which had been previously
+ # detected by the same scanner, and marks them as resolved
+ # on the default branch if they were not detected again.
class MarkAsResolvedService
- def self.execute(project, ingested_ids)
- new(project, ingested_ids).execute
+ def self.execute(scanner, ingested_ids)
+ new(scanner, ingested_ids).execute
end
- def initialize(project, ingested_ids)
- @project = project
+ def initialize(scanner, ingested_ids)
+ @scanner = scanner
@ingested_ids = ingested_ids
end
def execute
- vulnerabilities.present_on_default_branch.each_batch { |batch| process_batch(batch) }
+ return unless scanner
+
+ vulnerability_reads
+ .by_scanner(scanner)
+ .each_batch { |batch| process_batch(batch) }
end
private
- attr_reader :project, :ingested_ids
+ attr_reader :ingested_ids, :scanner
- delegate :vulnerabilities, to: :project, private: true
+ delegate :project, to: :scanner, private: true
+ 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) }
diff --git a/ee/spec/models/security/scan_spec.rb b/ee/spec/models/security/scan_spec.rb
index fe191121829f..ac2e7816c4db 100644
--- a/ee/spec/models/security/scan_spec.rb
+++ b/ee/spec/models/security/scan_spec.rb
@@ -435,4 +435,13 @@
expect(scan.project_id).to eq(scan.build.project_id)
expect(scan.pipeline_id).to eq(scan.build.commit_id)
end
+
+ describe '#scanner' do
+ let_it_be(:scan) { create(:security_scan, :with_findings) }
+ let_it_be(:scanner) { create(:vulnerabilities_scanner, project: scan.project, external_id: 'a-dast-scanner') }
+
+ it 'returns the matching vulnerability scanner' do
+ expect(scan.scanner).to eql(scanner)
+ end
+ end
end
diff --git a/ee/spec/models/vulnerabilities/read_spec.rb b/ee/spec/models/vulnerabilities/read_spec.rb
index 55c400b3fa7a..f2376cafe240 100644
--- a/ee/spec/models/vulnerabilities/read_spec.rb
+++ b/ee/spec/models/vulnerabilities/read_spec.rb
@@ -494,6 +494,21 @@
end
end
+ describe '.by_scanner' do
+ let_it_be(:scanner) { create(:vulnerabilities_scanner, project: project) }
+ let_it_be(:other_scanner) { create(:vulnerabilities_scanner, project: project) }
+ let_it_be(:finding) { create(:vulnerabilities_finding, scanner: scanner) }
+ let_it_be(:other_finding) { create(:vulnerabilities_finding, scanner: other_scanner) }
+ let_it_be(:vulnerability) { create(:vulnerability, project: project, present_on_default_branch: true, findings: [finding]) }
+ let_it_be(:vulnerability_for_another_scanner) { create(:vulnerability, project: project, present_on_default_branch: true, findings: [other_finding]) }
+
+ subject(:vulnerability_reads) { described_class.by_scanner(scanner) }
+
+ it 'returns records by given scanner' do
+ expect(vulnerability_reads.pluck(:vulnerability_id)).to match_array([vulnerability.id])
+ end
+ end
+
private
def create_vulnerability(severity: 7, confidence: 7, report_type: 0)
diff --git a/ee/spec/services/security/ingestion/ingest_report_service_spec.rb b/ee/spec/services/security/ingestion/ingest_report_service_spec.rb
index 47de752b6dfe..6531b5798234 100644
--- a/ee/spec/services/security/ingestion/ingest_report_service_spec.rb
+++ b/ee/spec/services/security/ingestion/ingest_report_service_spec.rb
@@ -2,11 +2,11 @@
require 'spec_helper'
-RSpec.describe Security::Ingestion::IngestReportService do
+RSpec.describe Security::Ingestion::IngestReportService, feature_category: :vulnerability_management do
let(:service_object) { described_class.new(security_scan) }
describe '#execute' do
- let(:security_scan) { create(:security_scan, scan_type: :sast) }
+ let(:security_scan) { create(:security_scan, :with_findings, scan_type: :sast) }
subject(:ingest_report) { service_object.execute }
@@ -26,6 +26,19 @@
expect(Security::Ingestion::IngestReportSliceService).to have_received(:execute).twice
end
+ context 'when ingesting vulnerabilities' do
+ let!(:scanner) { create(:vulnerabilities_scanner, project: security_scan.project, external_id: 'find_sec_bugs') }
+
+ it 'resolves the missing vulnerabilities' do
+ allow(Security::Ingestion::MarkAsResolvedService).to receive(:execute)
+
+ ingest_report
+
+ expect(Security::Ingestion::MarkAsResolvedService)
+ .to have_received(:execute).once.with(scanner, [1, 2])
+ end
+ end
+
context 'when ingesting a slice of vulnerabilities fails' do
let(:exception) { RuntimeError.new }
let(:expected_processing_error) { { 'type' => 'IngestionError', 'message' => 'Ingestion failed for some vulnerabilities' } }
diff --git a/ee/spec/services/security/ingestion/ingest_reports_service_spec.rb b/ee/spec/services/security/ingestion/ingest_reports_service_spec.rb
index 22cc24bec607..001e08a470e1 100644
--- a/ee/spec/services/security/ingestion/ingest_reports_service_spec.rb
+++ b/ee/spec/services/security/ingestion/ingest_reports_service_spec.rb
@@ -2,7 +2,7 @@
require 'spec_helper'
-RSpec.describe Security::Ingestion::IngestReportsService do
+RSpec.describe Security::Ingestion::IngestReportsService, feature_category: :vulnerability_management do
let(:service_object) { described_class.new(pipeline) }
let_it_be(:project) { create(:project) }
@@ -22,7 +22,6 @@
before do
allow(Security::Ingestion::IngestReportService).to receive(:execute).and_return(ids_1, ids_2)
- allow(Security::Ingestion::MarkAsResolvedService).to receive(:execute)
allow(Security::Ingestion::ScheduleMarkDroppedAsResolvedService).to receive(:execute)
end
@@ -36,13 +35,7 @@
it 'sets the resolved vulnerabilities, latest pipeline ID and has_vulnerabilities flag' do
expect { ingest_reports }.to change { project.reload.project_setting&.has_vulnerabilities }.to(true)
- .and change { project.reload.vulnerability_statistic&.latest_pipeline_id }.to(pipeline.id)
- end
-
- it 'calls MarkAsResolvedService with the recently ingested vulnerability IDs' do
- ingest_reports
-
- expect(Security::Ingestion::MarkAsResolvedService).to have_received(:execute).with(project, ids_1)
+ .and change { project.reload.vulnerability_statistic&.latest_pipeline_id }.to(pipeline.id)
end
it 'calls ScheduleMarkDroppedAsResolvedService with primary identifier IDs' do
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 a77e090c2df9..93afed379854 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
@@ -2,32 +2,74 @@
require 'spec_helper'
-RSpec.describe Security::Ingestion::MarkAsResolvedService do
+RSpec.describe Security::Ingestion::MarkAsResolvedService, feature_category: :vulnerability_management do
let_it_be(:project) { create(:project) }
- let_it_be(:non_default_vulnerability) { create(:vulnerability, project: project, present_on_default_branch: false) }
- let_it_be_with_reload(:not_ingested_vulnerability) { create(:vulnerability, project: project) }
- let_it_be_with_reload(:ingested_vulnerability) { create(:vulnerability, project: project) }
- let_it_be_with_reload(:generic_vulnerability) { create(:vulnerability, project: project, report_type: :generic) }
+ describe '#execute' do
+ context 'when using a vulnerability scanner' do
+ let(:command) { described_class.new(scanner, ingested_ids) }
+ let(:ingested_ids) { [] }
+ let_it_be(:scanner) { create(:vulnerabilities_scanner, project: project) }
- let(:ingested_ids) { [ingested_vulnerability.id] }
- let(:service_object) { described_class.new(project, ingested_ids) }
+ 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)]
+ )
- describe '#execute' do
- subject(:mark_as_resolved) { service_object.execute }
+ command.execute
+
+ expect(vulnerability.reload).to be_resolved_on_default_branch
+ end
+
+ it 'does not resolve vulnerabilities detected by a different scanner' do
+ vulnerability = create(:vulnerability, :sast, project: project, present_on_default_branch: true)
+
+ command.execute
+
+ expect(vulnerability.reload).not_to be_resolved_on_default_branch
+ end
+
+ it 'does not resolve generic vulnerabilities' do
+ vulnerability = create(:vulnerability, :generic, project: project)
- it 'marks the missing vulnerabilities as resolved on default branch except the generic ones' do
- expect { mark_as_resolved }
- .to change { not_ingested_vulnerability.reload.resolved_on_default_branch }.from(false).to(true)
- .and not_change { ingested_vulnerability.reload.resolved_on_default_branch }.from(false)
- .and not_change { generic_vulnerability.reload.resolved_on_default_branch }.from(false)
+ command.execute
+
+ expect(vulnerability.reload).not_to be_resolved_on_default_branch
+ end
+
+ context 'when a vulnerability is already ingested' do
+ let_it_be(:ingested_vulnerability) { create(:vulnerability, project: project) }
+
+ before do
+ ingested_ids << ingested_vulnerability.id
+ end
+
+ it 'does not resolve ingested vulnerabilities' do
+ command.execute
+
+ expect(ingested_vulnerability.reload).not_to be_resolved_on_default_branch
+ end
+ end
end
- it 'does not process vulnerabilities which are not present on the default branch' do
- expect(service_object).to receive(:process_batch)
- .with(match_array([not_ingested_vulnerability, ingested_vulnerability, generic_vulnerability]))
+ context 'when a scanner is not available' do
+ let(:command) { described_class.new(nil, []) }
+
+ it 'does not resolve any vulnerabilities' do
+ vulnerability = create(:vulnerability, :sast,
+ project: project,
+ present_on_default_branch: true,
+ resolved_on_default_branch: false,
+ findings: []
+ )
+
+ command.execute
- mark_as_resolved
+ expect(vulnerability.reload).not_to be_resolved_on_default_branch
+ end
end
end
end
--
GitLab