http: replace the hand-rolled client with ureq; HTTPS via rustls (rebase of #560) - #702
http: replace the hand-rolled client with ureq; HTTPS via rustls (rebase of #560)#702zmanian wants to merge 4 commits into
Conversation
- supports both native-tls and rustls
Rebase of @mzachar's #560 onto current main, with the TLS backend narrowed to rustls only. Implements #490. Deletes 553 lines of hand-rolled HTTP (request building, response parsing, chunked-transfer handling, the `PathBuf` type) in favour of `ureq` 3.4, and adds an `https` feature for TLS to `yubihsm-connector`. **rustls only.** #560 offered `native-tls`, `native-tls-vendored` and `rustls` behind a `_tls` marker feature. This keeps just rustls: no system OpenSSL, no vendored C build, one code path. That also removes the `_provider` cfg-juggling in `build_tls_config`, which existed solely to keep two mutually exclusive backends from colliding under `--all-features`. Conflicts were `modify/delete`: main had changed `connector/http/client/{connection,path}.rs`, which this deletes. Taking the deletion also retires the request-path validation added in #696 -- that guarded a hand-built request line which no longer exists, since ureq constructs and encodes the target itself. Dependency footprint, measured rather than estimated: --no-default-features 75 crates default (incl. http) 93 crates (+18: ureq and its tree) --features=https 100 crates (+7: rustls, ring, webpki, ...) `http` is a default feature, so the +18 reaches every consumer that does not opt out. That is the one thing worth a maintainer's explicit agreement, and it is called out in the PR description rather than buried here. Co-Authored-By: Michal Zachar <mzachar@users.noreply.github.com> Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e72fd524a1
ℹ️ 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".
| use ureq::Agent; | ||
| use uuid::Uuid; | ||
|
|
||
| const MAX_BODY_SIZE: u64 = 1024 ^ 3; /*1MB*/ |
There was a problem hiding this comment.
^ is bitwise XOR in Rust, so this constant is 1,027 bytes rather than 1 MiB. Any valid connector response above that size is silently truncated before protocol parsing; for example, Client::get_pseudo_random permits up to 2,045 random bytes, but such an HTTP response will now fail with an unexpected-length protocol error.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Real bug, fixed in a6a0b9f. ^ is XOR, so the limit was 1027 bytes, not the 1 MiB the comment claimed. Now 1024 * 1024. Your get_pseudo_random example is the sharpest case: it permits up to 2045 bytes, which would have been truncated at 1027 and then surfaced as an unexplained protocol length error rather than anything pointing at the real cause.
| .get("Content-Length") | ||
| .and_then(|len| len.to_str().ok()) | ||
| .and_then(|len| len.parse().ok()) | ||
| .map(Vec::with_capacity) |
There was a problem hiding this comment.
When the HTTP connector or an on-path peer returns an attacker-controlled Content-Length, this allocates the declared size before the reader's body limit is applied. A very large valid usize can therefore abort the process through OOM, while a value above the allocation limit can panic with capacity overflow; the previous client rejected responses above its fixed 64 KiB buffer, so the capacity hint should be bounded by the configured body limit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in a6a0b9f — the hint is now clamped with len.min(MAX_BODY_SIZE) before the allocation, so we reserve at most what we were willing to read anyway. Worth stating why this matters here specifically: the threat model for this crate already assumes the path to yubihsm-connector is hostile, which is the entire reason the SCP03 session exists on top of it. A header that can abort the client process is squarely inside that model, not outside it.
| /// Use https if true | ||
| #[cfg(feature = "https")] | ||
| pub tls: bool, |
There was a problem hiding this comment.
tls during deserialization
With the new https feature enabled, deserializing an existing HttpConfig that contains only addr, port, and timeout_ms now fails because Serde treats this new bool field as required. The Default implementation setting it to false is not consulted by the derived deserializer, so the field needs a Serde default to keep existing configuration files usable.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Correct, and it is a silent break for existing users rather than a cosmetic one: enabling https made every pre-existing config fail to deserialize, since the derived impl does not consult Default. Both tls and cacert now carry #[serde(default)] as of a6a0b9f.
Added a regression test, which reproduced it before the fix:
called `Result::unwrap()` on an `Err` value: Error("missing field `tls`", line: 1, column: 51)
and passes after.
…ig fields Addresses the three open Codex findings on #702. - `MAX_BODY_SIZE` was `1024 ^ 3`. `^` is XOR in Rust, not exponentiation, so the limit was 1027 bytes rather than the 1 MiB the comment claimed. Any connector response above that was truncated and then failed protocol parsing with a bogus length error -- `get_pseudo_random` permits up to 2045 bytes and would have hit this. - The `Content-Length` capacity hint was used verbatim. It is attacker controlled if anything on the path to the connector is, and it was applied before the reader's body limit, so a large declared length aborted the process on allocation. Clamp it to what we are willing to read anyway. - `tls` and `cacert` are new fields behind the `https` feature, and serde treats a plain `bool` as required, so enabling `https` broke deserialization of every existing config. `Default` is not consulted by the derived impl; the fields need `#[serde(default)]`. Covered by a regression test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…ault panic Two things, the second considerably more serious than the first. **native-tls is back**, per @mzachar's point on #560: dropping it removed the only option for targets where rustls' backend will not build, and for deployments that must go through a FIPS-validated OpenSSL. It is opt-in and rustls stays the default and the recommendation. https rustls (default, recommended) https-native-tls Schannel / Security.framework / OpenSSL https-native-tls-vendored the above, statically linking OpenSSL Shared plumbing now hangs off an internal `_tls` feature so the config fields and `build_tls_config` are gated once rather than per-backend. When both backends are compiled in -- which is what `--all-features` does -- rustls is selected, so the choice stays deterministic. **The rustls path panicked.** `build_tls_config` asks for `RootCerts::PlatformVerifier` whenever no `cacert` is configured, which is the default. ureq only implements that arm behind its `platform-verifier` feature; without it the other arm is a bare `panic!`, and `https = ["http", "ureq/rustls"]` did not enable it: thread 'https_without_cacert_returns_an_error_rather_than_panicking' panicked at ureq-3.4.0/src/tls/rustls.rs:184:17: Rustls + PlatformVerifier requires feature: platform-verifier So HTTPS with the documented default configuration aborted the process at connection time. `rustls-platform-verifier` was not in the dependency tree at all. The `https` feature now pulls `ureq/platform-verifier` in unconditionally. Covered by tests/https_config.rs, which needs no connector -- it points the client at a listener that accepts and drops, which is enough to force ureq to build the TLS connector and apply the root-cert choice. Verified against all four backend combinations. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
2 participants
Rebase of #560 (@mzachar) onto current main, with the TLS backend narrowed to rustls only. Implements #490.
What it does
Deletes 553 lines of hand-rolled HTTP — request building, response parsing, chunked-transfer handling, the
PathBuftype — in favour ofureq3.4, and adds anhttpsfeature for TLS toyubihsm-connector.rustls only
#560 offered
native-tls,native-tls-vendoredandrustlsbehind a_tlsmarker feature. This keeps just rustls: no system OpenSSL, no vendored C build, one code path. It also removes the_providercfg-juggling inbuild_tls_config, which existed only to stop two mutually-exclusive backends colliding under--all-features.One trap worth recording:
_tlsgated real code inconfig.rsandconnection.rs. Dropping it from the manifest without repointing those gates would have compiled the entire TLS path out silently — the build still succeeds, the feature just does nothing. The gates now readfeature = "https".Dependency footprint — measured, not estimated
httpis a default feature, so the +18 reaches every consumer that doesn't opt out — tmkms included. That's the part I'd want explicit agreement on, and it's a consequence of replacing the client rather than of adding TLS:ureqlands in the default build even whenhttpsis off.Three ways to shape it, if the default-build growth isn't wanted:
http, use ureq only forhttps. Default build unchanged; cost is maintaining two clients.httpfromdefault. Smallest default build; breaking change for anyone relying on the default feature set.I've implemented (1) because it's what #560 set out to do, but (2) is defensible and I'm happy to switch.
Note on #696
The
modify/deleteconflicts resolved by taking the deletion, which retires the request-path validation I added in #696. That guarded a hand-built request line that no longer exists — ureq constructs and encodes the target itself. Worth stating explicitly rather than letting a security fix quietly vanish in a rebase.Verification
@mzachar — this is your work rebased; the design discussion with @wiktor-k about collapsing
addr/port/tlsinto a singleurlis still a sensible follow-up and I haven't touched it.🤖 Generated with Claude Code