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

noproxy: explictly support space-separated no proxy hosts/domains #10209

Closed
wants to merge 1 commit into from
Closed

noproxy: explictly support space-separated no proxy hosts/domains #10209

wants to merge 1 commit into from

Conversation

michael-o
Copy link
Contributor

For a long time curl implicitly supported space-separated, along with comma-separated, hosts/domains either via API or no_proxy/NO_PROXY. To clarify and extend flexibility explicitly support/document the status quo now.

For the past 10 years or so I have been using this on FreeBSD to access the outside world with curl and especially with Git.

Motivation:

  1. Making the obvious official/public
  2. I have raised similar with py-requests and stated that curl unofficially supports this too. Well, the maintainers said: not documented, closed and locked which is a pity: Handle space-separated values in NO_PROXY psf/requests#5465

I have also put this into consideration.

For a long time curl implicitly supported space-seprated, along with
comma-separated, hosts/domains either via API or no_proxy/NO_PROXY.
To clarify and extend compatiblity explicitly support/document the
status quo now.
@bagder
Copy link
Member

bagder commented Jan 2, 2023

I checked, it was documented as a comma-separated list already before my recent refresh of the noproxy code. It was never documented or otherwise suggested that it was fine with just space separation. That was just accidentally working.

I don't quite understand what the benefit is (for the world) to make the formerly undocumented feature officially supported, especially when other implementations don't support space either and even that gitlab page suggests that the common style should be comma-separated.

I understand the benefit to you since doing as this PR would bring back a noproxy string you used before to work again. To me it seems to be less work to rather just insert commas in your list.

@michael-o
Copy link
Contributor Author

OK. let me clarify my motivation for this. There are aspects which led me to this PR:

  1. I have relied on undefined behavior for way too long (10+ years on HP-UX, FreeBSD and RHEL) and never cared. This is my failure and I don't want to continue it. I actually only noticed it when in 7.86.0 the noproxy stuff was broken and started to use py-requests in recent years which lead to the same issue. I also compared at least to FreeBSD's libfetch(3) which does both and even refers to curl 20+ years ago: https://github.com/freebsd/freebsd-src/blob/17f2b12a3877f460f72123242c068881806388c2/lib/libfetch/common.c#L1760-L1767
  2. Now I also tried to fix the root cause, but there is little progress for almost three years. I tried to go down to py-requests where my request was rejected and stated that curl does not do this, so we won't neither. Tried to reach out downstream to the FreeBSD port maintainer who rejected and requested to upstream this. So basically I am going circles now and am in a deadlock situation. That is why I wanted to reach out here because if this works here, likely others will react.

I see two options to resolve this properly:

  1. Make it official becaue it worked unofficially/unintentionally for 20+ years.
  2. Remove/tighten it and make it clear what the behavior is which will put some pressure on others to clean up their stuff.

Either way, a predictable, well understood behavior is important.

Let me know what you think!

@bagder
Copy link
Member

bagder commented Jan 3, 2023

Either way, a predictable, well understood behavior is important.

Agreed. The lowest common denominator among implementations as far as separators go seems to be the comma. Not space.

Making curl support spaces (again) will certainly bring back the previous functionality for users of curl and libcurl but it seems to little to unify the implementations; quite the opposite. It seems many of the implementations don't support space now and have rejected doing so. Opening up for space now will only bring back an uncertainty and ambiguity that doesn't help.

To me, it seems to be generally more helpful to the "noproxy community" at large to not allow spaces, since using commas has a much greater chance for interop.

This also matches what the gitlab page suggests a "standard" noproxy parser should do - and I believe curl actually works exactly like the bullet points say under "Standardizing" on that page.

@michael-o
Copy link
Contributor Author

michael-o commented Jan 3, 2023

To me, it seems to be generally more helpful to the "noproxy community" at large to not allow spaces, since using commas has a much greater chance for interop.

This would mean that you have to tighten the parser, no? This would be logical.

@bagder
Copy link
Member

bagder commented Jan 3, 2023

This would mean that you have to tighten the parser, no? This would be logical.

I thought you wanted to bring back space-separation because it does not currently work. What needs to be tightened do you mean?

@michael-o
Copy link
Contributor Author

michael-o commented Jan 3, 2023

This would mean that you have to tighten the parser, no? This would be logical.

I thought you wanted to bring back space-separation because it does not currently work. What needs to be tightened do you mean?

You misunderstood. It does work, but is not official. You said that it should not be supported, this would mean that it would not be supported unofficially (code change). I, of course, prefer to make it official, but, of course, the decision is up to you.

@bagder
Copy link
Member

bagder commented Jan 3, 2023

It does work, but is not official

Sorry for being slow.

So it does still work, and it has worked for a long time. That puts it in a slightly different light to me. But only slightly. It was never documented to work with spaces as separators and only a small subset of noproxy parsers seem to accept them as separators.

The remaining question is then probably only if we should rather leave it undocumented and still insist on commas in the documentation, or if we should go ahead and make commas required between patterns.

@michael-o
Copy link
Contributor Author

michael-o commented Jan 3, 2023

It does work, but is not official

Sorry for being slow.

So it does still work, and it has worked for a long time. That puts it in a slightly different light to me. But only slightly. It was never documented to work with spaces as separators and only a small subset of noproxy parsers seem to accept them as separators.

The remaining question is then probably only if we should rather leave it undocumented and still insist on commas in the documentation, or if we should go ahead and make commas required between patterns.

Correct, that is the cardinal question.

Working example with spaces:

