Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

20 changes: 18 additions & 2 deletions crates/ruborist/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -23,13 +23,29 @@ tokio-fs-ext = { workspace = true }
tokio-retry = "0.3"
tracing = { workspace = true }
# HTTP client - works on both native and WASM
#
# We use `rustls-tls-native-roots-no-provider` (instead of the default
# `rustls-tls-native-roots`) so reqwest doesn't pin the crypto provider
# to `ring`. We then build our own `rustls::ClientConfig` with
# `aws-lc-rs` (BoringSSL's crypto primitives) in `service/http.rs` and
# pass it via `Client::use_preconfigured_tls`. Measured on CI against
# npmjs.org: `ring` took 78 ms p50 / 154 ms max from CCS → first
# AppData per handshake (CPU-bound; 128 parallel handshakes serialise
# across 4 cores). aws-lc-rs's per-handshake CPU is roughly 3× faster.
reqwest = { version = "0.12", default-features = false, features = [
"http2",
"json",
"rustls-tls",
"rustls-tls-native-roots",
"rustls-tls-native-roots-no-provider",
"socks"
] }
# Our own rustls + aws-lc-rs crypto provider.
rustls = { version = "0.23", default-features = false, features = [
"aws-lc-rs",
"logging",
"std",
"tls12"
] }
rustls-native-certs = "0.8"
# Shared URL-hash + staging helpers for native-only resolvers (git / http)
sha2 = { workspace = true, optional = true }
tempfile = { workspace = true, optional = true }
Expand Down
46 changes: 45 additions & 1 deletion crates/ruborist/src/service/http.rs
Original file line number Diff line number Diff line change
Expand Up @@ -93,6 +93,49 @@ pub(crate) fn get_client() -> Result<&'static reqwest::Client> {
HTTP_CLIENT.as_ref().map_err(|e| anyhow!("{e}"))
}

/// Build a `rustls::ClientConfig` using the `aws-lc-rs` crypto provider
/// instead of reqwest's default `ring`. Measured on CI (4-core runner)
/// against npmjs.org, ring's per-TLS-handshake client-side CPU cost
/// (ECDHE key derivation + cert verification + Finished MAC) serialised
/// across 128 parallel handshakes into a 154 ms "CCS → first AppData"
/// span — the HTTP requests couldn't fire until all TLS crypto drained
/// through 4 async workers. aws-lc-rs uses BoringSSL's assembly-optimised
/// primitives and is roughly 3× faster at handshake work.
#[cfg(not(target_arch = "wasm32"))]
fn build_rustls_config() -> Result<rustls::ClientConfig> {
// Install aws-lc-rs as the default for any other rustls consumer in
// the process. Idempotent — only the first call per process wins.
let _ = rustls::crypto::aws_lc_rs::default_provider().install_default();

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.

medium

Calling install_default() sets the process-wide default crypto provider for rustls. While this is generally acceptable for a CLI tool, it can cause conflicts if this crate is ever used as a library within a larger application that has already initialized a different provider (like ring). Since you are already explicitly passing the provider via builder_with_provider on line 128, this global installation might be unnecessary unless you intend to affect other rustls consumers in the same process.


// Load OS root certs. On Linux this hits /etc/ssl/certs/,
// on macOS it queries the Security framework keychain. Called once
// per process via `HTTP_CLIENT`'s `LazyLock`.
let roots = rustls_native_certs::load_native_certs();
let mut root_store = rustls::RootCertStore::empty();
for cert in roots.certs {
// Best-effort: skip any cert rustls refuses (same tolerance
// native-tls shows). A hard fail here would brick every
// request on a box with one bad root in its trust store.
let _ = root_store.add(cert);
}
Comment on lines +115 to +120

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.

medium

If load_native_certs() fails to find any valid certificates (e.g., in a minimal container environment), root_store will be empty, causing all subsequent HTTPS requests to fail with certificate verification errors. While the best-effort approach is appropriate for individual certificate failures, it would be beneficial to log a warning if the resulting root_store is completely empty to aid in troubleshooting environment issues.

if !roots.errors.is_empty() {
tracing::debug!(
"rustls-native-certs reported {} non-fatal load issues",
roots.errors.len()
);
}

let config = rustls::ClientConfig::builder_with_provider(std::sync::Arc::new(
rustls::crypto::aws_lc_rs::default_provider(),
))
.with_safe_default_protocol_versions()
.map_err(|e| anyhow!("rustls safe_default_protocol_versions: {e}"))?
.with_root_certificates(root_store)
.with_no_client_auth();

Ok(config)
}

/// Create a [`reqwest::ClientBuilder`] with TLS, DNS caching, and proxy
/// from environment variables.
///
Expand All @@ -112,8 +155,9 @@ pub fn client_builder() -> Result<reqwest::ClientBuilder> {
let builder = {
use crate::service::dns::shared_resolver;

let tls_config = build_rustls_config()?;
let mut builder = builder
.use_rustls_tls()
.use_preconfigured_tls(tls_config)
.no_proxy()
.dns_resolver(shared_resolver());

Expand Down
Loading