Security Approvals not required when scans removed in MR
Summary
Users can work around Security Approvals groups by removing scanning jobs from the CI config, in the merge request they submit.
Steps to reproduce
- create a project where SAST and Dependency Scanning (DS) is enabled in the
master
branch - edit the project settings, and add a security approval group
- create a MR that removes SAST and DS from the CI configuration file
The source branch has no security scans but is not blocked, even though it may introduce no vulnerabilities (compared to the target branch, where the scans are configured).
Example Project
NOTE: For some reasons the MR is blocked when SAST is removed, which is the expected behavior. That's possibly a side-effect of #37237 (closed).
What is the current bug behavior?
The MR is NOT blocked. The approval widget says No approval required
, even though there's a Vulnerability-Check
group.
What is the expected correct behavior?
The MR is BLOCKED. Ideally, the error is explicit about the misconfiguration of the security scanning, in the source branch contributors are submitting.
Relevant logs and/or screenshots
Possible fixes
We can compare report types for pipeline from MR and target branch. If there is at least one report type missing in the MR pipeline we will enforce the policy.
diff --git a/lib/gitlab/ci/reports/security/reports.rb b/lib/gitlab/ci/reports/security/reports.rb
index b6372349f68a..2e4c626ed878 100644
--- a/lib/gitlab/ci/reports/security/reports.rb
+++ b/lib/gitlab/ci/reports/security/reports.rb
@@ -23,6 +23,8 @@ def findings
end
def violates_default_policy_against?(target_reports, vulnerabilities_allowed, severity_levels, vulnerability_states, report_types = [])
+ return true if scan_removed?(target_reports)
+
unsafe_findings_count(target_reports, severity_levels, vulnerability_states, report_types) > vulnerabilities_allowed
end
@@ -36,6 +38,10 @@ def unsafe_findings_count(target_reports, severity_levels, vulnerability_states,
new_uuids = unsafe_findings_uuids(severity_levels, report_types) - target_reports&.unsafe_findings_uuids(severity_levels, report_types).to_a
new_uuids.count
end
+
+ def scan_removed?(target_reports)
+ (target_reports&.reports&.keys.to_a - reports.keys).any?
+ end
end
end
end
Other ways to solve it?
Changing files related to CI should be restricted and always verified with CODEOWNERS
file so this can be solved by properly setting this file.
What kind of issues we might have when we solve it with proposed solution?
- There is still an option to modify
.gitlab-ci.yml
file to ie. include empty report. Then technically we will have all needed security reports and approval will not be needed, although it should be as the security job was modified.
Implementation plan
-
backend in /lib/gitlab/ci/reports/security/reports.rb
implement new methodscan_removed?(target_reports)
to verify if any scan was removed, this method should compare all reports from pipelines between target and source sha, -
backend in /lib/gitlab/ci/reports/security/reports.rb
modifyviolates_default_policy_against?
method to returntrue
when scan were removed from the pipeline configurationscan_removed?(target_reports)
, -
backend feature flag Add feature flag to be able to enable/disable this feature quickly,