Hi all,

Currently FwdState::negotiateSSL() operates on a TCP connection without a timeout. If, for example, the server never responds to Squid SSL Hello, the connection getstuck 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)

Moreover, if the client closes the connection while Squid is fruitlessly waiting for server SSL negotiation, the client connection will get into the CLOSE_WAIT state with a 1 day client_lifetime timeout. This patch does not address that CLOSE_WAIT problem directly.

This patch adds an SSL negotiation timeout for the server SSL connection and try to not exceed forword_timeout or peer_timeout while connecting to an SSL server.

Some notes:
- In this patch still the timeouts used for Ssl::PeerConnector are not accurate, they may be 5 secs more then the forward timeout or 1 second more than peer_connect timeout, but I think are enough reasonable.

- Please check and comment the new Comm::Connection::startTime()/::noteStart() mechanism. Now the Comm::Connection::startTime_ computed in Comm::Connection constructor and resets in Comm::ConnOpener::start() and Comm::TcpAcceptor::start()


This is a Measurement Factory project.
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

=== modified file 'src/FwdState.cc'
--- src/FwdState.cc	2014-06-24 22:52:53 +0000
+++ src/FwdState.cc	2014-06-27 13:56:41 +0000
@@ -692,42 +692,44 @@
     serverConn = conn;
     flags.connected_okay = true;
 
     debugs(17, 3, HERE << serverConnection() << ": '" << entry->url() << "'" );
 
     comm_add_close_handler(serverConnection()->fd, fwdServerClosedWrapper, this);
 
     if (serverConnection()->getPeer())
         peerConnectSucceded(serverConnection()->getPeer());
 
 #if USE_OPENSSL
     if (!request->flags.pinned) {
         if ((serverConnection()->getPeer() && serverConnection()->getPeer()->use_ssl) ||
                 (!serverConnection()->getPeer() && request->url.getScheme() == AnyP::PROTO_HTTPS) ||
                 request->flags.sslPeek) {
 
             HttpRequest::Pointer requestPointer = request;
             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<time_t>(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;
         }
     }
 #endif
 
     dispatch();
 }
 
 #if USE_OPENSSL
 void
 FwdState::connectedToPeer(Ssl::PeerConnectorAnswer &answer)
 {
     if (ErrorState *error = answer.error.get()) {
         fail(error);
         answer.error.clear(); // preserve error for errorSendComplete()
         self = NULL;
         return;
     }
 
@@ -740,71 +742,77 @@
 {
     debugs(17, 2, "fwdConnectTimeout: FD " << fd << ": '" << entry->url() << "'" );
     assert(serverDestinations[0] != NULL);
     assert(fd == serverDestinations[0]->fd);
 
     if (entry->isEmpty()) {
         ErrorState *anErr = new ErrorState(ERR_CONNECT_FAIL, Http::scGatewayTimeout, request);
         anErr->xerrno = ETIMEDOUT;
         fail(anErr);
 
         /* This marks the peer DOWN ... */
         if (serverDestinations[0]->getPeer())
             peerConnectFailed(serverDestinations[0]->getPeer());
     }
 
     if (Comm::IsConnOpen(serverDestinations[0])) {
         serverDestinations[0]->close();
     }
 }
 
-/**
- * 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()) {
         ctimeout = serverDestinations[0]->getPeer()->connect_timeout > 0 ?
                    serverDestinations[0]->getPeer()->connect_timeout : Config.Timeout.peer_connect;
     } else {
         ctimeout = Config.Timeout.connect;
     }
 
     /* calculate total forwarding timeout ??? */
     int ftimeout = Config.Timeout.forward - (squid_curtime - start_t);
     if (ftimeout < 0)
         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");
         ErrorState *anErr = new ErrorState(ERR_CANNOT_FORWARD, Http::scServiceUnavailable, request);
         fail(anErr);
         self = NULL; // refcounted
         return;
     }
 
     request->flags.pinned = false; // XXX: what if the ConnStateData set this to flag existing credentials?
     // XXX: answer: the peer selection *should* catch it and give us only the pinned peer. so we reverse the =0 step below.
     // XXX: also, logs will now lie if pinning is broken and leads to an error message.
     if (serverDestinations[0]->peerType == PINNED) {
         ConnStateData *pinned_connection = request->pinnedConnection();
         debugs(17,7, "pinned peer connection: " << pinned_connection);
         // pinned_connection may become nil after a pconn race
         if (pinned_connection)
             serverConn = pinned_connection->validatePinnedConnection(request, serverDestinations[0]->getPeer());
         else
             serverConn = NULL;
@@ -866,41 +874,41 @@
         }
 #endif
 
         dispatch();
         return;
     }
 
     // We will try to open a new connection, possibly to the same destination.
     // We reset serverDestinations[0] in case we are using it again because
     // ConnOpener modifies its destination argument.
     serverDestinations[0]->local.port(0);
     serverConn = NULL;
 
 #if URL_CHECKSUM_DEBUG
     entry->mem_obj->checkUrlChecksum();
 #endif
 
     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);
 }
 
 void
 FwdState::dispatch()
 {
     debugs(17, 3, clientConn << ": Fetching " << request->method << ' ' << entry->url());
     /*
      * Assert that server_fd is set.  This is to guarantee that fwdState
      * is attached to something and will be deallocated when server_fd
      * is closed.
      */
     assert(Comm::IsConnOpen(serverConn));
 
     fd_note(serverConnection()->fd, entry->url());
 
     fd_table[serverConnection()->fd].noteUse();
 

=== modified file 'src/FwdState.h'
--- src/FwdState.h	2014-06-24 22:52:53 +0000
+++ src/FwdState.h	2014-06-27 13:56:21 +0000
@@ -58,40 +58,41 @@
     static void Start(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *, const AccessLogEntryPointer &alp);
     /// Same as Start() but no master xaction info (AccessLogEntry) available.
     static void fwdStart(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *);
 
     /// This is the real beginning of server connection. Call it whenever
     /// the forwarding server destination has changed and a new one needs to be opened.
     /// Produces the cannot-forward error on fail if no better error exists.
     void startConnectionOrFail();
 
     void fail(ErrorState *err);
     void unregister(Comm::ConnectionPointer &conn);
     void unregister(int fd);
     void complete();
     void handleUnregisteredServerEnd();
     int reforward();
     bool reforwardableStatus(const Http::StatusCode s) const;
     void serverClosed(int fd);
     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();
     /// Pops a connection from connection pool if available. If not
     /// checks the peer stand-by connection pool for available connection.
     Comm::ConnectionPointer pconnPop(const Comm::ConnectionPointer &dest, const char *domain);
     void pconnPush(Comm::ConnectionPointer & conn, const char *domain);
 
     bool dontRetry() { return flags.dont_retry; }
 
     void dontRetry(bool val) { flags.dont_retry = val; }
 
     /** return a ConnectionPointer to the current server connection (may or may not be open) */
     Comm::ConnectionPointer const & serverConnection() const { return serverConn; };
 
 private:
     // hidden for safer management of self; use static fwdStart
     FwdState(const Comm::ConnectionPointer &client, StoreEntry *, HttpRequest *, const AccessLogEntryPointer &alp);
     void start(Pointer aSelf);
 

=== modified file 'src/PeerPoolMgr.cc'
--- src/PeerPoolMgr.cc	2014-06-05 14:57:58 +0000
+++ src/PeerPoolMgr.cc	2014-06-27 14:09:00 +0000
@@ -1,35 +1,36 @@
 #include "squid.h"
 #include "base/AsyncJobCalls.h"
 #include "base/RunnersRegistry.h"
 #include "CachePeer.h"
 #include "comm/Connection.h"
 #include "comm/ConnOpener.h"
 #include "Debug.h"
 #include "fd.h"
 #include "FwdState.h"
 #include "globals.h"
 #include "HttpRequest.h"
 #include "neighbors.h"
 #include "pconn.h"
 #include "PeerPoolMgr.h"
 #include "SquidConfig.h"
+#include "SquidTime.h"
 #if USE_OPENSSL
 #include "ssl/PeerConnector.h"
 #endif
 
 CBDATA_CLASS_INIT(PeerPoolMgr);
 
 #if USE_OPENSSL
 /// Gives Ssl::PeerConnector access to Answer in the PeerPoolMgr callback dialer.
 class MyAnswerDialer: public UnaryMemFunT<PeerPoolMgr, Ssl::PeerConnectorAnswer, Ssl::PeerConnectorAnswer&>,
         public Ssl::PeerConnector::CbDialer
 {
 public:
     MyAnswerDialer(const JobPointer &aJob, Method aMethod):
             UnaryMemFunT<PeerPoolMgr, Ssl::PeerConnectorAnswer, Ssl::PeerConnectorAnswer&>(aJob, aMethod, Ssl::PeerConnectorAnswer()) {}
 
     /* Ssl::PeerConnector::CbDialer API */
     virtual Ssl::PeerConnectorAnswer &answer() { return arg1; }
 };
 #endif
 
@@ -95,42 +96,48 @@
         /* it might have been a timeout with a partially open link */
         if (params.conn != NULL)
             params.conn->close();
         peerConnectFailed(peer);
         checkpoint("conn opening failure"); // may retry
         return;
     }
 
     Must(params.conn != NULL);
 
 #if USE_OPENSSL
     // Handle SSL peers.
     if (peer->use_ssl) {
         typedef CommCbMemFunT<PeerPoolMgr, CommCloseCbParams> CloserDialer;
         closer = JobCallback(48, 3, CloserDialer, this,
                              PeerPoolMgr::handleSecureClosure);
         comm_add_close_handler(params.conn->fd, closer);
 
         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;
     }
 #endif
 
     pushNewConnection(params.conn);
 }
 
 void
 PeerPoolMgr::pushNewConnection(const Comm::ConnectionPointer &conn)
 {
     Must(validPeer());
     Must(Comm::IsConnOpen(conn));
     peer->standby.pool->push(conn, NULL /* domain */);
     // push() will trigger a checkpoint()
 }
 
 #if USE_OPENSSL
 void
 PeerPoolMgr::handleSecuredPeer(Ssl::PeerConnectorAnswer &answer)

