]> git.ipfire.org Git - thirdparty/squid.git/commitdiff
Fix "BUG: Lost previously bumped from-Squid connection" (#460)
authorChristos Tsantilas <christos@chtsanti.net>
Mon, 9 Sep 2019 14:14:50 +0000 (14:14 +0000)
committerSquid Anubis <squid-anubis@squid-cache.org>
Tue, 10 Sep 2019 01:49:24 +0000 (01:49 +0000)
FwdState assumed that PeerSelector always returns the connection pinned
by previous SslBump steps. That assumption was wrong in two cases:

1. The previously pinned connection got closed.
2. PeerSelector policies prevent pinned connection reuse. For example,
    connection destination is now denied by cache_peer_access rules.

PeerSelector now returns a PINNED selection even if a pinned connection
cannot or should not be used. The initiator is now fully responsible for
checking the returned connection usability, including the new
ConnStateData::pinning::peerAccessDenied flag. Unusable pinned
connection is now treated as any other fatal (for the transaction)
forwarding problem rather than an internal BUG.

The above changes do not change traffic on the wire but remove bogus
level-1 BUG messages from cache.log.

We also polished which error page is returned depending on the pinning
validation problem: ERR_ZERO_SIZE_OBJECT is returned when the validation
failed because of the peer disappearance or to-server connection
closure. Other cases use ERR_CANNOT_FORWARD. Eventually, these errors
can be detailed further to distinguish various problems. We may also
want to generalize ERR_CONFLICT_HOST and/or ERR_FORWARDING_DENIED to
make them applicable in this context.

This is a Measurement Factory project.

src/FwdState.cc
src/client_side.cc
src/client_side.h
src/peer_select.cc
src/tests/stub_client_side.cc
src/tunnel.cc

index bd4d9440d3ea07e34e49fe647fdcd18a05fde1d4..f8954f7185c393ec5d902cbd7e2d15f9539002b3 100644 (file)
@@ -558,30 +558,6 @@ FwdState::noteDestination(Comm::ConnectionPointer path)
 
     debugs(17, 3, path);
 
-    // Requests bumped at step2+ require their pinned connection. Since we
-    // failed to reuse the pinned connection, we now must terminate the
-    // bumped request. For client-first and step1 bumped requests, the
-    // from-client connection is already bumped, but the connection to the
-    // server is not established/pinned so they must be excluded. We can
-    // recognize step1 bumping by nil ConnStateData::serverBump().
-#if USE_OPENSSL
-    const auto clientFirstBump = request->clientConnectionManager.valid() &&
-                                 (request->clientConnectionManager->sslBumpMode == Ssl::bumpClientFirst ||
-                                  (request->clientConnectionManager->sslBumpMode == Ssl::bumpBump && !request->clientConnectionManager->serverBump())
-                                 );
-#else
-    const auto clientFirstBump = false;
-#endif /* USE_OPENSSL */
-    if (request->flags.sslBumped && !clientFirstBump) {
-        // TODO: Factor out/reuse as Occasionally(DBG_IMPORTANT, 2[, occurrences]).
-        static int occurrences = 0;
-        const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2;
-        debugs(17, level, "BUG: Lost previously bumped from-Squid connection. Rejecting bumped request.");
-        fail(new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al));
-        self = nullptr; // refcounted
-        return;
-    }
-
     destinations->addPath(path);
 
     if (Comm::IsConnOpen(serverConn)) {
@@ -994,6 +970,8 @@ FwdState::connectStart()
 {
     debugs(17, 3, *destinations << " to " << entry->url());
 
+    Must(!request->pinnedConnection());
+
     assert(!destinations->empty());
     assert(!opening());
 
@@ -1032,15 +1010,12 @@ FwdState::usePinned()
     const auto connManager = request->pinnedConnection();
     debugs(17, 7, "connection manager: " << connManager);
 
-    // the client connection may close while we get here, nullifying connManager
-    const auto temp = connManager ? connManager->borrowPinnedConnection(request) : nullptr;
-    debugs(17, 5, "connection: " << temp);
-
-    // the previously pinned idle peer connection may get closed (by the peer)
-    if (!Comm::IsConnOpen(temp)) {
-        syncHierNote(temp, connManager ? connManager->pinning.host : request->url.host());
+    try {
+        serverConn = ConnStateData::BorrowPinnedConnection(request, al);
+        debugs(17, 5, "connection: " << serverConn);
+    } catch (ErrorState * const anErr) {
+        syncHierNote(nullptr, connManager ? connManager->pinning.host : request->url.host());
         serverConn = nullptr;
-        const auto anErr = new ErrorState(ERR_ZERO_SIZE_OBJECT, Http::scServiceUnavailable, request, al);
         fail(anErr);
         // Connection managers monitor their idle pinned to-server
         // connections and close from-client connections upon seeing
@@ -1058,7 +1033,7 @@ FwdState::usePinned()
 
     // the server may close the pinned connection before this request
     const auto reused = true;
-    syncWithServerConn(temp, connManager->pinning.host, reused);
+    syncWithServerConn(serverConn, connManager->pinning.host, reused);
 
     dispatch();
 }
index 693a7d95080a2a1b60becffaebd3daee9ed98f5e..67d689b8deffdf26d3766f353a0a976e4571898e 100644 (file)
@@ -2204,6 +2204,7 @@ ConnStateData::ConnStateData(const MasterXaction::Pointer &xact) :
     pinning.pinned = false;
     pinning.auth = false;
     pinning.zeroReply = false;
+    pinning.peerAccessDenied = false;
     pinning.peer = NULL;
 
     // store the details required for creating more MasterXaction objects as new requests come in
@@ -3908,37 +3909,47 @@ ConnStateData::clientPinnedConnectionRead(const CommIoCbParams &io)
         clientConnection->close();
 }
 
-const Comm::ConnectionPointer
-ConnStateData::validatePinnedConnection(HttpRequest *request)
+Comm::ConnectionPointer
+ConnStateData::borrowPinnedConnection(HttpRequest *request, const AccessLogEntryPointer &ale)
 {
-    debugs(33, 7, HERE << pinning.serverConnection);
+    debugs(33, 7, pinning.serverConnection);
+    Must(request);
 
-    bool valid = true;
-    if (!Comm::IsConnOpen(pinning.serverConnection))
-        valid = false;
-    else if (pinning.auth && pinning.host && request && strcasecmp(pinning.host, request->url.host()) != 0)
-        valid = false;
-    else if (request && pinning.port != request->url.port())
-        valid = false;
-    else if (pinning.peer && !cbdataReferenceValid(pinning.peer))
-        valid = false;
-
-    if (!valid) {
-        /* The pinning info is not safe, remove any pinning info */
+    const auto pinningError = [&](const err_type type) {
         unpinConnection(true);
-    }
+        HttpRequestPointer requestPointer = request;
+        return ErrorState::NewForwarding(type, requestPointer, ale);
+    };
+
+    if (!Comm::IsConnOpen(pinning.serverConnection))
+        throw pinningError(ERR_ZERO_SIZE_OBJECT);
 
+    if (pinning.auth && pinning.host && strcasecmp(pinning.host, request->url.host()) != 0)
+        throw pinningError(ERR_CANNOT_FORWARD); // or generalize ERR_CONFLICT_HOST
+
+    if (pinning.port != request->url.port())
+        throw pinningError(ERR_CANNOT_FORWARD); // or generalize ERR_CONFLICT_HOST
+
+    if (pinning.peer && !cbdataReferenceValid(pinning.peer))
+        throw pinningError(ERR_ZERO_SIZE_OBJECT);
+
+    if (pinning.peerAccessDenied)
+        throw pinningError(ERR_CANNOT_FORWARD); // or generalize ERR_FORWARDING_DENIED
+
+    stopPinnedConnectionMonitoring();
     return pinning.serverConnection;
 }
 
 Comm::ConnectionPointer
-ConnStateData::borrowPinnedConnection(HttpRequest *request)
+ConnStateData::BorrowPinnedConnection(HttpRequest *request, const AccessLogEntryPointer &ale)
 {
-    debugs(33, 7, pinning.serverConnection);
-    if (validatePinnedConnection(request) != nullptr)
-        stopPinnedConnectionMonitoring();
+    if (const auto connManager = request ? request->pinnedConnection() : nullptr)
+        return connManager->borrowPinnedConnection(request, ale);
 
-    return pinning.serverConnection; // closed if validation failed
+    // ERR_CANNOT_FORWARD is somewhat misleading here; we can still forward, but
+    // there is no point since the client connection is now gone
+    HttpRequestPointer requestPointer = request;
+    throw ErrorState::NewForwarding(ERR_CANNOT_FORWARD, requestPointer, ale);
 }
 
 void
@@ -3966,6 +3977,7 @@ ConnStateData::unpinConnection(const bool andClose)
     safe_free(pinning.host);
 
     pinning.zeroReply = false;
+    pinning.peerAccessDenied = false;
 
     /* NOTE: pinning.pinned should be kept. This combined with fd == -1 at the end of a request indicates that the host
      * connection has gone away */
index 73fd59690317dec06c548c861cbc1e6fa4a04a51..97183831b22cc97726faee6240d9d5027ff5be6a 100644 (file)
@@ -18,6 +18,7 @@
 #include "http/forward.h"
 #include "HttpControlMsg.h"
 #include "ipc/FdNotes.h"
+#include "log/forward.h"
 #include "proxyp/forward.h"
 #include "sbuf/SBuf.h"
 #include "servers/Server.h"
@@ -139,6 +140,7 @@ public:
         bool auth;               /* pinned for www authentication */
         bool reading;   ///< we are monitoring for peer connection closure
         bool zeroReply; ///< server closed w/o response (ERR_ZERO_SIZE_OBJECT)
+        bool peerAccessDenied; ///< cache_peer_access denied pinned connection reuse
         CachePeer *peer;             /* CachePeer the connection goes via */
         AsyncCall::Pointer readHandler; ///< detects serverConnection closure
         AsyncCall::Pointer closeHandler; /*The close handler for pinned server side connection*/
@@ -181,15 +183,10 @@ public:
     void pinBusyConnection(const Comm::ConnectionPointer &pinServerConn, const HttpRequest::Pointer &request);
     /// Undo pinConnection() and, optionally, close the pinned connection.
     void unpinConnection(const bool andClose);
-    /// Returns validated pinnned server connection (and stops its monitoring).
-    Comm::ConnectionPointer borrowPinnedConnection(HttpRequest *request);
-    /**
-     * Checks if there is pinning info if it is valid. It can close the server side connection
-     * if pinned info is not valid.
-     \param request   if it is not NULL also checks if the pinning info refers to the request client side HttpRequest
-     \return          The details of the server side connection (may be closed if failures were present).
-     */
-    const Comm::ConnectionPointer validatePinnedConnection(HttpRequest *request);
+
+    /// \returns validated pinned to-server connection, stopping its monitoring
+    /// \throws a newly allocated ErrorState if validation fails
+    static Comm::ConnectionPointer BorrowPinnedConnection(HttpRequest *, const AccessLogEntryPointer &);
     /**
      * returts the pinned CachePeer if exists, NULL otherwise
      */
@@ -333,6 +330,9 @@ protected:
     void abortChunkedRequestBody(const err_type error);
     err_type handleChunkedRequestBody();
 
+    /// ConnStateData-specific part of BorrowPinnedConnection()
+    Comm::ConnectionPointer borrowPinnedConnection(HttpRequest *, const AccessLogEntryPointer &);
+
     void startPinnedConnectionMonitoring();
     void clientPinnedConnectionRead(const CommIoCbParams &io);
 #if USE_OPENSSL
index 7bfb91b86580a292b94e44aa3605ba02760f76b3..61a7c59b3ef8abddc9758b1f9518c17e16404433 100644 (file)
@@ -564,17 +564,15 @@ PeerSelector::selectPinned()
     if (!request->pinnedConnection())
         return;
 
-    if (Comm::IsConnOpen(request->pinnedConnection()->validatePinnedConnection(request))) {
-        const auto pear = request->pinnedConnection()->pinnedPeer();
-        const bool usePinned = pear ? peerAllowedToUse(pear, this) : (direct != DIRECT_NO);
-        if (usePinned) {
-            addSelection(pear, PINNED);
-            if (entry)
-                entry->ping_status = PING_DONE; // skip ICP
-        }
-    }
-    // If the pinned connection is prohibited (for this request) or gone, then
+    const auto peer = request->pinnedConnection()->pinnedPeer();
+    const auto usePinned = peer ? peerAllowedToUse(peer, this) : (direct != DIRECT_NO);
+    // If the pinned connection is prohibited (for this request) then
     // the initiator must decide whether it is OK to open a new one instead.
+    request->pinnedConnection()->pinning.peerAccessDenied = !usePinned;
+
+    addSelection(peer, PINNED);
+    if (entry)
+        entry->ping_status = PING_DONE; // skip ICP
 }
 
 /**
index 3986130b7cbeb2c1a7c76a831339febfb2bba1bd..c97433aace85621caf5a797f361c5c1413ff54cd 100644 (file)
@@ -34,7 +34,7 @@ bool ConnStateData::handleRequestBodyData() STUB_RETVAL(false)
 void ConnStateData::pinBusyConnection(const Comm::ConnectionPointer &, const HttpRequest::Pointer &) STUB
 void ConnStateData::notePinnedConnectionBecameIdle(PinnedIdleContext) STUB
 void ConnStateData::unpinConnection(const bool) STUB
-const Comm::ConnectionPointer ConnStateData::validatePinnedConnection(HttpRequest *) STUB_RETVAL(nullptr)
+Comm::ConnectionPointer ConnStateData::BorrowPinnedConnection(HttpRequest *, const AccessLogEntryPointer &) STUB_RETVAL(nullptr)
 void ConnStateData::clientPinnedConnectionClosed(const CommCloseCbParams &) STUB
 void ConnStateData::connStateClosed(const CommCloseCbParams &) STUB
 void ConnStateData::requestTimeout(const CommTimeoutCbParams &) STUB
index 32d76466fa0b94411e951e81b52c4b180f8427ba..4c3d9e1672df0abbb2389114259f88c85eb1b293 100644 (file)
@@ -1034,18 +1034,6 @@ TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer)
     // and wait for the tunnelEstablishmentDone() call
 }
 
-static Comm::ConnectionPointer
-borrowPinnedConnection(HttpRequest *request)
-{
-    // pinned_connection may become nil after a pconn race
-    if (ConnStateData *pinned_connection = request ? request->pinnedConnection() : nullptr) {
-        Comm::ConnectionPointer serverConn = pinned_connection->borrowPinnedConnection(request);
-        return serverConn;
-    }
-
-    return nullptr;
-}
-
 void
 TunnelStateData::noteDestination(Comm::ConnectionPointer path)
 {
@@ -1184,27 +1172,26 @@ TunnelStateData::usePinned()
 {
     Must(request);
     const auto connManager = request->pinnedConnection();
-    const auto serverConn = borrowPinnedConnection(request.getRaw());
-    debugs(26,7, "pinned peer connection: " << serverConn);
-    if (!Comm::IsConnOpen(serverConn)) {
-        syncHierNote(serverConn, connManager ? connManager->pinning.host : request->url.host());
+    try {
+        const auto serverConn = ConnStateData::BorrowPinnedConnection(request.getRaw(), al);
+        debugs(26, 7, "pinned peer connection: " << serverConn);
+
+        // Set HttpRequest pinned related flags for consistency even if
+        // they are not really used by tunnel.cc code.
+        request->flags.pinned = true;
+        if (connManager->pinnedAuth())
+            request->flags.auth = true;
+
+        // the server may close the pinned connection before this request
+        const auto reused = true;
+        connectDone(serverConn, connManager->pinning.host, reused);
+    } catch (ErrorState * const error) {
+        syncHierNote(nullptr, connManager ? connManager->pinning.host : request->url.host());
         // a PINNED path failure is fatal; do not wait for more paths
-        sendError(new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request.getRaw(), al),
-                  "pinned path failure");
+        sendError(error, "pinned path failure");
         return;
     }
 
-    Must(connManager);
-
-    // Set HttpRequest pinned related flags for consistency even if
-    // they are not really used by tunnel.cc code.
-    request->flags.pinned = true;
-    if (connManager->pinnedAuth())
-        request->flags.auth = true;
-
-    // the server may close the pinned connection before this request
-    const auto reused = true;
-    connectDone(serverConn, connManager->pinning.host, reused);
 }
 
 CBDATA_CLASS_INIT(TunnelStateData);