From: Christos Tsantilas <christos@chtsanti.net>
Date: Sat, 23 Oct 2021 06:53:57 +0000 (+0000)
Subject: BUG: Unexpected state while connecting to ... server, part 2 (#917)
X-Git-Tag: SQUID_6_0_1~279
X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=4647c8bda42dd52a1a7fd8d70dae4a06e75ba8de;p=thirdparty%2Fsquid.git

BUG: Unexpected state while connecting to ... server, part 2 (#917)

These BUG messages (discussed and removed in a recent commit 2b6b1bc)
exposed several bugs. This change fixes a case where a BUG message was
triggered by the following Must() violation:

    check failed: !csd->serverBump() ||
        csd->serverBump()->at(...tlsBump1, XactionStep::tlsBump2)
    exception location: PeekingPeerConnector.cc(173) initialize

The above Must() assumed that PeekingPeerConnector always changes the
SslBump step to step3. However, that assumption was wrong because
PeekingPeerConnector may run multiple times (and the step is recorded
outside the connector object). When FwdState reforwarded a failed
attempt, PeekingPeerConnector would find itself at step3.

Instead of fixing the Must(), we fixed a bigger bug: SslBump step3 must
start when we decide to communicate with the server, not in the middle
of that communication. This fix may affect some esoteric configurations
that use at_step ACLs outside ssl_bump _and_ rely on the wrong step
change timing, but, technically, such configurations are not officially
supported.

More step boundary fixes are needed. There is a (much bigger) ongoing
project dedicated to those changes.

This is a Measurement Factory project.
---

diff --git a/src/client_side.cc b/src/client_side.cc
index 9f14323b3e..5c5c809f45 100644
--- a/src/client_side.cc
+++ b/src/client_side.cc
@@ -2948,6 +2948,8 @@ ConnStateData::parseTlsHandshake()
         getSslContextStart();
         return;
     } else if (sslServerBump->act.step1 == Ssl::bumpServerFirst) {
+        debugs(83, 5, "server-first skips step2; start forwarding the request");
+        sslServerBump->step = XactionStep::tlsBump3;
         Http::StreamPointer context = pipeline.front();
         ClientHttpRequest *http = context ? context->http : nullptr;
         // will call httpsPeeked() with certificate and connection, eventually
@@ -3074,6 +3076,7 @@ ConnStateData::startPeekAndSplice()
     inBuf.clear();
 
     debugs(83, 5, "Peek and splice at step2 done. Start forwarding the request!!! ");
+    sslServerBump->step = XactionStep::tlsBump3;
     FwdState::Start(clientConnection, sslServerBump->entry, sslServerBump->request.getRaw(), http ? http->al : NULL);
 }
 
diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc
index 4df73bac0f..2cc422a510 100644
--- a/src/ssl/PeekingPeerConnector.cc
+++ b/src/ssl/PeekingPeerConnector.cc
@@ -25,6 +25,28 @@
 
 CBDATA_NAMESPACED_CLASS_INIT(Ssl, PeekingPeerConnector);
 
+Ssl::PeekingPeerConnector::PeekingPeerConnector(HttpRequestPointer &aRequest,
+        const Comm::ConnectionPointer &aServerConn,
+        const Comm::ConnectionPointer &aClientConn,
+        AsyncCall::Pointer &aCallback,
+        const AccessLogEntryPointer &alp,
+        const time_t timeout):
+    AsyncJob("Ssl::PeekingPeerConnector"),
+    Security::PeerConnector(aServerConn, aCallback, alp, timeout),
+    clientConn(aClientConn),
+    splice(false),
+    serverCertificateHandled(false)
+{
+    request = aRequest;
+
+    if (const auto csd = request->clientConnectionManager.valid()) {
+        const auto serverBump = csd->serverBump();
+        Must(serverBump);
+        Must(serverBump->at(XactionStep::tlsBump3));
+    }
+    // else the client is gone, and we cannot check the step, but must carry on
+}
+
 void
 Ssl::PeekingPeerConnector::cbCheckForPeekAndSpliceDone(const Acl::Answer aclAnswer, void *data)
 {
@@ -45,13 +67,6 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceDone(const Acl::Answer aclAnswer
 void
 Ssl::PeekingPeerConnector::checkForPeekAndSplice()
 {
-    // Mark Step3 of bumping
-    if (request->clientConnectionManager.valid()) {
-        if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) {
-            serverBump->step = XactionStep::tlsBump3;
-        }
-    }
-
     handleServerCertificate();
 
     ACLFilledChecklist *acl_checklist = new ACLFilledChecklist(
@@ -167,7 +182,6 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession)
         if (hostName)
             SSL_set_ex_data(serverSession.get(), ssl_ex_index_server, (void*)hostName);
 
-        Must(!csd->serverBump() || csd->serverBump()->at(XactionStep::tlsBump1, XactionStep::tlsBump2));
         if (csd->sslBumpMode == Ssl::bumpPeek || csd->sslBumpMode == Ssl::bumpStare) {
             auto clientSession = fd_table[clientConn->fd].ssl.get();
             Must(clientSession);
diff --git a/src/ssl/PeekingPeerConnector.h b/src/ssl/PeekingPeerConnector.h
index 388c49772a..944ff24c13 100644
--- a/src/ssl/PeekingPeerConnector.h
+++ b/src/ssl/PeekingPeerConnector.h
@@ -25,15 +25,7 @@ public:
                          const Comm::ConnectionPointer &aClientConn,
                          AsyncCall::Pointer &aCallback,
                          const AccessLogEntryPointer &alp,
-                         const time_t timeout = 0) :
-        AsyncJob("Ssl::PeekingPeerConnector"),
-        Security::PeerConnector(aServerConn, aCallback, alp, timeout),
-        clientConn(aClientConn),
-        splice(false),
-        serverCertificateHandled(false)
-    {
-        request = aRequest;
-    }
+                         time_t timeout = 0);
 
     /* Security::PeerConnector API */
     virtual bool initialize(Security::SessionPointer &);