=== modified file 'src/comm/ConnOpener.cc'
--- src/comm/ConnOpener.cc	2014-06-11 10:25:38 +0000
+++ src/comm/ConnOpener.cc	2014-06-27 14:32:42 +0000
@@ -210,40 +210,41 @@
 {
     Must(conn_ != NULL);
     Must(temporaryFd_ >= 0);
 
     cleanFd();
 
     conn_->fd = temporaryFd_;
     temporaryFd_ = -1;
 }
 
 void
 Comm::ConnOpener::start()
 {
     Must(conn_ != NULL);
 
     /* outbound sockets have no need to be protocol agnostic. */
     if (!(Ip::EnableIpv6&IPV6_SPECIAL_V4MAPPING) && conn_->remote.isIPv4()) {
         conn_->local.setIPv4();
     }
 
+    conn_->noteStart();
     if (createFd())
         doConnect();
 }
 
 /// called at the end of Comm::ConnOpener::DelayedConnectRetry event
 void
 Comm::ConnOpener::restart()
 {
     debugs(5, 5, conn_ << " restarting after sleep");
     calls_.sleep_ = false;
 
     if (createFd())
         doConnect();
 }
 
 /// Create a socket for the future connection or return false.
 /// If false is returned, done() is guaranteed to return true and end the job.
 bool
 Comm::ConnOpener::createFd()
 {

=== modified file 'src/comm/Connection.cc'
--- src/comm/Connection.cc	2014-04-30 10:50:09 +0000
+++ src/comm/Connection.cc	2014-06-27 14:39:25 +0000
@@ -5,68 +5,70 @@
 #include "comm/Connection.h"
 #include "fde.h"
 #include "neighbors.h"
 #include "SquidTime.h"
 
 class CachePeer;
 bool
 Comm::IsConnOpen(const Comm::ConnectionPointer &conn)
 {
     return conn != NULL && conn->isOpen();
 }
 
 Comm::Connection::Connection() :
         local(),
         remote(),
         peerType(HIER_NONE),
         fd(-1),
         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.
 }
 
 static int64_t lost_conn = 0;
 Comm::Connection::~Connection()
 {
     if (fd >= 0) {
         debugs(5, 4, "BUG #3329: Orphan Comm::Connection: " << *this);
         debugs(5, 4, "NOTE: " << ++lost_conn << " Orphans since last started.");
         close();
     }
 
     cbdataReferenceDone(peer_);
 }
 
 Comm::ConnectionPointer
 Comm::Connection::copyDetails() const
 {
     ConnectionPointer c = new Comm::Connection;
 
     c->local = local;
     c->remote = remote;
     c->peerType = peerType;
     c->tos = tos;
     c->nfmark = nfmark;
     c->flags = flags;
+    c->startTime_ = startTime_;
 
     // ensure FD is not open in the new copy.
     c->fd = -1;
 
     // ensure we have a cbdata reference to peer_ not a straight ptr copy.
     c->peer_ = cbdataReference(getPeer());
 
     return c;
 }
 
 void
 Comm::Connection::close()
 {
     if (isOpen()) {
         comm_close(fd);
         fd = -1;
         if (CachePeer *p=getPeer())
             peerConnClosed(p);
     }
 }

