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 · 35 comments
Closed

cmd/go: add GOINSECURE for insecure dependencies #32966

witchard opened this issue Jul 6, 2019 · 35 comments
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Milestone

Comments

@witchard
Copy link
Contributor

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
@bcmills
Copy link
Contributor

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
Copy link
Contributor

bcmills commented Jul 11, 2019

CC @FiloSottile @jayconrod @rsc

@witchard
Copy link
Contributor Author

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
Copy link

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

@FiloSottile
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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
Copy link
Contributor

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
Copy link
Contributor Author

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

@bradfitz
Copy link
Contributor

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 The path to resolution is known, but the work has not been done. 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
Copy link

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

@witchard
Copy link
Contributor Author

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
@dayadev
Copy link

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
Copy link
Contributor

@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
Copy link

dayadev commented Dec 5, 2019

@FiloSottile Thank you!

@arvenil

This comment has been minimized.

@bcmills

This comment has been minimized.

@arvenil
Copy link

arvenil commented Oct 13, 2020

What I'm trying to say is that I would expect GOINSECURE to work in that case but it doesn't and it's not obvious for me why not.

@FiloSottile
Copy link
Contributor

What I'm trying to say is that I would expect GOINSECURE to work in that case but it doesn't and it's not obvious for me why not.

@arvenil GOINSECURE applies to module import paths, not to the URL they are fetched from. That is intentional, because its intended use is for when a certain subset of modules are fetched over secure connections (like a VPN) without HTTPS.

Anyway, your issue should not require GOINSECURE, and instead you should find a way to make HTTPS verification work as expected. The resources @bcmills shared can help you with that.

@golang golang locked and limited conversation to collaborators Oct 15, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FrozenDueToAge modules NeedsFix The path to resolution is known, but the work has not been done. Proposal Proposal-Accepted
Projects
None yet
Development

Successfully merging a pull request may close this issue.