CC @golang/vulndb

This would definitely be a welcome addition, just came to make this proposal request and found this was proposed already - I remember having to create a(n) (experimental) transformation layer to integrate it into Code Scanning for my personal projects but it would be a great if we can get first class support for SARIF through this tool. I believe the recent changes before 1.0 release (creating an output Handler interface) really has all the groundwork for this addition.

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@zpavlinovic to look into this.

It sounds like we will find some way to produce SARIF for this use case, likely as a separate tool.

An open question about SARIF support for Github code scanning is how to present witness call stacks. AFAIK, the dependency code cannot be annotated and parts of the call stack will refer to such code by definition. (Even if it could be annotated, it is not clear how users would assemble a call stack from a bunch of annotations spread over different files. IDEs can step through the call stack, but I don't think that is an option here.)

One way to go about this is to create an annotation/alert for the require lines in the go.mod file. The alert message would then show govulncheck-detected call stacks that lead to the vulnerable symbols defined in the annotated module with the accompanying information. Thoughts?

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

Is it going to be part of govulncheck command line tool's option, or just a feature used for the github action?
I think the former would be more useful if we decide to commit to maintain this feature.
If it's only for the github action, why can't that be implemented as a separate github action that translates the govulncheck's JSON output to SARIF format? I thought users who care about the SARIF format could chain the actions.

It is supposed to be a separate command that govulncheck-action will call. It won't be part of govulncheck command.

The question is how to create Sarif output so it makes sense for Github code scanning. In particular, to what code should the alerts be associated?

It is supposed to be a separate command that govulncheck-action will call. It won't be part of govulncheck command.

Will this tool be under x/vuln/cmd?

I originally thought we'd let the community write and maintain the conversion tool. If the Go team is committed to support the SARIF format anyway, why do we want to keep this as a separate command instead of having govulncheck just natively support SARIF as one of its supported output format?

SARIF format is useful beyond the GitHub code scanning. Many services, apps, and editor extensions ingest the format. For example, it's a possibility to let the SARIF viewer extension in VS Code to handle display of govulncheck findings (instead of depending on gopls or vscode-go).

The question is how to create Sarif output so it makes sense for Github code scanning. In particular, to what code should the alerts be associated?

Annotating go.mod will work for applications like GitHub code scanning. And if most of findings can be fixed by simply upgrading the module, I think that fits nicely.

Alternative ways to consider are all the entry points or the first place in the module that leads eventually to the vulnerable symbol. That potentially allows SARIF-based applications' grouping, sorting, and filtering findings to be more powerful, but associating the module upgrade with the result seems tricky.

Keeping the tool separate allows tools to be pipelined, so for instance
govulncheck -json -> vulnfilter -> vulntosarif
might be a theoretical chain , where the middle step is a tool that modifies the json stream before it gets converted to SARIF.

I guess the question is, is SARIF ubiquitous enough to be a blessed format, in which case adding it as an output directly to govulncheck makes sense, but if it is just one among many, then I would rather keep it in a separate tool.
If we did merge it in, we can still get the above pipeline, it would just look something like
govulncheck -json -> vulnignore -> govulncheck -sarif -mode=convert
either way I think we should prototype it in a separate tool, the code should be easy to merge in if we decide that it should be at a future date.

Also the prototype will probably want to use owenrumney/go-sarif as it seems to be the current library of choice for dealing with sarif, but that might not be a reasonable dependency for govulncheck itself.

I think the tool doesn't need to support GitHub Code Scanning specifically, as long as it adheres to the SARIF spec, it would should work with almost all SARIF based tools.

Regarding how to associate vulnerable parts of the code, the SARIF spec supports something called the CodeFlows property and ThreadFlows.

The idea behind CodeFlows is to show the call stack from the (potentially vulnerable) user input right down to the vulnerable symbol (think sources and syncs from taint analysis). The spec allows for us to thoroughly help the user with identifying which parts of the code and their dependencies are vulnerable and the call stack is very useful when auditing.

Another thing to note here is that it can have more than one CodeFlows so it could potentially also have the path from the vulnerable usage of a symbol within a dependency to the go.mod file. Though which option is more suitable here is up to interpretation.

is SARIF ubiquitous enough to be a blessed format, in which case adding it as an output directly to govulncheck makes sense,

I hope the answer is yes from my quick googling. (gitlab, sonar, datadog, vscode, jetbrains qodana, etc and i see various libraries). I guess it is more likely for enterprises to go with something with published spec for their filtering integration instead of developing a tool that only works with govulncheck's json format.

Producer-wise, staticcheck seems to have unofficial(?) support in progress, and golangci-lint has an open issue (not sure if that will ever implemented). Products like snyk, checkmarx supports sarif.

Google's osv scanner also reports sarif.

Wait, I think google/osv-scanner integrates govulncheck internally. Can users use google/osv-scanner instead? Do they have github action?

but if it is just one among many, then I would rather keep it in a separate tool.

In this case, the question is whether it should be part of the official go project.

It looks like currently rust is leaning towards depending on community-maintained solutions for the github action or the sarif conversion. (https://github.com/rust-lang/rust-clippy issue 8122)

I did start work on SARIF support in Staticcheck but eventually got side-tracked. My two cents: it's the only format that's somewhat widely supported, and it allows some nice integrations with GitHub.

The spec is big, but you only need a tiny subset of it for something like Staticcheck, and even less for govulncheck. I don't think there's much value in depending on a third party dependency for generating SARIF, as it's primarily structs, constants, and json tags. Outputting SARIF isn't intrinsically complex, once you've found the parts of the spec that apply to you.

Consumers of SARIF are rather hit and miss in what parts of SARIF they support and how well, which can make it difficult to rely on less common SARIF features to expose your information. This also makes it a "fun" experience to debug your SARIF output, as for example the VSCode extension to look at SARIF can disagree with GitHub's interpretation of it.

@ianthehat @zpavlinovic

govulncheck -json -> vulnignore -> govulncheck -sarif -mode=convert

It depends on how Sarif format will be used. If it's decided to associate findings on go.mod file require lines, the second govulncheck -sarif -mode=convert may need to read/process the go.mod source file so it may be beyond a pure data conversion unless govulncheck -json carries the necessary info.

These details about exactly how to implement the conversion can be handled outside the proposal process.

It looks like the scope of the proposal needs to be updated. It is no longer about github-action.

• should the transformation be done outside vs inside the existing tool? (Equivalent to api change)
  -> Debatable.
• if done outside, should the new conversion tool be part of the go project? (Long term maintainability)
  -> SARIF is pretty universal. Future requests like x/vuln: Add option to govulncheck to output VEX statement #62486 can benefit from this work. Maintenance of SARIF format conversion doesn't seem to take a lot of efforts at the moment.

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

Change https://go.dev/cl/549376 mentions this issue: internal/scan: add sarif flag

Change https://go.dev/cl/549815 mentions this issue: internal/sarif: add rules

Change https://go.dev/cl/549775 mentions this issue: internal/sarif: add handler

Change https://go.dev/cl/550015 mentions this issue: internal/sarif: add result message

Change https://go.dev/cl/549995 mentions this issue: internal/sarif: add result stubs to run object

Change https://go.dev/cl/567455 mentions this issue: internal/govulncheck: add scan mode to config

Change https://go.dev/cl/550736 mentions this issue: cmd/govulncheck: add sarif test for binaries

Change https://go.dev/cl/550735 mentions this issue: internal/sarif: add stacks

Change https://go.dev/cl/551375 mentions this issue: internal/sarif: add code flows

Change https://go.dev/cl/552955 mentions this issue: internal/sarif: add region part of the physical location

Change https://go.dev/cl/558016 mentions this issue: internal/sarif: compute relative paths for findings

Change https://go.dev/cl/558017 mentions this issue: internal/sarif: add missing required Message field

Change https://go.dev/cl/580955 mentions this issue: internal/sarif,cmd/govulncheck: publicize sarif

Change https://go.dev/cl/581076 mentions this issue: internal/sarif: remove originalURIBaseIds