From 3260369838a3cb78e7eb7c850fc5c24e05c297c2 Mon Sep 17 00:00:00 2001 From: Ilia Alshanetsky Date: Sun, 18 Oct 2026 11:49:09 -0400 Subject: [PATCH] Bound the TLS handshake by the connect timeout connection_connect_timeout only covers the TCP connect poll in SocketConnect(). SSLSocket then runs a blocking SSL_connect() whose reads are bounded by SO_RCVTIMEO, i.e. the receive timeout, which defaults to infinite, so a peer that accepts the connection but never answers the ClientHello blocks the client forever and failover to the next endpoint never happens. I run the handshake non-blocking in SSLSocket::Handshake() and poll against the connect timeout, throwing std::system_error(ETIMEDOUT) on expiry so ResetConnectionEndpoint() moves on to the next endpoint. --- clickhouse/base/socket.cpp | 28 +++++++++++++++++++++ clickhouse/base/socket.h | 6 +++++ clickhouse/base/sslsocket.cpp | 33 ++++++++++++++++++++++++- clickhouse/base/sslsocket.h | 2 ++ clickhouse/client.h | 4 ++- ut/ssl_ut.cpp | 46 +++++++++++++++++++++++++++++++---- 6 files changed, 112 insertions(+), 7 deletions(-) diff --git a/clickhouse/base/socket.cpp b/clickhouse/base/socket.cpp index 8499e66b5..e4eebebd5 100644 --- a/clickhouse/base/socket.cpp +++ b/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(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; diff --git a/clickhouse/base/socket.h b/clickhouse/base/socket.h index 9bd9ca349..b336eb51b 100644 --- a/clickhouse/base/socket.h +++ b/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_; }; diff --git a/clickhouse/base/sslsocket.cpp b/clickhouse/base/sslsocket.cpp index a9e3e6da1..a8ea1cbe1 100644 --- a/clickhouse/base/sslsocket.cpp +++ b/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(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 ctx(SSL_CTX_new(TLS_client_method()), &SSL_CTX_free); diff --git a/clickhouse/base/sslsocket.h b/clickhouse/base/sslsocket.h index 32ae3bb7d..84cf11e34 100644 --- a/clickhouse/base/sslsocket.h +++ b/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_; }; diff --git a/clickhouse/client.h b/clickhouse/client.h index dfe315369..2d4cfa88d 100644 --- a/clickhouse/client.h +++ b/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. diff --git a/ut/ssl_ut.cpp b/ut/ssl_ut.cpp index 1cb7dd359..cb22fb134 100644 --- a/ut/ssl_ut.cpp +++ b/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 #include #include +#include + +#include + +#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,