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/internal/modfetch: treat malformed versions as “not found” on the proxy path #32955

Open
zx2c4 opened this issue Jul 5, 2019 · 25 comments

Comments

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Jul 5, 2019

For Go 1.11 and 1.12, I have in my go.mod something like:

replace (
        github.com/lxn/walk => golang.zx2c4.com/wireguard/windows pkg/walk
        github.com/lxn/win => golang.zx2c4.com/wireguard/windows pkg/walk-win
)

I use this to indicate that usage of github.com/lxn/walk should be replaced with the latest contents of a branch called pkg/walk from golang.zx2c4.com/wireguard/windows.

With 1.13beta1, this now breaks with a message:

/home/zx2c4/Projects/wireguard-windows/go.mod:16: replace golang.zx2c4.com/wireguard/windows: version "pkg/walk" invalid: version "pkg/walk" invalid: disallowed version string
/home/zx2c4/Projects/wireguard-windows/go.mod:17: replace golang.zx2c4.com/wireguard/windows: version "pkg/walk-win" invalid: version "pkg/walk-win" invalid: disallowed version string

This poses a significant problem.

@oiooj oiooj added the modules label Jul 5, 2019
@cespare

This comment has been minimized.

Copy link
Contributor

@cespare cespare commented Jul 5, 2019

/cc @bcmills

@ALTree ALTree changed the title modules in 1.13beta1 break backwards compatibility with branch name specifier cmd/go: modules in 1.13beta1 break backwards compatibility with branch name specifier Jul 6, 2019
@ALTree ALTree added this to the Go1.13 milestone Jul 6, 2019
@zx2c4

This comment has been minimized.

Copy link
Contributor Author

@zx2c4 zx2c4 commented Jul 8, 2019

Should this have the release-blocker tag? It's certainly a regression from 1.11 and 1.12.

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 8, 2019

@bcmills @jayconrod I think this is a side-effect of changing GOPROXY.

$ GOOS=windows GOPROXY=direct GONOSUM=.* go1.13beta get golang.zx2c4.com/wireguard/windows@pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk

The problem can be reproducible with go1.12 if GOPROXY is set.

GOOS=windows GOPROXY=https://proxy.golang.org GONOSUM=.* go get golang.zx2c4.com/wireguard/windows@pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk
go: finding golang.zx2c4.com/wireguard pkg/walk
go: finding golang.zx2c4.com pkg/walk
go get golang.zx2c4.com/wireguard/windows@pkg/walk: disallowed version string "pkg/walk"
@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 8, 2019

Such version strings are not accepted by golang.org/x/mod/module package that is used by both go command and proxy.golang.org as well. The module proxy protocol doc (go help goproxy) does not specify an acceptable set of version strings.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question? (That probably determines whether this gets the release-blocker label.)

@jayconrod

This comment has been minimized.

Copy link
Contributor

@jayconrod jayconrod commented Jul 8, 2019

This might be a limitation in the proxy, rather than the Go command.

With go1.13beta1, the command below succeeds:

$ GOPROXY=direct go mod download -json golang.zx2c4.com/wireguard/windows@pkg/walk

However, it fails with GOPROXY=https://proxy.golang.org/. Specifically, this query fails:

$ curl https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info
bad request: invalid escaped version "pkg/walk": invalid char '/'

When the proxy is enabled, this is the first request the Go command will send in order to find out the canonical version for pkg/walk.

@cuonglm

This comment has been minimized.

Copy link
Contributor

@cuonglm cuonglm commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question? (That probably determines whether this gets the release-blocker label.)

Set GOPROXY=direct works for me:

cuonglm :: /tmp/test » GOPROXY=direct go mod tidy
golang.zx2c4.com/wireguard/windows pkg/walk
go: finding golang.zx2c4.com/wireguard/windows pkg/walk
golang.zx2c4.com/wireguard/windows pkg/walk-win
go: finding golang.zx2c4.com/wireguard/windows pkg/walk-win
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills bcmills modified the milestones: Go1.14, Go1.13 Jul 8, 2019
@bcmills bcmills changed the title cmd/go: modules in 1.13beta1 break backwards compatibility with branch name specifier proxy.golang.org: proxy rejects branch names that successfully resolve in 'direct' mode Jul 8, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 8, 2019

@hyangah

Such version strings are not accepted by golang.org/x/mod/module package

The general solution here may be for the proxy to treat any (non-pseudo-) version that fails module.Check as a potential branch name.

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 8, 2019

@bcmills @jayconrod

Note that the same code is being used in the go command.
The golang.org/x/mod/module package is a copy of src/cmd/go/internal/module package. The error message users observed is probably before hitting the network. (src/cmd/go/internal/modfetch/proxy.go)

@rsc

@heschik

This comment has been minimized.

Copy link
Contributor

@heschik heschik commented Jul 8, 2019

It wasn't clear to me, so for the record: the problem is that the module.Escape/UnescapeVersion functions, which are only used on the proxy code paths in the go command, don't allow slashes. So, yes, proxy.golang.org doesn't support them, but more importantly, the go command never even gets that far because it can't encode the request properly.

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

@zx2c4 zx2c4 commented Jul 8, 2019

@zx2c4, can you confirm that setting GOPROXY=direct continues to allow the branch-names in question?

Confirmed.

@bcmills bcmills changed the title proxy.golang.org: proxy rejects branch names that successfully resolve in 'direct' mode cmd/go/internal/modfetch: proxy path rejects branch names that successfully resolve in 'direct' mode Jul 8, 2019
@bcmills bcmills modified the milestones: Go1.14, Go1.13 Jul 8, 2019
@andybons andybons modified the milestones: Go1.13, Go1.14 Jul 8, 2019
@zx2c4

