Security scan opens issues for releases without vulnerabilities

[bewebi]

Overview

Security alert issues, such as solo-io/gloo#10039, are being opened for releases that do not have any vulnerabilities according to the scans
This causes confusion, as the opening of Security Alert issues implies that vulnerabilities are present and need to be addressed, and simply closing them is not a sustainable workaround since a new issue will be reopened the next time the scan runs

Cause

This appears to be due to a change from #525 whereby we now always populate vulnerabilityMd, adding a message that explicitly states that no vulnerabilities were found for each image for which no vulnerabilities were found

This breaks an assumption made here in GithubIssueWriter.Write() that if there are no vulnerabilities, the argument passed as vulnerabilityMarkdown will be empty

Further Investigation

We (Gateway team) should check in with Platform team to learn about their Trivy Issue usage and whether/why they're OK with Github issues always being opened

Solution

SecurityScanRepo.RunMarkdownScan() currently always calls GithubIssueWriter.Write() and relies on that function to determine whether an issue should be written, however since the original code was written, the issue-writing became one implementation of an IssueWriter interface rather than a stand-alone function

The LocalIssueWriter implementation of Write() does not attempt to determine whether an issue should be written, and the description of Write() in the interface definition does not allude to this being part of the function's responsibility

Therefore we should determine whether an issue should be written prior to calling Write() (ie within SecurityScanRep.RunMarkdownScan()) and remove code from GithubIssueWriter.Write() that attempts to determine whether an issue should be written

[sam-heilbron]

Nice find! In the end, this issue creation is purely a tactic used internally to notify engineers of work that needs to be tackled, and doesn't have an impact on what is published in our user-facing docs, correct?

If that is the case, since we know why this is occurring, I would be ok just leaving it as is, and basically err on the side of over-creating issues, even if there is nothing to be done.

@nfuden @bewebi any concerns with that?

[bewebi]

The process for generating the Gloo Gateway docs page with CVE reporting is a bit convoluted and not well documented, but effectively isolated from the opening of issues

To summarize:

• The docs-gen workflow, which runs on push to LTS branches, among other times, invokes the build-site make target here
• build-site invokes build-docs.sh here, which in turn invokes the site-release target here, which has site-common as a prerequisite
• site-common generates security scan Markdown here for both OSS and EE, sending output from each to a file in the content/static/content directory
  • This output consists of scan results in Markdown format for every release on an LTS branch since the MIN_SCANNED_VERSION
  • The gen-security-scan-md command, defined here is invoked, and generates output by retrieving files for each scanned image from a GCloud bucket here
  • Those files are created when a security scan is run by these lines in go-utils code
  • In the case of Gloo Gateway, these scans are run weekly on Monday mornings as part of this Github action, with the scans run via the run-security-scan target and the generated files are subsequently uploaded to Google storage via the publish-security-scan target
• site-common then splits the Markdown files into separate files for each release here
• These files populate the security update docs pages by reference here and here

This summary should probably be in a README of some sort in the Gateway OSS repo, but leaving it here for the moment

As far as whether to actually act on this issue, I would still be in favor, as it's confusing to have Security Alert issues when there are no vulnerabilities with potentially serious ramifications
If we have a practice of checking all of the issues every Monday we shouldn't miss anything, but we can no longer rely on the existence of an issue to indicate that there are vulnerabilities to resolve
This is confusing, especially for outside observers who might wonder why there are so many Security Alert issues, and as a team we may not always be on top of actually opening them to check that they're empty since they're usually false alarms
It would be much better to return to the previous behavior whereby an issue was only opened when a vulnerability was found and action was required

Before doing so, we should check in with Platform to make sure that resolving this bug would not break any of their workflows and, if it would, we should collaborate on a solution that works for both teams