------------------------------------------------------------ revno: 13484 revision-id: chtsanti@users.sourceforge.net-20140710144753-ewrjoym32fapn2mx parent: squid3@treenet.co.nz-20140710140134-zi3c52gbhstoxu7s committer: Christos Tsantilas branch nick: trunk timestamp: Thu 2014-07-10 17:47:53 +0300 message: SSL Server connect I/O timeout FwdState::negotiateSSL() operates on a TCP connection without a timeout. If, for example, the server never responds to Squid SSL Hello, the connection get stuck forever. This happens in real world when, for example, a client is trying to establish an SSL connection through bumping Squid to an HTTP server that does not speak SSL and does not detect initial request garbage (from HTTP point of view) This patch adds support for timeout to SSL negotiation procedure and sets this timeout so that it does not exceed peer_connect or forward_timeout. This is a Measurement Factory project ------------------------------------------------------------ # Bazaar merge directive format 2 (Bazaar 0.90) # revision_id: chtsanti@users.sourceforge.net-20140710144753-\ # ewrjoym32fapn2mx # target_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # testament_sha1: 362a70ae2e40b8aa283009f087c4474058451fbb # timestamp: 2014-07-10 14:54:46 +0000 # source_branch: http://bzr.squid-cache.org/bzr/squid3/trunk/ # base_revision_id: squid3@treenet.co.nz-20140710140134-\ # zi3c52gbhstoxu7s # # Begin patch === modified file 'src/FwdState.cc' --- src/FwdState.cc 2014-06-24 22:52:53 +0000 +++ src/FwdState.cc 2014-07-10 14:47:53 +0000 @@ -709,8 +709,10 @@ AsyncCall::Pointer callback = asyncCall(17,4, "FwdState::ConnectedToPeer", FwdStatePeerAnswerDialer(&FwdState::connectedToPeer, this)); + // Use positive timeout when less than one second is left. + const time_t sslNegotiationTimeout = max(static_cast(1), timeLeft()); Ssl::PeerConnector *connector = - new Ssl::PeerConnector(requestPointer, serverConnection(), callback); + new Ssl::PeerConnector(requestPointer, serverConnection(), callback, sslNegotiationTimeout); AsyncJob::Start(connector); // will call our callback return; } @@ -757,21 +759,9 @@ } } -/** - * Called after Forwarding path selection (via peer select) has taken place. - * And whenever forwarding needs to attempt a new connection (routing failover) - * We have a vector of possible localIP->remoteIP paths now ready to start being connected. - */ -void -FwdState::connectStart() +time_t +FwdState::timeLeft() const { - assert(serverDestinations.size() > 0); - - debugs(17, 3, "fwdConnectStart: " << entry->url()); - - if (!request->hier.first_conn_start.tv_sec) // first attempt - request->hier.first_conn_start = current_time; - /* connection timeout */ int ctimeout; if (serverDestinations[0]->getPeer()) { @@ -787,7 +777,25 @@ ftimeout = 5; if (ftimeout < ctimeout) - ctimeout = ftimeout; + return (time_t)ftimeout; + else + return (time_t)ctimeout; +} + +/** + * Called after forwarding path selection (via peer select) has taken place + * and whenever forwarding needs to attempt a new connection (routing failover). + * We have a vector of possible localIP->remoteIP paths now ready to start being connected. + */ +void +FwdState::connectStart() +{ + assert(serverDestinations.size() > 0); + + debugs(17, 3, "fwdConnectStart: " << entry->url()); + + if (!request->hier.first_conn_start.tv_sec) // first attempt + request->hier.first_conn_start = current_time; if (serverDestinations[0]->getPeer() && request->flags.sslBumped) { debugs(50, 4, "fwdConnectStart: Ssl bumped connections through parent proxy are not allowed"); @@ -883,7 +891,7 @@ GetMarkingsToServer(request, *serverDestinations[0]); calls.connector = commCbCall(17,3, "fwdConnectDoneWrapper", CommConnectCbPtrFun(fwdConnectDoneWrapper, this)); - Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, ctimeout); + Comm::ConnOpener *cs = new Comm::ConnOpener(serverDestinations[0], calls.connector, timeLeft()); if (host) cs->setHost(host); AsyncJob::Start(cs); === modified file 'src/FwdState.h' --- src/FwdState.h 2014-06-24 22:52:53 +0000 +++ src/FwdState.h 2014-07-10 14:47:53 +0000 @@ -75,6 +75,7 @@ void connectStart(); void connectDone(const Comm::ConnectionPointer & conn, Comm::Flag status, int xerrno); void connectTimeout(int fd); + time_t timeLeft() const; ///< the time left before the forwarding timeout expired bool checkRetry(); bool checkRetriable(); void dispatch(); === modified file 'src/PeerPoolMgr.cc' --- src/PeerPoolMgr.cc 2014-06-05 14:57:58 +0000 +++ src/PeerPoolMgr.cc 2014-07-10 14:47:53 +0000 @@ -13,6 +13,7 @@ #include "pconn.h" #include "PeerPoolMgr.h" #include "SquidConfig.h" +#include "SquidTime.h" #if USE_OPENSSL #include "ssl/PeerConnector.h" #endif @@ -112,8 +113,14 @@ securer = asyncCall(48, 4, "PeerPoolMgr::handleSecuredPeer", MyAnswerDialer(this, &PeerPoolMgr::handleSecuredPeer)); + + const int peerTimeout = peer->connect_timeout > 0 ? + peer->connect_timeout : Config.Timeout.peer_connect; + const int timeUsed = squid_curtime - params.conn->startTime(); + // Use positive timeout when less than one second is left for conn. + const int timeLeft = max(1, (peerTimeout - timeUsed)); Ssl::PeerConnector *connector = - new Ssl::PeerConnector(request, params.conn, securer); + new Ssl::PeerConnector(request, params.conn, securer, timeLeft); AsyncJob::Start(connector); // will call our callback return; } === modified file 'src/comm/ConnOpener.cc' --- src/comm/ConnOpener.cc 2014-06-11 10:25:38 +0000 +++ src/comm/ConnOpener.cc 2014-07-10 14:47:53 +0000 @@ -227,6 +227,7 @@ conn_->local.setIPv4(); } + conn_->noteStart(); if (createFd()) doConnect(); } === modified file 'src/comm/Connection.cc' --- src/comm/Connection.cc 2014-04-30 10:50:09 +0000 +++ src/comm/Connection.cc 2014-07-10 14:47:53 +0000 @@ -22,7 +22,8 @@ tos(0), nfmark(0), flags(COMM_NONBLOCKING), - peer_(NULL) + peer_(NULL), + startTime_(squid_curtime) { *rfc931 = 0; // quick init the head. the rest does not matter. } @@ -50,6 +51,7 @@ c->tos = tos; c->nfmark = nfmark; c->flags = flags; + c->startTime_ = startTime_; // ensure FD is not open in the new copy. c->fd = -1; === modified file 'src/comm/Connection.h' --- src/comm/Connection.h 2014-02-21 10:46:19 +0000 +++ src/comm/Connection.h 2014-07-10 14:47:53 +0000 @@ -47,6 +47,7 @@ #include "eui/Eui48.h" #include "eui/Eui64.h" #endif +#include "SquidTime.h" #include #include @@ -114,6 +115,10 @@ */ void setPeer(CachePeer * p); + /** The time the connection started */ + time_t startTime() const {return startTime_;} + + void noteStart() {startTime_ = squid_curtime;} private: /** These objects may not be exactly duplicated. Use copyDetails() instead. */ Connection(const Connection &c); @@ -153,6 +158,9 @@ private: /** cache_peer data object (if any) */ CachePeer *peer_; + + /** The time the connection object was created */ + time_t startTime_; }; }; // namespace Comm === modified file 'src/comm/TcpAcceptor.cc' --- src/comm/TcpAcceptor.cc 2014-06-09 13:18:48 +0000 +++ src/comm/TcpAcceptor.cc 2014-07-10 14:47:53 +0000 @@ -93,6 +93,8 @@ setListen(); + conn->noteStart(); + // if no error so far start accepting connections. if (errcode == 0) SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0); === modified file 'src/ssl/PeerConnector.cc' --- src/ssl/PeerConnector.cc 2014-05-07 14:40:05 +0000 +++ src/ssl/PeerConnector.cc 2014-07-10 14:47:53 +0000 @@ -28,11 +28,14 @@ Ssl::PeerConnector::PeerConnector( HttpRequestPointer &aRequest, const Comm::ConnectionPointer &aServerConn, - AsyncCall::Pointer &aCallback): + AsyncCall::Pointer &aCallback, + const time_t timeout): AsyncJob("Ssl::PeerConnector"), request(aRequest), serverConn(aServerConn), - callback(aCallback) + callback(aCallback), + negotiationTimeout(timeout), + startTime(squid_curtime) { // if this throws, the caller's cb dialer is not our CbDialer Must(dynamic_cast(callback->getDialer())); @@ -178,6 +181,20 @@ } void +Ssl::PeerConnector::setReadTimeout() +{ + int timeToRead; + if (negotiationTimeout) { + const int timeUsed = squid_curtime - startTime; + const int timeLeft = max(0, static_cast(negotiationTimeout - timeUsed)); + timeToRead = min(static_cast(::Config.Timeout.read), timeLeft); + } else + timeToRead = ::Config.Timeout.read; + AsyncCall::Pointer nil; + commSetConnTimeout(serverConnection(), timeToRead, nil); +} + +void Ssl::PeerConnector::negotiateSsl() { if (!Comm::IsConnOpen(serverConnection()) || fd_table[serverConnection()->fd].closing()) @@ -386,6 +403,7 @@ switch (ssl_error) { case SSL_ERROR_WANT_READ: + setReadTimeout(); Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, this, 0); return; === modified file 'src/ssl/PeerConnector.h' --- src/ssl/PeerConnector.h 2014-05-07 14:40:05 +0000 +++ src/ssl/PeerConnector.h 2014-07-10 14:47:53 +0000 @@ -71,14 +71,17 @@ * The caller must monitor the connection for closure because this * job will not inform the caller about such events. \par - * 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: Neither tunnel.cc - * nor forward.cc have a "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. + * PeerConnector class curently supports a form of SSL negotiation timeout, + * which accounted only when sets the read timeout from SSL peer. + * 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 does 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. @@ -100,7 +103,7 @@ public: PeerConnector(HttpRequestPointer &aRequest, const Comm::ConnectionPointer &aServerConn, - AsyncCall::Pointer &aCallback); + AsyncCall::Pointer &aCallback, const time_t timeout = 0); virtual ~PeerConnector(); protected: @@ -121,6 +124,10 @@ /// handler to monitor the socket. bool prepareSocket(); + /// Sets the read timeout to avoid getting stuck while reading from a + /// silent server + void setReadTimeout(); + void initializeSsl(); ///< Initializes SSL state /// Performs a single secure connection negotiation step. @@ -162,6 +169,8 @@ Comm::ConnectionPointer serverConn; ///< TCP connection to the peer AsyncCall::Pointer callback; ///< we call this with the results AsyncCall::Pointer closeHandler; ///< we call this when the connection closed + time_t negotiationTimeout; ///< the ssl connection timeout to use + time_t startTime; ///< when the peer connector negotiation started CBDATA_CLASS2(PeerConnector); };