This comment has been minimized.

Copy link
Contributor Author

@zx2c4 zx2c4 commented Jul 9, 2019

Looking at the above, there appears to be some kind of bizarre tug-a-war with the milestone tags. I assume this is 1.13 material, considering this is some form of regression and we're in the beta period where we're supposed to be investigating those.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

@zx2c4, @andybons is in the process of re-milestoning all of the non-release-blocking Go1.13 issues and this probably got swept up in a search. Probably this should be release-blocking, and then it won't get caught in that query. 🙂

@bcmills bcmills modified the milestones: Go1.14, Go1.13 Jul 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

Thinking about this some more: even if the proxy path in the go command is fundamentally broken, and even if the fix for that bug is not small enough for Go 1.13, there is a simple fix within the proxy itself: specifically, to serve a 404 or 410 (“I can't help you with that.”) instead of a 400 (“Nobody can help you with that.”)

@bcmills bcmills changed the title cmd/go/internal/modfetch: proxy path rejects branch names that successfully resolve in 'direct' mode proxy.golang.org: proxy serves status 400 for branch names that successfully resolve in 'direct' mode Jul 9, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 9, 2019

Retitling to accurately reflect the immediate fix. Once that's addressed, we can look in greater depth at the go command's proxy path in general (probably for 1.14, but I'll see if @jayconrod or @rsc have any ideas that we could implement in the shorter term).

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 9, 2019

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 10, 2019

Ah, right. Got it. Sorry for my confusion!

@bcmills bcmills changed the title proxy.golang.org: proxy serves status 400 for branch names that successfully resolve in 'direct' mode cmd/go/internal/modfetch: treat malformed versions as “not found” on the proxy path Jul 10, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 10, 2019

This has an interesting secondary interaction with proxy fallback. Since we can't ask the proxy about the requested version string at all, we don't know whether the proxy would have rejected that version, so we can't safely proceed to direct in the fallback list (#31913 (comment)).

@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jul 10, 2019

Change https://golang.org/cl/185598 mentions this issue: cmd/go/internal/modfetch: treat invalid version strings as "not found" in (*proxyRepo).Stat

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 10, 2019

The fix is complicated and the workaround of an explicit GOPROXY=direct continues to work, so unlabeling as release-blocker.

@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 12, 2019

Thanks @bcmills.
I hoped to see the module package and the protocol accept wider variety of version strings rather than workaround. The vcs branch, tag names may have a wider range of characters than the encoding/decoding functions in module package would assume.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 12, 2019

I'm hoping we can widen the accepted strings too, but I think any change to the escaping logic is probably too subtle to make 1.13.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Jul 16, 2019

I agree that changes here are too subtle for Go 1.13.
For Go 1.14 the first question is "should we make direct mode and proxy mode match as far as what they accept for versions" and the second is "if so, how? restrict direct mode or loosen proxy mode?"

Are slashes in git branch names very common?

@zx2c4

This comment has been minimized.

Copy link
Contributor Author

@zx2c4 zx2c4 commented Jul 16, 2019

Are slashes in git branch names very common?

Yes, extremely common. For example, some projects use it to denote the owner of the branch jd/something. Others use it for particular version partions. One of the most common ways of serving git repos to groups is a piece of software called gitolite, perhaps you've heard of it. One of its key features is that it can give granular permissions to branches based on a / hierarchy. This in turn matches the actual way git stores tags: in a directory hierarchy, with / being the directory separator.

@bcmills bcmills modified the milestones: Go1.13, Go1.14 Jul 16, 2019
@hyangah

This comment has been minimized.

Copy link
Contributor

@hyangah hyangah commented Jul 31, 2019

fyi - proxy.golang.org now serves 404/410 for this

$ curl -i  https://proxy.golang.org/golang.zx2c4.com/wireguard/windows/@v/pkg/walk.info
HTTP/1.1 410 Gone
Content-Type: text/plain; charset=UTF-8
...
bad request: invalid escaped version "pkg/walk": invalid char '/'
pchampio added a commit to go-flutter-desktop/hover that referenced this issue Sep 26, 2019
Go 1.13 introduced a proxy for go modules versioning.
This proxy doesn't allow to `go get` package that have '/' in the
version string.

eg `go get github.com/go-flutter-desktop/go-flutter@v0.30.0` works.
But `go get get github.com/go-flutter-desktop/go-flutter@feature/test`
wont because of the '/' after the '@'.

The issue is reported in golang/go#32955
A fix is to by-pass the PROXY by using the `GOPROXY=direct` venv.

Go 1.13 also doesn't fetch '@latest' when the tag is already valid.
This commit sets '@latest' to the version string when needed (hover upgrade)
pchampio added a commit to go-flutter-desktop/hover that referenced this issue Sep 26, 2019
Go 1.13 introduced a proxy for go modules versioning.
This proxy doesn't allow to `go get` package that have '/' in the
version string.

eg `go get github.com/go-flutter-desktop/go-flutter@v0.30.0` works.
But `go get get github.com/go-flutter-desktop/go-flutter@feature/test`
wont because of the '/' after the '@'.

The issue is reported in golang/go#32955
A fix is to by-pass the PROXY by using the `GOPROXY=direct` venv.

Go 1.13 also doesn't fetch '@latest' when the tag is already valid.
This commit sets '@latest' to the version string when needed (hover upgrade)
@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.