[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Christian Stewart]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Anisse Astier]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Christian Stewart]

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 

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Anisse Astier]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Christian Stewart]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Anisse Astier]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Christian Stewart]

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

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Peter Korsgaard]

>>>>> "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.

-- 
Bye, Peter Korsgaard

[Buildroot] [PATCH 1/1] go: explicitly disable modules to avoid unintended network lookup

[Peter Korsgaard]

>>>>> "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.

-- 
Bye, Peter Korsgaard