Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/go: use go.sum hashes from all downloaded modules #28802

Closed
bcmills opened this issue Nov 14, 2018 · 8 comments
Closed

cmd/go: use go.sum hashes from all downloaded modules #28802

bcmills opened this issue Nov 14, 2018 · 8 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 14, 2018

This proposal is an alternative to #24117 (CC: @rsc @FiloSottile).

Rather than maintaining a global go.sum file, I propose to merge in the go.sum files from all modules that provide packages in the transitive import graph of the main module.

Specifically:

  • Whenever we load the source code from a module, we should also load the contents of its go.sum file.
  • If the new go.sum file contains a conflicting checksum for some other entity (a module or go.sum file) loaded in the course of the same go command invocation, the build should fail.
  • Conflicts for entities that are not loaded in the course of the same go command invocation should not cause the build to fail: we have no way to report which of the conflicting checksums (if any) is correct, and the fact that the conflict exists is (by the previous rules) immaterial to the actual operation requested by the user.

Here is my rationale:

go.sum files may contain errors. For example, they may contain an erroneous checksum due to a bug in the go command (such as #26794 or #27868), or due to memory or disk corruption (of the go.sum file itself or on the machine of the user who created the go.sum file).

To allow such errors to be corrected over time, it is important that we ignore the contents of go.sum files for modules (and versions of modules) that do not contribute meaningfully to the build: if we load the go.mod file for some version of a module but end up using a newer version instead (or not importing any packages from it at all!), then we should ignore errors in the rest of its code, including its go.sum file.

On the other hand, if we build code from a module using some version of a dependency known to that module, it is important that we use the same version it was (or may have been) tested against. This is especially important for builds performed outside of any module (#24250): in that case there is no root go.sum file that we can check against, and we may be running in a completely clean image (such as in a CI system) and not have anything else to bootstrap against.

In contrast to a “global” go.sum file (#24117), this approach makes checksum failures reproducible: if someone reports an invalid checksum for some module, they can also determine exactly where that checksum came from so that others can investigate and, ultimately, fix the erroneous checksum without specific user intervention.

@bcmills bcmills added Proposal NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. GoCommand cmd/go modules labels Nov 14, 2018
@bcmills bcmills added this to the Go1.13 milestone Nov 14, 2018
@tv42
Copy link

tv42 commented Dec 3, 2018

I like this; checking more just isn't likely to hurt (apart from more CPU & IO burned).

I'm not sure it's really in any way in conflict with a "global" (per-user) go.sum, though. The global go.sum's job is to make the TOFU security property sticky to the user, not to the project. For example, if you start a new project and go get something, a global go.sum would have sums for any modules you've used in other projects previously (even if the actual source had been cleaned from the cache).

Then again, TOFU on this level doesn't prevent a new version from being malware, so the extra assurance is pretty small...

@rsc
Copy link
Contributor

rsc commented Dec 12, 2018

Let's talk at some point about this. I fear it will be abused and would rather establish a good global reference.

@bcmills
Copy link
Contributor Author

bcmills commented Dec 13, 2018

Per in-person discussion this morning, this has a clear benefit when operating outside of any main module, but it's much less obvious inside a module (where there is generally already a single, authoritative go.mod file).

For 1.13, I'll send a CL to enable it only in the former case, and we can consider expanding it depending on how that goes.

@rsc
Copy link
Contributor

rsc commented Jan 9, 2019

Tentatively accepted per discussion.

@rsc rsc changed the title proposal: cmd/go: use go.sum hashes from all downloaded modules cmd/go: use go.sum hashes from all downloaded modules Jan 9, 2019
@bcmills bcmills added early-in-cycle A change that should be done early in the 3 month dev cycle. NeedsFix The path to resolution is known, but the work has not been done. labels Jan 18, 2019
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Jan 18, 2019
@bcmills bcmills self-assigned this Feb 5, 2019
@bcmills bcmills modified the milestones: Go1.13, Go1.14 Apr 19, 2019
@rsc
Copy link
Contributor

rsc commented Apr 30, 2019

@bcmills and I talked about this a bit more at some point after my last message. As proposed, this would make it impossible to override a bad go.sum in a dependency. I'm a bit uncomfortable with that, especially now that we have the checksum database available to address many of the same issues. I think we should probably say the checksum database is enough and not do this recursive check.

@bcmills, what do you think?

@bcmills
Copy link
Contributor Author

bcmills commented Apr 30, 2019

I agree; given the checksum database and our experience with the difficulty of fixing other transitive module errors (such as path mismatches), I don't think the risk of hard-to-repair errors is worth the marginal-at-best gain in security.

@bcmills bcmills closed this as completed Apr 30, 2019
@manikesh
Copy link

Does anything change with go.sum file as everything runs well in my local windows machine but the moment same go.sum and go.mod files are committed and being build using jenkins, we get checksum error, anything specific to get it fixed? we are using go 1.11 and on mod

@bcmills
Copy link
Contributor Author

bcmills commented May 14, 2019

@manikesh, see #29278.

@golang golang locked and limited conversation to collaborators May 13, 2020
@rsc rsc unassigned bcmills Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge GoCommand cmd/go modules NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

No branches or pull requests

5 participants