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

Comments

Projects
None yet
10 participants
@forrejam
Copy link

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

This comment has been minimized.

Copy link
Member

commented Aug 15, 2016

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.

@bradfitz bradfitz closed this Aug 15, 2016

@kapilt

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jun 30, 2017

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link

commented Jul 10, 2017

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

@Rudikza

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor

commented Oct 29, 2017

@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

This comment has been minimized.

Copy link
Member

commented Oct 29, 2017

I'm still on leave. /cc @tombergan

gopherbot pushed a commit to golang/proposal that referenced this issue Nov 2, 2017

Proposal: Add support for CIDR notation in no_proxy variable
For golang/go#16704.

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

This comment has been minimized.

Copy link
Member

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.

@tombergan

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link

commented Nov 3, 2017

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

@fraenkel

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link

commented May 29, 2018

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

net/http: vendor x/net/http/httpproxy, use it in net/http
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

This comment has been minimized.

Copy link

commented May 30, 2018

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

@matthewdupre

This comment has been minimized.

Copy link

commented Jun 8, 2018

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

@Justin-DynamicD

This comment has been minimized.

Copy link

commented Jul 5, 2018

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

This comment has been minimized.

Copy link
Author

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

This comment has been minimized.

Copy link

commented Jul 6, 2018

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

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 6, 2018

gopherbot pushed a commit to golang/net that referenced this issue Jul 9, 2018

http/httpproxy: support CIDR notation and ports with NO_PROXY
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

This comment has been minimized.

Copy link

commented Jul 9, 2018

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

@gopherbot

This comment has been minimized.

Copy link

commented Jul 9, 2018

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

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