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: load $GOROOT/go.env before loading user go.env #57179

Closed
rsc opened this issue Dec 8, 2022 · 22 comments
Closed

cmd/go: load $GOROOT/go.env before loading user go.env #57179

rsc opened this issue Dec 8, 2022 · 22 comments

Comments

@rsc
Copy link
Contributor

rsc commented Dec 8, 2022

I've been looking at the patches that distributions apply to Go. We'd like to remove as many as possible, so that once reproducible builds land (#24904, #57007), Linux distributions that rebuild from source should still be shipping the identical binaries that are distributed on https://go.dev/dl.

Debian patches are here: https://sources.debian.org/patches/golang-1.15/1.15.15-1~deb11u4/
Fedora patches are here: https://src.fedoraproject.org/rpms/golang/tree/rawhide

Debian's are all test changes. Fedora has one non-test change in cmd/link that maybe we should pick up if gcc has been fixed. Fedora also changes the default values for GOPROXY and GOSUMDB. Setting aside the fact that they are opening their users to supply chain attacks, it would be good if they didn't have to modify the binaries to do it. We also expect over on #57001 that Fedora will want to default GOTOOLCHAIN=local, and perhaps Debian will too.

Today the default Go environment variables are modified using 'go env -w' which writes to filepath.Join(os.UserConfigDir(), "go/env"). On my Mac that's $HOME/Library/Application Support/go/env. On Linux that's $HOME/.config/go/env.

I propose that when the go command loads that file, it first loads $GOROOT/go.env and then the user file. Any setting in the user file overwrites an equivalent setting in the $GOROOT/go.env, and the environment overwrites both (as always).

Then distributions that want to modify the defaults do not need to edit source code and create non-standard binaries. They can just write a $GOROOT/go.env.

@rsc rsc added this to the Proposal milestone Dec 8, 2022
@seankhliao
Copy link
Member

naming it go.env, would people think that it's like go.mod / go.work and want to use it locally (ask for it as a feature request) for per project (build) config?

@mvdan
Copy link
Member

mvdan commented Dec 9, 2022

They could come to that wrong conclusion with any filename, I think :) Even with base.env, one might think that it would work in any module directory.

I personally think proper docs would be enough. This would realistically only appear in downstream Go distributions and packages, and in general, users wouldn't go look inside GOROOT directly.

@rsc
Copy link
Contributor Author

rsc commented Dec 10, 2022

Perhaps we should accept a go.env in the work module or next to go.work as well. There have been various issues asking for per-module configuration in go.mod. Go.mod is not the right place for that, which is why those were declined, but if we already have the search order $GOROOT/go.env, $userconfigdir/go.env, it might make sense to insert $workdir/go.env between them, where $workdir is the directory containing the go.work (if any) or else the one containing the work module's go.mod.

Of course, go.env would only apply in the work module and would be ignored in dependencies. There are security concerns to think through for people who clone other repos and then open them in editors. We'd also have to decide what GOENV means and especially what GOENV=off means.

This would address:

In #57001, there is some complication due to having both the GOTOOLCHAIN environment variable and the toolchain go.mod line. If the work module's go.env file were a thing, we could probably drop the toolchain line and rely on the ability to set GOTOOLCHAIN in the work module go.env instead.

@rsc
Copy link
Contributor Author

rsc commented Dec 14, 2022

This proposal has been added to the active column of the proposals project
and will now be reviewed at the weekly proposal review meetings.
— rsc for the proposal review group

@rsc
Copy link
Contributor Author

rsc commented Jan 4, 2023

The reactions here are uniformly positive. Does anyone object to accepting this proposal?

@thepudds
Copy link
Contributor

thepudds commented Jan 4, 2023

Hi Russ, minor clarification -- is a module/workspace go.env as described above in #57179 (comment) currently considered part of this proposal, or is that just a possible future extension?

@hyangah
Copy link
Contributor

hyangah commented Jan 5, 2023

is a module/workspace go.env as described above in #57179 (comment) currently considered part of this proposal, or is that just a possible future extension?

