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

Is use of ancient Proxy-Connection header intentional? #633

Closed
bradfitz opened this issue Feb 3, 2016 · 7 comments
Closed

Is use of ancient Proxy-Connection header intentional? #633

bradfitz opened this issue Feb 3, 2016 · 7 comments
Assignees
Labels

Comments

@bradfitz
Copy link

bradfitz commented Feb 3, 2016

@bagder, et al,

In migrating Google internal builds to Go 1.6 from Go 1.5, and we just noticed something odd with curl.

As background, Go 1.6 includes HTTP/2, and on by default.

One team reported 400 Bad Request errors from Google's GFEs (front end load balancers) when using http2. This is because a Go process via its included ReverseProxy was proxying a git request where libcurl set "Proxy-Connection: keep-alive" to the GFE, and the GFE implements RFC 7540 section 8.1.2.2 (no connection-specific header fields) and Go's included ReverseProxy removes hop-by-hop HTTP headers, but forgot to remove the ancient and never-standardized "Proxy-Connection" hop-by-hop header.

Go's bug is now fixed in golang/go@91911e3 but...

Is there any reason that libcurl still sends Proxy-Connection headers?

http://tools.ietf.org/html/rfc7230 says:

   One attempted solution was the introduction of a Proxy-Connection
   header field, targeted specifically at proxies.  In practice, this
   was also unworkable, because proxies are often deployed in multiple
   layers, bringing about the same problem discussed above.

   As a result, clients are encouraged not to send the Proxy-Connection
   header field in any requests.

In curl, I see:

lib/http.c:                     "%s" /* Proxy-Connection */
lib/http.c:                      !Curl_checkProxyheaders(conn, "Proxy-Connection:"))?
lib/http.c:                     "Proxy-Connection: Keep-Alive\r\n":"",
lib/http.c:                               "Proxy-Connection:", "keep-alive")) {
lib/http.c:       * 'Proxy-Connection: keep-alive' line tells us the
lib/http.c:      connkeep(conn, "Proxy-Connection keep-alive"); /* don't close */
lib/http.c:                               "Proxy-Connection:", "close")) {
lib/http.c:      connclose(conn, "Proxy-Connection: asked to close after done");
lib/http_proxy.c:        if(!Curl_checkProxyheaders(conn, "Proxy-Connection:"))
lib/http_proxy.c:          proxyconn = "Proxy-Connection: Keep-Alive\r\n";
lib/http_proxy.c:                           "%s", /* Proxy-Connection */
lib/http_proxy.c:                                             "Proxy-Connection:", "close"))

Thanks!

@bradfitz
Copy link
Author

bradfitz commented Feb 3, 2016

Equivalent Mozilla bug removing it from @mnot: https://bugzilla.mozilla.org/show_bug.cgi?id=570283

@bagder
Copy link
Member

bagder commented Feb 3, 2016

Thanks for digging this up!

Here's what I found when digging deep into our history: The header was added in May 2005 in commit 5d9fc28 and was explained (by me) here. The explanation there is not very detailed: "address (rare) problems with some proxies" and I can for the life of me not remember those details anymore.

Given the age of that change, the current RFC statement and the removal of this header from Firefox (back in 2012), I think we have a good argument to remove it from curl as well.

@bagder bagder added the HTTP label Feb 3, 2016
@bagder
Copy link
Member

bagder commented Feb 3, 2016

@bradfitz
Copy link
Author

bradfitz commented Feb 3, 2016

Took me a second to find the tweet: https://twitter.com/mnot/status/695013719563108352 :)

@bagder
Copy link
Member

bagder commented Feb 4, 2016

I've mentioned my intention to remove this header from future requests just to see if someone is aware of an actual use case for it. Barring that, I have a local patch for this that I intend to merge into master after the pending patch release (7.47.1).

@bagder bagder self-assigned this Feb 6, 2016
@bagder bagder closed this as completed in 113f04e Feb 8, 2016
@bagder
Copy link
Member

bagder commented Feb 8, 2016

Thanks, merged in master now and targeted for next release!

@bradfitz
Copy link
Author

bradfitz commented Feb 9, 2016

Thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Development

No branches or pull requests

2 participants