Bound the TLS handshake with connectTimeoutMs

After the TCP connect the socket is blocking and bio_recv called recv
without waiting unless a stop token was attached, so a server or an
https:// proxy that accepted the connection and never sent its
ServerHello held send(), send_stream() and download_to_file() for good.

setup_tls now sets a deadline on the socket at the bottom for the
duration of the handshake. bio_recv waits against it (and against the
token, if any) through Socket::wait_before_recv, and a handshake that
runs out of time fails with "TLS handshake timed out". The deadline
covers the handshake with the target, with an https:// proxy and with
the target inside an https:// proxy's tunnel. connect_over takes the
timeout as a trailing argument; -1, the default, waits without limit.

Author: yspbwx2010

CHANGELOG.md

@@ -1,5 +1,26 @@
 # Changelog
 
+## Unreleased
+
+The TLS handshake is bounded by `connectTimeoutMs`. It had no limit: after the
+TCP connection was up, a server (or an https:// proxy) that accepted it and never
+sent its ServerHello held `send`, `send_stream` and `download_to_file` for good
+unless the caller passed a stop token.
+
+* The handshake gives up once `connectTimeoutMs` has passed since it began, and
+  `statusText` is `TLS handshake timed out`. This covers the handshake with the
+  target, with an https:// proxy, and with the target inside an https:// proxy's
+  tunnel. The limit is on the handshake as a whole, so a peer that sends a byte
+  at a time does not hold it open.
+* Behaviour change: a handshake slower than `connectTimeoutMs` (10 s by default)
+  now fails where it used to complete. Each step of setting up a connection (the
+  TCP connect, a proxy's reply, a handshake) has `connectTimeoutMs` to itself, so
+  the whole can take several times that.
+* `TlsSocket::connect_over` takes a trailing `handshakeTimeoutMs`, `-1` (the
+  default) for no limit; a call by name compiles unchanged, a pointer to it needs
+  the new parameter in its type. `Socket` gains `set_deadline`, `deadline_hit`
+  and `wait_before_recv`.
+
 ## 0.3.4
 
 A request in flight can be abandoned from another thread. Everything is added

README.md

@@ -79,7 +79,7 @@ includes in that file, or use libc++ 23 or libstdc++.
 
 | field | default | what it decides |
 | --- | --- | --- |
-| `connectTimeoutMs` | 10000 | TCP connect |
+| `connectTimeoutMs` | 10000 | setting up the connection: the TCP connect, each wait for a proxy's reply, and the TLS handshake, with the proxy and with the target (the handshake as a whole, not each read in it) |
 | `readTimeoutMs` | 60000 | any single read, and the total wait on a blocked write |
 | `verifySsl` | true | verify the server certificate against the CA bundle (`SSL_CERT_FILE`, else the Windows `ROOT` certificate store in Windows Sockets builds, else a system location); the connection fails if the certificate is not trusted, has expired or is for another host, or if no bundle is found |
 | `keepAlive` | true | reuse connections between requests |

src/http.cppm

@@ -1052,10 +1052,10 @@ private:
             }
             if (tunnel.proxyTls) {
                 return sock.connect_over(std::move(tunnel.proxyTls), parsed.host.c_str(),
-                                         config_.verifySsl);
+                                         config_.verifySsl, config_.connectTimeoutMs);
             }
             return sock.connect_over(std::move(tunnel.socket), parsed.host.c_str(),
-                                     config_.verifySsl);
+                                     config_.verifySsl, config_.connectTimeoutMs);
         }
         return sock.connect(parsed.host.c_str(), parsed.port,
                             config_.connectTimeoutMs, config_.verifySsl);

src/socket.cppm

@@ -52,7 +52,7 @@ public:
 
     // Move constructor
     Socket(Socket&& other) noexcept
