From: Alex Rousskov <rousskov@measurement-factory.com>
Date: Tue, 27 Oct 2020 23:33:39 +0000 (+0000)
Subject: Do not send keep-alive or close in HTTP Upgrade requests (#732)
X-Git-Tag: 4.15-20210522-snapshot~47
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=1f482f82a6345a13e9211d5b3601f15aedf822ae;p=thirdparty%2Fsquid.git

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 14907fff8e..4bf28df80a 100644
--- 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 499cfc7068..b1febfc45b 100644
--- 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.