Go: cmd/go/internal/modfetch: treat malformed versions as “not found” on the proxy path

[zx2c4]

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.

[cespare]

/cc @bcmills

[zx2c4]

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

[hyangah]

@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]

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]

@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 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]

@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

[bcmills]

@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]

@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]

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]

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

Confirmed.

[zx2c4]

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]

@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]

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]

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]

Note: the go command failed before sending a request to proxy.

On Tue, Jul 9, 2019, 6:34 PM Bryan C. Mills [e-mail]
wrote:

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.”)

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/golang/go/issues/32955?email_source=notifications&email_token=ABGESL2F6BHDGULN464FMT3P6UG5RA5CNFSM4H6KJO22YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODZRXJKA#issuecomment-509834408,
or mute the thread
https://github.com/notifications/unsubscribe-auth/ABGESL3VR3Z3GCVOMDJ27HTP6UG5RANCNFSM4H6KJO2Q
.

[bcmills]

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

[bcmills]

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 (https://github.com/golang/go/issues/31913#issuecomment-490507980).

[gopherbot]

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

[bcmills]

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

[hyangah]

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]

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]

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]

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.

[hyangah]

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 '/'

[utrack]

This bug also affects module imports from a subdirectory of another module.

Here's an example tree for a repo:

github.com/foo/bar
├── pkg
│       └── baz
│           └── go.mod
└── go.mod

If I were to import package github.com/foo/bar/pkg/baz then go would try to fetch tag pkg/baz/v1.0.0 automatically (that would fail).

What're the next steps for that one - do we want to fix go or proxy (athens)?

[bcmills]

@utrack, the versions in the proxy protocol are not the same as the tags in the underlying repo. The tag for the module in the pkg/baz subdirectory is indeed pkg/baz/v1.0.0, but once the module is extracted into the module cache that version is encoded as simply v1.0.0 (because the pkg/baz portion is already encoded in the module cache).

[hyangah]

@utrack try the -x flag to see how the go command works with the proxy. That will show the behavior @bcmills described above. go get github.com/foo/bar/pkg/[email protected] or go get github.com/foo/bar/pkg/baz will work as expected.

go get github.com/foo/bar/pkg/baz@pkg/baz/v1.0.0 won't due to this issue but not sure how often ppl want to fetch a module/package in that way.

[4F2E4A2E]

Using the hash of the branch git rev-parse did it for me, see the comment: https://github.com/golang/go/issues/34175#issuecomment-529502530