=== modified file 'src/comm/Connection.h'
--- src/comm/Connection.h	2014-02-21 10:46:19 +0000
+++ src/comm/Connection.h	2014-06-27 14:29:46 +0000
@@ -30,40 +30,41 @@
  *  Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111, USA.
  *
  *
  * Copyright (c) 2003, Robert Collins <robe...@squid-cache.org>
  * Copyright (c) 2010, Amos Jeffries <amosjeffr...@squid-cache.org>
  */
 
 #ifndef _SQUIDCONNECTIONDETAIL_H_
 #define _SQUIDCONNECTIONDETAIL_H_
 
 #include "comm/forward.h"
 #include "defines.h"
 #include "hier_code.h"
 #include "ip/Address.h"
 #include "MemPool.h"
 #include "typedefs.h"
 #if USE_SQUID_EUI
 #include "eui/Eui48.h"
 #include "eui/Eui64.h"
 #endif
+#include "SquidTime.h"
 
 #include <iosfwd>
 #include <ostream>
 
 class CachePeer;
 
 namespace Comm
 {
 
 /* TODO: make these a struct of boolean flags members in the connection instead of a bitmap.
  * we can't do that until all non-comm code uses Commm::Connection objects to create FD
  * currently there is code still using comm_open() and comm_openex() synchronously!!
  */
 #define COMM_UNSET              0x00
 #define COMM_NONBLOCKING        0x01  // default flag.
 #define COMM_NOCLOEXEC          0x02
 #define COMM_REUSEADDR          0x04  // shared FD may be both accept()ing and read()ing
 #define COMM_DOBIND             0x08  // requires a bind()
 #define COMM_TRANSPARENT        0x10  // arrived via TPROXY
 #define COMM_INTERCEPTION       0x20  // arrived via NAT
@@ -97,79 +98,86 @@
      */
     ConnectionPointer copyDetails() const;
 
     /** Close any open socket. */
     void close();
 
     /** determine whether this object describes an active connection or not. */
     bool isOpen() const { return (fd >= 0); }
 
     /** retrieve the CachePeer pointer for use.
      * The caller is responsible for all CBDATA operations regarding the
      * used of the pointer returned.
      */
     CachePeer * getPeer() const;
 
     /** alter the stored CachePeer pointer.
      * Perform appropriate CBDATA operations for locking the CachePeer pointer
      */
     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);
 
     /** These objects may not be exactly duplicated. Use copyDetails() instead. */
     Connection & operator =(const Connection &c);
 
 public:
     /** Address/Port for the Squid end of a TCP link. */
     Ip::Address local;
 
     /** Address for the Remote end of a TCP link. */
     Ip::Address remote;
 
     /** Hierarchy code for this connection link */
     hier_code peerType;
 
     /** Socket used by this connection. Negative if not open. */
     int fd;
 
     /** Quality of Service TOS values currently sent on this connection */
     tos_t tos;
 
     /** Netfilter MARK values currently sent on this connection */
     nfmark_t nfmark;
 
     /** COMM flags set on this connection */
     int flags;
 
     char rfc931[USER_IDENT_SZ];
 
 #if USE_SQUID_EUI
     Eui::Eui48 remoteEui48;
     Eui::Eui64 remoteEui64;
 #endif
 
 private:
     /** cache_peer data object (if any) */
     CachePeer *peer_;