-        : fd_(other.fd_), stop_(std::move(other.stop_)) {
+        : fd_(other.fd_), stop_(std::move(other.stop_)), deadline_(other.deadline_) {
         other.fd_ = INVALID_SOCKET_FD;
     }
 
@@ -62,6 +62,7 @@ public:
             close();
             fd_ = other.fd_;
             stop_ = std::move(other.stop_);
+            deadline_ = other.deadline_;
             other.fd_ = INVALID_SOCKET_FD;
         }
         return *this;
@@ -77,6 +78,32 @@ public:
 
     [[nodiscard]] bool stop_possible() const { return stop_.stop_possible(); }
 
+    // Bounds the TLS handshake: until it is cleared, `wait_before_recv` gives up
+    // once this time has passed. Cleared once the handshake is over.
+    void set_deadline(std::optional<std::chrono::steady_clock::time_point> deadline) {
+        deadline_ = deadline;
+        deadline_hit_ = false;
+    }
+
+    // True once `wait_before_recv` gave up because of the deadline.
+    [[nodiscard]] bool deadline_hit() const { return deadline_hit_; }
+
+    // What `bio_recv` calls ahead of a `recv` that would otherwise block. True
+    // when a `recv` now will not block. With neither a token nor a deadline it
+    // answers true without waiting, so the plain `recv` is as it was.
+    bool wait_before_recv() {
+        if (!stop_.stop_possible() && !deadline_) return true;
+        int wait = -1;
+        if (deadline_) {
+            const auto left = std::chrono::ceil<std::chrono::milliseconds>(
+                *deadline_ - std::chrono::steady_clock::now()).count();
+            wait = left > 0 ? static_cast<int>(left) : 0;
+        }
+        if (wait_readable(wait)) return true;
+        if (deadline_ && !stop_.stop_requested()) deadline_hit_ = true;
+        return false;
+    }
+
     bool connect(const char* host, int port, int timeoutMs) {
         // Close existing connection if any
         if (is_valid()) {
@@ -342,6 +369,8 @@ public:
 private:
     SocketHandle fd_ = INVALID_SOCKET_FD;
     std::stop_token stop_;
+    std::optional<std::chrono::steady_clock::time_point> deadline_;
+    bool deadline_hit_ = false;
 
     // poll_fd in slices while a token is attached. A negative timeout waits
     // without limit. Not std::min: <winsock2.h> defines a `min` macro.

src/tls.cppm

@@ -94,8 +94,9 @@ static int bio_send(void* ctx, const unsigned char* buf, size_t len) {
 // complete and correct.
 static int bio_recv(void* ctx, unsigned char* buf, size_t len) {
     auto* sock = static_cast<Socket*>(ctx);
-    // With a stop token, wait here so the recv below cannot block past a stop.
-    if (sock->stop_possible() && !sock->wait_readable(-1)) {
+    // With a stop token or a handshake deadline, wait here so the recv below
+    // cannot block past a stop or the deadline.
+    if (!sock->wait_before_recv()) {
         return MBEDTLS_ERR_NET_RECV_FAILED;
     }
     int ret = sock->read(reinterpret_cast<char*>(buf), static_cast<int>(len));
@@ -208,20 +209,24 @@ public:
 
     // Connect over an already-established Socket (e.g. a proxy tunnel).
     // Takes ownership of the socket and performs TLS handshake on top of it.
-    bool connect_over(Socket&& socket, const char* host, bool verifySsl) {
+    // The handshake gives up after `handshakeTimeoutMs`; a negative value waits
+    // without limit.
+    bool connect_over(Socket&& socket, const char* host, bool verifySsl,
+                      int handshakeTimeoutMs = -1) {
         error_.clear();
         socket_ = std::move(socket);
-        return setup_tls(host, verifySsl);
+        return setup_tls(host, verifySsl, handshakeTimeoutMs);
     }
 
     // Run the handshake inside another TLS session, which is how a client
     // reaches a target through an https:// proxy: TLS to the proxy, CONNECT
     // inside it, then this session to the target inside the tunnel. Takes
     // ownership of `lower`, which must already be past its CONNECT.
-    bool connect_over(std::unique_ptr<TlsSocket> lower, const char* host, bool verifySsl) {
+    bool connect_over(std::unique_ptr<TlsSocket> lower, const char* host, bool verifySsl,
+                      int handshakeTimeoutMs = -1) {
         error_.clear();
         lower_ = std::move(lower);
-        return setup_tls(host, verifySsl);
+        return setup_tls(host, verifySsl, handshakeTimeoutMs);
     }
 
     bool connect(const char* host, int port, int timeoutMs, bool verifySsl) {
@@ -231,7 +236,7 @@ public:
             return false;
         }
 
-        return setup_tls(host, verifySsl);
+        return setup_tls(host, verifySsl, timeoutMs);
     }
 
     // The read that says which of the four things happened. Prefer it over
@@ -371,7 +376,16 @@ private:
         socket_.close();
     }
 
-    bool setup_tls(const char* host, bool verifySsl) {
+    // As `set_stop`, the deadline belongs to the socket at the bottom, which
+    // inside a tunnel is the one beneath `lower_`.
+    void set_deadline(std::optional<std::chrono::steady_clock::time_point> deadline) {
+        if (lower_) lower_->set_deadline(deadline);
+        else socket_.set_deadline(deadline);
+    }
+
+    bool deadline_hit() const { return lower_ ? lower_->deadline_hit() : socket_.deadline_hit(); }
+
+    bool setup_tls(const char* host, bool verifySsl, int handshakeTimeoutMs) {
         state_ = std::make_unique<TlsState>();
 
         int ret = mbedtls_ctr_drbg_seed(
@@ -429,17 +443,30 @@ private:
         // Set BIO callbacks using our Socket, or the session beneath this one
         bind_bio();
 
-        // Perform TLS handshake
-        while ((ret = mbedtls_ssl_handshake(&state_->ssl)) != 0) {
-            if (ret != MBEDTLS_ERR_SSL_WANT_READ && ret != MBEDTLS_ERR_SSL_WANT_WRITE) {
-                if (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED) {
-                    char info[512] = {};
-                    mbedtls_x509_crt_verify_info(info, sizeof info, "",
-                        mbedtls_ssl_get_verify_result(&state_->ssl));
-                    return fail("certificate verification failed: " + one_line(info));
-                }
-                return fail("TLS handshake failed: " + mbedtls_message(ret));
+        // Perform TLS handshake. The socket is blocking, so `bio_recv` waits
+        // against the deadline before it reads, and the deadline is taken off
+        // again before the connection is used.
+        std::optional<std::chrono::steady_clock::time_point> deadline;
+        if (handshakeTimeoutMs >= 0) {
+            deadline = std::chrono::steady_clock::now()
+                     + std::chrono::milliseconds(handshakeTimeoutMs);
+        }
+        set_deadline(deadline);
+        while ((ret = mbedtls_ssl_handshake(&state_->ssl)) == MBEDTLS_ERR_SSL_WANT_READ
+               || ret == MBEDTLS_ERR_SSL_WANT_WRITE) {}
+        const bool timedOut = deadline_hit();
+        set_deadline({});
+        if (ret != 0) {
+            if (ret == MBEDTLS_ERR_X509_CERT_VERIFY_FAILED) {
+                char info[512] = {};
+                mbedtls_x509_crt_verify_info(info, sizeof info, "",
+                    mbedtls_ssl_get_verify_result(&state_->ssl));
+                return fail("certificate verification failed: " + one_line(info));
+            }
+            if (ret == MBEDTLS_ERR_NET_RECV_FAILED && timedOut) {
+                return fail("TLS handshake timed out");
             }
+            return fail("TLS handshake failed: " + mbedtls_message(ret));
         }
 
         return true;