Bound the TLS handshake by the connect timeout

clickhouse/base/socket.cpp

@@ -355,6 +355,34 @@ void Socket::Close() {
     handle_ = INVALID_SOCKET;
 }
 
+void Socket::SetNonBlocking(bool value) {
+    SetNonBlock(handle_, value);
+}
+
+void Socket::WaitFor(short events, std::chrono::milliseconds timeout, const char* timeout_message) {
+    pollfd fd;
+    fd.fd      = handle_;
+    fd.events  = events;
+    fd.revents = 0;
+
+    const ssize_t rval = Poll(&fd, 1, timeout.count() < 0 ? -1 : static_cast<int>(timeout.count()));
+    if (rval == -1) {
+        const int err = getSocketErrorCode();
+#if !defined(_win_)
+        // Callers loop until their own deadline, so an interrupted wait is just an early wakeup.
+        if (err == EINTR) return;
+#endif
+        throw std::system_error(err, getErrorCategory(), "fail to poll socket");
+    }
+    if (rval == 0) {
+#if defined(_win_)
+        throw std::system_error(WSAETIMEDOUT, getErrorCategory(), timeout_message);
+#else
+        throw std::system_error(ETIMEDOUT, getErrorCategory(), timeout_message);
+#endif
+    }
+}
+
 void Socket::SetTcpKeepAlive(int idle, int intvl, int cnt) noexcept {
     int val = 1;
 

clickhouse/base/socket.h

@@ -128,6 +128,12 @@ class Socket : public SocketBase {
     Socket& operator = (const Socket&) = delete;
     void Close();
 
+    void SetNonBlocking(bool value);
+
+    /// Waits until one of `events` is ready on the socket; a negative timeout waits forever.
+    /// Throws std::system_error with a timed-out code when the timeout expires first.
+    void WaitFor(short events, std::chrono::milliseconds timeout, const char* timeout_message);
+
     SOCKET handle_;
 };
 

clickhouse/base/sslsocket.cpp

@@ -244,7 +244,7 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SocketTimeoutParams& time
         configureSSL(ssl_params.configuration, ssl);
 
     SSL_set_connect_state(ssl);
-    HANDLE_SSL_ERROR(ssl, SSL_connect(ssl));
+    Handshake(ssl, timeout_params.connect_timeout);
     HANDLE_SSL_ERROR(ssl, SSL_set_mode(ssl, SSL_MODE_AUTO_RETRY));
 
     if (const auto verify_result = SSL_get_verify_result(ssl); !ssl_params.skip_verification && verify_result != X509_V_OK) {
@@ -268,6 +268,37 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SocketTimeoutParams& time
 //    }
 }
 
+void SSLSocket::Handshake(SSL* ssl, std::chrono::milliseconds timeout) {
+    // A blocking SSL_connect() is bounded only by SO_RCVTIMEO, which is the query receive timeout
+    // (infinite by default), so run the handshake non-blocking against the connect timeout instead.
+    using Clock      = std::chrono::steady_clock;
+    const auto start = Clock::now();
+
+    SetNonBlocking(true);
+    ERR_clear_error();
+    for (;;) {
+        const int ret = SSL_connect(ssl);
+        if (ret > 0) break;
+
+        const int error = SSL_get_error(ssl, ret);
+        short events    = 0;
+        if (error == SSL_ERROR_WANT_READ)
+            events = POLLIN;
+        else if (error == SSL_ERROR_WANT_WRITE)
+            events = POLLOUT;
+        else
+            throwSSLError(ssl, error, LOCATION, "SSL_connect(ssl)");
+
+        auto remaining = timeout;
+        if (timeout.count() >= 0) {
+            const auto elapsed = std::chrono::duration_cast<std::chrono::milliseconds>(Clock::now() - start);
+            remaining          = elapsed < timeout ? timeout - elapsed : std::chrono::milliseconds(0);
+        }
+        WaitFor(events, remaining, "TLS handshake timed out");
+    }
+    SetNonBlocking(false);
+}
+
 void SSLSocket::validateParams(const SSLParams & ssl_params) {
     // We need either SSL or SSL_CTX to properly validate configuration, so create a temporary one.
     std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> ctx(SSL_CTX_new(TLS_client_method()), &SSL_CTX_free);

clickhouse/base/sslsocket.h

@@ -63,6 +63,8 @@ class SSLSocket : public Socket {
 
     static void validateParams(const SSLParams & ssl_params);
 private:
+    void Handshake(SSL* ssl, std::chrono::milliseconds timeout);
+
     std::unique_ptr<SSL, void (*)(SSL *s)> ssl_;
 };
 

clickhouse/client.h

@@ -115,7 +115,9 @@ struct ClientOptions {
     // TCP options
     DECLARE_FIELD(tcp_nodelay, bool, TcpNoDelay, true);
 
-    /// Connection socket connect timeout. If the timeout is negative then the connect operation will never timeout.
+    /// Connection socket connect timeout. It bounds the TCP connect and, for TLS connections, the TLS handshake separately,
+    /// so establishing a TLS connection can take up to twice this timeout.
+    /// If the timeout is negative then the connect operation will never timeout.
     DECLARE_FIELD(connection_connect_timeout, std::chrono::milliseconds, SetConnectionConnectTimeout, std::chrono::seconds(5));
 
     /// Connection socket timeout. If the timeout is set to zero then the operation will never timeout.

ut/ssl_ut.cpp

@@ -1,12 +1,15 @@
 /** Collection of integration tests that validate TLS-connectivity to a CH server
  */
-#include "readonly_client_test.h"
-#include "connection_failed_client_test.h"
-#include "utils.h"
-
-#include <openssl/tls1.h>
 #include <openssl/ssl2.h>
 #include <openssl/ssl3.h>
+#include <openssl/tls1.h>
+
+#include <cerrno>
+
+#include "connection_failed_client_test.h"
+#include "readonly_client_test.h"
+#include "tcp_server.h"
+#include "utils.h"
 
 namespace {
     using namespace clickhouse;
@@ -123,6 +126,39 @@ TEST(OpenSSLConfiguration, InValidValues) {
     }
 }
 
+TEST(OpenSSLConnection, HandshakeIsBoundedByConnectTimeout) {
+    // The kernel completes the TCP handshake from the listen backlog, but nobody ever
+    // answers the ClientHello. Each endpoint must give up after the connect timeout and
+    // rotation must move on to the next one; the receive timeout is only a hang guard.
+    const int first_port  = 19980;
+    const int second_port = 19981;
+    LocalTcpServer first(first_port);
+    LocalTcpServer second(second_port);
+    first.start();
+    second.start();
+
+    const auto connect_timeout = std::chrono::milliseconds(500);
+    const auto start           = std::chrono::steady_clock::now();
+    try {
+        Client client(ClientOptions()
+                          .SetEndpoints({{"127.0.0.1", first_port}, {"127.0.0.1", second_port}})
+                          .SetSendRetries(1)
+                          .SetConnectionConnectTimeout(connect_timeout)
+                          .SetConnectionRecvTimeout(std::chrono::seconds(10))
+                          .SetSSLOptions(ClientOptions::SSLOptions().SetSkipVerification(true)));
+        FAIL() << "TLS handshake completed against a peer that never answered";
+    } catch (const std::system_error& e) {
+#if defined(_win_)
+        EXPECT_EQ(WSAETIMEDOUT, e.code().value()) << e.what();
+#else
+        EXPECT_EQ(ETIMEDOUT, e.code().value()) << e.what();
+#endif
+    }
+    const auto elapsed = std::chrono::steady_clock::now() - start;
+
+    EXPECT_GE(elapsed, 2 * connect_timeout);
+    EXPECT_LT(elapsed, 2 * connect_timeout + std::chrono::seconds(2));
+}
 
 //INSTANTIATE_TEST_SUITE_P(
 //    Remote_GH_API_TLS_WithStringConfig, ReadonlyClientTest,