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

net/http, x/net/http/httpproxy: http_proxy is being used for https requests #40909

Closed
tkopecek opened this issue Aug 19, 2020 · 7 comments
Closed
Labels
Milestone

Comments

@tkopecek
Copy link

@tkopecek tkopecek commented Aug 19, 2020

Go handles http_proxy/https_proxy/no_proxy in non-standard way. According to source comment http_proxy is used even for https urls. This is counterintuitive and not-working if it is not overriden.

My usecase is that I've local squid running with http_proxy exported. Nevertheless, squid is configured to handle also https but it is not propagated because it is using untrusted self-signed certificate. Go tries to connect to https via the proxy and fails with the reasonable certificate signed by unknown authority message. But at first place it shouldn't have used that proxy at all.

Code failing on this is referenced here

@gopherbot
Copy link

@gopherbot gopherbot commented Aug 20, 2020

Change https://golang.org/cl/249440 mentions this issue: http/httpproxy: match http scheme when selecting http_proxy

@ALTree ALTree changed the title http_proxy is being used for https requests x/net: http_proxy is being used for https requests Aug 20, 2020
@gopherbot gopherbot added this to the Unreleased milestone Aug 20, 2020
@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Aug 20, 2020

That CL's code seems fine, @fraenkel, but it's a behavior change away from the documented behavior (and changes the documented behavior), so the decision on whether to do this should be made intentionally.

I'm pretty sure the old behavior (of HTTP_PROXY also applying to "https" scheme URLs when HTTPS_PROXY was not present) was intentional but I don't have the time to go digging through git history to figure out whose behavior we were copying at the time, but I thought we were.

/cc @rsc who might also remember and should decide who makes this decision.

@opennota
Copy link

@opennota opennota commented Aug 21, 2020

One can tunnel any protocol through an HTTP proxy: https://wiki.squid-cache.org/Features/HTTPS#CONNECT_tunnel

@fraenkel
Copy link
Contributor

@fraenkel fraenkel commented Aug 21, 2020

While CONNECT is the mechanism used, this is about the environment variables. all_proxy was meant to be the catch all but that is not implemented.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Oct 6, 2020

@neild, this should probably go in now-ish with release notes for Go 1.16 so we can see if anybody is surprised during the rcs/betas.

@neild
Copy link
Contributor

@neild neild commented Oct 13, 2020

Googling for http_proxy and https_proxy turns up mostly references to curl and wget in the first page of results, and I've confirmed that curl/wget don't use the value of the http_proxy environment variable for HTTPS connections. So while on one hand this is a behavioral change, it appears to be a change that brings http/httpproxy's behavior in line with the most common definition of these environment variables.

Tentatively SGTM.

gopherbot pushed a commit to golang/net that referenced this issue Oct 16, 2020
Protocol specific proxies must match based on scheme.

If the https proxy is no configured, and the proxy for a https URL is
requested, no proxy should be returned.

Updates golang/go#40909

Change-Id: I62dfcf95d819c634e8f2862e891877a4eb55fca7
Reviewed-on: https://go-review.googlesource.com/c/net/+/249440
Trust: Brad Fitzpatrick <bradfitz@golang.org>
Reviewed-by: Damien Neil <dneil@google.com>
@dmitshur dmitshur modified the milestones: Unreleased, Go1.16 Nov 23, 2020
@dmitshur dmitshur changed the title x/net: http_proxy is being used for https requests x/net/http/httpproxy: http_proxy is being used for https requests Nov 23, 2020
@dmitshur
Copy link
Member

@dmitshur dmitshur commented Nov 23, 2020

Closing as fixed. (We still need to document this change in Go 1.16 release notes, but that's tracked separately.)

The code change has happened in x/net in CL 249440, which got pulled into the standard library in CL 266898.

@dmitshur dmitshur closed this Nov 23, 2020
@dmitshur dmitshur changed the title x/net/http/httpproxy: http_proxy is being used for https requests net/http, x/net/http/httpproxy: http_proxy is being used for https requests Nov 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
8 participants
You can’t perform that action at this time.