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: validate module proxy URLs received from go-get=1 queries #32006

Open
maikelmclauflin opened this issue May 13, 2019 · 7 comments
Open
Labels
GoCommand cmd/go help wanted modules NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@maikelmclauflin
Copy link

maikelmclauflin commented May 13, 2019

What version of Go are you using (go version)?

Go tip at CL 170879 or later.

Does this issue reproduce with the latest release?

no

What operating system and processor architecture are you using (go env)?

go env Output
$ go env

GOARCH="amd64"
GOBIN=""
GOCACHE=SHOWS_CORRECTLY
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GOOS="darwin"
GOPATH=SHOWS_CORRECTLY
GOPROXY=""
GORACE=""
GOROOT="/usr/local/Cellar/go/1.12.2/libexec"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.12.2/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=SHOWS_CORRECTLY
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/j0/qxnj38ld075bj80p9hs2dqlw0000gn/T/go-build144819988=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

on travis, when running tests against master, modules are unable to be downloaded. i am unsure if this is because of travis or if it can be replicated elsewhere

What did you expect to see?

successful download of modules

What did you see instead?


go: github.com/golang-migrate/migrate/v4@v4.3.0 requires
	github.com/fsouza/fake-gcs-server@v1.5.0 requires
	cloud.google.com/go@v0.36.0 requires
	golang.org/x/build@v0.0.0-20190111050920-041ab4dc3f9d requires
	dmitri.shuralyov.com/app/changes@v0.0.0-20180602232624-0a106ad413e3: Get https:///dmitri.shuralyov.com/app/changes/@v/v0.0.0-20180602232624-0a106ad413e3.info: http: no Host in request URL
The command "go mod download" failed and exited with 1 during .

specifically, the 3x slashes (/) in the url. removing the slash resolves correctly

@andybons andybons changed the title url fails to download dep cmd/go: extra slash added to url causing invalid url of module dependency May 13, 2019
@andybons andybons added modules NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels May 13, 2019
@andybons andybons added this to the Go1.13 milestone May 13, 2019
@andybons
Copy link
Member

@jayconrod @bcmills

@bcmills
Copy link
Contributor

bcmills commented May 13, 2019

dmitri.shuralyov.com is currently serving very unusual go-import tags:

~/go/src$ curl -L dmitri.shuralyov.com/app/changes?go-get=1
<meta name="go-import" content="dmitri.shuralyov.com/app/changes git https://dmitri.shuralyov.com/app/changes">
<meta name="go-import" content="dmitri.shuralyov.com/app/changes mod https://">
<meta name="go-source" content="dmitri.shuralyov.com/app/changes https://dmitri.shuralyov.com/app/changes https://gotools.org/dmitri.shuralyov.com/app/changes https://gotools.org/dmitri.shuralyov.com/app/changes#{file}-L{line}">

Note the second line: it includes a bare https://, relying on the module proxy protocol to fill in the hostname. According to @dmitshur that used to work (in Go 1.11), but it no longer does and I suspect that that's a good thing.

@bcmills
Copy link
Contributor

bcmills commented May 13, 2019

Assigning to Dmitri to either update his server, or make a convincing argument that we should allow the bare https://. 🙂

@maikelmclauflin
Copy link
Author

maikelmclauflin commented May 13, 2019

🙂thanks @bcmills

@dmitshur
Copy link
Contributor

dmitshur commented May 13, 2019

I've edited the original issue to make it more visible that this affects Go tip only. Go 1.12 and Go 1.11 are not affected.

