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: considering supporting CIDR notation in no_proxy env variable #16704

Closed
forrejam opened this issue Aug 15, 2016 · 23 comments
Closed

net/http: considering supporting CIDR notation in no_proxy env variable #16704

forrejam opened this issue Aug 15, 2016 · 23 comments
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Milestone

Comments

@forrejam
Copy link

forrejam commented Aug 15, 2016

Feature request - It would be helpful if the net/http client supported CIDR notation in the no_proxy environment variable, or allowed another way to configure the net/http client to ignore the http_proxy for large networks (ie 10.0.0.0/8 etc).

I had no luck finding an official "spec" for the no_proxy environment variable, but I believe supporting this notation would cause no issues and would allow users of net/http to add proxy exclusions for large networks.

Python requests library implements this feature with the no_proxy env variable - https://github.com/kennethreitz/requests/blob/master/requests/utils.py#L569
and most browsers allow some sort of CIDR notation / wildcards for proxy exclusions.

During my searching, I came across an old issue #2158 where the functionality was discussed but not really addressed (as far as the CIDR notation goes)

  1. What version of Go are you using (go version)?
    go version go1.6.3 linux/amd64
  2. What operating system and processor architecture are you using (go env)?
    GOARCH="amd64"
    GOHOSTARCH="amd64"
    GOHOSTOS="linux"
    GOOS="linux"
  3. What did you do?
    Tried to use CIDR notation in the no_proxy env variable to exclude net/http client from using web proxy specified in http_proxy env variable for private network 10.0.0.0/8
  4. What did you expect to see?
    Expected http client library to ignore http_proxy for addresses in the 10.0.0.0/8 network.
  5. What did you see instead?
    net/http client tried to use the http_proxy for get/post requests to addresses in 10.0.0.0/8 network.

Cheers, and thanks for golang!

@bradfitz
Copy link
Contributor

I'd rather not extend the functionality of ProxyFromEnvironment, especially when there's no spec for it.

