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

diff --git a/src/FwdState.cc b/src/FwdState.cc
index 8b5b2d1863417ec01d640d37fd1306b8fb4dc4fd..9936d79bb8cb2cff653b35cdb5ceb3cb0b57fe17 100644 (file)
--- a/src/FwdState.cc
+++ b/src/FwdState.cc
@@ -709,8 +709,10 @@ FwdState::connectDone(const Comm::ConnectionPointer &conn, Comm::Flag status, in
             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;
         }
@@ -757,21 +759,9 @@ FwdState::connectTimeout(int fd)
     }
 }
 
-/**
- * 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 @@ FwdState::connectStart()
         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 @@ FwdState::connectStart()
     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);

diff --git a/src/FwdState.h b/src/FwdState.h
index e78aa905ade88f3452ba095341e222a7c72f870c..64cb006b7cd103f96c8adb891d6141610a8f59d8 100644 (file)
--- a/src/FwdState.h
+++ b/src/FwdState.h
@@ -75,6 +75,7 @@ public:
     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();

diff --git a/src/PeerPoolMgr.cc b/src/PeerPoolMgr.cc
index 2a6de4262c0f39d327dab5ef37af9d8cc0cb4176..7ffc9f6290f564a3949e9f8f65e87845356fc149 100644 (file)
--- a/src/PeerPoolMgr.cc
+++ b/src/PeerPoolMgr.cc
@@ -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 @@ PeerPoolMgr::handleOpenedConnection(const CommConnectCbParams &params)
 
         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;
     }

diff --git a/src/comm/ConnOpener.cc b/src/comm/ConnOpener.cc
index 3e4b98c7ec7b3a4f7f9130089db7f1c5b24b3dbb..11ecb1421a2310f8ea5e743301039bbd2d2d8274 100644 (file)
--- a/src/comm/ConnOpener.cc
+++ b/src/comm/ConnOpener.cc
@@ -227,6 +227,7 @@ Comm::ConnOpener::start()
         conn_->local.setIPv4();
     }
 
+    conn_->noteStart();
     if (createFd())
         doConnect();
 }

diff --git a/src/comm/Connection.cc b/src/comm/Connection.cc
index 79f3e35b866b2782df25edc82c11d0280ebbd738..21779976fbd098dedc94e762c2086f009ff87e71 100644 (file)
--- a/src/comm/Connection.cc
+++ b/src/comm/Connection.cc
@@ -22,7 +22,8 @@ Comm::Connection::Connection() :
         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 @@ Comm::Connection::copyDetails() const
     c->tos = tos;
     c->nfmark = nfmark;
     c->flags = flags;
+    c->startTime_ = startTime_;
 
     // ensure FD is not open in the new copy.
     c->fd = -1;

diff --git a/src/comm/Connection.h b/src/comm/Connection.h
index f2f5dc057d2108422d1cfb8ca60513a38b3eae32..5b700c7fc2b15f664d16aed819b00c8c2707305b 100644 (file)
--- a/src/comm/Connection.h
+++ b/src/comm/Connection.h
@@ -47,6 +47,7 @@
 #include "eui/Eui48.h"
 #include "eui/Eui64.h"
 #endif
+#include "SquidTime.h"
 
 #include <iosfwd>
 #include <ostream>
@@ -114,6 +115,10 @@ public:
      */
     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 @@ public:
 private:
     /** cache_peer data object (if any) */
     CachePeer *peer_;
+
+    /** The time the connection object was created */
+    time_t startTime_;
 };
 
 }; // namespace Comm

diff --git a/src/comm/TcpAcceptor.cc b/src/comm/TcpAcceptor.cc
index 198a5204cd4335131cb213c6520c2e2a34d3ee52..f4b6c66f6bb4166701c2b352be71d4e0b577ff86 100644 (file)
--- a/src/comm/TcpAcceptor.cc
+++ b/src/comm/TcpAcceptor.cc
@@ -93,6 +93,8 @@ Comm::TcpAcceptor::start()
 
     setListen();
 
+    conn->noteStart();
+
     // if no error so far start accepting connections.
     if (errcode == 0)
         SetSelect(conn->fd, COMM_SELECT_READ, doAccept, this, 0);

diff --git a/src/ssl/PeerConnector.cc b/src/ssl/PeerConnector.cc
index 4fa8be988dd145092a8c0de7c856add3bc66c1e1..2f78a05b210673293d1d8d94f66f9523c8731c6d 100644 (file)
--- a/src/ssl/PeerConnector.cc
+++ b/src/ssl/PeerConnector.cc
@@ -28,11 +28,14 @@ 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()));
@@ -177,6 +180,20 @@ Ssl::PeerConnector::initializeSsl()
     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()
 {
@@ -386,6 +403,7 @@ Ssl::PeerConnector::handleNegotiateError(const int ret)
     switch (ssl_error) {
 
     case SSL_ERROR_WANT_READ:
+        setReadTimeout();
         Comm::SetSelect(fd, COMM_SELECT_READ, &NegotiateSsl, this, 0);
         return;
 

diff --git a/src/ssl/PeerConnector.h b/src/ssl/PeerConnector.h
index 44732c47a0a4d1aecd62c7ed9de47611002e3cf2..ea31d87b4b9986dd14c9f9255b3624e805d881c4 100644 (file)
--- a/src/ssl/PeerConnector.h
+++ b/src/ssl/PeerConnector.h
@@ -71,14 +71,17 @@ public:
  * 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:
 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 @@ protected:
     /// 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 @@ private:
     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);
 };