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: add GOINSECURE for insecure dependencies #32966

Closed
witchard opened this issue Jul 6, 2019 · 31 comments
Closed

cmd/go: add GOINSECURE for insecure dependencies #32966

witchard opened this issue Jul 6, 2019 · 31 comments

Comments

@witchard
Copy link
Contributor

@witchard witchard commented Jul 6, 2019

As discussed in #32104, #31741 and #27332; there is sometimes a need for users to fetch dependencies in an insecure manner (e.g. where dependencies are on servers with certificates that are not trusted by the system, or where the server is not https at all). The current go get command supports a -insecure flag for this use-case; however this is not supported by the new go mod commands. The -insecure flag is probably overkill in most cases and could lead to users fetching dependencies insecurely by accident.

I propose the addition of two new environment variables that would be used by all commands fetching dependencies. The first of these would provide the go tools with additional CA certificates to trust (in situations where the user is unable to modify the system trust, or where they only want to trust a certificate for the duration of the go command). The second would list servers where insecure fetching is allowed. For example, these could be:

  • GOTRUST=pathToCA1.pem,pathToCA2.pem - Defines a comma separated list of CA certificate files to trust along side the system ones.
  • GOINSECURE=foo.com,*.bar.com - Defines a comma separated list of hostnames (possibly with globs) where insecure fetches (I guess either over https due to untrusted authority, or over http) are allowed.

If this were implemented, then I would also propose the removal of the -insecure flag from go get.

I would be willing to work on this issue if it were accepted, but I don't really know where to start!

@gopherbot gopherbot added this to the Proposal milestone Jul 6, 2019
@gopherbot gopherbot added the Proposal label Jul 6, 2019
@bcmills bcmills added the modules label Jul 11, 2019
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 11, 2019

What's the use-case for GOTRUST? (Under what conditions would you want to build — and likely run — arbitrary source code signed by a CA that you don't trust enough to add to your system roots?)

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Jul 11, 2019

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Jul 12, 2019

The only situation I can think of is if a user does not have permission to modify their system trust. Pythons pip tool has similar options (https://pip.pypa.io/en/stable/reference/pip/#general-options), trusted-host is similar to GO_INSECURE and cert similar to GO_TRUST.

@rsc rsc changed the title proposal: improve ability to fetch insecure dependencies proposal: cmd/go: improve ability to fetch insecure dependencies Jul 16, 2019
@ernestoaparicio

This comment has been minimized.

Copy link

@ernestoaparicio ernestoaparicio commented Jul 30, 2019

Second this. Ran into an issue using Athens go proxy because it uses go mod download and that doesn't support --insecure.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Jul 31, 2019

I am in favor of GOINSECURE.

GOTRUST is complicated. On Windows, for example, we don't get to mix system and custom roots, because system verification is performed by calling the system API. We probably can get there by attempting two verifications in VerifyPeerCertificate.

I'd like to hear more support for the use case before introducing complexity for it. (But OTOH I don't want users reverting to GOINSECURE when unnecessary.)

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Jul 31, 2019

Ah interesting, I hadn’t appreciated that complexity. I wonder if GOTRUST could be of some form like host1:certfile1,host2:certfile2,... so you could then choose a custom cert for specific hosts; and then it would only use that cert for access to that host and not touch system ones? The env variable looks a bit more complex though.

I agree that forcing a users to use GOINSECURE to access a resource
that they just haven’t got a mechanism to trust a cert for is a bit odd.

@xrfang

This comment has been minimized.

Copy link

@xrfang xrfang commented Aug 22, 2019

A usecase for GOINSECURE is that the package is hosted on an Intranet. Is it possible to add a switch to allow inscure fetch for ALL intranet hosted packages?

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Aug 22, 2019

@xrfang - If all the intranet module sources share a common domain, e.g foo.company.org, bar.company.org then you’d just set it to *.company.org.

@xrfang

This comment has been minimized.

Copy link

@xrfang xrfang commented Aug 23, 2019

@witchard my suggestion is not based on domain matching, but by IP address, if any domain is resolved to internal IPs, or a specific CIDR range, then it is considered internal.

I personally think it is not hard to implement, and might be useful.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Aug 23, 2019

@xrfang oh I see, I guess that could work - so you’d put something like 192.168.1.0/24 in GOINSECURE?

I wonder if anything similar has been considered for GONOSUMDB and GONOPROXY? I feel like the go team would probably want it to be consistent.