You can implement net/http.Transport.Proxy yourself (https://golang.org/pkg/net/http/#Transport) for specific behavior.

@kapilt
Copy link

kapilt commented Jun 30, 2017

@bradfitz afaics the entire use of NO_PROXY and proxy env vars is effectively convention, and afaics.. the issue with transport construction, is that as go based software becomes more popular its not even an option for the users to set that up themselves, their dependent on std lib behavior. ie. here's a recent debug session i had where curl and python respect it but a go based binary (etcdctl) does not. this makes quite a bit painful for users of go software, and i'm seeing it filed multiple times on software written in go (particularly docker/moby).

$ export NO_PROXY=s3.amazonaws.com,localhost,127.0.0.1,169.254.169.254,10.0.0.0/8"

curl works with it

$ curl -X GET http://10.96.232.136:6666/v2/members
{"members":[{"id":"ce2a822cea30bfca","name":"calico","peerURLs":["http://localhost:2380","http://localhost:7001"],"clientURLs":["http://10.203.19.218:6666"]}]}

as does python

$ python
Python 2.7.12 (default, Nov 19 2016, 06:48:10) 
[GCC 5.4.0 20160609] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import requests
>>> requests.get('http://10.96.232.136:6666/v2/members').json()
{u'members': [{u'clientURLs': [u'http://10.203.19.218:6666'], u'id': u'ce2a822cea30bfca', u'peerURLs': [u'http://localhost:2380', u'http://localhost:7001'], u'name': u'calico'}]}

as does ruby

$ irb
irb(main):001:0> require 'net/http'
=> true
irb(main):002:0> uri = URI('http://10.96.232.136:6666/v2/members')
=> #<URI::HTTP http://10.96.232.136:6666/v2/members>
irb(main):003:0> Net::HTTP.get(uri)
=> "{\"members\":[{\"id\":\"ce2a822cea30bfca\",\"name\":\"calico\",\"peerURLs\":[\"http://localhost:2380\",\"http://localhost:7001\"],\"clientURLs\":[\"http://10.203.19.218:6666\"]}]}\n"
irb(main):004:0> 

but go based binaries don't

$ ./etcdctl --peers http://10.96.232.136:6666/ cluster-health
cluster may be unhealthy: failed to list members
Error:  unexpected status code 407

@bradfitz bradfitz changed the title CIDR notation in no_proxy env variable for net/http client net/http: considering supporting CIDR notation in no_proxy env variable Jun 30, 2017
@bradfitz
Copy link
Contributor

@kapilt, okay, I'll reopen, but I don't plan to work on this myself.

@bradfitz bradfitz reopened this Jun 30, 2017
@bradfitz bradfitz added FeatureRequest help wanted NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Jun 30, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Unplanned Jun 30, 2017
@bradfitz
Copy link
Contributor

For this to move forward, sometime should try to first write a "spec" for it, documenting which other languages/libraries already implement said spec. (But if languages Foo and Bar both just use libcurl or whatever, say that.)

@forrejam
Copy link
Author

forrejam commented Jul 2, 2017

Looking at implementations across other languages and browsers, a full implementation of a no_proxy spec can get complicated quite quickly, and there are arguments about what is right and what is wrong.

I vote to keep it simple with domain, ip and CIDR exclusions. This is what python requests library does. My need for no_proxy in corporate and government environments is just excluding domains and private subnets (10.0.0.0/8 etc).

Ports, URL schemes, IP wildcards (10.*.3.*), and the special keywords like <local> I believe should be avoided.

Following simple rules:

  1. Environment variable must be no_proxy or NO_PROXY
  2. Should contain a comma-separated list of domains, ip addresses, or networks.
  3. Parent domain will exclude parent and all sub domains (ie google.com would exclude google.com and code.google.com)
  4. Domains in the following form: google.com, .google.com and *.google.com are all equivalent
  5. If sub domain is provided, parent domain(s) is not matched (ie code.google.com would not exclude google.com)
  6. IP addresses (ie 127.0.0.1, 192.168.1.1) (IPv6?)
  7. Networks in CIDR notation (ie 192.168.0.0/16) (IPv6?)
  8. Both localhost and 127.0.0.1 are excluded by default
  9. All other notations are ignored

References

python requests library (very simple - domain, ip, CIDR check)
https://github.com/requests/requests/blob/master/requests/utils.py#L629
https://github.com/requests/requests/blob/master/requests/utils.py#L583

Chromium (more complex - support for different schemes, ports and parts of domains)
https://cs.chromium.org/chromium/src/net/proxy/proxy_bypass_rules.h?l=142
https://cs.chromium.org/chromium/src/net/proxy/proxy_bypass_rules.h?l=153
https://cs.chromium.org/chromium/src/net/proxy/proxy_bypass_rules.cc?l=255
https://cs.chromium.org/chromium/src/net/proxy/proxy_config_service_linux_unittest.cc?q=no_proxy+package:%5Echromium$&l=1076

Some more reading
https://developer.mozilla.org/en-US/docs/No_Proxy_For_configuration
curl/curl#1208
https://www.gnu.org/software/wget/manual/html_node/Proxies.html
https://github.com/libproxy/libproxy/blob/master/libproxy/modules/ignore_domain.cpp
https://github.com/libproxy/libproxy/blob/master/libproxy/modules/ignore_hostname.cpp
https://github.com/libproxy/libproxy/blob/master/libproxy/modules/ignore_ip.cpp

@gopherbot
Copy link

CL https://golang.org/cl/47853 mentions this issue.

@Rudikza
Copy link

Rudikza commented Jul 10, 2017

I'd like to help out on this so I created the above proposal and hopefully, it get's approved and then I can get started on the coding side with a little nudge in the right direction 😁 I added @forrejam as a contributor to the proposal doc because he did quite a bit of the research.

@fraenkel
Copy link
Contributor

@bradfitz Is there anything else that needs to be done to consider this for inclusion in a future release? I like many others are hitting issues with Go programs not recognizing CIDR ranges in the NO_PROXY while other applications/languages accept them. One has to become creative when specifying a NO_PROXY to work around the limitations of any Go program.

@bradfitz
Copy link
Contributor

I'm still on leave. /cc @tombergan

gopherbot pushed a commit to golang/proposal that referenced this issue Nov 2, 2017
For golang/go#16704.

Change-Id: Id718d290628d9a1e723f7df0434ded30c3f08e02
Reviewed-on: https://go-review.googlesource.com/47853
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2017

I'm back from leave. I've submitted your proposal doc CL. Visible at https://github.com/golang/proposal/blob/master/design/16704-cidr-notation-no-proxy.md

What's not entirely clear from your doc is who all supports CIDRs in $no_proxy. Just Python? Or others?

This seems fine, though. Want to prepare a CL?

It won't happen for Go 1.10, though, which entered feature freeze today.

@bradfitz bradfitz added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. and removed NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. labels Nov 2, 2017
@tombergan
Copy link
Contributor

What's not entirely clear from your doc is who all supports CIDRs in $no_proxy. Just Python? Or others?

@forrejam's comment has a nice algorithm summary and answers the question of who supports CIDRs in no_proxy. @kapilt's comment has another summary. Looks like chrome, firefox, libproxy, python, ruby, and curl support CIDRs in $no_proxy.

@bradfitz
Copy link
Contributor

bradfitz commented Nov 2, 2017

@tombergan, ah, right. I had expected that to be summarized in the doc, though.

In any case, I'm fine with this. Somebody can send a CL.

@gopherbot
Copy link

Change https://golang.org/cl/75730 mentions this issue: net/http: NO_PROXY supports CIDR notation and ports

@bradfitz bradfitz modified the milestones: Unplanned, Go1.11 Nov 3, 2017
@bradfitz bradfitz added release-blocker NeedsFix The path to resolution is known, but the work has not been done. labels Nov 3, 2017
@gopherbot gopherbot removed the NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. label Nov 3, 2017
@fraenkel
Copy link
Contributor

fraenkel commented May 9, 2018

@bradfitz Given your last comment in my change, should I just abandon this change and create a new one for x/net/http/proxy?
Looks like we could attempt to reuse bits from x/net/proxy or else we have two different ways to parse no_proxy. Although I believe we already have differences between the two. Just a thought.

@gopherbot
Copy link

Change https://golang.org/cl/68091 mentions this issue: net/http: vendor x/net/http/httpproxy, use it in net/http

gopherbot pushed a commit that referenced this issue May 29, 2018
From x/net git rev c7086645de2.

Updates #16704

Change-Id: I4d642478fc69a52c973964845fca2fd402716e57
Reviewed-on: https://go-review.googlesource.com/68091
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
Run-TryBot: Brad Fitzpatrick <bradfitz@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/115255 mentions this issue: http/httpproxy: support CIDR notation and ports with NO_PROXY

@matthewdupre
Copy link

I've hit this problem too - another +1 for an upstream fix. (We might work around it ourselves for now.)

@Justin-DynamicD
Copy link

Found this thread while investigating Packer issues (compiled in GoLang) in which it is implied a fix may go out in 1.11 due this month? Hope so.

At any rate let me +1 this as well. Proxy use in certain verticals is common, and in the age of automation we often find ourselves needing to hit IPs as DNS simply isn't available yet.

@forrejam
Copy link
Author

forrejam commented Jul 6, 2018

From what I can see in https://go-review.googlesource.com/c/net/+/115255 it looks like it is progressing! :)

@jansmets
Copy link

jansmets commented Jul 6, 2018

Someone please fix this. I wasted a lifetime figuring this out. Thank you :-)

@bradfitz
Copy link
Contributor

bradfitz commented Jul 6, 2018

@jansmets, see https://golang.org/wiki/NoPlusOne

gopherbot pushed a commit to golang/net that referenced this issue Jul 9, 2018
NO_PROXY includes support for CIDR, and notations can also
match exactly on port information if provided.
When specifying a port with IPv6, the address must be enclosed with
square brackets, [IPv6 address]:port.

Updates golang/go#16704 (fixes after vendor into std)

Change-Id: Ideb61a9ec60a6b1908f5a2c885cd6d9dd10c37cf
Reviewed-on: https://go-review.googlesource.com/115255
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/122655 mentions this issue: vendor: update vendored x/net/http/httpproxy

@gopherbot
Copy link

Change https://golang.org/cl/122619 mentions this issue: http/httpproxy: update documentation for httpproxy

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
FeatureRequest FrozenDueToAge help wanted NeedsFix The path to resolution is known, but the work has not been done. release-blocker
Projects
None yet
Development

No branches or pull requests

10 participants