I think the work module go.env #57179 (comment) should be a separate proposal. Introducing $GOROOT/go.env is less controversial and may be necessary to help the reproducible builds & extended forward compatibility support move forward smoothely. But I guess the work module go.env idea has more details to clarify.

  • why introducing a separate file (go.env) instead of extending go.work?
  • will all go env vars be allowed in the work module's go.env? (GOROOT/GOPATH/GOBIN/CC/CXX/AR/GOPROXY/GOSUMDB/*FLAGS...)
  • can there be an option to disable the work module's go.env? Or should this go.env be ignored by default?
    (For example, an editor may choose to ignore the go.env until a user explicitly approve it is trusted - like VS Code's workspace trust mode).
  • meaning of GOENV.
  • tool to help users debug how they ended up with certain go env vars. (We saw many users confused after running go env -w long ago and didn't realize their global go env was affected. Now there will be two more sources).

@rsc
Copy link
Contributor Author

rsc commented Jan 11, 2023

Thanks @hyangah. I started answering these questions but there are too many significant problems with the per-module go.env that don't arise with the $GOROOT/go.env. Let's keep this proposal scoped to just the $GOROOT/go.env.

The $GOROOT/go.env source is an alternative to editing the source code to change the hard-coded defaults, so that's less of a change.

@rsc
Copy link
Contributor Author

rsc commented Jan 11, 2023

Marking this likely accept but only for $GOROOT/go.env, not a per-module go.env.

@rsc
Copy link
Contributor Author

rsc commented Jan 11, 2023

Based on the discussion above, this proposal seems like a likely accept.
— rsc for the proposal review group

@gopherbot
Copy link
Contributor

Change https://go.dev/cl/462198 mentions this issue: cmd/go: introduce GOROOT/go.env and move proxy/sumdb config there

@rsc rsc reopened this Jan 18, 2023
@rsc
Copy link
Contributor Author

rsc commented Jan 18, 2023

I confused myself and submitted this before it was accepted. Reopening.

@rsc
Copy link
Contributor Author

rsc commented Jan 18, 2023

No change in consensus, so accepted. 🎉
This issue now tracks the work of implementing the proposal.
— rsc for the proposal review group

@rsc rsc changed the title proposal: cmd/go: load $GOROOT/go.env before loading user go.env cmd/go: load $GOROOT/go.env before loading user go.env Jan 18, 2023
@rsc rsc modified the milestones: Proposal, Backlog Jan 18, 2023
@rsc
Copy link
Contributor Author

rsc commented Jan 18, 2023

Done.

@rsc rsc closed this as completed Jan 18, 2023
@dmitshur dmitshur modified the milestones: Backlog, Go1.21 Jan 20, 2023
@thockin
Copy link

thockin commented Jan 20, 2023

Is there a write-up of the issues with a per-module or per-workspace go.env? That seems like it would be helpful in Kubernetes, especially once we adopt go workspaces fully. Our need for wrapper scripts will approach zero.

@balasanjay
Copy link
Contributor

How is directly downloading the binaries exposing users more than trusting sum.golang.org/proxy.golang.org?

What if the source repository had logic that looked (roughly) like this:

if (request is from specific IP space) { return subtly evil code; }
else { return benign code; }

With GOSUMDB disabled, authors in the victim IP space would see no indication that anything bad was happening.

With GOSUMDB enabled, there's a globally unique hash for every version of a package, so the source repository could not do such a thing without one of the two versions being rejected by the global checksum database. (And clients are not "trusting" this global checksum database so much as cryptographically proving that its committing to a single hash per package version, proving that its never rolling back history, etc using the same underlying tech as Certificate Transparency). See https://www.youtube.com/watch?v=KqTySYYhPUE for a great talk at Gophercon 2019 about the exact strategy.

kxxt pushed a commit to pacman-for-android/go that referenced this issue Sep 7, 2023
* New upstream release.
* Include temporary fix for default environment variables[0].

[0]: golang/go#57179
SuperSandro2000 pushed a commit to SuperSandro2000/nixpkgs that referenced this issue Jan 7, 2024
fix build error: "GOPROXY list is not the empty string, but contains no entries"

see: golang/go#57179
@golang golang locked and limited conversation to collaborators Aug 20, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests