Skip to content

rust: forward intercepted HTTP via hyper+native-tls to drop ring - #1973

Draft
Shivam60 wants to merge 5 commits into
github:mainfrom
Shivam60:shchaw/drop-reqwest-remove-ring
Draft

rust: forward intercepted HTTP via hyper+native-tls to drop ring#1973
Shivam60 wants to merge 5 commits into
github:mainfrom
Shivam60:shchaw/drop-reqwest-remove-ring

Conversation

@Shivam60

Copy link
Copy Markdown
Contributor

reqwest (forward_http) pins rustls/hyper-rustls optionals in Cargo.lock, dragging in ring (CG High, MVS-2022-374v-6mvc) even though only native-TLS is used and ring is never compiled. Replace reqwest with its underlying hyper+hyper-tls native-TLS stack (native-tls ALPN preserves HTTP/2). Removes ring/rustls entirely; all tests pass, clippy clean.

Comment thread rust/tests/e2e/mcp_oauth.rs Fixed
Two fixes so this actually drops ring and passes CI:

- Commit the regenerated Cargo.lock. Cargo.toml had already moved off reqwest
  to the hyper stack, but the lockfile was not updated, so ring/rustls/reqwest
  were still locked (the whole point is to remove them) and the lock was out of
  sync with the manifest, failing the Rust test jobs. Regenerating removes ring,
  rustls, hyper-rustls, tokio-rustls and untrusted.

- Rewrite OAuthMcpServer::requests over the hyper-util legacy Client. The
  previous version called hyper::body::to_bytes, which no longer exists in
  hyper 1.x, and had trailing whitespace that failed cargo fmt --check.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@Shivam60
Shivam60 marked this pull request as ready for review July 13, 2026 16:30
@Shivam60
Shivam60 requested a review from a team as a code owner July 13, 2026 16:30
Copilot AI review requested due to automatic review settings July 13, 2026 16:30

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot encountered an error: Your billing is not configured or you have Copilot licenses from multiple standalone organizations or enterprises. To use premium requests, select a billing entity via the GitHub site, under Settings > Copilot > Features.

@stephentoub stephentoub left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does this no longer respect proxy configuration, e.g. HTTP_PROXY, HTTPS_PROXY, and NO_PROXY?

@SteveSandersonMS

Copy link
Copy Markdown
Contributor

Manual validation: HTTP/2 regression reproduced

I independently reproduced a functional regression in forward_http while reviewing this PR, by exercising the real request path against a live HTTP/2-only endpoint (https://api.github.com) on both branches over the same network path:

  • main (reqwest/native-tls): request succeeds at the protocol level — returns a normal 403 (missing User-Agent), i.e. the TLS/HTTP framing negotiation was transparent.
  • This PR (hyper + hyper-tls/native-tls): the same request returns 400 An HTTP/1.x request was sent to an HTTP/2 only endpoint. — reproducible on every run.

Likely cause: setting ALPN preferences (h2, http/1.1) on the native-tls/hyper-tls connector only affects the TLS handshake. hyper_util::client::legacy::Client does not automatically switch request framing to HTTP/2 based on the connector's negotiated ALPN result — that plumbing (checking negotiated_alpn() on the TLS stream and driving an http2::handshake on that connection) is something reqwest does internally but isn't replicated here. So despite the doc comment ("native-tls ALPN preserves HTTP/2"), the new client is effectively HTTP/1.1-only in practice.

Why this is merge-blocking: forward_http is the general-purpose passthrough for intercepted/forwarded HTTP traffic (used as the default of CopilotRequestHandler::send_request), so this isn't a theoretical edge case — any upstream that requires HTTP/2 will now hard-fail where it worked before. Neither this PR nor the existing suite (mcp_oauth.rs) has any HTTP/2 coverage (the only http2/h2 matches in the whole diff are in the doc comment and the ALPN string list), so nothing caught this before merge.

Suggested paths forward:

  1. Add explicit HTTP/2 upgrade handling — inspect the negotiated ALPN protocol on the TLS stream post-handshake and drive an http2::handshake/HTTP/2 connection when negotiated, instead of relying on the legacy client's default HTTP/1.1 framing, or
  2. Use a TLS backend that already handles this automatically without pulling in ring (e.g. hyper-rustls configured with the aws-lc-rs or ring-free crypto provider), which keeps hyper's automatic ALPN → HTTP/2 upgrade path intact while still resolving the Cargo.lock compliance flag.

Either way, please add an e2e regression test against a real or mocked HTTP/2-only upstream before merging, so this doesn't silently regress again.

I'm converting this to draft for now — please mark it ready for review again once the HTTP/2 handling is fixed and validated against an HTTP/2-only endpoint.

@SteveSandersonMS
SteveSandersonMS marked this pull request as draft July 31, 2026 13:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants