An entry in go.sum does not represent a dependency

[08d2]

I have just received Dependabot alerts based on entries in a go.sum file in my repository. But an entry in go.sum does not represent a dependency of my project, it represents a module that the Go toolchain had to verify when computing the dependency graph.

[jeffwidman]

Is this a security alert or a PR bumping an outdated package? I think this repo is just for the logic for bumping outdated deps and issues with security alerts are supposed to be reported elsewhere (I have no idea where unfortunately).

That said, we hit the problem with go.sum files listing transitive dependencies which were flagged by the Dependabot security scanner at my day job. We were able to work around it by pruning out the transitive deps from go.sum using go mod tidy -compat=1.17.

As long as you're using 1.17, that is a fast fix for the spurious security alerts. Unfortunately it'll break all your Dependabot PR's because they'll reset go.sum every time so you'll have to run go mod tidy -compat=1.17 on each of them before merging.

Additionally, the -compat=1.17 flag will likely break on go < 1.17. Although it looks like this will be the new default behavior based on how the 1.18 draft release notes are worded... I'm very curious to see how that ends up working with 1.16 code.

[jurre]

Yeah @jeffwidman is right that the alerting logic is not based on the code in dependabot-core here, but I will make sure to point the team that works on alerts at this issue though.

[adonovan]

@08d2, you are correct that that go.mod file does not represent a dependency graph, and that it includes modules that were downloaded only to compute the dependency graph. Specifically, the go.sum file contains two kinds of entries: regular entries, which indicate that the .zip file of the module is needed during the build, and /go.mod-suffixed entries, which indicate that a given module contributes only version constraints to the build. In order to compute an approximation of actual dependencies, GitHub's tools such as Dependabot ignore the second kind of edge, since code in those modules cannot affect the behavior of your program, but all other modules are downloaded during the build and are thus considered potentially relevant to the behavior of your program.

You might argue that the module dependency graph is a crude overapproximation of the set of packages used in the build, and that it gives rise to infeasible dependency paths---and you would be right. However, a package-level dependency analysis would require a totally different approach---and it too would produce infeasible paths at function granularity.

It would be an interesting project to build a Go-specific tool that computed the set of vulnerable dependencies using the finest available dependency information, perhaps based on a call graph. [Update: govulncheck now exists.]

[08d2]

@jeffwidman

the problem with go.sum files listing transitive dependencies which were flagged by the Dependabot security scanner at my day job.

It's important to be clear, here:go.sum is a checksum file, not a lockfile, not a manifest file. It does not list or represent dependencies at all. A tool like Dependabot should completely ignore go.sum. The dependency graph of a Go module is better captured by the output of go list commands.

[08d2]

Is this a security alert or a PR bumping an outdated package?

The alerts I saw were part of github.com/org/repo/security/dependabot

[adonovan]

The dependency graph of a Go module is better captured by the output of go list commands.

You're absolutely right, but the architecture of GitHub's dependency computation system assumes that the graph is expressed in a small number of manifest files, so running go list on the actual Go source is out of the question.

[08d2]

@adonovan

GitHub's dependency computation system assumes that the graph is expressed in a small number of manifest files, so running go list on the actual Go source is out of the question.

So, concretely, GitHub's (apparently opt-out) Dependabot feature created false-positive "security alerts" in my repo, based on some entries in a go.sum file file in a subdirectory somewhere. I think these false positive alerts are really bad and need to be fixed! 😬 And I think it's bad enough that if the only way to fix them is to disable Dependabot for Go altogether, that's preferable to the current state.

[adonovan]

I think these false positive alerts are really bad

I agree that false positives are frustrating, especially if they cannot be easily dismissed.

There are several ways we could resolve this:

1. you disable Dependabot Alerts for your repo.
2. we extend the Dependabot Alerts UI to provide a means for users to select which manifest files they care about.
3. we remove go.sum support from GitHub's dependency computation system (but leave go.mod).

Which approach is best depends on the answer to these questions:

• Do many users find the level of false positives from go.sum files to be troublesome?
• Is the precision of go.sum significantly worse than comparable transitively closed manifests in other ecosystems?

I don't know the answer to either question, but perhaps our product folks do.

