<!-- Implementation issues are used break-up a large piece of work into small, discrete tasks that can move independently through the build workflow steps. They're typically used to populate a Feature Epic. Once created, an implementation issue is usually refined in order to populate and review the implementation plan and weight. Example workflow: https://about.gitlab.com/handbook/engineering/development/threat-management/planning/diagram.html#plan --> [[_TOC_]] ## Glossary - [Primary Identifier](https://docs.gitlab.com/ee/user/application_security/terminology/#primary-identifier): The first identifier in finding - in other words, order matters! - CVS: Continuous Vulnerability Scanning - the new method of vulnerability scanning where findings are decoupled from pipelines. ## Why are we doing this work <!-- A brief explanation of the why, not the what or how. Assume the reader doesn't know the background and won't have time to dig-up information from comment threads. --> The Gemnasium analyzers create a custom primary identifier when a vulnerability is added to the security report. This is an important detail, because finding equality is based on the primary identifier. Since the primary identifier is assumed to be stable, we must re- create this custom identifier, _and_ ensure that it is _always_ the first identifier in the array of identifiers for an advisory. See https://gitlab.com/gitlab-org/gitlab/-/merge_requests/121607#note_1466014286 for more context of discovery. This is consistent with Dependency Scanning JSON reports created by Gemnasium. See https://gitlab.com/gitlab-org/security-products/analyzers/gemnasium/-/blob/f52965010d4d69d7bca39c00f9bc52cd65f7e0e7/qa/expect/js-npm/default/gl-dependency-scanning-report.json#L69-82 (excerpt): ```json { "identifiers": [ { "type": "gemnasium", "name": "Gemnasium-4774cd67-936f-419e-8533-ae5cfe7db9f9", "value": "4774cd67-936f-419e-8533-ae5cfe7db9f9", "url": "https://gitlab.com/gitlab-org/security-products/gemnasium-db/-/blob/v1.2.142/npm/lodash/CVE-2019-10744.yml" }, { "type": "cve", "name": "CVE-2019-10744", "value": "CVE-2019-10744", "url": "https://cve.mitre.org/cgi-bin/cvename.cgi?name=CVE-2019-10744" } ] } ``` ## Relevant links <!-- Information that the developer might need to refer to when implementing the issue. - [Design Issue](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>) - [Design 1](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>/designs/<image>.png) - [Design 2](https://gitlab.com/gitlab-org/gitlab/-/issues/<id>/designs/<image>.png) - [Similar implementation](https://gitlab.com/gitlab-org/gitlab/-/merge_requests/<id>) --> ## Scope This is scoped only to findings related to dependency scanning and **does not** include container scanning. ## Non-functional requirements <!-- Add details for required items and delete others. --> - [ ] Documentation: - [ ] Feature flag: - [ ] Performance: - [x] Testing: Ensure specs pass after removing stubbed Gemansium identifiers in security report builder specs. ## Proposal(s) ### Create the identifier at runtime :star: **Pros** - Avoid re-exports if bugs caught in the identifier or we re-work the identifier. **Cons** - Rewrites are always risky. Programming paradigms change between languages, and introduce the possibility that something does not match the previous implementation 1:1. ### Create the identifier at export **Pros** - Saves some cycles when generating the findings for the synthesized security report that CVS uses. - Can re-use the Go code since the external db components are also written in Go. This has served well, and the chance of a bug is very low. - You can add the permalink to the file at export time since the exporter has access to the GLAD source. - It usually takes less time to merge MRs in the exporter. **Cons** - Any changes to the algorithm require another export of all GLAD advisories. ### Deprecate and remove the custom identifier from the analyzers **Pros** - Removed code is always good and reduces complexity. **Cons** - Deprecation is not straight forward, and might even require some cleanup on our behalf. This is made even more difficult for self-hosted customers. - What will take its place? We'll need to select a new primary identifier, and this is non-trivial. ## Implementation plan <!-- Steps and the parts of the code that will need to get updated. The plan can also call-out responsibilities for other team members or teams and can be split into smaller MRs to simplify the code review process. e.g.: - MR 1: Part 1 - [ ] ~frontend Step 1 - [ ] ~frontend Step 2 - MR 2: Part 2 - [ ] ~backend Step 1 - [ ] ~backend Step 2 - MR 3: Part 3 - [ ] ~frontend Step 1 - [ ] ~frontend Step 2 --> The proposal to create the identifier at runtime, i.e. in the Rails monolith, has been chosen. - [x] Update the `Gitlab::VulnerabiltyScanning::FindingBuilder` class so that it has a `#primary_identifier` private method. - [x] Override this method in the child class `Gitlab::VulnerabiltyScanning::DependencyScanning::FindingBuilder` so that it creates the Gemnasium identifier and returns it. - [x] The gemnasium primary identifier is constructed in Go [originally](https://gitlab.com/gitlab-org/security-products/analyzers/gemnasium/-/blob/15012e3d38efc2875934e7c209751d2165167d20/convert/vulnerability_converter.go#L84-91). It's constructed like this: ```ruby primary_identifier ||= "Gemnasium-#{advisory.advisory_xid}" ``` - [x] To prevent allocating each time this is called, memoize it. - [x] Update security report builder specs to remove the `gemnasium` identifiers. Verify that the primary identifier is the same even after removing the stubbed [identifiers](https://gitlab.com/gitlab-org/gitlab/-/blob/703f12ad8dd62c8dd6cda013a196065e9436805e/ee/spec/lib/gitlab/vulnerability_scanning/security_report_builder_spec.rb#L28-29). /cc @gonzoyumo @fcatteau @ifrenkel --- <!-- Workflow and other relevant labels # ~"group::" ~"Category:" ~"GitLab Ultimate" Other settings you might want to include when creating the issue. # /assign @ # /epic & --> ## Verification steps <!-- Add verification steps to help GitLab team members test the implementation. This is particularly useful during the MR review and the ~"workflow::verification" step. You may not know exactly what the verification steps should be during issue refinement, so you can always come back later to add them. 1. Check-out the corresponding branch 1. ... 1. Profit! -->