+
+    /** The time the connection object was created */
+    time_t startTime_;
 };
 
 }; // namespace Comm
 
 MEMPROXY_CLASS_INLINE(Comm::Connection);
 
 // NP: Order and namespace here is very important.
 //     * The second define inlines the first.
 //     * Stream inheritance overloading is searched in the global scope first.
 
 inline std::ostream &
 operator << (std::ostream &os, const Comm::Connection &conn)
 {
     os << "local=" << conn.local << " remote=" << conn.remote;
     if (conn.fd >= 0)
         os << " FD " << conn.fd;
     if (conn.flags != COMM_UNSET)
         os << " flags=" << conn.flags;
 #if USE_IDENT
     if (*conn.rfc931)

=== modified file 'src/comm/TcpAcceptor.cc'
--- src/comm/TcpAcceptor.cc	2014-06-09 13:18:48 +0000
+++ src/comm/TcpAcceptor.cc	2014-06-27 14:33:21 +0000
@@ -76,40 +76,42 @@
     unsubscribe("subscription change");
     theCallSub = aSub;
 }
 
 void
 Comm::TcpAcceptor::unsubscribe(const char *reason)
 {
     debugs(5, 5, HERE << status() << " AsyncCall Subscription " << theCallSub << " removed: " << reason);
     theCallSub = NULL;
 }
 
 void
 Comm::TcpAcceptor::start()
 {
     debugs(5, 5, HERE << status() << " AsyncCall Subscription: " << theCallSub);
 
     Must(IsConnOpen(conn));
 
     setListen();
 
+    conn->noteStart();
+
     // if no error so far start accepting connections.
     if (errcode == 0)
         SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0);
 }
 
 bool
 Comm::TcpAcceptor::doneAll() const
 {
     // stop when FD is closed
     if (!IsConnOpen(conn)) {
         return AsyncJob::doneAll();
     }
 
     // stop when handlers are gone
     if (theCallSub == NULL) {
         return AsyncJob::doneAll();
     }
 
     // open FD with handlers...keep accepting.
     return false;

=== modified file 'src/ssl/PeerConnector.cc'
--- src/ssl/PeerConnector.cc	2014-05-07 14:40:05 +0000
+++ src/ssl/PeerConnector.cc	2014-06-27 14:36:45 +0000
@@ -11,45 +11,48 @@
 #include "comm/Loops.h"
 #include "errorpage.h"
 #include "fde.h"
 #include "globals.h"
 #include "HttpRequest.h"
 #include "neighbors.h"
 #include "SquidConfig.h"
 #include "ssl/cert_validate_message.h"
 #include "ssl/Config.h"
 #include "ssl/ErrorDetail.h"
 #include "ssl/helper.h"
 #include "ssl/PeerConnector.h"
 #include "ssl/ServerBump.h"
 #include "ssl/support.h"
 
 CBDATA_NAMESPACED_CLASS_INIT(Ssl, PeerConnector);
 
 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<CbDialer*>(callback->getDialer()));
 }
 
 Ssl::PeerConnector::~PeerConnector()
 {
     debugs(83, 5, "Peer connector " << this << " gone");
 }
 
 bool Ssl::PeerConnector::doneAll() const
 {
     return (!callback || callback->canceled()) && AsyncJob::doneAll();
 }
 
 /// Preps connection and SSL state. Calls negotiate().
 void
 Ssl::PeerConnector::start()
 {
     AsyncJob::start();
@@ -161,40 +164,54 @@
             // check->fd(fd); XXX: need client FD here
             SSL_set_ex_data(ssl, ssl_ex_index_cert_error_check, check);
         }
     }
 
     // store peeked cert to check SQUID_X509_V_ERR_CERT_CHANGE
     X509 *peeked_cert;
     if (request->clientConnectionManager.valid() &&
             request->clientConnectionManager->serverBump() &&
             (peeked_cert = request->clientConnectionManager->serverBump()->serverCert.get())) {
         CRYPTO_add(&(peeked_cert->references),1,CRYPTO_LOCK_X509);
         SSL_set_ex_data(ssl, ssl_ex_index_ssl_peeked_cert, peeked_cert);
     }
 
     fd_table[fd].ssl = ssl;
     fd_table[fd].read_method = &ssl_read_method;
     fd_table[fd].write_method = &ssl_write_method;
 }
 
 void
