Follow-up: Make Dependency a pointer in Location
Follow-up of gitlab-org/security-products/sast!113 (merged)
We've introduced a field .location.dependency
to the report syntax shared b/w Dependency Scanning and SAST so that we can track the package/module that is affected by a vulnerability. This new field is relevant in the context of DS but not in the context of SAST. The side-effect is that we've got empty .location.dependency
fields in SAST reports.
Proposal
The common library should be flexible and make possible for the location to differ b/w SAST and DS. This can be achieve by using pointers instead of a shared Location
type (Go struct).
Implementation plan
-
Update the common library (completed with gitlab-org/security-products/analyzers/common!92 (merged)) - Make
Location.Dependency
a pointer - Change
Issue.ID()
to make it work with pointers; see gitlab-org/security-products/analyzers/common!92 (comment 343516937) and https://gitlab.com/gitlab-org/security-products/analyzers/common/-/blob/writehash/issue/issue.go#L42-81 - Update unit tests
- Make
-
Upgrade SAST projects - Remove
location.dependency
from the expected report of theintegration test
- Remove
location.dependency
from the expected reports of the test projects
- Remove
-
Upgrade Dependency Scanning projects - Make
Location.Dependency
a pointer
- Make
-
Upgrade Container Scanning project (klar) - Make
Location.Dependency
a pointer - Update tests with new IDs
- Make
Risks
Removing dependency
from SAST and Container Scanning reports should have no unintended side-effect:
- The Rails backend uses the location fingerprint to compare two sets of vulnerabilities, and ultimately to generate the vulnerability diff in the MR. But
Location
models for SAST and CS only use a subset of thelocation
JSON object, anddependency
is ignored. See https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/lib/gitlab/ci/parsers/security/sast.rb#L14 - The Rails backend tracks vulnerability feedback using the
cve
JSON field (to be removed), and will use theLocation
model in the future. Again, theLocation
model is specific to each report type and contains a subset of what's in thelocation
JSON field. See #205489 (closed) - The common library turns the
Location
into a fingerprint when removing the duplicates, but this has no impact since theDependency
is empty. Note that the dedupe function was used by the Docker-in-Docker orchestrator which has been deprecated. - The common library sorts vulnerabilities using the
CompareKey
, which doesn't include theLocation.Dependency
in the case of SAST and CS. Also, the order of the vulnerabilities doesn't have to be stable anymore: the Rails backend doesn't except any particular order, and the QA jobs sort the reports prior to comparing them.
All that can be proven by creating a MR in a scanned project, and tweaking the SAST scanning jobs so that they don't have .vulnerabilities[].location.dependency
fields anymore.
after_script
to be added to the SAST scanning job when the analyzer is based on a Debian image:
after_script:
- apt-get update && apt-get install -y jq
- jq 'del(.vulnerabilities[]|.location.dependency)' gl-sast-report.json > no-dependency.json
- cp no-dependency.json gl-sast-report.json
First line must be replaced with apk update && apk add jq
when the analyzer is based on a Alpine Linux image;