fix(io): bound the TLS handshake performed when tunneling HTTPS through a proxy - #2309
Conversation
…S through a proxy The TLS handshake timeout configured on HttpClient only covered direct connections; when a proxy is configured, hyper-http-proxy::ProxyConnector performs its own separate TLS handshake over the CONNECT tunnel that wasn't bounded by any timeout, so a stalled proxy could hang a request indefinitely. Switches to a fork of hyper-http-proxy that adds a set_tls_handshake_timeout API and wires it up with the same timeout used for direct connections.
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn tls_handshake_timeout_fires_against_a_stalled_proxy_tunnel() { |
There was a problem hiding this comment.
I could go either way on this test. It duplicates a test in the upstream crate, but also it checks that we wired the timeout through correctly. I'm inclined to leave it.
There was a problem hiding this comment.
Not a bad idea to verify the upstream crate behavior doesn't change.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8467084388
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Binary Size Analysis (Agent Data Plane)Baseline: 4d40f35 · Comparison: 731106d · diff ✅ Binary size difference within thresholdChanges by Module
Detailed Symbol Changes |
There was a problem hiding this comment.
With tls_handshake_timeout: 0 and an HTTPS proxy, the new wiring converts the documented disabled value into an active zero-length proxy deadline, causing new tunneled connections to fail immediately. Preserve zero as None when configuring the proxy connector.
🤖 Datadog Autotest · Commit 8467084 · What is Autotest? · @DataDog review to ask questions · Any feedback? Reach out in #autotest
Regression Detector (Agent Data Plane)Run ID: Optimization Goals: ✅ No significant changes detectedFine details of change detection per experiment (5)Experiments configured
Bounds Checks: ✅ Passed (5)
ExplanationA change is flagged as a regression when |Δ mean %| > 5.00% in the regressing direction for its optimization goal AND SMP marks the experiment as a regression ( |
…nels A zero tls_handshake_timeout means "disabled" for direct connections, but it was passed through to the proxy connector as Some(Duration::ZERO), which hyper-http-proxy treats as an active zero-length deadline, failing every tunneled HTTPS connection through a proxy immediately. Map zero to None before handing it to the proxy connector, and add a test.
Points Cargo.lock at the fork commit that addresses upstream review feedback on the TLS handshake timeout PR (metalbear-co/hyper-http-proxy#9).
metalbear-co/hyper-http-proxy#9 merged and released as 1.2.0, so the fork patch is no longer needed.
The git patch this allowance covered was already replaced on this branch, first by a different fork and now by the released upstream crate, so no dependency resolves to that source anymore.
…gh a proxy (#2309) ## Human Summary Use the upstream `hyper-http-proxy` crate, which now [supports setting a timeout on TLS handshakes](metalbear-co/hyper-http-proxy#9). This started out as a fork while the change was upstream, but that PR has since merged and been released as 1.2.0, so we no longer need it. ## AI Summary When a proxy is configured, `HttpClient`'s TLS handshake timeout only bounds direct connections — `hyper_http_proxy::ProxyConnector` performs a separate TLS handshake over the CONNECT tunnel to reach HTTPS destinations, and that handshake was previously unbounded, so a stalled or unresponsive proxy could hang a request indefinitely. This adds a `set_tls_handshake_timeout` API to `hyper-http-proxy` (contributed upstream via [metalbear-co/hyper-http-proxy#9](metalbear-co/hyper-http-proxy#9), merged and released as 1.2.0) and wires it up using the same handshake timeout already configured for direct connections. ## Test plan - [x] Added `tls_handshake_timeout_fires_against_a_stalled_proxy_tunnel`, which emulates a proxy that completes the CONNECT tunnel but never speaks TLS, and asserts the client's configured handshake timeout ends the request. Co-authored-by: jesse.szwedko <jesse.szwedko@datadoghq.com> f76d1fb
| } | ||
|
|
||
| #[tokio::test] | ||
| async fn tls_handshake_timeout_fires_against_a_stalled_proxy_tunnel() { |
There was a problem hiding this comment.
Not a bad idea to verify the upstream crate behavior doesn't change.
| [patch.crates-io] | ||
| # Forked version of `hyper-http-proxy` that removes an unused dependency on `rustls-native-certs`, which transitively depends | ||
| # on a version of `rustls-pemfile` that is no longer maintained and triggers a hit when running `cargo deny`. | ||
| hyper-http-proxy = { git = "https://github.com/tobz/hyper-http-proxy.git", branch = "main" } | ||
|
|
Human Summary
Use the upstream
hyper-http-proxycrate, which now supports setting a timeout on TLS handshakes. This started out as a fork while the change was upstream, but that PR has since merged and been released as 1.2.0, so we no longer need it.AI Summary
When a proxy is configured,
HttpClient's TLS handshake timeout only bounds direct connections —hyper_http_proxy::ProxyConnectorperforms a separate TLS handshake over the CONNECT tunnel to reach HTTPS destinations, and that handshake was previously unbounded, so a stalled or unresponsive proxy could hang a request indefinitely. This adds aset_tls_handshake_timeoutAPI tohyper-http-proxy(contributed upstream via metalbear-co/hyper-http-proxy#9, merged and released as 1.2.0) and wires it up using the same handshake timeout already configured for direct connections.Test plan
tls_handshake_timeout_fires_against_a_stalled_proxy_tunnel, which emulates a proxy that completes the CONNECT tunnel but never speaks TLS, and asserts the client's configured handshake timeout ends the request.