Do not send keep-alive or close in HTTP Upgrade requests (#732)

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.

diff --git a/src/http.cc b/src/http.cc
index 14907fff8e95d446fb2b0f9ace13bc62d71a0965..4bf28df80a098b5c4e28f87a65a3c9bbb9e52a04 100644 (file)
--- a/src/http.cc
+++ b/src/http.cc
@@ -1124,12 +1124,18 @@ HttpStateData::statusIfComplete() const
         return COMPLETE_NONPERSISTENT_MSG;
 
     /** \par
-     * If we didn't send a keep-alive request header, then this
+     * If we sent a Connection:close request header, then this
      * can not be a persistent connection.
      */
     if (!flags.keepalive)
         return COMPLETE_NONPERSISTENT_MSG;
 
+    /** \par
+     * If we banned reuse, then this cannot be a persistent connection.
+     */
+    if (flags.forceClose)
+        return COMPLETE_NONPERSISTENT_MSG;
+
     /** \par
      * If we haven't sent the whole request then this can not be a persistent
      * connection.
@@ -2012,10 +2018,11 @@ HttpStateData::httpBuildRequestHeader(HttpRequest * request,
         delete cc;
     }
 
-    // Always send Connection because HTTP/1.0 servers need explicit "keep-alive"
-    // while HTTP/1.1 servers need explicit "close", and we do not always know
-    // the server expectations.
-    hdr_out->putStr(Http::HdrType::CONNECTION, flags.keepalive ? "keep-alive" : "close");
+    // Always send Connection because HTTP/1.0 servers need explicit
+    // "keep-alive", HTTP/1.1 servers need explicit "close", Upgrade recipients
+    // need bare "upgrade", and we do not always know the server expectations.
+    if (!hdr_out->has(Http::HdrType::CONNECTION)) // forwardUpgrade() may add it
+        hdr_out->putStr(Http::HdrType::CONNECTION, flags.keepalive ? "keep-alive" : "close");
 
     /* append Front-End-Https */
     if (flags.front_end_https) {
@@ -2092,6 +2099,18 @@ HttpStateData::forwardUpgrade(HttpHeader &hdrOut)
          * regardless of the security implications.
          */
         hdrOut.putStr(Http::HdrType::CONNECTION, "upgrade");
+
+        // Connection:close and Connection:keepalive confuse some Upgrade
+        // recipients, so we do not send those headers. Our Upgrade request
+        // implicitly offers connection persistency per HTTP/1.1 defaults.
+        // Update the keepalive flag to reflect that offer.
+        // * If the server upgrades, then we would not be talking HTTP past the
+        //   HTTP 101 control message, and HTTP persistence would be irrelevant.
+        // * Otherwise, our request will contradict onoff.server_pconns=off or
+        //   other no-keepalive conditions (if any). We compensate by copying
+        //   the original no-keepalive decision now and honoring it later.
+        flags.forceClose = !flags.keepalive;
+        flags.keepalive = true; // should already be true in most cases
     }
 }
 
@@ -2327,7 +2346,7 @@ HttpStateData::buildRequestPrefix(MemBuf * mb)
     /* build and pack headers */
     {
         HttpHeader hdr(hoRequest);
-        forwardUpgrade(hdr);
+        forwardUpgrade(hdr); // before httpBuildRequestHeader() for CONNECTION
         httpBuildRequestHeader(request.getRaw(), entry, fwd->al, &hdr, flags);
 
         if (request->flags.pinned && request->flags.connectionAuth)

diff --git a/src/http/StateFlags.h b/src/http/StateFlags.h
index 499cfc7068b484dcd2946e2b23741dac42ee9b3f..b1febfc45ba84e5162b9c077e063ce41d8c5b996 100644 (file)
--- a/src/http/StateFlags.h
+++ b/src/http/StateFlags.h
@@ -16,7 +16,14 @@ class StateFlags
 {
 public:
     unsigned int front_end_https = 0; ///< send "Front-End-Https: On" header (off/on/auto=2)
-    bool keepalive = false; ///< whether to keep the connection persistent
+
+    /// whether the Squid-sent request offers to keep the connection persistent
+    bool keepalive = false;
+
+    /// whether Squid should not keep the connection persistent despite offering
+    /// to do so previously (and setting the keepalive flag)
+    bool forceClose = false;
+
     bool only_if_cached = false;
 
     /// Whether we are processing an HTTP 1xx control message.