From: Alex Rousskov Date: Mon, 9 Aug 2021 21:47:07 +0000 (-0400) Subject: Improved Security::PeerConnector::serverConn management X-Git-Url: http://git.ipfire.org/gitweb/gitweb.cgi?a=commitdiff_plain;h=4e5ae02a71e157e64acb7c40be23b444f16c01ef;p=thirdparty%2Fsquid.git Improved Security::PeerConnector::serverConn management Similar to the previous branch commit When sending a negative answer, we would set answer().conn to an open connection, async-send the answer, and then hurry to close the connection using our pointer to the shared Connection object. If everything went according to the plan, the recipient would get a non-nil but closed Connection object. Now, a negative answer simply means no connection at all. Same for a tunneled answer. Probably fixed a bug in the official code where PeerConnector::negotiate() assumed that a sslFinalized() does not return true after callBack(). It may (e.g., when CertValidationHelper::Submit() throws). Same for PeekingPeerConnector::checkForPeekAndSpliceMatched(). Also reduced Security::PeerConnector exposure to a closed Connection object when serverConn is closed externally. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index 4371b8ea5d..c02b5833bf 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -1001,9 +1001,10 @@ FwdState::connectedToPeer(Security::EncryptorAnswer &answer) ErrorState *error = nullptr; if ((error = answer.error.get())) { - Must(!Comm::IsConnOpen(answer.conn)); + assert(!answer.conn); answer.error.clear(); // preserve error for errorSendComplete() } else if (answer.tunneled) { + assert(!answer.conn); // TODO: When ConnStateData establishes tunnels, its state changes // [in ways that may affect logging?]. Consider informing // ConnStateData about our tunnel or otherwise unifying tunnel diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc index 996997d57d..9e0bf1b4d0 100644 --- a/src/PeerPoolMgr.cc +++ b/src/PeerPoolMgr.cc @@ -145,14 +145,16 @@ PeerPoolMgr::handleSecuredPeer(Security::EncryptorAnswer &answer) return; } + assert(!answer.tunneled); if (answer.error.get()) { - if (answer.conn != NULL) - answer.conn->close(); + assert(!answer.conn); // PeerConnector calls peerConnectFailed() for us; checkpoint("conn securing failure"); // may retry return; } + // TODO: Check for .closing() like FwdState::connectedToPeer() does? + pushNewConnection(answer.conn); } diff --git a/src/adaptation/icap/Xaction.cc b/src/adaptation/icap/Xaction.cc index 77360fb88d..6e4cf75ab4 100644 --- a/src/adaptation/icap/Xaction.cc +++ b/src/adaptation/icap/Xaction.cc @@ -714,10 +714,9 @@ Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer) { encryptionWait.finish(); + assert(!answer.tunneled); if (answer.error.get()) { - // XXX: Security::PeerConnector should do that for negative answers instead. - if (answer.conn != NULL) - answer.conn->close(); + assert(!answer.conn); // TODO: Refactor dieOnConnectionFailure() to be usable here as well. debugs(93, 2, typeName << " TLS negotiation to " << service().cfg().uri << " failed"); @@ -729,6 +728,7 @@ Adaptation::Icap::Xaction::handleSecuredPeer(Security::EncryptorAnswer &answer) debugs(93, 5, "TLS negotiation to " << service().cfg().uri << " complete"); // XXX: answer.conn could be closing here. Missing a syncWithComm equivalent? + // TODO: Check for .closing() like FwdState::connectedToPeer() does? useIcapConnection(answer.conn); } diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index d28169f4df..d06e281ee2 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -92,9 +92,7 @@ Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) closeHandler = nullptr; if (serverConn) { - // TODO: Calling PconnPool::noteUses() should not be our responsibility. - if (noteFwdPconnUse && serverConn->isOpen()) - fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); + countFailingConnection(); serverConn->noteClosure(); serverConn = nullptr; } @@ -223,7 +221,8 @@ Security::PeerConnector::negotiate() if (!sslFinalized()) return; - sendSuccess(); + if (callback) // true sslFinalized() may bail(), calling the callback + sendSuccess(); } bool @@ -298,7 +297,8 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe if (!errDetails && !validatorFailed) { noteNegotiationDone(NULL); - sendSuccess(); + if (callback) + sendSuccess(); return; } @@ -563,66 +563,76 @@ Security::PeerConnector::noteNegotiationError(const int ret, const int ssl_error bail(anErr); } +Security::EncryptorAnswer & +Security::PeerConnector::answer() +{ + assert(callback); + const auto dialer = dynamic_cast(callback->getDialer()); + assert(dialer); + return dialer->answer(); +} + void Security::PeerConnector::bail(ErrorState *error) { Must(error); // or the recipient will not know there was a problem - Must(callback != NULL); - CbDialer *dialer = dynamic_cast(callback->getDialer()); - Must(dialer); - dialer->answer().error = error; - - if (serverConnection()) { - if (const auto p = serverConnection()->getPeer()) - peerConnectFailed(p); + answer().error = error; + + if (const auto failingConnection = serverConn) { + countFailingConnection(); + disconnect(); + failingConnection->close(); } callBack(); - disconnect(); - - // TODO: Close before callBack(); do not pretend to send an open connection. - if (Comm::IsConnOpen(serverConn)) { - if (noteFwdPconnUse) - fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); - assert(!closeHandler); // the above disconnect() removes it - serverConn->close(); - } - serverConn = nullptr; } void Security::PeerConnector::sendSuccess() { - callBack(); + assert(Comm::IsConnOpen(serverConn)); + answer().conn = serverConn; disconnect(); + callBack(); +} + +void +Security::PeerConnector::countFailingConnection() +{ + assert(serverConn); + if (const auto p = serverConn->getPeer()) + peerConnectFailed(p); + // TODO: Calling PconnPool::noteUses() should not be our responsibility. + if (noteFwdPconnUse && serverConn->isOpen()) + fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); } void Security::PeerConnector::disconnect() { - if (!Comm::IsConnOpen(serverConnection())) - return; + const auto stillOpen = Comm::IsConnOpen(serverConn); if (closeHandler) { - comm_remove_close_handler(serverConnection()->fd, closeHandler); + if (stillOpen) + comm_remove_close_handler(serverConn->fd, closeHandler); closeHandler = nullptr; } - commUnsetConnTimeout(serverConnection()); + if (stillOpen) + commUnsetConnTimeout(serverConn); + + serverConn = nullptr; } void Security::PeerConnector::callBack() { - debugs(83, 5, "TLS setup ended for " << serverConnection()); + debugs(83, 5, "TLS setup ended for " << answer().conn); AsyncCall::Pointer cb = callback; // Do this now so that if we throw below, swanSong() assert that we _tried_ // to call back holds. callback = NULL; // this should make done() true - CbDialer *dialer = dynamic_cast(cb->getDialer()); - Must(dialer); - dialer->answer().conn = serverConnection(); // may be nil ScheduleCallHere(cb); } diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index 46bcca963f..f886b6ee76 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -150,6 +150,9 @@ protected: /// a bail(), sendSuccess() helper: stops monitoring the connection void disconnect(); + /// updates connection usage history before the connection is closed + void countFailingConnection(); + /// If called the certificates validator will not used void bypassCertValidator() {useCertValidator_ = false;} @@ -157,6 +160,9 @@ protected: /// logging void recordNegotiationDetails(); + /// convenience method to get to the answer fields + EncryptorAnswer &answer(); + HttpRequestPointer request; ///< peer connection trigger or cause Comm::ConnectionPointer serverConn; ///< TCP connection to the peer AccessLogEntryPointer al; ///< info for the future access.log entry diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index e48e39b35b..088f8290d6 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -105,10 +105,8 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode acti splice = true; // Ssl Negotiation stops here. Last SSL checks for valid certificates // and if done, switch to tunnel mode - if (sslFinalized()) { - debugs(83,5, "Abort NegotiateSSL on FD " << serverConn->fd << " and splice the connection"); + if (sslFinalized() && callback) callBack(); - } } } @@ -270,8 +268,11 @@ Ssl::PeekingPeerConnector::startTunneling() auto b = SSL_get_rbio(session.get()); auto srvBio = static_cast(BIO_get_data(b)); + debugs(83, 5, "will tunnel instead of negotiating TLS"); switchToTunnel(request.getRaw(), clientConn, serverConn, srvBio->rBufData()); - tunnelInsteadOfNegotiating(); + answer().tunneled = true; + disconnect(); + callBack(); } void @@ -392,13 +393,4 @@ Ssl::PeekingPeerConnector::serverCertificateVerified() } } -void -Ssl::PeekingPeerConnector::tunnelInsteadOfNegotiating() -{ - Must(callback != NULL); - CbDialer *dialer = dynamic_cast(callback->getDialer()); - Must(dialer); - dialer->answer().tunneled = true; - debugs(83, 5, "The SSL negotiation with server aborted"); -} diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index 9ee690ec4b..e11d62a564 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -70,7 +70,9 @@ void PeerConnector::bail(ErrorState *) STUB void PeerConnector::sendSuccess() STUB void PeerConnector::callBack() STUB void PeerConnector::disconnect() STUB +void PeerConnector::countFailingConnection() STUB void PeerConnector::recordNegotiationDetails() STUB +EncryptorAnswer &PeerConnector::answer() STUB_RETREF(EncryptorAnswer) } #include "security/PeerOptions.h" diff --git a/src/tunnel.cc b/src/tunnel.cc index 6d5eddd447..b7ce574cb3 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -979,7 +979,7 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) if (!answer.positive()) { sawProblem = true; - Must(!answer.conn); + assert(!answer.conn); } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { sawProblem = true; closePendingConnection(answer.conn, "conn was closed while waiting for tunnelEstablishmentDone"); @@ -1199,8 +1199,9 @@ TunnelStateData::noteSecurityPeerConnectorAnswer(Security::EncryptorAnswer &answ encryptionWait.finish(); ErrorState *error = nullptr; + assert(!answer.tunneled); if ((error = answer.error.get())) { - Must(!Comm::IsConnOpen(answer.conn)); + assert(!answer.conn); answer.error.clear(); } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request.getRaw(), al);