Do not send keep-alive or close in HTTP Upgrade requests

[rousskov]

A presence of a Connection:keep-alive or Connection:close header in an
Upgrade request sent by Squid breaks some Google Voice services. FWIW,
these headers are not present in RFC 7230 Upgrade example, and popular
client requests do not send them.

• In most cases, Squid was sending Connection:keep-alive which is
  redundant in Upgrade requests (because they are HTTP/1.1).

• In rare cases (e.g., server_persistent_connections=off), Squid was
  sending Connection:close. Since we cannot send that header and remain
  compatible with popular Upgrade-dependent services, we now do not send
  it but treat the connection as non-persistent if the server does not
  upgrade and expects us to continue reusing the connection.

[rousskov]

The primary/first part (branch commit 5dd22d6) has been tested and appears to resolve the problem when Squid was sending a Connection:keep-alive header and receiving an HTTP/1.1 400 Bad Request response (without headers or body).

The second part (branch commit ac2df70) handles the case when Squid was sending a Connection:close header; that change has not been tested.

AFAICT from manual tests, Google Voice service does not like either header in the Upgrade request.

[yadij]

* In rare cases (e.g., server_persistent_connections=off), Squid was
  sending Connection:close. Since we cannot send that header and remain
  compatible with popular Upgrade-dependent services, we now do not send
  it but treat the connection as non-persistent if the server does not
  upgrade and expects us to continue reusing the connection.

Please do not. server_persistent_connections=off is an explicit admin policy choice to override HTTP/1.1 default persistence. We MUST send "Connection:close" on HTTP/1.1 and later messages to enforce that admins policy. They can remove the setting to let Squid perform the default working behaviour.

Long-term we should switch these persistence config settings from onoff to YesNoNone so we can explicitly send keep-alive/close when admin explicitly require one or other behaviour. While still doing best-effort behaviour like this PR has in the None case.

In regards to testing; has the existence of Connection with other values been checked to see if it is the hop-by-hop or persistence which breaks?

[rousskov]

server_persistent_connections=off is an explicit admin policy choice to override HTTP/1.1 default persistence.

Which this PR honors IMO -- Squid does not reuse HTTP connections if server_persistent_connections is off.

We MUST send "Connection:close" on HTTP/1.1 and later messages to enforce that admins policy.

Why? AFAICT, when server_persistent_connections is off, Squid should not reuse to-server connections, and this PR does not reuse them in that case, but why do we MUST send Connection:close in Upgrade requests as well? Yes, it would be nice to do that, but why do we really have to, especially when we know it is going to break many of those (explicitly enabled by the admin!) Upgrade attempts?

They can remove the setting to let Squid perform the default working behaviour.

Let's assume the admin had a good/valid reason to disable persistent connections with servers. They cannot just go back to the default behavior. Why do we have to prevent that same admin from supporting HTTP Upgrade (with admin-selected servers or for admin-specified protocols)? This PR gives the admin the choice.

N.B. Persistence does not matter (is not applicable) if the Upgrade is successful (we are no longer speaking HTTP at that point). It only matters for rare cases where the server refuses to upgrade. In those cases, Squid will close the connection after handling the (negative) server response to the Upgrade request if server_persistent_connections was set to off, honoring admin wishes for no connection reuse.

has the existence of Connection with other values been checked to see if it is the hop-by-hop or persistence which breaks?

I am not sure I understand the "to see..." part of the question. FWIW, in my tests, the current Google Voice service implementation appears to look at the last Connection header field item received. If that item is not "upgrade", the service considers the HTTP request invalid, responding with HTTP 400 "Bad Request" message. The previous items are irrelevant.

The service implementation changes in response to bug reports, and I would not be surprised if it starts tolerating other Connection items order, but given what we now know about this and other Upgrade-related agents (both clients and servers), it is unwise to send anything other than "Connection: upgrade" to them. Too many will break.

[rousskov]

@yadij, please see whether my response addressed your concerns. If it does, I will clear this PR for merging. If it does not, let's continue the discussion to find some common ground. The ball is on your side.

[yadij]

Sorry, I mistook the comment as applicable to response messages where there is a MUST requirement. On re-review I see it is for requests which leave the Connection:close sending as optional. I am approving for merge as-implemented.

Your response answers my question about testing. That clearly indicates a buggy Voice service. If you can please document your tests and report to the relevant Google dev if you can find them and/or the HTTPbis WG so the spec authors can be aware of this case for the latest spec update.

[rousskov]

If you can please document your tests and report to the relevant Google dev if you can find them and/or the HTTPbis WG so the spec authors can be aware of this case for the latest spec update.

Done. The tests I used are basic curl requests with -H options to specify upgrade and "extra" Connection headers. I feel a bit uncomfortable publishing the exact settings for script kiddies to abuse, and they do not seem particularly relevant or important in this context.