@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Aug 23, 2019

if any domain is resolved to internal IPs, or a specific CIDR range, then it is considered internal

That would not be secure. DNS resolution is ordinarily unsecured, so with such a configuration option enabled all an attacker would have to do to downgrade security for a package would be to fake its IP address. It's also unclear to me what use cases that addresses that can't be addressed with a GOINSECURE list.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Aug 24, 2019

Based on the comments so far then, would it make sense to proceed with just GOINSECURE?

GOINSECURE would be defined as: a comma separated list of hostnames (with globs) where insecure fetches (either over https due to untrusted authority, or over http) are allowed. Example: GOINSECURE=foo.com,*.bar.com.

GOTRUST and anything based on ip subnets out of scope for now.

@xrfang

This comment has been minimized.

Copy link

@xrfang xrfang commented Aug 24, 2019

@witchard this means, to achieve what I want, I just set GOTRUST=192.168.0.0/24, and leave GOINSECURE empty. Then any package repo that resolve to that ip range will be considered trusted, and allow an http fetch?

@FiloSottile yes it is insecure, but I don't think it will be even more insecure to specify ip range than to specify package names. Both relies on DNS anyway.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Aug 24, 2019

@xrfang what @FiloSottile is saying is that a malicious actor on your network could modify DNS to point github.com at a local IP and then (if you had GOINSECURE for local IPs) they could get control of your packages from github.com. This wouldn’t be good and so having a blanket setting for local IPs is probably not a good idea. I.e. it is less secure to allow IP ranges in GOINSECURE.

GOINSECURE is essentially saying “I know packages from this domain are not coming to me over a trusted channel”. Therefore a malicious actor on your network could give you malicious packages for that domain, but for that domain only - and other packages from other domains are still protected.

As such the only way you’d achieve what you want would only be to put all the domains for your local packages in GOINSECURE I’m afraid.

@xrfang

This comment has been minimized.

Copy link

@xrfang xrfang commented Aug 26, 2019

@witchard OK thanks for pointing out. I didn't notice this possibility. However, I still consider that allow GOINSECURE to specify ip ranges is useful, provided that the implication is well documented. Its up to the core team to decide.

@rsc

This comment has been minimized.

Copy link
Contributor

@rsc rsc commented Sep 17, 2019

It seems like the discussion here has converged on (1) adding GOINSECURE and (2) not adding GOTRUST.

Is that accurate? Thanks.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Sep 17, 2019

Yes that’s my understanding. What needs to happen next? I’m willing to put some time into developing the feature but might need some pointers towards the right areas of the code to look at.

@wuxc

This comment has been minimized.

Copy link

@wuxc wuxc commented Oct 9, 2019

what about add a git+ssh vcs path pattern in cmd/go/internal/get/vcs.go? A dynamic vcs path parser
and an environment variable like 'GOPRIVATE_VCS=myprivate.site/;another.me/'


