From: Christos Tsantilas Date: Thu, 21 May 2020 22:22:22 +0000 (+0000) Subject: Detail TLS and CONNECT cache_peer negotiation failures (#518) X-Git-Tag: 4.15-20210522-snapshot~112 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=25b0ce4;p=thirdparty%2Fsquid.git Detail TLS and CONNECT cache_peer negotiation failures (#518) Before PeerConnector and Tunneler were introduced, FwdState and TunnelStateData naturally owned their to-server connection. When CONNECT and TLS negotiation were outsourced, we kept that ownership to minimize changes and simplify negotiation code. That was wrong because FwdState and TunnelStateData, as connection owners, had to monitor for connection closures but could not distinguish basic TCP peer closures from complex CONNECT/TLS negotiation failures that required further detailing. The user got generic error messages instead of details known to negotiators. Now, Ssl::PeerConnector and Http::Tunneler jobs own the connection they work with and, hence, are responsible for monitoring it and, upon successful negotiation, returning it to the initiators. In case of problems, these jobs send detailed errors to the initiators instead. Passing connection ownership to and from a helper job is difficult because the connection may be either closed or begin to close (e.g. by shutdown) while the callback is pending without working close handlers. Many changes focus on keeping Connection::fd in sync with Comm. Also improved tunnel.cc mimicking of (better) FwdState code: Partially open connections after Comm::ConnOpener failures are now closed, and Http::Tunneler failures are now retried. This is a Measurement Factory project. --- diff --git a/src/FwdState.cc b/src/FwdState.cc index e08781d9a0..6f32877b02 100644 --- a/src/FwdState.cc +++ b/src/FwdState.cc @@ -118,6 +118,18 @@ FwdState::abort(void* d) fwd->stopAndDestroy("store entry aborted"); } +void +FwdState::closePendingConnection(const Comm::ConnectionPointer &conn, const char *reason) +{ + debugs(17, 3, "because " << reason << "; " << conn); + assert(!serverConn); + assert(!closeHandler); + if (IsConnOpen(conn)) { + fwdPconnPool->noteUses(fd_table[conn->fd].pconn.uses); + conn->close(); + } +} + void FwdState::closeServerConnection(const char *reason) { @@ -753,6 +765,24 @@ FwdState::handleUnregisteredServerEnd() retryOrBail(); } +/// starts a preparation step for an established connection; retries on failures +template +void +FwdState::advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep) +{ + // TODO: Extract destination-specific handling from FwdState so that all the + // awkward, limited-scope advanceDestination() calls can be replaced with a + // single simple try/catch,retry block. + try { + startStep(); + // now wait for the step callback + } catch (...) { + debugs (17, 2, "exception while trying to " << stepDescription << ": " << CurrentException); + closePendingConnection(conn, "connection preparation exception"); + retryOrBail(); + } +} + /// called when a to-peer connection has been successfully obtained or /// when all candidate destinations have been tried and all have failed void @@ -764,22 +794,31 @@ FwdState::noteConnection(HappyConnOpener::Answer &answer) Must(n_tries <= answer.n_tries); // n_tries cannot decrease n_tries = answer.n_tries; - if (const auto error = answer.error.get()) { + ErrorState *error = nullptr; + if ((error = answer.error.get())) { flags.dont_retry = true; // or HappyConnOpener would not have given up syncHierNote(answer.conn, request->url.host()); - fail(error); + Must(!Comm::IsConnOpen(answer.conn)); answer.error.clear(); // preserve error for errorSendComplete() + } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { + syncHierNote(answer.conn, request->url.host()); + closePendingConnection(answer.conn, "conn was closed while waiting for noteConnection"); + error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al); + } + + if (error) { + fail(error); retryOrBail(); // will notice flags.dont_retry and bail return; } - syncWithServerConn(answer.conn, request->url.host(), answer.reused); - - if (answer.reused) + if (answer.reused) { + syncWithServerConn(answer.conn, request->url.host(), answer.reused); return dispatch(); + } // Check if we need to TLS before use - if (const CachePeer *peer = serverConnection()->getPeer()) { + if (const auto *peer = answer.conn->getPeer()) { // Assume that it is only possible for the client-first from the // bumping modes to try connect to a remote server. The bumped // requests with other modes are using pinned connections or fails. @@ -793,20 +832,27 @@ FwdState::noteConnection(HappyConnOpener::Answer &answer) if (originWantsEncryptedTraffic && // the "encrypted traffic" part !peer->options.originserver && // the "through a proxy" part !peer->secure.encryptTransport) // the "exclude HTTPS proxies" part - return establishTunnelThruProxy(); + return advanceDestination("establish tunnel through proxy", answer.conn, [this,&answer] { + establishTunnelThruProxy(answer.conn); + }); } - secureConnectionToPeerIfNeeded(); + secureConnectionToPeerIfNeeded(answer.conn); } void -FwdState::establishTunnelThruProxy() +FwdState::establishTunnelThruProxy(const Comm::ConnectionPointer &conn) { AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::tunnelEstablishmentDone", Http::Tunneler::CbDialer(&FwdState::tunnelEstablishmentDone, this)); HttpRequest::Pointer requestPointer = request; - const auto tunneler = new Http::Tunneler(serverConnection(), requestPointer, callback, connectingTimeout(serverConnection()), al); + const auto tunneler = new Http::Tunneler(conn, requestPointer, callback, connectingTimeout(serverConnection()), al); + + // TODO: Replace this hack with proper Comm::Connection-Pool association + // that is not tied to fwdPconnPool and can handle disappearing pools. + tunneler->noteFwdPconnUse = true; + #if USE_DELAY_POOLS Must(serverConnection()->getPeer()); if (!serverConnection()->getPeer()->options.no_delay) @@ -820,11 +866,18 @@ FwdState::establishTunnelThruProxy() void FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) { - if (answer.positive()) { - if (answer.leftovers.isEmpty()) { - secureConnectionToPeerIfNeeded(); - return; - } + ErrorState *error = nullptr; + if (!answer.positive()) { + Must(!Comm::IsConnOpen(answer.conn)); + error = answer.squidError.get(); + Must(error); + answer.squidError.clear(); // preserve error for fail() + } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { + // The socket could get closed while our callback was queued. + // We close Connection here to sync Connection::fd. + closePendingConnection(answer.conn, "conn was closed while waiting for tunnelEstablishmentDone"); + error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al); + } else if (!answer.leftovers.isEmpty()) { // This should not happen because TLS servers do not speak first. If we // have to handle this, then pass answer.leftovers via a PeerConnector // to ServerBio. See ClientBio::setReadBufData(). @@ -832,33 +885,26 @@ FwdState::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) const auto level = (occurrences++ < 100) ? DBG_IMPORTANT : 2; debugs(17, level, "ERROR: Early data after CONNECT response. " << "Found " << answer.leftovers.length() << " bytes. " << - "Closing " << serverConnection()); - fail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request, al)); - closeServerConnection("found early data after CONNECT response"); + "Closing " << answer.conn); + error = new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request, al); + closePendingConnection(answer.conn, "server spoke before tunnelEstablishmentDone"); + } + if (error) { + fail(error); retryOrBail(); return; } - // TODO: Reuse to-peer connections after a CONNECT error response. - - if (const auto peer = serverConnection()->getPeer()) - peerConnectFailed(peer); - - const auto error = answer.squidError.get(); - Must(error); - answer.squidError.clear(); // preserve error for fail() - fail(error); - closeServerConnection("Squid-generated CONNECT error"); - retryOrBail(); + secureConnectionToPeerIfNeeded(answer.conn); } /// handles an established TCP connection to peer (including origin servers) void -FwdState::secureConnectionToPeerIfNeeded() +FwdState::secureConnectionToPeerIfNeeded(const Comm::ConnectionPointer &conn) { assert(!request->flags.pinned); - const CachePeer *p = serverConnection()->getPeer(); + const auto p = conn->getPeer(); const bool peerWantsTls = p && p->secure.encryptTransport; // userWillTlsToPeerForUs assumes CONNECT == HTTPS const bool userWillTlsToPeerForUs = p && p->options.originserver && @@ -872,56 +918,70 @@ FwdState::secureConnectionToPeerIfNeeded() const bool needTlsToOrigin = !p && request->url.getScheme() == AnyP::PROTO_HTTPS && !clientFirstBump; if (needTlsToPeer || needTlsToOrigin || needsBump) { - HttpRequest::Pointer requestPointer = request; - AsyncCall::Pointer callback = asyncCall(17,4, - "FwdState::ConnectedToPeer", - FwdStatePeerAnswerDialer(&FwdState::connectedToPeer, this)); - const auto sslNegotiationTimeout = connectingTimeout(serverConnection()); - Security::PeerConnector *connector = nullptr; -#if USE_OPENSSL - if (request->flags.sslPeek) - connector = new Ssl::PeekingPeerConnector(requestPointer, serverConnection(), clientConn, callback, al, sslNegotiationTimeout); - else -#endif - connector = new Security::BlindPeerConnector(requestPointer, serverConnection(), callback, al, sslNegotiationTimeout); - AsyncJob::Start(connector); // will call our callback - return; + return advanceDestination("secure connection to peer", conn, [this,&conn] { + secureConnectionToPeer(conn); + }); } // if not encrypting just run the post-connect actions - successfullyConnectedToPeer(); + successfullyConnectedToPeer(conn); +} + +/// encrypts an established TCP connection to peer (including origin servers) +void +FwdState::secureConnectionToPeer(const Comm::ConnectionPointer &conn) +{ + HttpRequest::Pointer requestPointer = request; + AsyncCall::Pointer callback = asyncCall(17,4, + "FwdState::ConnectedToPeer", + FwdStatePeerAnswerDialer(&FwdState::connectedToPeer, this)); + const auto sslNegotiationTimeout = connectingTimeout(conn); + Security::PeerConnector *connector = nullptr; +#if USE_OPENSSL + if (request->flags.sslPeek) + connector = new Ssl::PeekingPeerConnector(requestPointer, conn, clientConn, callback, al, sslNegotiationTimeout); + else +#endif + connector = new Security::BlindPeerConnector(requestPointer, conn, callback, al, sslNegotiationTimeout); + connector->noteFwdPconnUse = true; + AsyncJob::Start(connector); // will call our callback } /// called when all negotiations with the TLS-speaking peer have been completed void FwdState::connectedToPeer(Security::EncryptorAnswer &answer) { - if (ErrorState *error = answer.error.get()) { - fail(error); + ErrorState *error = nullptr; + if ((error = answer.error.get())) { + Must(!Comm::IsConnOpen(answer.conn)); answer.error.clear(); // preserve error for errorSendComplete() - if (CachePeer *p = serverConnection()->getPeer()) - peerConnectFailed(p); - serverConnection()->close(); - return; - } - - if (answer.tunneled) { + } else if (answer.tunneled) { // TODO: When ConnStateData establishes tunnels, its state changes // [in ways that may affect logging?]. Consider informing // ConnStateData about our tunnel or otherwise unifying tunnel // establishment [side effects]. - unregister(serverConn); // async call owns it now complete(); // destroys us return; + } else if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { + closePendingConnection(answer.conn, "conn was closed while waiting for connectedToPeer"); + error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request, al); } - successfullyConnectedToPeer(); + if (error) { + fail(error); + retryOrBail(); + return; + } + + successfullyConnectedToPeer(answer.conn); } /// called when all negotiations with the peer have been completed void -FwdState::successfullyConnectedToPeer() +FwdState::successfullyConnectedToPeer(const Comm::ConnectionPointer &conn) { + syncWithServerConn(conn, request->url.host(), false); + // should reach ConnStateData before the dispatched Client job starts CallJobHere1(17, 4, request->clientConnectionManager, ConnStateData, ConnStateData::notePeerConnection, serverConnection()); @@ -1146,7 +1206,7 @@ FwdState::dispatch() // Set the dont_retry flag because this is not a transient (network) error. flags.dont_retry = true; if (Comm::IsConnOpen(serverConn)) { - serverConn->close(); + serverConn->close(); // trigger cleanup } break; } diff --git a/src/FwdState.h b/src/FwdState.h index 6881726747..f9cd6d37b2 100644 --- a/src/FwdState.h +++ b/src/FwdState.h @@ -102,6 +102,9 @@ public: void dontRetry(bool val) { flags.dont_retry = val; } + /// get rid of a to-server connection that failed to become serverConn + void closePendingConnection(const Comm::ConnectionPointer &conn, const char *reason); + /** return a ConnectionPointer to the current server connection (may or may not be open) */ Comm::ConnectionPointer const & serverConnection() const { return serverConn; }; @@ -131,14 +134,18 @@ private: /// (in order to retry or reforward a failed request) bool pinnedCanRetry() const; + template + void advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep); + ErrorState *makeConnectingError(const err_type type) const; void connectedToPeer(Security::EncryptorAnswer &answer); static void RegisterWithCacheManager(void); - void establishTunnelThruProxy(); + void establishTunnelThruProxy(const Comm::ConnectionPointer &); void tunnelEstablishmentDone(Http::TunnelerAnswer &answer); - void secureConnectionToPeerIfNeeded(); - void successfullyConnectedToPeer(); + void secureConnectionToPeerIfNeeded(const Comm::ConnectionPointer &); + void secureConnectionToPeer(const Comm::ConnectionPointer &); + void successfullyConnectedToPeer(const Comm::ConnectionPointer &); /// stops monitoring server connection for closure and updates pconn stats void closeServerConnection(const char *reason); diff --git a/src/clients/HttpTunneler.cc b/src/clients/HttpTunneler.cc index fe05eb61aa..a4957f73f3 100644 --- a/src/clients/HttpTunneler.cc +++ b/src/clients/HttpTunneler.cc @@ -18,6 +18,8 @@ #include "http/one/ResponseParser.h" #include "http/StateFlags.h" #include "HttpRequest.h" +#include "neighbors.h" +#include "pconn.h" #include "SquidConfig.h" #include "StatCounters.h" @@ -25,6 +27,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Http, Tunneler); Http::Tunneler::Tunneler(const Comm::ConnectionPointer &conn, const HttpRequest::Pointer &req, AsyncCall::Pointer &aCallback, time_t timeout, const AccessLogEntryPointer &alp): AsyncJob("Http::Tunneler"), + noteFwdPconnUse(false), connection(conn), request(req), callback(aCallback), @@ -41,6 +44,7 @@ Http::Tunneler::Tunneler(const Comm::ConnectionPointer &conn, const HttpRequest: assert(callback); assert(dynamic_cast(callback->getDialer())); url = request->url.authority(); + watchForClosures(); } Http::Tunneler::~Tunneler() @@ -73,11 +77,22 @@ Http::Tunneler::start() Must(url.length()); Must(lifetimeLimit >= 0); + // we own this Comm::Connection object and its fd exclusively, but must bail + // if others started closing the socket while we were waiting to start() + assert(Comm::IsConnOpen(connection)); + if (fd_table[connection->fd].closing()) { + bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); + return; + } + const auto peer = connection->getPeer(); - Must(peer); // bail if our peer was reconfigured away + // bail if our peer was reconfigured away + if (!peer) { + bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scInternalServerError, request.getRaw(), al)); + return; + } request->prepForPeering(*peer); - watchForClosures(); writeRequest(); startReadingResponse(); } @@ -85,8 +100,8 @@ Http::Tunneler::start() void Http::Tunneler::handleConnectionClosure(const CommCloseCbParams ¶ms) { - mustStop("server connection gone"); - callback = nullptr; // the caller must monitor closures + closer = nullptr; + bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); } /// make sure we quit if/when the connection is gone @@ -104,12 +119,11 @@ Http::Tunneler::watchForClosures() comm_add_close_handler(connection->fd, closer); } +/// The connection read timeout callback handler. void -Http::Tunneler::handleException(const std::exception& e) +Http::Tunneler::handleTimeout(const CommTimeoutCbParams &) { - debugs(83, 2, e.what() << status()); - connection->close(); - bailWith(new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw(), al)); + bailWith(new ErrorState(ERR_CONNECT_FAIL, Http::scGatewayTimeout, request.getRaw(), al)); } void @@ -255,8 +269,11 @@ Http::Tunneler::readMore() Comm::Read(connection, reader); AsyncCall::Pointer nil; + typedef CommCbMemFunT TimeoutDialer; + AsyncCall::Pointer timeoutCall = JobCallback(93, 5, + TimeoutDialer, this, Http::Tunneler::handleTimeout); const auto timeout = Comm::MortalReadTimeout(startTime, lifetimeLimit); - commSetConnTimeout(connection, timeout, nil); + commSetConnTimeout(connection, timeout, timeoutCall); } /// Parses [possibly incomplete] CONNECT response and reacts to it. @@ -342,13 +359,51 @@ Http::Tunneler::bailWith(ErrorState *error) { Must(error); answer().squidError = error; + + if (const auto p = connection->getPeer()) + peerConnectFailed(p); + callBack(); + disconnect(); + + if (noteFwdPconnUse) + fwdPconnPool->noteUses(fd_table[connection->fd].pconn.uses); + // TODO: Reuse to-peer connections after a CONNECT error response. + connection->close(); + connection = nullptr; +} + +void +Http::Tunneler::sendSuccess() +{ + assert(answer().positive()); + callBack(); + disconnect(); +} + +void +Http::Tunneler::disconnect() +{ + if (closer) { + comm_remove_close_handler(connection->fd, closer); + closer = nullptr; + } + + if (reader) { + Comm::ReadCancel(connection->fd, reader); + reader = nullptr; + } + + // remove connection timeout handler + commUnsetConnTimeout(connection); } void Http::Tunneler::callBack() { debugs(83, 5, connection << status()); + if (answer().positive()) + answer().conn = connection; auto cb = callback; callback = nullptr; ScheduleCallHere(cb); @@ -361,8 +416,7 @@ Http::Tunneler::swanSong() if (callback) { if (requestWritten && tunnelEstablished) { - assert(answer().positive()); - callBack(); // success + sendSuccess(); } else { // we should have bailed when we discovered the job-killing problem debugs(83, DBG_IMPORTANT, "BUG: Unexpected state while establishing a CONNECT tunnel " << connection << status()); @@ -370,16 +424,6 @@ Http::Tunneler::swanSong() } assert(!callback); } - - if (closer) { - comm_remove_close_handler(connection->fd, closer); - closer = nullptr; - } - - if (reader) { - Comm::ReadCancel(connection->fd, reader); - reader = nullptr; - } } const char * diff --git a/src/clients/HttpTunneler.h b/src/clients/HttpTunneler.h index fad34d618a..08803eed99 100644 --- a/src/clients/HttpTunneler.h +++ b/src/clients/HttpTunneler.h @@ -26,15 +26,9 @@ typedef RefCount AccessLogEntryPointer; namespace Http { -/// Establishes an HTTP CONNECT tunnel through a forward proxy. -/// -/// The caller receives a call back with Http::TunnelerAnswer. -/// -/// The caller must monitor the connection for closure because this job will not -/// inform the caller about such events. -/// -/// This job never closes the connection, even on errors. If a 3rd-party closes -/// the connection, this job simply quits without informing the caller. +/// Negotiates an HTTP CONNECT tunnel through a forward proxy using a given +/// (open and, if needed, encrypted) TCP connection to that proxy. Owns the +/// connection during these negotiations. The caller receives TunnelerAnswer. class Tunneler: virtual public AsyncJob { CBDATA_CLASS(Tunneler); @@ -71,6 +65,9 @@ public: void setDelayId(DelayId delay_id) {delayId = delay_id;} #endif + /// hack: whether the connection requires fwdPconnPool->noteUses() + bool noteFwdPconnUse; + protected: /* AsyncJob API */ virtual ~Tunneler(); @@ -81,7 +78,7 @@ protected: void handleConnectionClosure(const CommCloseCbParams&); void watchForClosures(); - void handleException(const std::exception&); + void handleTimeout(const CommTimeoutCbParams &); void startReadingResponse(); void writeRequest(); void handleWrittenRequest(const CommIoCbParams&); @@ -89,9 +86,19 @@ protected: void readMore(); void handleResponse(const bool eof); void bailOnResponseError(const char *error, HttpReply *); + + /// sends the given error to the initiator void bailWith(ErrorState*); + + /// sends the ready-to-use tunnel to the initiator + void sendSuccess(); + + /// a bailWith(), sendSuccess() helper: sends results to the initiator void callBack(); + /// a bailWith(), sendSuccess() helper: stops monitoring the connection + void disconnect(); + TunnelerAnswer &answer(); private: diff --git a/src/clients/HttpTunnelerAnswer.cc b/src/clients/HttpTunnelerAnswer.cc index cf14250379..0c8ef4059d 100644 --- a/src/clients/HttpTunnelerAnswer.cc +++ b/src/clients/HttpTunnelerAnswer.cc @@ -32,6 +32,9 @@ Http::operator <<(std::ostream &os, const TunnelerAnswer &answer) if (answer.peerResponseStatus != Http::scNone) os << ' ' << answer.peerResponseStatus; + if (answer.conn) + os << ' ' << answer.conn; + os << ']'; return os; } diff --git a/src/clients/HttpTunnelerAnswer.h b/src/clients/HttpTunnelerAnswer.h index 01676d0642..03aa1f11f0 100644 --- a/src/clients/HttpTunnelerAnswer.h +++ b/src/clients/HttpTunnelerAnswer.h @@ -43,6 +43,8 @@ public: /// the status code of the successfully parsed CONNECT response (or scNone) StatusCode peerResponseStatus = scNone; + + Comm::ConnectionPointer conn; }; std::ostream &operator <<(std::ostream &, const Http::TunnelerAnswer &); diff --git a/src/security/BlindPeerConnector.cc b/src/security/BlindPeerConnector.cc index 0a4a7f273f..cfd8fd8a61 100644 --- a/src/security/BlindPeerConnector.cc +++ b/src/security/BlindPeerConnector.cc @@ -7,6 +7,7 @@ */ #include "squid.h" +#include "AccessLogEntry.h" #include "CachePeer.h" #include "comm/Connection.h" #include "errorpage.h" diff --git a/src/security/PeerConnector.cc b/src/security/PeerConnector.cc index 95a28d9d76..c9510edd4a 100644 --- a/src/security/PeerConnector.cc +++ b/src/security/PeerConnector.cc @@ -15,8 +15,11 @@ #include "Downloader.h" #include "errorpage.h" #include "fde.h" +#include "FwdState.h" #include "http/Stream.h" #include "HttpRequest.h" +#include "neighbors.h" +#include "pconn.h" #include "security/NegotiationHistory.h" #include "security/PeerConnector.h" #include "SquidConfig.h" @@ -31,6 +34,7 @@ CBDATA_NAMESPACED_CLASS_INIT(Security, PeerConnector); Security::PeerConnector::PeerConnector(const Comm::ConnectionPointer &aServerConn, AsyncCall::Pointer &aCallback, const AccessLogEntryPointer &alp, const time_t timeout) : AsyncJob("Security::PeerConnector"), + noteFwdPconnUse(false), serverConn(aServerConn), al(alp), callback(aCallback), @@ -39,14 +43,17 @@ Security::PeerConnector::PeerConnector(const Comm::ConnectionPointer &aServerCon useCertValidator_(true), certsDownloads(0) { - debugs(83, 5, "Security::PeerConnector constructed, this=" << (void*)this); + debugs(83, 5, serverConn); + // if this throws, the caller's cb dialer is not our CbDialer Must(dynamic_cast(callback->getDialer())); -} -Security::PeerConnector::~PeerConnector() -{ - debugs(83, 5, "Security::PeerConnector destructed, this=" << (void*)this); + // watch for external connection closures + Must(Comm::IsConnOpen(serverConn)); + Must(!fd_table[serverConn->fd].closing()); + typedef CommCbMemFunT Dialer; + closeHandler = JobCallback(9, 5, Dialer, this, Security::PeerConnector::commCloseHandler); + comm_add_close_handler(serverConn->fd, closeHandler); } bool Security::PeerConnector::doneAll() const @@ -61,8 +68,16 @@ Security::PeerConnector::start() AsyncJob::start(); debugs(83, 5, "this=" << (void*)this); + // we own this Comm::Connection object and its fd exclusively, but must bail + // if others started closing the socket while we were waiting to start() + assert(Comm::IsConnOpen(serverConn)); + if (fd_table[serverConn->fd].closing()) { + bail(new ErrorState(ERR_CONNECT_FAIL, Http::scBadGateway, request.getRaw(), al)); + return; + } + Security::SessionPointer tmp; - if (prepareSocket() && initialize(tmp)) + if (initialize(tmp)) negotiate(); else mustStop("Security::PeerConnector TLS socket initialize failed"); @@ -71,34 +86,25 @@ Security::PeerConnector::start() void Security::PeerConnector::commCloseHandler(const CommCloseCbParams ¶ms) { + closeHandler = nullptr; + debugs(83, 5, "FD " << params.fd << ", Security::PeerConnector=" << params.data); - connectionClosed("Security::PeerConnector::commCloseHandler"); + const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scServiceUnavailable, request.getRaw(), al); +#if USE_OPENSSL + err->detail = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, nullptr, nullptr); +#endif + bail(err); } void -Security::PeerConnector::connectionClosed(const char *reason) +Security::PeerConnector::commTimeoutHandler(const CommTimeoutCbParams &) { - debugs(83, 5, reason << " socket closed/closing. this=" << (void*)this); - mustStop(reason); - callback = NULL; -} - -bool -Security::PeerConnector::prepareSocket() -{ - debugs(83, 5, serverConnection() << ", this=" << (void*)this); - if (!Comm::IsConnOpen(serverConnection()) || fd_table[serverConnection()->fd].closing()) { - connectionClosed("Security::PeerConnector::prepareSocket"); - return false; - } - - debugs(83, 5, serverConnection()); - - // watch for external connection closures - typedef CommCbMemFunT Dialer; - closeHandler = JobCallback(9, 5, Dialer, this, Security::PeerConnector::commCloseHandler); - comm_add_close_handler(serverConnection()->fd, closeHandler); - return true; + debugs(83, 5, serverConnection() << " timedout. this=" << (void*)this); + const auto err = new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scGatewayTimeout, request.getRaw(), al); +#if USE_OPENSSL + err->detail = new Ssl::ErrorDetail(SQUID_ERR_SSL_HANDSHAKE, nullptr, nullptr); +#endif + bail(err); } bool @@ -163,9 +169,6 @@ Security::PeerConnector::recordNegotiationDetails() void Security::PeerConnector::negotiate() { - if (!Comm::IsConnOpen(serverConnection())) - return; - const int fd = serverConnection()->fd; if (fd_table[fd].closing()) return; @@ -204,7 +207,7 @@ Security::PeerConnector::negotiate() if (!sslFinalized()) return; - callBack(); + sendSuccess(); } bool @@ -241,7 +244,6 @@ Security::PeerConnector::sslFinalized() noteNegotiationDone(anErr); bail(anErr); - serverConn->close(); return true; } } @@ -259,9 +261,6 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe Ssl::ErrorDetail *errDetails = NULL; bool validatorFailed = false; - if (!Comm::IsConnOpen(serverConnection())) { - return; - } if (Debug::Enabled(83, 5)) { Security::SessionPointer ssl(fd_table[serverConnection()->fd].ssl); @@ -281,7 +280,7 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe if (!errDetails && !validatorFailed) { noteNegotiationDone(NULL); - callBack(); + sendSuccess(); return; } @@ -296,7 +295,6 @@ Security::PeerConnector::sslCrtvdHandleReply(Ssl::CertValidationResponse::Pointe noteNegotiationDone(anErr); bail(anErr); - serverConn->close(); return; } #endif @@ -473,9 +471,11 @@ Security::PeerConnector::noteWantRead() #endif // read timeout to avoid getting stuck while reading from a silent server - AsyncCall::Pointer nil; + typedef CommCbMemFunT TimeoutDialer; + AsyncCall::Pointer timeoutCall = JobCallback(83, 5, + TimeoutDialer, this, Security::PeerConnector::commTimeoutHandler); const auto timeout = Comm::MortalReadTimeout(startTime, negotiationTimeout); - commSetConnTimeout(serverConnection(), timeout, nil); + commSetConnTimeout(serverConnection(), timeout, timeoutCall); Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, new Pointer(this), 0); } @@ -545,13 +545,34 @@ Security::PeerConnector::bail(ErrorState *error) Must(dialer); dialer->answer().error = error; + if (const auto p = serverConnection()->getPeer()) + peerConnectFailed(p); + + callBack(); + disconnect(); + + if (noteFwdPconnUse) + fwdPconnPool->noteUses(fd_table[serverConn->fd].pconn.uses); + serverConn->close(); + serverConn = nullptr; +} + +void +Security::PeerConnector::sendSuccess() +{ callBack(); - // Our job is done. The callabck recipient will probably close the failed - // peer connection and try another peer or go direct (if possible). We - // can close the connection ourselves (our error notification would reach - // the recipient before the fd-closure notification), but we would rather - // minimize the number of fd-closure notifications and let the recipient - // manage the TCP state of the connection. + disconnect(); +} + +void +Security::PeerConnector::disconnect() +{ + if (closeHandler) { + comm_remove_close_handler(serverConnection()->fd, closeHandler); + closeHandler = nullptr; + } + + commUnsetConnTimeout(serverConnection()); } void @@ -563,10 +584,6 @@ Security::PeerConnector::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 - - // remove close handler - comm_remove_close_handler(serverConnection()->fd, closeHandler); - CbDialer *dialer = dynamic_cast(cb->getDialer()); Must(dialer); dialer->answer().conn = serverConnection(); diff --git a/src/security/PeerConnector.h b/src/security/PeerConnector.h index 73933564e5..2e5efceb19 100644 --- a/src/security/PeerConnector.h +++ b/src/security/PeerConnector.h @@ -31,34 +31,12 @@ namespace Security { /** - * Initiates encryption on a connection to peers or servers. - * Despite its name does not perform any connect(2) operations. + * Initiates encryption of a given open TCP connection to a peer or server. + * Despite its name does not perform any connect(2) operations. Owns the + * connection during TLS negotiations. The caller receives EncryptorAnswer. * * Contains common code and interfaces of various specialized PeerConnector's, * including peer certificate validation code. - \par - * The caller receives a call back with Security::EncryptorAnswer. If answer.error - * is not nil, then there was an error and the encryption to the peer or server - * was not fully established. The error object is suitable for error response - * generation. - \par - * The caller must monitor the connection for closure because this - * job will not inform the caller about such events. - \par - * PeerConnector class currently supports a form of TLS negotiation timeout, - * which is accounted only when sets the read timeout from encrypted peers/servers. - * For a complete solution, the caller must monitor the overall connection - * establishment timeout and close the connection on timeouts. This is probably - * better than having dedicated (or none at all!) timeouts for peer selection, - * DNS lookup, TCP handshake, SSL handshake, etc. Some steps may have their - * own timeout, but not all steps should be forced to have theirs. - * XXX: tunnel.cc and probably other subsystems do not have an "overall - * connection establishment" timeout. We need to change their code so that they - * start monitoring earlier and close on timeouts. This change may need to be - * discussed on squid-dev. - \par - * This job never closes the connection, even on errors. If a 3rd-party - * closes the connection, this job simply quits without informing the caller. */ class PeerConnector: virtual public AsyncJob { @@ -81,7 +59,10 @@ public: AsyncCall::Pointer &aCallback, const AccessLogEntryPointer &alp, const time_t timeout = 0); - virtual ~PeerConnector(); + virtual ~PeerConnector() = default; + + /// hack: whether the connection requires fwdPconnPool->noteUses() + bool noteFwdPconnUse; protected: // AsyncJob API @@ -90,17 +71,12 @@ protected: virtual void swanSong(); virtual const char *status() const; + /// The connection read timeout callback handler. + void commTimeoutHandler(const CommTimeoutCbParams &); + /// The comm_close callback handler. void commCloseHandler(const CommCloseCbParams ¶ms); - /// Inform us that the connection is closed. Does the required clean-up. - void connectionClosed(const char *reason); - - /// Sets up TCP socket-related notification callbacks if things go wrong. - /// If socket already closed return false, else install the comm_close - /// handler to monitor the socket. - bool prepareSocket(); - /// \returns true on successful TLS session initialization virtual bool initialize(Security::SessionPointer &); @@ -160,12 +136,18 @@ protected: /// mimics FwdState to minimize changes to FwdState::initiate/negotiateSsl Comm::ConnectionPointer const &serverConnection() const { return serverConn; } - void bail(ErrorState *error); ///< Return an error to the PeerConnector caller + /// sends the given error to the initiator + void bail(ErrorState *error); + + /// sends the encrypted connection to the initiator + void sendSuccess(); - /// Callback the caller class, and pass the ready to communicate secure - /// connection or an error if PeerConnector failed. + /// a bail(), sendSuccess() helper: sends results to the initiator void callBack(); + /// a bail(), sendSuccess() helper: stops monitoring the connection + void disconnect(); + /// If called the certificates validator will not used void bypassCertValidator() {useCertValidator_ = false;} diff --git a/src/ssl/PeekingPeerConnector.cc b/src/ssl/PeekingPeerConnector.cc index a1e9a2fe16..6973b79ee3 100644 --- a/src/ssl/PeekingPeerConnector.cc +++ b/src/ssl/PeekingPeerConnector.cc @@ -92,8 +92,9 @@ Ssl::PeekingPeerConnector::checkForPeekAndSpliceMatched(const Ssl::BumpMode acti al->ssl.bumpMode = finalAction; if (finalAction == Ssl::bumpTerminate) { - serverConn->close(); + bail(new ErrorState(ERR_SECURE_CONNECT_FAIL, Http::scForbidden, request.getRaw(), al)); clientConn->close(); + clientConn = nullptr; } else if (finalAction != Ssl::bumpSplice) { //Allow write, proceed with the connection srvBio->holdWrite(false); @@ -140,11 +141,13 @@ Ssl::PeekingPeerConnector::initialize(Security::SessionPointer &serverSession) if (!Security::PeerConnector::initialize(serverSession)) return false; + // client connection supplies TLS client details and is also used if we + // need to splice or terminate the client and server connections + if (!Comm::IsConnOpen(clientConn)) + return false; + if (ConnStateData *csd = request->clientConnectionManager.valid()) { - // client connection is required in the case we need to splice - // or terminate client and server connections - assert(clientConn != NULL); SBuf *hostName = NULL; //Enable Status_request TLS extension, required to bump some clients @@ -245,6 +248,10 @@ Ssl::PeekingPeerConnector::noteNegotiationDone(ErrorState *error) if (!error) { serverCertificateVerified(); if (splice) { + if (!Comm::IsConnOpen(clientConn)) { + bail(new ErrorState(ERR_GATEWAY_FAILURE, Http::scInternalServerError, request.getRaw(), al)); + throw TextException("from-client connection gone", Here()); + } switchToTunnel(request.getRaw(), clientConn, serverConn); tunnelInsteadOfNegotiating(); } diff --git a/src/tests/stub_libsecurity.cc b/src/tests/stub_libsecurity.cc index b801e1a797..10793dbbe6 100644 --- a/src/tests/stub_libsecurity.cc +++ b/src/tests/stub_libsecurity.cc @@ -50,14 +50,12 @@ namespace Security { PeerConnector::PeerConnector(const Comm::ConnectionPointer &, AsyncCall::Pointer &, const AccessLogEntryPointer &, const time_t) : AsyncJob("Security::PeerConnector") {STUB} -PeerConnector::~PeerConnector() {STUB} void PeerConnector::start() STUB bool PeerConnector::doneAll() const STUB_RETVAL(true) void PeerConnector::swanSong() STUB const char *PeerConnector::status() const STUB_RETVAL("") void PeerConnector::commCloseHandler(const CommCloseCbParams &) STUB -void PeerConnector::connectionClosed(const char *) STUB -bool PeerConnector::prepareSocket() STUB_RETVAL(false) +void PeerConnector::commTimeoutHandler(const CommTimeoutCbParams &) STUB bool PeerConnector::initialize(Security::SessionPointer &) STUB_RETVAL(false) void PeerConnector::negotiate() STUB bool PeerConnector::sslFinalized() STUB_RETVAL(false) @@ -67,7 +65,9 @@ void PeerConnector::noteWantWrite() STUB void PeerConnector::noteNegotiationError(const int, const int, const int) STUB // virtual Security::ContextPointer getTlsContext() = 0; void PeerConnector::bail(ErrorState *) STUB +void PeerConnector::sendSuccess() STUB void PeerConnector::callBack() STUB +void PeerConnector::disconnect() STUB void PeerConnector::recordNegotiationDetails() STUB } diff --git a/src/tunnel.cc b/src/tunnel.cc index 16caef596d..02472665ad 100644 --- a/src/tunnel.cc +++ b/src/tunnel.cc @@ -111,9 +111,12 @@ public: /// starts connecting to the next hop, either for the first time or while /// recovering from the previous connect failure void startConnecting(); + void closePendingConnection(const Comm::ConnectionPointer &conn, const char *reason); + + void retryOrBail(); /// called when negotiations with the peer have been successfully completed - void notePeerReadyToShovel(); + void notePeerReadyToShovel(const Comm::ConnectionPointer &); class Connection { @@ -179,7 +182,8 @@ public: void copyRead(Connection &from, IOCB *completion); /// continue to set up connection to a peer, going async for SSL peers - void connectToPeer(); + void connectToPeer(const Comm::ConnectionPointer &); + void secureConnectionToPeer(const Comm::ConnectionPointer &); /* PeerSelectionInitiator API */ virtual void noteDestination(Comm::ConnectionPointer conn) override; @@ -233,8 +237,15 @@ private: void usePinned(); - /// callback handler after connection setup (including any encryption) - void connectedToPeer(Security::EncryptorAnswer &answer); + /// callback handler for the Security::PeerConnector encryptor + void noteSecurityPeerConnectorAnswer(Security::EncryptorAnswer &); + + /// called after connection setup (including any encryption) + void connectedToPeer(const Comm::ConnectionPointer &); + void establishTunnelThruProxy(const Comm::ConnectionPointer &); + + template + void advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep); /// details of the "last tunneling attempt" failure (if it failed) ErrorState *savedError = nullptr; @@ -679,6 +690,15 @@ tunnelTimeout(const CommTimeoutCbParams &io) tunnelState->server.closeIfOpen(); } +void +TunnelStateData::closePendingConnection(const Comm::ConnectionPointer &conn, const char *reason) +{ + debugs(26, 3, "because " << reason << "; " << conn); + assert(!server.conn); + if (IsConnOpen(conn)) + conn->close(); +} + void TunnelStateData::Connection::closeIfOpen() { @@ -776,6 +796,11 @@ static void tunnelStartShoveling(TunnelStateData *tunnelState) { assert(!tunnelState->waitingForConnectExchange); + assert(tunnelState->server.conn); + AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", + CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); + commSetConnTimeout(tunnelState->server.conn, Config.Timeout.read, timeoutCall); + *tunnelState->status_ptr = Http::scOkay; if (tunnelState->logTag_ptr) tunnelState->logTag_ptr->update(LOG_TCP_TUNNEL); @@ -838,35 +863,51 @@ TunnelStateData::tunnelEstablishmentDone(Http::TunnelerAnswer &answer) waitingForConnectExchange = false; - if (answer.positive()) { + auto sawProblem = false; + + if (!answer.positive()) { + sawProblem = true; + Must(!Comm::IsConnOpen(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"); + } + + if (!sawProblem) { + assert(answer.positive()); // paranoid // copy any post-200 OK bytes to our buffer preReadServerData = answer.leftovers; - notePeerReadyToShovel(); + notePeerReadyToShovel(answer.conn); return; } - // TODO: Reuse to-peer connections after a CONNECT error response. - - // TODO: We can and, hence, should close now, but tunnelServerClosed() - // cannot yet tell whether ErrorState is still writing an error response. - // server.closeIfOpen(); - if (!clientExpectsConnectResponse()) { // closing the non-HTTP client connection is the best we can do - debugs(50, 3, server.conn << " closing on CONNECT-to-peer error"); - server.closeIfOpen(); + debugs(50, 3, client.conn << " closing on CONNECT-to-peer error"); + client.closeIfOpen(); return; } - ErrorState *error = answer.squidError.get(); - Must(error); - answer.squidError.clear(); // preserve error for errorSendComplete() - sendError(error, "tunneler returns error"); + ErrorState *error = nullptr; + if (answer.positive()) { + error = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request.getRaw(), al); + } else { + error = answer.squidError.get(); + Must(error); + answer.squidError.clear(); // preserve error for errorSendComplete() + } + assert(error); + saveError(error); + retryOrBail(); } void -TunnelStateData::notePeerReadyToShovel() +TunnelStateData::notePeerReadyToShovel(const Comm::ConnectionPointer &conn) { + server.conn = conn; + // Start monitoring for server-side connection problems + comm_add_close_handler(server.conn->fd, tunnelServerClosed, this); + if (!clientExpectsConnectResponse()) tunnelStartShoveling(this); // ssl-bumped connection, be quiet else { @@ -896,17 +937,42 @@ tunnelErrorComplete(int fd/*const Comm::ConnectionPointer &*/, void *data, size_ tunnelState->server.conn->close(); } +/// reacts to the current destination failure +void +TunnelStateData::retryOrBail() +{ + // Since no TCP payload has been passed to client or server, we may + // TCP-connect to other destinations (including alternate IPs). + + if (!FwdState::EnoughTimeToReForward(startTime)) + return sendError(savedError, "forwarding timeout"); + + if (!destinations->empty()) + return startConnecting(); + + if (!PeerSelectionInitiator::subscribed) + return sendError(savedError, "tried all destinations"); +} + void TunnelStateData::noteConnection(HappyConnOpener::Answer &answer) { calls.connector = nullptr; connOpener.clear(); - if (const auto error = answer.error.get()) { + ErrorState *error = nullptr; + if ((error = answer.error.get())) { + Must(!Comm::IsConnOpen(answer.conn)); syncHierNote(answer.conn, request->url.host()); + 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); + closePendingConnection(answer.conn, "conn was closed while waiting for noteConnection"); + } + + if (error) { saveError(error); - answer.error.clear(); // savedError has it now - sendError(savedError, "tried all destinations"); + retryOrBail(); return; } @@ -917,17 +983,12 @@ void TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *origin, const bool reused) { Must(Comm::IsConnOpen(conn)); - server.conn = conn; if (reused) ResetMarkingsToServer(request.getRaw(), *conn); // else Comm::ConnOpener already applied proper/current markings - syncHierNote(server.conn, request->url.host()); - - request->hier.resetPeerNotes(conn, origin); - if (al) - al->hier.resetPeerNotes(conn, origin); + syncHierNote(conn, origin); #if USE_DELAY_POOLS /* no point using the delayIsNoDelay stuff since tunnel is nice and simple */ @@ -938,7 +999,6 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or netdbPingSite(request->url.host()); request->peer_host = conn->getPeer() ? conn->getPeer()->host : nullptr; - comm_add_close_handler(conn->fd, tunnelServerClosed, this); bool toOrigin = false; // same semantics as StateFlags::toOrigin if (const auto * const peer = conn->getPeer()) { @@ -950,14 +1010,10 @@ TunnelStateData::connectDone(const Comm::ConnectionPointer &conn, const char *or } if (!toOrigin) - connectToPeer(); + connectToPeer(conn); else { - notePeerReadyToShovel(); + notePeerReadyToShovel(conn); } - - AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", - CommTimeoutCbPtrFun(tunnelTimeout, this)); - commSetConnTimeout(conn, Config.Timeout.read, timeoutCall); } void @@ -1007,38 +1063,85 @@ tunnelStart(ClientHttpRequest * http) } void -TunnelStateData::connectToPeer() +TunnelStateData::connectToPeer(const Comm::ConnectionPointer &conn) { - if (CachePeer *p = server.conn->getPeer()) { - if (p->secure.encryptTransport) { - AsyncCall::Pointer callback = asyncCall(5,4, - "TunnelStateData::ConnectedToPeer", - MyAnswerDialer(&TunnelStateData::connectedToPeer, this)); - auto *connector = new Security::BlindPeerConnector(request, server.conn, callback, al); - AsyncJob::Start(connector); // will call our callback - return; - } + if (const auto p = conn->getPeer()) { + if (p->secure.encryptTransport) + return advanceDestination("secure connection to peer", conn, [this,&conn] { + secureConnectionToPeer(conn); + }); } - Security::EncryptorAnswer nil; - connectedToPeer(nil); + connectedToPeer(conn); } +/// encrypts an established TCP connection to peer void -TunnelStateData::connectedToPeer(Security::EncryptorAnswer &answer) +TunnelStateData::secureConnectionToPeer(const Comm::ConnectionPointer &conn) +{ + AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::noteSecurityPeerConnectorAnswer", + MyAnswerDialer(&TunnelStateData::noteSecurityPeerConnectorAnswer, this)); + const auto connector = new Security::BlindPeerConnector(request, conn, callback, al); + AsyncJob::Start(connector); // will call our callback +} + +/// starts a preparation step for an established connection; retries on failures +template +void +TunnelStateData::advanceDestination(const char *stepDescription, const Comm::ConnectionPointer &conn, const StepStart &startStep) +{ + // TODO: Extract destination-specific handling from TunnelStateData so that + // all the awkward, limited-scope advanceDestination() calls can be replaced + // with a single simple try/catch,retry block. + try { + startStep(); + // now wait for the step callback + } catch (...) { + debugs (26, 2, "exception while trying to " << stepDescription << ": " << CurrentException); + closePendingConnection(conn, "connection preparation exception"); + if (!savedError) + saveError(new ErrorState(ERR_CANNOT_FORWARD, Http::scInternalServerError, request.getRaw(), al)); + retryOrBail(); + } +} + +/// callback handler for the connection encryptor +void +TunnelStateData::noteSecurityPeerConnectorAnswer(Security::EncryptorAnswer &answer) { if (ErrorState *error = answer.error.get()) { + Must(!Comm::IsConnOpen(answer.conn)); answer.error.clear(); // sendError() will own the error sendError(error, "TLS peer connection error"); return; } + if (!Comm::IsConnOpen(answer.conn) || fd_table[answer.conn->fd].closing()) { + sendError(new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request.getRaw(), al), "connecion gone"); + closePendingConnection(answer.conn, "conn was closed while waiting for noteSecurityPeerConnectorAnswer"); + return; + } + + connectedToPeer(answer.conn); +} + +void +TunnelStateData::connectedToPeer(const Comm::ConnectionPointer &conn) +{ + advanceDestination("establish tunnel through proxy", conn, [this,&conn] { + establishTunnelThruProxy(conn); + }); +} + +void +TunnelStateData::establishTunnelThruProxy(const Comm::ConnectionPointer &conn) +{ assert(!waitingForConnectExchange); AsyncCall::Pointer callback = asyncCall(5,4, "TunnelStateData::tunnelEstablishmentDone", Http::Tunneler::CbDialer(&TunnelStateData::tunnelEstablishmentDone, this)); - const auto tunneler = new Http::Tunneler(server.conn, request, callback, Config.Timeout.lifetime, al); + const auto tunneler = new Http::Tunneler(conn, request, callback, Config.Timeout.lifetime, al); #if USE_DELAY_POOLS tunneler->setDelayId(server.delayId); #endif @@ -1240,6 +1343,9 @@ TunnelStateData::notifyConnOpener() void switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm::ConnectionPointer &srvConn) { + Must(Comm::IsConnOpen(clientConn)); + Must(Comm::IsConnOpen(srvConn)); + debugs(26,5, "Revert to tunnel FD " << clientConn->fd << " with FD " << srvConn->fd); /* Create state structure. */ @@ -1277,10 +1383,6 @@ switchToTunnel(HttpRequest *request, Comm::ConnectionPointer &clientConn, Comm:: else request->prepForDirect(); - AsyncCall::Pointer timeoutCall = commCbCall(5, 4, "tunnelTimeout", - CommTimeoutCbPtrFun(tunnelTimeout, tunnelState)); - commSetConnTimeout(srvConn, Config.Timeout.read, timeoutCall); - // we drain any already buffered from-server data below (rBufData) fd_table[srvConn->fd].useDefaultIo();