Bound the TLS handshake with connectTimeoutMs (#25)

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 sets a deadline of connectTimeoutMs on the socket at the bottom for
the duration of the handshake; bio_recv waits against it (and against the
token, if any), and a handshake that runs out of time fails with "TLS handshake
timed out". It covers the target, an https:// proxy, and the target inside an
https:// proxy's tunnel. A wait that a signal ends early is waited again; the
deadline is hit only when the clock has passed it.

TlsSocket::connect_over takes a trailing handshakeTimeoutMs (-1, the default,
waits without limit); TlsSocket::connect bounds the handshake with the timeout
it already took. Socket gains set_deadline, deadline_hit and wait_before_recv.

CI: the openkal job on Windows finds Git for Windows's CA bundle instead of
naming mingw64/ (Git 2.56 moved it under ucrt64/), and the Windows job runs
test_handshake_timeout.

Co-authored-by: Sunrisepeak <speakshen@163.com>

Author: yspbwx2010

.github/workflows/ci.yml

@@ -288,10 +288,26 @@ jobs:
       # "Platforms"), so the program is given a bundle: Git for Windows's, copied
       # beside it, because openkal resolves a relative name against the working
       # directory and names no absolute path.
+      #
+      # Git is asked where its bundle is, and the install searched after that,
+      # rather than the directory named: the runner image that brought Git for
+      # Windows 2.56 has no `mingw64/etc/ssl/certs/ca-bundle.crt`, and the job
+      # failed there before the program ran.
       - name: Run above openkal
         run: |
           set -o pipefail
-          cp "/c/Program Files/Git/mingw64/etc/ssl/certs/ca-bundle.crt" ca-bundle.crt
+          git --version
+          ca=$(git config --system --get http.sslcainfo || true)
+          if [ -n "$ca" ]; then ca=$(cygpath -u "$ca"); fi
+          if [ ! -f "$ca" ]; then
+            ca=$(find "/c/Program Files/Git" -path '*/ssl/certs/ca-bundle.crt' -print -quit)
+          fi
+          if [ ! -f "$ca" ]; then
+            echo "::error::no CA bundle in the Git for Windows install"
+            exit 1
+          fi
+          echo "CA bundle: $ca"
+          cp "$ca" ca-bundle.crt
           SSL_CERT_FILE=ca-bundle.crt ./smoke.exe 2>&1 | tee run.log
           grep -q '^cancellation: ok' run.log
 
@@ -352,7 +368,7 @@ jobs:
       - name: The hermetic tests
         run: |
           set -o pipefail
-          for t in test_framing test_tls_verify test_pool test_proxy test_cancel; do
+          for t in test_framing test_tls_verify test_pool test_proxy test_cancel test_handshake_timeout; do
             mcpp test "$t" 2>&1 | tee "$t.log"
             grep -q "^$t \.\.\. ok" "$t.log"
           done

CHANGELOG.md

@@ -1,5 +1,28 @@
 # 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` bounds the handshake with the `timeoutMs` it already
+  took for the TCP connect, so a caller of it directly sees the change too.
+* `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,8 @@ 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_)
+        , deadline_hit_(other.deadline_hit_) {
         other.fd_ = INVALID_SOCKET_FD;
     }
 
@@ -62,6 +63,8 @@ public:
             close();
             fd_ = other.fd_;
             stop_ = std::move(other.stop_);
+            deadline_ = other.deadline_;
+            deadline_hit_ = other.deadline_hit_;
             other.fd_ = INVALID_SOCKET_FD;
         }
         return *this;
@@ -77,6 +80,42 @@ 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.
+    //
+    // A wait that ends before the deadline is waited again for what is left: a
+    // signal ends `poll` with EINTR even under SA_RESTART, which the blocking
+    // `recv` this wait stands in front of would have restarted. The deadline is
+    // hit only when the clock says so.
+    bool wait_before_recv() {
+        if (!stop_.stop_possible() && !deadline_) return true;
+        for (;;) {
+            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 (stop_.stop_requested() || !deadline_) return false;
+            if (std::chrono::steady_clock::now() >= *deadline_) {
+                deadline_hit_ = true;
+                return false;
+            }
+        }
+    }
+
     bool connect(const char* host, int port, int timeoutMs) {
         // Close existing connection if any
         if (is_valid()) {
@@ -342,6 +381,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;