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: do not treat branches with semantic-version names as releases #35671

Closed
bcmills opened this issue Nov 18, 2019 · 14 comments
Closed

cmd/go: do not treat branches with semantic-version names as releases #35671

bcmills opened this issue Nov 18, 2019 · 14 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Security
Milestone

Comments

@bcmills
Copy link
Contributor

bcmills commented Nov 18, 2019

#33558 (comment) reports a situation in which a v2.0.0 branch was created and subsequently removed, but proxy.golang.org cached a v2.0.0+incompatible release.

If someone creates a branch named v2.0.0, they probably intend for that branch to contain the development leading up to v2.0.0, not for that branch as first published to be the definitive final release of v2.0.0. The go command should not mistake the former for the latter.

CC @jayconrod @mattn @thepudds

@bcmills bcmills added NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. modules labels Nov 18, 2019
@bcmills bcmills added this to the Go1.15 milestone Nov 18, 2019
@bcmills
Copy link
Contributor Author

bcmills commented Nov 18, 2019

See #29731, which also involves confusion between branches and versions.

@hajimehoshi
Copy link
Member

Any updates on this? I think this is a very scary issue, since if a collaborator created a vN branch deliberately or not once, the repository's versioning would be corrupted forever...

@jayconrod jayconrod modified the milestones: Go1.16, Go1.17 Jan 7, 2021
@ianlancetaylor
Copy link
Contributor

Is this going to happen for 1.17? Thanks.

@bcmills bcmills modified the milestones: Go1.17, Backlog Apr 22, 2021
@bcmills bcmills self-assigned this Dec 1, 2021
@bcmills bcmills modified the milestones: Backlog, Go1.18 Dec 1, 2021
@bcmills bcmills added NeedsFix The path to resolution is known, but the work has not been done. and removed help wanted labels Dec 1, 2021
@gopherbot gopherbot removed the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Dec 1, 2021
@gopherbot
Copy link

Change https://golang.org/cl/378399 mentions this issue: cmd/go/internal/modfetch: add a reproducer for #35671

@gopherbot
Copy link

Change https://golang.org/cl/378400 mentions this issue: cmd/go/internal/modfetch: do not short-circuit canonical versions

@FiloSottile
Copy link
Contributor

@gopherbot please open backport issues. Materializing versions from branches might be unexpected and bypass ACLs that limit the creation of tags but not branches. It's also better for the version list to be consistent across supported Go versions.

@gopherbot
Copy link

Backport issue(s) opened: #50686 (for 1.16), #50687 (for 1.17).

Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases.

@gopherbot
Copy link

Change https://golang.org/cl/382594 mentions this issue: vcs-test/README.md: warn about Google Cloud Storage default caching

gopherbot pushed a commit to golang/build that referenced this issue Feb 3, 2022
I don't know when the default Cache-Control policy was added, but it
makes it very difficult to fix mistakes in zipfiles uploaded to
vcs-test. Fortunately, the docs for 'gsutil setmeta' give a recipe for
how to fix it, if the user remembers to do so.

For golang/go#35671

Change-Id: I86d365dfae127f3a5230ef4a3c772b17db261096
Reviewed-on: https://go-review.googlesource.com/c/build/+/382594
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/382835 mentions this issue: [release-branch.go1.17] cmd/go/internal/modfetch: do not short-circuit canonical versions

@gopherbot
Copy link

Change https://golang.org/cl/382839 mentions this issue: [release-branch.go1.16] cmd/go/internal/modfetch: do not short-circuit canonical versions

gopherbot pushed a commit that referenced this issue Feb 7, 2022
…t canonical versions

Since at least CL 121857, the conversion logic in
(*modfetch).codeRepo.Stat has had a short-circuit to use the version
requested by the caller if it successfully resolves and is already
canonical.

However, we should not use that version if it refers to a branch
instead of a tag, because branches (unlike tags) usually do not refer
to a single, stable release: a branch named "v1.0.0" may be for the
development of the v1.0.0 release, or for the development of patches
based on v1.0.0, but only one commit (perhaps at the end of that
branch — but possibly not even written yet!) can be that specific
version.

We already have some logic to prefer tags that are semver-equivalent
to the version requested by the caller. That more general case
suffices for exact equality too — so we can eliminate the
special-case, fixing the bug and (happily!) also somewhat simplifying
the code.

Updates #35671
Fixes #50687
Fixes CVE-2022-23773

