From: Christos Tsantilas Date: Mon, 9 Sep 2019 14:14:50 +0000 (+0000) Subject: Fix "BUG: Lost previously bumped from-Squid connection" (#460) X-Git-Tag: SQUID_5_0_1~48 X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=daf80700130b6f98256b75c511916d1a79b23597;p=thirdparty%2Fsquid.git Fix "BUG: Lost previously bumped from-Squid connection" (#460) 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. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index bd4d9440d3..f8954f7185 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -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(); } diff --git a/src/client_side.cc b/src/client_side.cc index 693a7d9508..67d689b8de 100644 --- a/src/client_side.cc +++ b/src/client_side.cc @@ -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 */ diff --git a/src/client_side.h b/src/client_side.h index 73fd596903..97183831b2 100644 --- a/src/client_side.h +++ b/src/client_side.h @@ -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 diff --git a/src/peer_select.cc b/src/peer_select.cc index 7bfb91b865..61a7c59b3e 100644 --- a/src/peer_select.cc +++ b/src/peer_select.cc @@ -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 } /** diff --git a/src/tests/stub_client_side.cc b/src/tests/stub_client_side.cc index 3986130b7c..c97433aace 100644 --- a/src/tests/stub_client_side.cc +++ b/src/tests/stub_client_side.cc @@ -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 diff --git a/src/tunnel.cc b/src/tunnel.cc index 32d76466fa..4c3d9e1672 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -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);