| Message ID | 20190126102355.24766-1-christian@paral.in |
|---|---|
| State | Accepted |
| Commit | f7a2870dd1fef9ee41e78ea1bcbb2ec61e82eb67 |
| Headers | show |
| Series | [1/1] go: explicitly disable modules to avoid unintended network lookup | expand |
On Sat, Jan 26, 2019 at 02:23:55AM -0800, Christian Stewart wrote: > Go "modules" refers to the dependency fetching, verification (hashing), and > version control system built into Go as of 1.11. > > It is not desirable to have Go modules enabled in Buildroot in the normal case, > as Buildroot manages downloading the sources, and third party dependency > managers are typically not used. > > In the absence of the GO111MODULE environment variable, the Go compiler will > correctly compile using the "vendor" version of dependencies downloaded by > Buildroot during the compilation process for Go-based packages. > > However, if the user sets the GO111MODULE=yes environment variable, the Go > compiler will download the Go dependencies for Buildroot packages, using the > modules system. This is potentially unintended behavior from user environment > variables. > > This commit sets the GO111MODULE=off variable in the Go target and host > compilation environments, disabling Go modules support for Buildroot mainline > packages. > > Signed-off-by: Christian Stewart <christian@paral.in> > --- > package/go/go.mk | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/package/go/go.mk b/package/go/go.mk > index 7755a3f785..1558b55e21 100644 > --- a/package/go/go.mk > +++ b/package/go/go.mk > @@ -42,6 +42,7 @@ HOST_GO_ROOT = $(HOST_DIR)/lib/go > # For the convienience of target packages. > HOST_GO_TOOLDIR = $(HOST_GO_ROOT)/pkg/tool/linux_$(GO_GOARCH) > HOST_GO_TARGET_ENV = \ > + GO111MODULE=off \ > GOARCH=$(GO_GOARCH) \ > GOROOT="$(HOST_GO_ROOT)" \ > CC="$(TARGET_CC)" \ > @@ -61,6 +62,7 @@ endif > # The go build system is not compatible with ccache, so use > # HOSTCC_NOCCACHE. See https://github.com/golang/go/issues/11685. > HOST_GO_MAKE_ENV = \ > + GO111MODULE=off \ > GOROOT_BOOTSTRAP=$(HOST_GO_BOOTSTRAP_ROOT) \ > GOROOT_FINAL=$(HOST_GO_ROOT) \ > GOROOT="$(@D)" \ > -- > 2.19.2 > I like the general idea, but after reading on go build automatic download, I wonder if setting GOPROXY=off isn't a better solution (see go help goproxy) : it works without disabling modules (we don't know how long disabling them will work), and it reaches the same goal: no "automatic" download will be done. Also, in our case, we set GOPATH= to a temporary workspace during the build, so we shouldn't have modules activated during the build. What do you think ? Anisse
Anisse, Christian Stewart <christian@paral.in> writes: > Go "modules" refers to the dependency fetching, verification (hashing), and > version control system built into Go as of 1.11. > > In the absence of the GO111MODULE environment variable, the Go compiler will > correctly compile using the "vendor" version of dependencies downloaded by > Buildroot during the compilation process for Go-based packages. You suggested instead setting GOPROXY=off, this would not have the intended effect. It is indeed correct to disable modules altogether as this patch does. Disabling modules is not something that breaks Go or hinders it in any way. It simply disables the versioning and network dependency fetch mechanism. This is the correct approach to ensuring that the Go compiler will always use the vendor and gopaths correctly as set up by the Buildroot build system. I still recommend to merge this patch as is. Best, Christian
Hi Christian, On Tue, Feb 12, 2019 at 01:14:21AM -0800, Christian Stewart wrote: > Anisse, > > Christian Stewart <christian@paral.in> writes: > > Go "modules" refers to the dependency fetching, verification (hashing), and > > version control system built into Go as of 1.11. > > > > In the absence of the GO111MODULE environment variable, the Go compiler will > > correctly compile using the "vendor" version of dependencies downloaded by > > Buildroot during the compilation process for Go-based packages. > > You suggested instead setting GOPROXY=off, this would not have the > intended effect. What do you mean ? Isn't blocking any network fetching the goal ? > > It is indeed correct to disable modules altogether as this patch does. > Disabling modules is not something that breaks Go or hinders it in any > way. It simply disables the versioning and network dependency fetch > mechanism. This is the correct approach to ensuring that the Go compiler > will always use the vendor and gopaths correctly as set up by the > Buildroot build system. So does setting GOPATH, if I'm reading the doc correctly. Which is already done in pkg-golang.mk > > I still recommend to merge this patch as is. > Merging this as-is isn't a bad idea in the short term. It makes explicit a decision that is currently implicit (we use GOPATH). I'm more worried how we can deal with this if we start adding packages that rely on go.mod or go-get-ability for building, and don't have any vendoring. It seems that no in-tree package are currently depending on other go packages, so we can say that this is a problem to be handled once we have a few of those. In summary, I agree it should be merged, I just wanted to discuss the implications. Regards, Anisse
Hi Anisse, You are perhaps mistaking GO111MODULE to be a temporary flag that will be removed in the future. This is not the case. GOPROXY=off and GO111MODULE=off do not have the same effect. Engaging the Go modules system occurs whenever you are outside GOPATH or when you have the GO111MODULES=on environment variable set. We are inside GOPATH in /some/ cases in Buildroot. This does not affect the situation when GO111MODULE=on. GOPATH is ignored. Even if go.mod is nonexistent the system will download modules from the internet. In Buildroot, we generally go to great lengths to check archive integrity with hash files and generally avoid downloading anything outside of the main Buildroot download process. This is to ensure that you can backup the dl/ folder and still produce a working build. The Go modules system exists purely as a depedency version control and downloading mechanism in Go. It is NOT anything related to how the code is assembled within GoPath. GO111MODULE is not slated for removal anytime soon. Anisse Astier <anisse@astier.eu> writes: >> You suggested instead setting GOPROXY=off, this would not have the >> intended effect. > > What do you mean ? Isn't blocking any network fetching the goal ? If you do this reproduction with ANY > go1.11 version of Buildroot today: export GO111MODULE=on make docker-engine Then Buildroot will leak the GO111MODULE variable into the build process, and Go will start downloading dependencies from the internet and ignore GOPATH. Here is a reproduction, where Go makes network calls: https://asciinema.org/a/pKktUhsN2NDoWkiCEgW9iUPbA This is clearly UNINTENDED and has bit me before as I had GO111MODULE set in my ambient working environment and found to my surprise that Buildroot had downloaded Go dependencies out-of-band during the process. Now if I also set GOPROXY=off, Go STILL downloads depdencies from the internet. The flag has NO EFFECT whatsoever. It doesn't even disable downloading network dependencies at all! Reproduce: https://asciinema.org/a/dCOpJqCvIraP4QXd969nsxabS In this case it actually broke the build! So, yes, setting GO111MODULE=off is absolutely required in the host and target envirionments for Buildroot for Go for the purposes of disabling the modules system (a extremely common move usually triggered implicitly by GOPATH existing, but not explicitly). > So does setting GOPATH, if I'm reading the doc correctly. Which is > already done in pkg-golang.mk This is not effective when GO111MODULE=on in the parent env. > I'm more worried how we can deal with this if we start adding packages > that rely on go.mod or go-get-ability for building, and don't have any > vendoring. It seems that no in-tree package are currently depending on > other go packages, so we can say that this is a problem to be handled > once we have a few of those. There is no situation from my view where Buildroot would use Go modules. There is simply no reason to engage a code package manager inside Buildroot. If we need cross dependencies between packages the best way to do so is to set up a Buildroot "host" gopath and copy the dependency sources or simlinks there. > In summary, I agree it should be merged, I just wanted to discuss the > implications. Granted, this is completely valid and I should have provided the above reproductions before. But, in my view this is a security issue. Having GO111MODULE leak into the target environment and cause differing behavior is not intended. Best regards, Christian
Hi Christian, On Tue, Feb 12, 2019 at 04:32:08PM -0800, Christian Stewart wrote: > Hi Anisse, > > You are perhaps mistaking GO111MODULE to be a temporary flag that will > be removed in the future. This is not the case. GOPROXY=off and > GO111MODULE=off do not have the same effect. > > Engaging the Go modules system occurs whenever you are outside GOPATH or > when you have the GO111MODULES=on environment variable set. We are > inside GOPATH in /some/ cases in Buildroot. This does not affect the > situation when GO111MODULE=on. GOPATH is ignored. Even if go.mod is > nonexistent the system will download modules from the internet. I missed the part about the leaking environment in the commit message, my bad. It's absolutely critical to guard against this. > > In Buildroot, we generally go to great lengths to check archive > integrity with hash files and generally avoid downloading anything > outside of the main Buildroot download process. This is to ensure that > you can backup the dl/ folder and still produce a working build. > > The Go modules system exists purely as a depedency version control and > downloading mechanism in Go. It is NOT anything related to how the code > is assembled within GoPath. GO111MODULE is not slated for removal > anytime soon. > > > Anisse Astier <anisse@astier.eu> writes: > >> You suggested instead setting GOPROXY=off, this would not have the > >> intended effect. > > > > What do you mean ? Isn't blocking any network fetching the goal ? > > If you do this reproduction with ANY > go1.11 version of Buildroot > today: > > export GO111MODULE=on > make docker-engine > > Then Buildroot will leak the GO111MODULE variable into the build > process, and Go will start downloading dependencies from the internet > and ignore GOPATH. > > Here is a reproduction, where Go makes network calls: > > https://asciinema.org/a/pKktUhsN2NDoWkiCEgW9iUPbA > > This is clearly UNINTENDED and has bit me before as I had GO111MODULE > set in my ambient working environment and found to my surprise that > Buildroot had downloaded Go dependencies out-of-band during the process. > > Now if I also set GOPROXY=off, Go STILL downloads depdencies from the > internet. The flag has NO EFFECT whatsoever. It doesn't even disable > downloading network dependencies at all! > > Reproduce: https://asciinema.org/a/dCOpJqCvIraP4QXd969nsxabS > > In this case it actually broke the build! What happened here is that there was an automatic conversion from vendor.conf to a go.mod, and then the build tried fetching the modules (again), but was forbidden by the GOPROXY=off. So you're right that GOPROXY=off isn't sufficient for blocking network calls since the go.mod conversion process does some git fetch. > > So, yes, setting GO111MODULE=off is absolutely required in the host and > target envirionments for Buildroot for Go for the purposes of disabling > the modules system (a extremely common move usually triggered implicitly > by GOPATH existing, but not explicitly). > > > So does setting GOPATH, if I'm reading the doc correctly. Which is > > already done in pkg-golang.mk > > This is not effective when GO111MODULE=on in the parent env. > > > I'm more worried how we can deal with this if we start adding packages > > that rely on go.mod or go-get-ability for building, and don't have any > > vendoring. It seems that no in-tree package are currently depending on > > other go packages, so we can say that this is a problem to be handled > > once we have a few of those. > > There is no situation from my view where Buildroot would use Go modules. > There is simply no reason to engage a code package manager inside > Buildroot. If we need cross dependencies between packages the best way > to do so is to set up a Buildroot "host" gopath and copy the dependency > sources or simlinks there. Agreed. > > > In summary, I agree it should be merged, I just wanted to discuss the > > implications. > > Granted, this is completely valid and I should have provided the above > reproductions before. But, in my view this is a security issue. Having > GO111MODULE leak into the target environment and cause differing > behavior is not intended. I should have read the commit message more thoroughly as well since you mentioned this issue. Nitpick: the message says GO111MODULE=yes instead of GO111MODULE=on. Thanks for taking the time to answer. Anisse
Hi All, Anisse Astier <anisse@astier.eu> writes: >> If you do this reproduction with ANY > go1.11 version of Buildroot >> today: >> >> export GO111MODULE=on >> make docker-engine >> >> Then Buildroot will leak the GO111MODULE variable into the build >> process, and Go will start downloading dependencies from the internet >> and ignore GOPATH. >> >> Here is a reproduction, where Go makes network calls: >> >> https://asciinema.org/a/pKktUhsN2NDoWkiCEgW9iUPbA This is still the case today. What are the statuses of these patches? - https://patchwork.ozlabs.org/patch/1031429/ - https://patchwork.ozlabs.org/patch/1048076/ Best regards, Christian
>>>>> "Christian" == Christian Stewart <christian@paral.in> writes: > Go "modules" refers to the dependency fetching, verification (hashing), and > version control system built into Go as of 1.11. > It is not desirable to have Go modules enabled in Buildroot in the normal case, > as Buildroot manages downloading the sources, and third party dependency > managers are typically not used. > In the absence of the GO111MODULE environment variable, the Go compiler will > correctly compile using the "vendor" version of dependencies downloaded by > Buildroot during the compilation process for Go-based packages. > However, if the user sets the GO111MODULE=yes environment variable, the Go > compiler will download the Go dependencies for Buildroot packages, using the > modules system. This is potentially unintended behavior from user environment > variables. > This commit sets the GO111MODULE=off variable in the Go target and host > compilation environments, disabling Go modules support for Buildroot mainline > packages. Committed after changing =yes to =on in the commit message, thanks.
>>>>> "Christian" == Christian Stewart <christian@paral.in> writes: > Hi All, > Anisse Astier <anisse@astier.eu> writes: >>> If you do this reproduction with ANY > go1.11 version of Buildroot >>> today: >>> >>> export GO111MODULE=on >>> make docker-engine >>> >>> Then Buildroot will leak the GO111MODULE variable into the build >>> process, and Go will start downloading dependencies from the internet >>> and ignore GOPATH. >>> >>> Here is a reproduction, where Go makes network calls: >>> >>> https://asciinema.org/a/pKktUhsN2NDoWkiCEgW9iUPbA > This is still the case today. What are the statuses of these patches? > - https://patchwork.ozlabs.org/patch/1031429/ I have just applied it now. > - https://patchwork.ozlabs.org/patch/1048076/ This is for next / post 2019.02. I haven't looked closely at it yet, but will do so soon.
diff --git a/package/go/go.mk b/package/go/go.mk index 7755a3f785..1558b55e21 100644 --- a/package/go/go.mk +++ b/package/go/go.mk @@ -42,6 +42,7 @@ HOST_GO_ROOT = $(HOST_DIR)/lib/go # For the convienience of target packages. HOST_GO_TOOLDIR = $(HOST_GO_ROOT)/pkg/tool/linux_$(GO_GOARCH) HOST_GO_TARGET_ENV = \ + GO111MODULE=off \ GOARCH=$(GO_GOARCH) \ GOROOT="$(HOST_GO_ROOT)" \ CC="$(TARGET_CC)" \ @@ -61,6 +62,7 @@ endif # The go build system is not compatible with ccache, so use # HOSTCC_NOCCACHE. See https://github.com/golang/go/issues/11685. HOST_GO_MAKE_ENV = \ + GO111MODULE=off \ GOROOT_BOOTSTRAP=$(HOST_GO_BOOTSTRAP_ROOT) \ GOROOT_FINAL=$(HOST_GO_ROOT) \ GOROOT="$(@D)" \
Go "modules" refers to the dependency fetching, verification (hashing), and version control system built into Go as of 1.11. It is not desirable to have Go modules enabled in Buildroot in the normal case, as Buildroot manages downloading the sources, and third party dependency managers are typically not used. In the absence of the GO111MODULE environment variable, the Go compiler will correctly compile using the "vendor" version of dependencies downloaded by Buildroot during the compilation process for Go-based packages. However, if the user sets the GO111MODULE=yes environment variable, the Go compiler will download the Go dependencies for Buildroot packages, using the modules system. This is potentially unintended behavior from user environment variables. This commit sets the GO111MODULE=off variable in the Go target and host compilation environments, disabling Go modules support for Buildroot mainline packages. Signed-off-by: Christian Stewart <christian@paral.in> --- package/go/go.mk | 2 ++ 1 file changed, 2 insertions(+)