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: go get ignores go.mod from remote repo, can escalate into a security vuln #38196

Closed
karalabe opened this issue Apr 1, 2020 · 14 comments
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Milestone

Comments

@karalabe
Copy link
Contributor

karalabe commented Apr 1, 2020

Happens with all Go versions ever supporting modules.


Repro: run outside of a Go module, I just want to install a Go binary:

go install -v github.com/ethereum/go-ethereum/cmd/geth

What happens:

go/src/github.com/ethereum/go-ethereum/metrics/influxdb/influxdb.go:10:2: cannot find package "github.com/influxdata/influxdb/client" in any of:
	/opt/google/go/src/github.com/influxdata/influxdb/client (from $GOROOT)
	/tmp/go/src/github.com/influxdata/influxdb/client (from $GOPATH)

Why? Because Go flat out ignores the module file distributed in that repo; and upstream nuked their repository to pieces so there's no code there any more.


Doing it manually works:

$ git clone https://github.com/ethereum/go-ethereum
$ cd go-ethereum
$ go install ./cmd/geth

This is apparently the same bug that was a release blocker in Go 1.12, and then just ignored and locked: #24250

@karalabe
Copy link
Contributor Author

karalabe commented Apr 1, 2020

Here's a way to escalate this bug into a security vulnerability: https://github.com/karalabe/go-issue-38196-keygen

@karalabe karalabe changed the title go get ignores module file from the remote repo go get ignores go.mod from remote repo, can escalate into a security vuln Apr 1, 2020
@seankhliao
Copy link
Member

I believe this is go in gopath mode working as it always has been

if you want to install in module mode GO111MODULE=on go get github.com/karalabe/go-issue-38196-keygen

related #30515

@ALTree
Copy link
Member

ALTree commented Apr 1, 2020

This is apparently the same bug that was a release blocker in Go 1.12, and then just ignored and locked: #24250

The issue you linked (#24250) was marked as a 1.12 release blocker, fixed by CL 148517, which was included in 1.12, and thus closed by the gopherbot when the commit was merged.

This is the opposite of "ignored and locked". Did you link the wrong issue?

@karalabe
Copy link
Contributor Author

karalabe commented Apr 1, 2020

I believe this is go in gopath mode working as it always has been

if you want to install in module mode GO111MODULE=on go get github.com/karalabe/go-issue-38196-keygen

@seankhliao

$ GO111MODULE=on go get github.com/karalabe/go-issue-38196-keygen
go: downloading github.com/karalabe/go-issue-38196-keygen v0.0.0-20200401121913-5c894df7c1c5
go: github.com/karalabe/go-issue-38196-keygen upgrade => v0.0.0-20200401121913-5c894df7c1c5

$ go-issue-38196-keygen
Generating crypto key: insecure key

Also as far as I know as of Go 1.13, the default is on, so that should not make much of a difference.

The issue you linked (#24250) was marked as a 1.12 release blocker, fixed by CL 148517, which was included in 1.12, and thus closed by the gopherbot when the commit was merged.
This is the opposite of "ignored and locked". Did you link the wrong issue?

Hmm, I did not see that part, only the last lines in the issue:

@golang golang locked and limited conversation to collaborators on Feb 26
@gopherbot gopherbot added the FrozenDueToAge label on Feb 26

The last comments from that issue seem to be exactly this: installing a binary will not use the proper go modules.

@karalabe
Copy link
Contributor Author

karalabe commented Apr 1, 2020

@sean-jc Hmm, scratch that, apparently go get without GOBIN stashed the binary somewhere weird:

$ GOBIN=`pwd` GO111MODULE=on go get github.com/karalabe/go-issue-38196-keygen
go: github.com/karalabe/go-issue-38196-keygen upgrade => v0.0.0-20200401121913-5c894df7c1c5

$ ./go-issue-38196-keygen 
Generating crypto key: secure key

@seankhliao
Copy link
Member

Also as far as I know as of Go 1.13, the default is on, so that should not make much of a difference.

GO111MODULE continues to default to auto as of go1.14

@myitcv
Copy link
Member

myitcv commented Apr 1, 2020

Please can I humbly make a suggestion:

Go flat out ignores

then just ignored and locked

Well, shit.

(last one quoted from the linked repro repository).

There is no need to resort to such hyperbole, not least when you might not be in possession of all the facts.

Instead, asking questions is more powerful way of conveying the same point:

Go appears to ignore... if I've understood this correctly?

appears to have been closed without a solution?

This is somewhat surprising

@karalabe
Copy link
Contributor Author

karalabe commented Apr 1, 2020

@myitcv Fair points. It's just a bit frustrating when I see the following timelines:

rsc added NeedsFix release-blocker labels on Apr 9, 2019
rsc modified the milestones: Go1.13, Go1.14 on May 9, 2019
jayconrod modified the milestones: Go1.14, Go1.15 on Nov 11, 2019

It's fine to keep pushing a feature/fix if it doesn't make it into a release. It's just strange that something is important enough to be tagged a release-blocker, and then pushed 3 releases and counting. It seems to indicate that there is a lack of interest to solve the issue, even though everyone agrees on its severity. Hence why my strong language.

@myitcv
Copy link
Member

myitcv commented Apr 1, 2020

Understood, but I think the first step should be asking questions to confirm you have the correct understanding/interpretation; that costs nothing and builds rapport with people, rather than potentially putting people's backs up.

Your frustration is shared by a number of people through #30515 and friends. This particular topic has been raised on the golang-tools calls many times. Unfortunately more pressing issues have pushed this down the priority list.

@andybons andybons changed the title go get ignores go.mod from remote repo, can escalate into a security vuln cmd/go: go get ignores go.mod from remote repo, can escalate into a security vuln Apr 1, 2020
@andybons andybons added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Apr 1, 2020
@andybons andybons added this to the Unplanned milestone Apr 1, 2020
@andybons andybons added the GoCommand cmd/go label Apr 1, 2020
@andybons
Copy link
Member

andybons commented Apr 1, 2020

@jayconrod @bcmills @matloob

@bcmills
Copy link
Contributor

bcmills commented Apr 1, 2020

@karalabe, issue #24250 was fixed in Go 1.12. But read the issue title carefully (emphasis mine):

cmd/go: allow "go get" when outside a module in module mode

You have to be in module mode for it to take effect. The way to activate module mode is to either invoke the go command within the working directory of a module, or explicitly set GO111MODULE=on. (This behavior is documented in https://golang.org/cmd/go/#hdr-Module_support. Improving discoverability for go command documentation is #33637.)

You can use go env GOMOD to confirm whether you are in module mode: it will report a non-empty value if (and only if) the go command is in module mode. (We ask for the output of go env in the issue template in part so that we can figure out whether the go command was in module mode — please take care to fill out the whole template next time.)

@bcmills
Copy link
Contributor

bcmills commented Apr 1, 2020

Here's a way to escalate this bug into a security vulnerability

That is GOPATH mode working as it has always worked. It is only a security vulnerability to the extent that GOPATH mode is less secure than we would like it to be.

If you want to ensure that your users install your binary in module mode, you can ask them to set GO111MODULE=on in your install instructions. Otherwise, this issue is essentially the same as #30228 (default GO111MODULE to on), which was replaced by #31857 (revert to auto with changes) for Go 1.13.

At some point we should make another run at defaulting GO111MODULE to on, but that will need another proposal.

@bcmills bcmills added the WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided. label Apr 1, 2020
@karalabe
Copy link
Contributor Author

karalabe commented Apr 2, 2020

That is GOPATH mode working as it has always worked. It is only a security vulnerability to the extent that GOPATH mode is less secure than we would like it to be.

Technically you are right. Practically I'd argue a bit against it, because whilst it is exactly as secure as it was before, most people (myself included) don't know about this silent revertal to the old behavior in certain cases. Since the go.mod file is in the repo, I would have never considered that there's an easy way to go around it.

If you want to ensure that your users install your binary in module mode, you can ask them to set GO111MODULE=on in your install instructions.

That is certainly doable, but my guess would be that most Go devs don't read the manual on how to install a binary, since Go made it so simple since forever. Maybe it's just me getting too used to it.

Otherwise, this issue is essentially the same as #30228 (default GO111MODULE to on), which was replaced by #31857 (revert to auto with changes) for Go 1.13.

Feel free to close this issue.

@jayconrod
Copy link
Contributor

Closing. This will be resolved eventually when GO111MODULE defaults to on. Until then, we can't make significant changes to GOPATH mode without breaking people who rely on the current behavior.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge GoCommand cmd/go NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. WaitingForInfo Issue is not actionable because of missing required information, which needs to be provided.
Projects
None yet
Development

No branches or pull requests

8 participants