Add tracking info to Secret Detection

What does this MR do and why?

Context

Whenever a Secret Detection finding is created for a secret and if the same secret moves within the file, a new finding gets created along with the previous one, ending up with two findings pointing at the same secret. The new findings continue to be generated until the secret is remediated. So, we decided to solve the problem following the footsteps of SAST (which faced the same problem earlier) i.e., by introducing a tracking signature in the final report and the Rails monolith will use the signature during CI builds to determine if the new findings should be created or refer to the existing findings. For more details, refer to the linked issue.

Why this MR

This MR handles the rails monolith side of business i.e., uses tracking signature info of each finding in the SD report to ensure new findings are not generated if there are findings already present in the system for the same secret.

Technically, this MR does the following:

1. Introduces new tracking algorithm type: rule_value
2. Enables UUID override logic for Secret Detection

You may find this feature's impact on the current system here

Relevant Issue Numbers

• Track Secret Detection findings by filename and... (#434096 - closed)

How to set up and validate locally

1. Update a project with Secret Detection CI configuration to point at the patched secrets analyzer image:

# .gitlab-ci.yml
include:
  - template: Jobs/Secret-Detection.gitlab-ci.yml

secret_detection:
  image:
    name: registry.gitlab.com/gitlab-org/security-products/analyzers/secrets:vbhat-tracking-signature

2. Commit a dummy secret, say glpat-12345123451234512345 in one of the files.
3. Push the commit that triggers the secret_detection CI job in the Pipeline.
4. Ensure the Secret Detection report was ingested successfully
5. Open GDK rails console
6. Check Vulnerabilities::FindingSignature.where(algorithm_type: 5).count.positive? indicating newly created SD finding having rule_value(i.e. 5) as the tracking algorithm.

changed milestone to %16.11

added backend devopssecure featureenhancement groupsecret detection sectionsec typefeature labels

assigned to @vbhat161

	3 Warnings
	50c899bc: The commit body should not contain more than 72 characters per line. For more information, take a look at our Commit message guidelines.
	3024d2ff: The commit subject must start with a capital letter. For more information, take a look at our Commit message guidelines.
	featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart. For more information, see: The Handbook page on merge request types. The definition of done documentation.

Reviewer roulette

Category	Reviewer	Maintainer
backend	@j_lar (UTC+2, 3.5 hours behind author)	@ck3g (UTC+2, 3.5 hours behind author)
~"Verify"	Reviewer review is optional for ~"Verify"	@ghavenga (UTC+2, 3.5 hours behind author)

Please check reviewer's status!

• Reviewer is available!
• Reviewer is unavailable!

Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.

If needed, you can retry the danger-review job that generated this comment.

Generated by Danger

mentioned in issue #434096 (closed)

mentioned in merge request gitlab-org/security-products/analyzers/secrets!289 (closed)

@rossfuhrman @theoretick Do you guys mind doing an initial round of review on this MR? I've verified two scenarios where this change could impact that I know of - 1. When storing new findings 2. When comparing pipeline reports(head and base), both are working as expected on GDK. It would be great if you could point out if I've missed considering any scenarios. Thanks!

Note: I'm yet to add rspec tests for some of the implementation bits.

requested review from @theoretick and @rossfuhrman

approved this merge request

added pipeline:mr-approved label

@theoretick, thanks for approving this merge request.

This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break master, a new pipeline will be started shortly.

Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.

E2E Test Result Summary

allure-report-publisher generated test report!

e2e-test-on-gdk: test report for 47499600

expand test summary

+------------------------------------------------------------------+
|                          suites summary                          |
+-------------+--------+--------+---------+-------+-------+--------+
|             | passed | failed | skipped | flaky | total | result |
+-------------+--------+--------+---------+-------+-------+--------+
| Create      | 87     | 0      | 9       | 0     | 96    | ✅     |
| Plan        | 51     | 0      | 2       | 0     | 53    | ✅     |
| Govern      | 66     | 0      | 0       | 0     | 66    | ✅     |
| Verify      | 35     | 0      | 1       | 0     | 36    | ✅     |
| Data Stores | 31     | 0      | 0       | 0     | 31    | ✅     |
| Package     | 24     | 0      | 6       | 0     | 30    | ✅     |
| Release     | 5      | 0      | 0       | 0     | 5     | ✅     |
| Monitor     | 7      | 0      | 0       | 0     | 7     | ✅     |
| Analytics   | 2      | 0      | 0       | 0     | 2     | ✅     |
| Manage      | 0      | 0      | 1       | 0     | 1     | ➖     |
+-------------+--------+--------+---------+-------+-------+--------+
| Total       | 308    | 0      | 19      | 0     | 327   | ✅     |
+-------------+--------+--------+---------+-------+-------+--------+

@vbhat161 Some end-to-end (E2E) tests should run based on the stage label.

Please start the trigger-omnibus-and-follow-up-e2e job in the qa stage and wait for the tests in the follow-up-e2e:package-and-test-ee pipeline to pass before merging this MR. Do not use Auto-merge, unless these tests have already completed successfully, because a failure in these tests do not block the auto-merge. (E2E tests are computationally intensive and don't run automatically for every push/rebase, so we ask you to run this job manually at least once.)

To run all E2E tests, apply the pipeline:run-all-e2e label and run a new pipeline.

E2E test jobs are allowed to fail due to flakiness. See current failures at the latest pipeline triage issue.

Once done, apply the emoji on this comment.

Team members only: for any questions or help, reach out on the internal #test-platform Slack channel.

approved this merge request

added 2 commits

• 24f3a229 - Include gitleaks for uuid update task
• 76326d46 - Track Secret Detection vulnerabilities as they move within the file

Compare with previous version

reset approvals from @theoretick by pushing to the branch

@rossfuhrman I need your assistance. I have removed some of the existing code, mainly the part that checks for the "report type" to run the de-duplication of findings. As Lucas pointed out above, having a tracking signature alone is enough to decide whether to run the de-duplication action. I've added tests for the newly added changes, however, I couldn't identify rspec tests for the remaining changes. For the removed report type check change, the existing tests are already centered around the tracking signature check regardless of the report type making the removed change unaffected. Do you see any possibility of adding tests for the remaining changes?

added 1 commit

• 20105c59 - Fix breaking rspec test for updating findings uuid

Compare with previous version