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

Comments

Projects
None yet
2 participants
@bradfitz

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

This comment has been minimized.

Show comment
Hide comment

bradfitz commented Feb 3, 2016

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 3, 2016

Member

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.

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

This comment has been minimized.

Show comment
Hide comment
@bradfitz

This comment has been minimized.

Show comment
Hide comment

bradfitz commented Feb 3, 2016

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

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 4, 2016

Member

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).

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 in 113f04e Feb 8, 2016

@bagder

This comment has been minimized.

Show comment
Hide comment
@bagder

bagder Feb 8, 2016

Member

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

Member

bagder commented Feb 8, 2016

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

@bradfitz

This comment has been minimized.

Show comment
Hide comment

bradfitz commented Feb 9, 2016

Thanks!

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