I think there are two problems, one in cmd/go and the other in the way dmitri.shuralyov.com/app/changes vanity import path is currently served:

  1. The go command can return a better error message when it encounters an invalid/unsupported module proxy URL. "https://" is a valid URL: it specifies the HTTPS scheme, but no host and no path; see https://play.golang.org/p/vmTPnZffjDf. But I don't think it's a valid module proxy URL. I think a module proxy URL must have a non-empty host.

  2. The dmitri.shuralyov.com/app/changes vanity import path currently uses "https://" as the module proxy URL. This used to work in Go 1.11 and 1.12, perhaps unintentionally (there weren't any test cases for it), and it no longer works in Go tip after CL 170879. I think the vanity import path should change to return a module proxy URL with a non-empty host, so that it'll continue to work in Go 1.13 and later.

    Edit on May 18, 2019: This has been fixed in commit shurcooL/home@17791dc, see cmd/go: validate module proxy URLs received from go-get=1 queries #32006 (comment).

Since it was only affecting tip and not currently released stable versions of Go, I was using the aforementioned vanity import path as an opportunity to understand the new Go tip behavior better and learn what the best way to handle such URLs would be.

I plan to fix the dmitri.shuralyov.com/app/changes vanity import path in the coming days, but please let me know if it not working in Go tip is a more disruptive problem than I'm aware of and I'll do it sooner.

Background

The module proxy protocol documentation did not specify very formally how the URL of the module proxy was joined with the rest of the path. It just said:

The GET requests sent to a Go module proxy are:

GET $GOPROXY/<module>/@v/list returns a list of all known versions of the given module, one per line.

The GET $GOPROXY/<module>/@v/list text doesn't make it clear what kind of valid URLs are permitted for $GOPROXY. "https://" happened to work in 1.11 and 1.12.

The advantage of that is that it allowed a vanity import path to serve a module at a URL without repetition. I.e., for a vanity import path example.com/foo, its @v/list endpoint could be served at "https://example.com/foo/@v/list" instead of "https://example.com/example.com/foo/@v/list" or "https://example.com/moduleproxy/example.com/foo/@v/list", which is more verbose.

However, after spending more time thinking about it and discussing with some people, I think the $GOPROXY URL needs to have a supported scheme and a non-empty host. I was not able to find a coherent and simple way that a URL with an empty host could be made to work.

Unfortunately, that means a module with a vanity import path being served by its own domain will have module proxy URLs that are more verbose due to repeating the host, but that seems to be a favorable trade-off to make. I'm happy to hear if anyone has more thoughts on this.

@dmitshur dmitshur changed the title cmd/go: extra slash added to url causing invalid url of module dependency cmd/go: returns confusing error for module specifying an invalid module proxy URL May 13, 2019
@bcmills bcmills added help wanted NeedsFix The path to resolution is known, but the work has not been done. labels May 13, 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 May 13, 2019
dmitshur added a commit to shurcooL/home that referenced this issue May 18, 2019
Previously, the URL of the module proxy used to serve modules beginning
with the path dmitri.shuralyov.com contained an empty host value.
This worked successfully in Go 1.11 and Go 1.12, leading to GET requests
without stuttering. For example, to fetch a list of versions in the
dmitri.shuralyov.com/state module, the request was:

	GET https://dmitri.shuralyov.com/state/@v/list

However, a module proxy URL with an empty host no longer works in Go tip
as of https://golang.org/cl/170879, and evidence suggests that is the
better behavior to continue to enforce.

There are plans to document the requirement of a module proxy URL to
have a non-empty host. It's also planned to improve the error reporting
when the go command encounters an invalid module proxy URLs, so users
will have a better idea of what went wrong. See issue golang/go#32006.

To be compatible with Go 1.11, 1.12, and the future Go 1.13, this commit
changes the module proxy URL used to serve own modules from "https://"
to "https://dmitri.shuralyov.com/api/module". Future module proxy
requests will look like:

	GET https://dmitri.shuralyov.com/api/module/dmitri.shuralyov.com/state/@v/list

The module proxy protocol is meant primarily for machines rather than
humans, so the longer path that stutters is an acceptable compromise.

A benefit of the new proxy URL is that we no longer need to check
if a request matches. Instead, all HTTP requests with a "/api/module/"
path prefix can be immediately recognized as requests to the module
proxy, and handled as such.

As part of embracing the machine-focused aspect of these URLs,
also remove the optional human-friendly behavior of redirecting
when the module path and version are not encoded in the request URL.
Instead, serve a 400 Bad Request error. A correct module proxy client
will not make the mistake of forgetting to encode those values.

Updates #24
Updates golang/go#32006
@dmitshur
Copy link
Contributor

dmitshur commented May 18, 2019

I've made a change to the module proxy URL used to serve modules beginning with the path dmitri.shuralyov.com in commit shurcooL/home@17791dc. The URL now contains a non-empty host:

$ curl -L 'https://dmitri.shuralyov.com/app/changes?go-get=1'
<meta name="go-import" content="dmitri.shuralyov.com/app/changes git https://dmitri.shuralyov.com/app/changes">
<meta name="go-import" content="dmitri.shuralyov.com/app/changes mod https://dmitri.shuralyov.com/api/module">
<meta name="go-source" content="dmitri.shuralyov.com/app/changes https://dmitri.shuralyov.com/app/changes https://gotools.org/dmitri.shuralyov.com/app/changes https://gotools.org/dmitri.shuralyov.com/app/changes#{file}-L{line}">

As a result, those modules can be fetched successfully with Go 1.11, 1.12, and tip.

To make it possible to continue to test this issue and make improvements to the error reporting done by the go command when it encounters an invalid module proxy URL, I've left a test module that still uses "https://" as the module proxy URL here:

https://dmitri.shuralyov.com/test/modtest2

Here's the current error message from latest Go tip (commit 1ab063c) when trying to fetch it (directly, without a local GOPROXY):

$ gotip version
go version devel +1ab063c Fri May 17 22:32:30 2019 +0000 darwin/amd64
$ export GO111MODULE=on
$ export GOPROXY=direct
$ export GOPATH=$(mktemp -d)
$ cd $(mktemp -d)
$ gotip mod init m
go: creating new go.mod: module m
$ gotip get dmitri.shuralyov.com/test/modtest2
go: finding dmitri.shuralyov.com/test/modtest2 v0.0.0
go: dmitri.shuralyov.com/test/modtest2@v0.0.0: Get https:///dmitri.shuralyov.com/test/modtest2/@v/v0.0.0.info: http: no Host in request URL

@jayconrod jayconrod changed the title cmd/go: returns confusing error for module specifying an invalid module proxy URL cmd/go: validate module proxy URLs received from go-get=1 queries May 30, 2019
@jayconrod jayconrod modified the milestones: Go1.13, Go1.14 May 30, 2019
@gopherbot
Copy link

Change https://golang.org/cl/191945 mentions this issue: cmd/go: validate module proxy URLs received from go get queries

@rsc rsc modified the milestones: Go1.14, Backlog Oct 9, 2019
dmitshur added a commit to shurcooL/home that referenced this issue Nov 18, 2019
This change adds a test module that is served over a module proxy
URL with an empty host (i.e., "https://"). It exists only for testing
purposes, since such module proxy URLs are not valid as of Go 1.13,
and all normal modules are being served with a module proxy URL that
has non-empty host as of commit 17791dc.

Updates #24
Updates golang/go#32006
@dmitshur dmitshur added the GoCommand cmd/go label Jul 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
GoCommand cmd/go help wanted modules NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

7 participants