+Ssl::PeerConnector::setReadTimeout()
+{
+    int timeToRead;
+    if (negotiationTimeout) {
+        const int timeUsed = squid_curtime - startTime;
+        const int timeLeft = max(0, static_cast<int>(negotiationTimeout - timeUsed));
+        timeToRead = min(static_cast<int>(::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())
         return;
 
     const int fd = serverConnection()->fd;
     SSL *ssl = fd_table[fd].ssl;
     const int result = SSL_connect(ssl);
     if (result <= 0) {
         handleNegotiateError(result);
         return; // we might be gone by now
     }
 
     if (request->clientConnectionManager.valid()) {
         // remember the server certificate from the ErrorDetail object
         if (Ssl::ServerBump *serverBump = request->clientConnectionManager->serverBump()) {
             serverBump->serverCert.reset(SSL_get_peer_certificate(ssl));
 
             // remember validation errors, if any
             if (Ssl::CertErrors *errs = static_cast<Ssl::CertErrors *>(SSL_get_ex_data(ssl, ssl_ex_index_ssl_errors)))
@@ -369,40 +386,41 @@
     CallJobHere(83, 7, pc, Ssl::PeerConnector, negotiateSsl);
 }
 
 void
 Ssl::PeerConnector::handleNegotiateError(const int ret)
 {
     const int fd = serverConnection()->fd;
     unsigned long ssl_lib_error = SSL_ERROR_NONE;
     SSL *ssl = fd_table[fd].ssl;
     int ssl_error = SSL_get_error(ssl, ret);
 
 #ifdef EPROTO
     int sysErrNo = EPROTO;
 #else
     int sysErrNo = EACCES;
 #endif
 
     switch (ssl_error) {
 
     case SSL_ERROR_WANT_READ:
+        setReadTimeout();
         Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, this, 0);
         return;
 
     case SSL_ERROR_WANT_WRITE:
         Comm::SetSelect(fd, COMM_SELECT_WRITE, &NegotiateSsl, this, 0);
         return;
 
     case SSL_ERROR_SSL:
     case SSL_ERROR_SYSCALL:
         ssl_lib_error = ERR_get_error();
 
         // store/report errno when ssl_error is SSL_ERROR_SYSCALL, ssl_lib_error is 0, and ret is -1
         if (ssl_error == SSL_ERROR_SYSCALL && ret == -1 && ssl_lib_error == 0)
             sysErrNo = errno;
 
         debugs(83, DBG_IMPORTANT, "Error negotiating SSL on FD " << fd <<
                ": " << ERR_error_string(ssl_lib_error, NULL) << " (" <<
                ssl_error << "/" << ret << "/" << errno << ")");
 
         break; // proceed to the general error handling code

=== modified file 'src/ssl/PeerConnector.h'
--- src/ssl/PeerConnector.h	2014-05-07 14:40:05 +0000
+++ src/ssl/PeerConnector.h	2014-06-27 14:36:26 +0000
@@ -54,90 +54,97 @@
     /// answer recepients must clear the error member in order to keep its info
     /// XXX: We should refcount ErrorState instead of cbdata-protecting it.
     CbcPointer<ErrorState> error; ///< problem details (nil on success)
 };
 
 /**
  \par
  * Connects Squid client-side to an SSL peer (cache_peer ... ssl).
  * Handles peer certificate validation.
  * Used by TunnelStateData, FwdState, and PeerPoolMgr to start talking to an
  * SSL peer.
  \par
  * The caller receives a call back with PeerConnectorAnswer. If answer.error
  * is not nil, then there was an error and the SSL connection to the SSL peer
  * 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
- * 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.
 */
 class PeerConnector: virtual public AsyncJob
 {
 public:
     /// Callback dialier API to allow PeerConnector to set the answer.
     class CbDialer
     {
     public:
         virtual ~CbDialer() {}
         /// gives PeerConnector access to the in-dialer answer
         virtual PeerConnectorAnswer &answer() = 0;
     };
 
     typedef RefCount<HttpRequest> HttpRequestPointer;
 
 public:
     PeerConnector(HttpRequestPointer &aRequest,
                   const Comm::ConnectionPointer &aServerConn,
-                  AsyncCall::Pointer &aCallback);
+                  AsyncCall::Pointer &aCallback, const time_t timeout = 0);
     virtual ~PeerConnector();
 
 protected:
     // AsyncJob API
     virtual void start();
     virtual bool doneAll() const;
     virtual void swanSong();
     virtual const char *status() const;
 
     /// The comm_close callback handler.
     void commCloseHandler(const CommCloseCbParams &params);
 
     /// 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();
 
+    /// 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.
     /// It is called multiple times untill the negotiation finish or aborted.
     void negotiateSsl();
 
     /// Called when the SSL negotiation step aborted because data needs to
     /// be transferred to/from SSL server or on error. In the first case
     /// setups the appropriate Comm::SetSelect handler. In second case
     /// fill an error and report to the PeerConnector caller.
     void handleNegotiateError(const int result);
 
 private:
     PeerConnector(const PeerConnector &); // not implemented
     PeerConnector &operator =(const PeerConnector &); // not implemented
 
     /// 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
@@ -145,29 +152,31 @@
     /// Callback the caller class, and pass the ready to communicate secure
     /// connection or an error if PeerConnector failed.
     void callBack();
 
     /// Process response from cert validator helper
     void sslCrtvdHandleReply(Ssl::CertValidationResponse const &);
 
     /// Check SSL errors returned from cert validator against sslproxy_cert_error access list
     Ssl::CertErrors *sslCrtvdCheckForErrors(Ssl::CertValidationResponse const &, Ssl::ErrorDetail *&);
 
     /// Callback function called when squid receive message from cert validator helper
     static void sslCrtvdHandleReplyWrapper(void *data, Ssl::CertValidationResponse const &);
 
     /// A wrapper function for negotiateSsl for use with Comm::SetSelect
     static void NegotiateSsl(int fd, void *data);
 
     HttpRequestPointer request; ///< peer connection trigger or cause
     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);
 };
 
 std::ostream &operator <<(std::ostream &os, const Ssl::PeerConnectorAnswer &a);
 
 } // namespace Ssl
 
 #endif /* SQUID_PEER_CONNECTOR_H */

Reply via email to