// vcsPaths defines the meaning of import paths referring to
// commonly-used VCS hosting sites (github.com/user/dir)
// and import paths referring to a fully-qualified importPath
// containing a VCS type (foo.com/repo.git/dir)
var vcsPaths = []*vcsPath{
	// Github
	{
		prefix: "github.com/",
		regexp: lazyregexp.New(`^(?P<root>github\.com/[A-Za-z0-9_.\-]+/[A-Za-z0-9_.\-]+)(/[\p{L}0-9_.\-]+)*$`),
		vcs:    "git",
		repo:   "https://{root}",
		check:  noVCSSuffix,
	},

	// wuxc gitlab
	{
		prefix: "myprivate.site/",
		regexp: lazyregexp.New(`^(?P<root>myprivate\.site/[A-Za-z0-9_.\-]+/[A-Za-z0-9_.\-]+)(/[\p{L}0-9_.\-]+)*$`),
		vcs:    "git",
		repo:   "ssh://{root}",
		check:  noVCSSuffix,
	},
@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 10, 2019

@wuxc, we want to move toward fewer hard-coded paths in the go binary, not more. A .git suffix on the module path should already be sufficient to at least attempt a fetch via git+ssh.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Oct 10, 2019

Using the .git suffix does actually help in my situation, and removes most of the need for GOINSECURE for my use case. This in conjunction with git url rewriting (https://www.jvt.me/posts/2019/03/20/git-rewrite-url-https-ssh/) enables me to import something like go.company.org/foo/bar.git which actually clones from ssh://long.internal.boring.git.url/foo/bar.git. The one annoying thing about this is that the git is part of the module path which (a) looks ugly, and (b) becomes part of the name for main binaries installed with go install. I wonder if something like GONOPROXY could be extended so that you could specify the vcs type, in this example: GONOPROXY=git:go.company.org.

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 11, 2019

@witchard, GONOPROXY operates on package paths, but the reason we need the .git suffix is in the step that resolves a package path to a URL. So there would be a bit of an impedance mismatch there: GONOPROXY would have to make an assumption about the package-to-URL mapping that it does not require today.

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Oct 11, 2019

Yes, fair enough. This is all just making me wonder that if there were ways to express custom module-path to repo mappings then perhaps the need for insecure fetch disappears somewhat. The current mechanism for custom module base to repo mappings (html meta tags at a trusted https custom domain) is challenging to meet in corporate environments with tightly controlled networks. GOINSECURE allows us to relax some of that by removing the requirement for trusted https, but some way to specify explicit mappings might allow us to not need to have GOINSECURE at all.

Would it make sense for me to open a separate issue on this subject, or is it something that we wouldn’t want to consider?

@bcmills

This comment has been minimized.

Copy link
Member

@bcmills bcmills commented Oct 11, 2019

Would it make sense for me to open a separate issue on this subject, or is it something that we wouldn’t want to consider?

You can open a separate issue if you like, but just to set expectations I doubt that we will go that route. (We already have one mechanism for setting up custom import paths, so the bar for adding another should be very high — we don't add redundancy lightly.)

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Oct 11, 2019

Ok, I’ll leave it for now :-). What is the next step for progressing with this proposal?

@rsc rsc changed the title proposal: cmd/go: improve ability to fetch insecure dependencies proposal: cmd/go: add GOINSECURE for insecure dependencies Oct 15, 2019
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 16, 2019

It looks like people are in favor of adding GOINSECURE with a list of wildcarded host names, so this is a likely accept. Leaving open for one week for final comments.

-- for @golang/proposal-review

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Oct 23, 2019

No new comments in the past week, so accepting.

@bradfitz bradfitz changed the title proposal: cmd/go: add GOINSECURE for insecure dependencies cmd/go: add GOINSECURE for insecure dependencies Oct 23, 2019
@bradfitz bradfitz added the NeedsFix label Oct 23, 2019
@bradfitz bradfitz modified the milestones: Proposal, Backlog Oct 23, 2019
witchard added a commit to witchard/go that referenced this issue Nov 4, 2019
Enables insecure fetching of dependencies whos path matches those specified in
the enironment variable GOINSECURE.

Fixes golang#32966
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Nov 4, 2019

Change https://golang.org/cl/205238 mentions this issue: cmd/go/internal/modfetch: add GOINSECURE

@witchard

This comment has been minimized.

Copy link
Contributor Author

@witchard witchard commented Nov 5, 2019

Just a thought after an initial implementation on this... I think it makes sense that setting GOINSECURE should imply GONOPROXY. As insecure fetch implies you are accessing the resource directly. Does this make sense?

witchard added a commit to witchard/go that referenced this issue Nov 7, 2019
Enables insecure fetching of dependencies whos path matches those specified in
the enironment variable GOINSECURE.

Fixes golang#32966
witchard added a commit to witchard/go that referenced this issue Nov 8, 2019
Enables insecure fetching of dependencies whos path matches those specified in
the enironment variable GOINSECURE.

Fixes golang#32966
witchard added a commit to witchard/go that referenced this issue Nov 8, 2019
Enables insecure fetching of dependencies whos path matches those specified in
the enironment variable GOINSECURE.

Fixes golang#32966
@gopherbot gopherbot closed this in 42db1da Nov 8, 2019
@dayadev

This comment has been minimized.

Copy link

@dayadev dayadev commented Dec 5, 2019

@witchard may be an outdated question but what version of go has this GOINSECURE flag?

I am on go version go1.13.4 darwin/amd64 and I get error when I try to set the flag

go env -w: unknown go command variable GOINSECURE
@FiloSottile

This comment has been minimized.

Copy link
Member

@FiloSottile FiloSottile commented Dec 5, 2019

@dayadev it's going to be in Go 1.14, so you can use golang.org/dl/gotip to try it (but be warned that the dev tree is not stable).

@dayadev

This comment has been minimized.

Copy link

@dayadev dayadev commented Dec 5, 2019

@FiloSottile Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.