Change-Id: I2fd290190b8a99a580deec7e26d15659b58a50b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/378400
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fa4d9b8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/382835
gopherbot pushed a commit that referenced this issue Feb 7, 2022
…t canonical versions

Since at least CL 121857, the conversion logic in
(*modfetch).codeRepo.Stat has had a short-circuit to use the version
requested by the caller if it successfully resolves and is already
canonical.

However, we should not use that version if it refers to a branch
instead of a tag, because branches (unlike tags) usually do not refer
to a single, stable release: a branch named "v1.0.0" may be for the
development of the v1.0.0 release, or for the development of patches
based on v1.0.0, but only one commit (perhaps at the end of that
branch — but possibly not even written yet!) can be that specific
version.

We already have some logic to prefer tags that are semver-equivalent
to the version requested by the caller. That more general case
suffices for exact equality too — so we can eliminate the
special-case, fixing the bug and (happily!) also somewhat simplifying
the code.

Updates #35671
Fixes #50686
Fixes CVE-2022-23773

Change-Id: I2fd290190b8a99a580deec7e26d15659b58a50b0
Reviewed-on: https://go-review.googlesource.com/c/go/+/378400
Trust: Bryan Mills <bcmills@google.com>
Run-TryBot: Bryan Mills <bcmills@google.com>
Reviewed-by: Russ Cox <rsc@golang.org>
TryBot-Result: Gopher Robot <gobot@golang.org>
(cherry picked from commit fa4d9b8)
Reviewed-on: https://go-review.googlesource.com/c/go/+/382839
@thepudds
Copy link
Contributor

thepudds commented Feb 14, 2022

Is it expected that your local modules cache and proxy.golang.org continue to serve these types of 'bad' versions? My guess is yes, including to support reproducible builds, but wanted to confirm.

One related comment is it looks like rsc.io/quote might have deliberately tried to introduce this problem late last year, or at least it currently seems to have a v4.0.0 branch without a v4.0.0 tag, and proxy.golang.org serves v4.0.0 as a version:

https://proxy.golang.org/rsc.io/quote/v4/@v/list

It also seems go1.17.7 will be OK with these types of versions if your local module cache has been polluted:

export GOMODCACHE=$(mktemp -d)
export GOPROXY=direct
cd $(mktemp -d)
go mod init temp

# go1.17.7 fails with the expected new error:
#     rsc.io/quote/v4@v4.0.0: invalid version: resolves to version v4.0.0-20211101134634-bc30624959ad (v4.0.0 is not a tag) 
go1.17.7 get rsc.io/quote/v4@v4.0.0

# go1.17.6 succeeds:
go1.17.6 get rsc.io/quote/v4@v4.0.0

rm go.mod
go mod init temp

# go1.17.7 now also succeeds in adding rsc.io/quote/v4@v4.0.0, presumably due to module cache:
go1.17.7 get rsc.io/quote/v4@v4.0.0

Again, probably expected, but wanted to check.

The new v4 also happens to now show up in one of the modules tutorials, which might mean a tweak to the tutorial (#51188) if the proxy will continue to serve that v4 version.

Finally, sorry if any of this is off base or if I've forgotten something obvious.

@bcmills
Copy link
Contributor Author

bcmills commented Feb 14, 2022

Is it expected that your local modules cache and proxy.golang.org continue to serve these types of 'bad' versions? My guess is yes, including to support reproducible builds, but wanted to confirm.

It also seems go1.17.7 will be OK with these types of versions if your local module cache has been polluted:

Yes: any version that was already in the wild is already in the wild anyway; we don't have a mechanism to unpublish invalidated versions from module caches or proxies, although of course the author can retract the affected version(s) — see mattn/go-sqlite3#998 for an example.

The new v4 also happens to now show up in one of the modules tutorials, which might mean a tweak to the tutorial (#51188) if the proxy will continue to serve that v4 version.

Yeah, we should probably fix that. Thanks for flagging it!

@jing-rui
Copy link

jing-rui commented Mar 8, 2022

Is there any workaround for old versions(go1.15)?

@bcmills
Copy link
Contributor Author

bcmills commented Mar 8, 2022

@jing-rui, per the Go project's release policy, only the two most recent Go releases receive backport fixes.

The only workarounds I'm aware of for older versions are to upgrade, or to configure your environment to use a GOPROXY that itself runs an upgraded or patched version of the go command.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Security
Projects
None yet
Development

No branches or pull requests

8 participants