[Update: I'm talking about Dependabot Alerts, not the more sophisticated Dependabot core. Thanks @jurre for clarifying.]

[jeffwidman]

IIUC, running go mod tidy under go >= 1.17 creates a go.mod file that lists all the deps required to build the binary, both direct / indirect: https://go.dev/doc/go1.17#go-command

So in that case, it probably makes sense to disable the scanning of the go.sum file when the go.mod file has the line directive go 1.17 (or greater) in it.

[08d2]

There are several ways we could resolve this:

1. you disable Dependabot for your repo.
2. we extend the Dependabot UI to provide a means for users to select which manifest files they care about.
3. we remove go.sum support from GitHub's dependency computation system (but leave go.mod).

This is confusing to me. As far as I understand, go.sum is, factually, not a manifest file — it has at best a correlative relationship to its module's dependency graph. Is that not true?

[08d2]

@jeffwidman

IIUC, running go mod tidy under go >= 1.17 creates a go.mod file that lists all the deps required to build the binary, both direct / indirect:

IIUC go.mod file always defines the deps required to build the module, invariant of version.

[jeffwidman]

Yes, but previously it didn't necessarily list all the // indirect deps which were getting pulled in transitively into the final binary.

So previously a security scanner couldn't merely scan what was listed in in go.mod, it had to walk the dep tree. And to do that, it'd have to pull up each modules go.mod file. However, those go.mod files may list other packages that are required by the direct dep, but not actually used by the program being built, for example deps used only for tests of an intermediary in the chain.

Stepping back, from the POV of a security scanner, walking the dep tree often simply wasn't feasible, especially since they'd still struggle to identify which indirect deps made it into the final binary and which were only used by intermediate modules. That's why the scanners used go.sum as a rough proxy instead, since it's a superset of the required dep tree, and about what they'd get anyway if they were to build the tree themselves.

Now with 1.17, those indirect deps that that would be discovered by walking the dep tree, but aren't actually used in the final binary, get pruned out of the list in go.mod. And then the remaining // indirect deps are listed in go.mod (previously they were optional).

So now go.mod can be treated as a definitive source of "here's all the packages that are required in the final binary, w/o having to walk the dep tree".

So this makes the job of a security scanner much easier, since they can directly use go.mod since it will list the indirect deps if-and-only-if they're actually included in the final binary. So it should reduce their workload (no need to scan the extraneous stuff in go.sum), and reduce false positives (stuff listed is guaranteed to be in the final binary).

I should caveat this with the disclaimer that I'm not part of the go team, and this stuff is complicated, ie, I could be mistaken. The best resource I've found was the design doc for the module graph pruning feature that @bcmills put together here: golang/go#36460

[jurre]

Going into possible solution mode here.

So it's important to note that there are different systems in play here, that have different properties:

1. Dependency Graph: Computes a list of dependencies that a repo depends on, by analyzing files in the repo
2. Dependabot Alerts: Takes the list of dependencies from the Dependency Graph and compares them to our advisory db to generate alerts
3. dependabot-core / Dependabot Updates: Gets the alert and advisory data from the dependabot alerts system and attempts to generate a PR that fixes the issue. (There are some systems that orchestrate running dependabot-core, but it's a close enough proxy for what I'm trying to say next)

1 and 2 can't reasonably do any complicated computation given our current architecture, but for 3 we run complicated computation all the time. We actually already run go list as part of our workflow and would bail out trying to generate a PR here when we can't find the given dependency.

We could look into closing the alert when Dependabot Updates attempts to bump the alert that was generated for this dependency but the dependency doesn't appear in the go list command, assuming it was either removed since the alert was generated, or is not actually used in the project at hand.

This still leaves users with some noise to deal with, as they'd get notified about the alert being opened etc, but it might end up being a better experience?

There's a risk here that if we somehow fail to detect the dependency in dependabot-core/dependabot updates, but it does actually exist in the repo, we now close out a valid alert, so we'll need to be careful of we decide to go this route.

---

Just wanted to amplify that the problem we're describing here is not with dependabot-core. I'll keep the thread open here because the conversation is really interesting and I don't want to kill it by asking everyone to move to a different place.