$ curl --version
curl 7.87.0 (amd64-portbld-freebsd12.4) libcurl/7.87.0 OpenSSL/1.1.1s zlib/1.2.12 nghttp2/1.48.0
Release-Date: 2022-12-21
Protocols: file http https smtp smtps
Features: AsynchDNS GSS-API HSTS HTTP2 HTTPS-proxy IPv6 Kerberos Largefile libz SPNEGO SSL threadsafe UnixSockets
$ curl --verbose https://deblndw011x.ad001.siemens.net
* Uses proxy env variable NO_PROXY == 'localhost .siemens.net .siemens.com .siemens.de'
*   Trying 147.54.64.17:443...
* Connected to deblndw011x.ad001.siemens.net (147.54.64.17) port 443 (#0)
* ALPN: offers h2
* ALPN: offers http/1.1
*  CAfile: none
*  CApath: /etc/ssl/certs/
* [CONN-0-0][CF-SSL] TLSv1.3 (OUT), TLS handshake, Client hello (1):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Server hello (2):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Encrypted Extensions (8):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Certificate (11):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, CERT verify (15):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Finished (20):
* [CONN-0-0][CF-SSL] TLSv1.3 (OUT), TLS change cipher, Change cipher spec (1):
* [CONN-0-0][CF-SSL] TLSv1.3 (OUT), TLS handshake, Finished (20):
* SSL connection using TLSv1.3 / TLS_AES_256_GCM_SHA384
* ALPN: server accepted http/1.1
* Server certificate:
*  subject: C=DE; O=Siemens; OU=LDA IT PLM DW; CN=deblndw011x.ad001.siemens.net
*  start date: Jan 28 10:45:01 2022 GMT
*  expire date: Feb 28 09:15:13 2023 GMT
*  subjectAltName: host "deblndw011x.ad001.siemens.net" matched cert's "deblndw011x.ad001.siemens.net"
*  issuer: C=DE; ST=Bayern; L=Muenchen; O=Siemens; serialNumber=ZZZZZZB7; OU=Siemens Trust Center; CN=Siemens Issuing CA Intranet Server 2017
*  SSL certificate verify ok.
> GET / HTTP/1.1
> Host: deblndw011x.ad001.siemens.net
> User-Agent: curl/7.87.0
> Accept: */*
>
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* [CONN-0-0][CF-SSL] TLSv1.3 (IN), TLS handshake, Newsession Ticket (4):
* old SSL session ID is stale, removing
* Mark bundle as not supporting multiuse
< HTTP/1.1 200 OK
< Date: Tue, 03 Jan 2023 11:52:03 GMT
< Server: Apache
< X-Frame-Options: SAMEORIGIN
< Last-Modified: Mon, 13 Jun 2022 09:04:31 GMT
< ETag: "2d-5e150937365c0"
< Accept-Ranges: bytes
< Content-Length: 45
< Content-Type: text/html
<
<html><body><h1>It works!</h1></body></html>
* Connection #0 to host deblndw011x.ad001.siemens.net left intact

Through proxy:

$ curl --verbose https://google.com
* Uses proxy env variable NO_PROXY == 'localhost .siemens.net .siemens.com .siemens.de'
* Uses proxy env variable HTTPS_PROXY == 'http://de.coia.siemens.net:9400'
*   Trying 194.145.60.251:9400...
* Connected to de.coia.siemens.net (194.145.60.251) port 9400 (#0)
* allocate connect buffer
* Establish HTTP proxy tunnel to google.com:443
> CONNECT google.com:443 HTTP/1.1
> Host: google.com:443
> User-Agent: curl/7.87.0
> Proxy-Connection: Keep-Alive
>
< HTTP/1.1 200 Connection Established
< Proxy-Agent: Zscaler/6.2
<
* CONNECT phase completed
* CONNECT tunnel established, response 200
* ALPN: offers h2
* ALPN: offers http/1.1
*  CAfile: none
*  CApath: /etc/ssl/certs/

@bagder
Copy link
Member

bagder commented Jan 3, 2023

How about this approach:

We deprecate support for space-separated patterns in the noproxy string. By this I mean that we set a time in the future (at least 6 months into the future) when we declare we will remove support for them. Until then we produce a warning into the verbose output when a space-separated pattern is used.

This gives everyone a chance to react and transition until we take it away.

@michael-o
Copy link
Contributor Author

michael-o commented Jan 3, 2023

How about this approach:

We deprecate support for space-separated patterns in the noproxy string. By this I mean that we set a time in the future (at least 6 months into the future) when we declare we will remove support for them. Until then we produce a warning into the verbose output when a space-separated pattern is used.

This gives everyone a chance to react and transition until we take it away.

This is something I can live with. Willing to test a patch.

Consider that for proxy users in corporate environments it may take longer to catch up. I'd expect that end of 2023 or so is reasonable.

@bagder
Copy link
Member

bagder commented Jan 3, 2023

Ok, let's make it 18 months from now. July 2024. I'll make a PR you can try.

bagder added a commit that referenced this pull request Jan 3, 2023
bagder added a commit that referenced this pull request Jan 3, 2023
To be removed in July 2024.

Assisted-by: Michael Osipov
Fixes #10209
Closes #10215
@michael-o
Copy link
Contributor Author

Ok, let's make it 18 months from now. July 2024. I'll make a PR you can try.

Thanks, looking forward too.

@bagder bagder closed this in 7ad8a7b Jan 4, 2023
@michael-o michael-o deleted the no-proxy-space-separator-docs branch January 4, 2023 08:29
bch pushed a commit to bch/curl that referenced this pull request Jul 19, 2023
To be removed in July 2024.

Assisted-by: Michael Osipov
Fixes curl#10209
Closes curl#10215
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

None yet

2 participants