From 2dd79e372c689dd4c3882b38789dfdfdce364bcf Mon Sep 17 00:00:00 2001 From: John-David Dalton Date: Thu, 18 Jun 2026 17:38:13 -0400 Subject: [PATCH] feat(pacquet): honor NODE_EXTRA_CA_CERTS for custom CA trust (#12508) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(pacquet): honor NODE_EXTRA_CA_CERTS for custom CA trust pacquet's reqwest client trusts only its bundled webpki roots plus the .npmrc ca/cafile material — it ignores NODE_EXTRA_CA_CERTS. pnpm running on Node picks that variable up transitively via Node's TLS runtime, so a native port has to read it explicitly to keep real-world parity for users behind a corporate MITM proxy or a self-signed registry. Load the named PEM bundle as additional trust roots in default_client_builder (the single chokepoint every client routes through), keeping the .npmrc-derived TlsConfig env-free. Additive and lowest-priority; a missing, unreadable, or malformed file is silently ignored, matching pnpm's silent treatment of a missing cafile. Documented as the one deliberate exception to the TlsConfig no-env-vars parity note. * perf(pacquet): load NODE_EXTRA_CA_CERTS once per for_installs Addresses review on the NODE_EXTRA_CA_CERTS change: - Read and parse the bundle once in for_installs via load_node_extra_ca_certs(), then clone the parsed certs into the default client and each per-registry client. Previously default_client_builder re-read and re-parsed the file on every call, i.e. once per per-registry override. - Use the existing EnvGuard test helper (process-wide lock + restore on drop, panic-safe) instead of a hand-rolled lock with manual restore. The test now also asserts a valid bundle parses to one root and a missing file yields none. * test(pacquet): align NODE_EXTRA_CA_CERTS test with aube's Make the pacquet and aube tests mirror each other in form and coverage: exercise the same four cases in the same order — empty value, valid bundle (asserting one parsed root plus a successful client build), a readable non-PEM file, and a missing file — each asserting load_node_extra_ca_certs() yields the expected roots. Also align the env-var read in load_node_extra_ca_certs to the same let-else + filter form aube uses (no behavior change). * docs(pacquet): fix broken intra-doc links in load_node_extra_ca_certs The free function's doc used [`Self::for_installs`], but `Self` only resolves in impl/trait contexts, so rustdoc flagged it as an unresolved intra-doc link and the Rust CI Doc job (RUSTDOCFLAGS=-D warnings) failed. Reference [`ThrottledClient::for_installs`] instead. --- Cargo.lock | 1 + pacquet/crates/network/Cargo.toml | 7 +++-- pacquet/crates/network/src/lib.rs | 47 +++++++++++++++++++++++++++++ pacquet/crates/network/src/tests.rs | 43 ++++++++++++++++++++++++++ pacquet/crates/network/src/tls.rs | 8 +++++ 5 files changed, 103 insertions(+), 3 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8cd5a9d937..8b16255da2 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3983,6 +3983,7 @@ dependencies = [ "derive_more", "miette 7.6.0", "mockito", + "pacquet-testing-utils", "pipe-trait", "pretty_assertions", "reqwest 0.13.4", diff --git a/pacquet/crates/network/Cargo.toml b/pacquet/crates/network/Cargo.toml index 68d8ca3eaa..cb059f6d2b 100644 --- a/pacquet/crates/network/Cargo.toml +++ b/pacquet/crates/network/Cargo.toml @@ -19,9 +19,10 @@ tokio = { workspace = true, features = ["time"] } tracing = { workspace = true } [dev-dependencies] -mockito = { workspace = true } -pretty_assertions = { workspace = true } -tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } +mockito = { workspace = true } +pacquet-testing-utils = { workspace = true } +pretty_assertions = { workspace = true } +tokio = { workspace = true, features = ["macros", "rt-multi-thread"] } [lints] workspace = true diff --git a/pacquet/crates/network/src/lib.rs b/pacquet/crates/network/src/lib.rs index a45a4227b3..90f2f98f3e 100644 --- a/pacquet/crates/network/src/lib.rs +++ b/pacquet/crates/network/src/lib.rs @@ -283,6 +283,10 @@ impl ThrottledClient { let https = proxy.https_proxy.as_deref().map(parse_proxy_url).transpose()?; let http = proxy.http_proxy.as_deref().map(parse_proxy_url).transpose()?; let no_proxy = Arc::new(NoProxyMatcher::from(proxy.no_proxy.as_ref())); + // Read once here, not inside `build_client`: `for_installs` + // builds one client per per-registry override, so loading the + // bundle per call would re-read and re-parse it N times. + let extra_ca_certs = load_node_extra_ca_certs(); let build_client = |effective_tls: &TlsConfig| -> Result { let mut builder = default_client_builder(settings); @@ -292,6 +296,11 @@ impl ThrottledClient { if let Some(url) = http.clone() { builder = builder.proxy(build_scheme_proxy(url, "http", Arc::clone(&no_proxy))); } + // Lowest-priority additive roots; `apply_tls` layers the + // `.npmrc` ca/cafile roots on top next. + for cert in &extra_ca_certs { + builder = builder.add_root_certificate(cert.clone()); + } builder = apply_tls(builder, effective_tls)?; Ok(builder.build().expect("build reqwest client with default timeouts and proxy")) }; @@ -411,6 +420,44 @@ fn default_client_builder(settings: &NetworkSettings) -> reqwest::ClientBuilder .hickory_dns(true) } +/// Load the PEM bundle named by `NODE_EXTRA_CA_CERTS` as extra trust +/// roots, to be added to every client `for_installs` builds. +/// +/// `NODE_EXTRA_CA_CERTS` is the standard Node convention for appending +/// a CA to the default trust store. pnpm-on-Node inherits that trust +/// implicitly because it runs inside Node; pacquet is a native binary, +/// so to keep real-world parity for users behind a corporate MITM proxy +/// it reads the variable explicitly. This is the one deliberate +/// exception to the ".npmrc-only, no env vars" TLS parity policy +/// documented in [`tls::TlsConfig`]: the variable is a process-global +/// Node convention rather than a pnpm setting, and Node already honors +/// it for pnpm today — so reading it *restores* parity rather than +/// diverging from it. The certs are added in +/// [`ThrottledClient::for_installs`] (not [`apply_tls`]) so the +/// `.npmrc`-derived [`TlsConfig`] stays env-free. +/// +/// Read and parsed once per [`ThrottledClient::for_installs`] call — +/// that constructor builds one client per per-registry override, so +/// loading here (rather than inside the per-client builder) avoids +/// re-reading and re-parsing the bundle N times during startup. +/// +/// The resulting certs are additive and lowest-priority: layered under +/// the `.npmrc` `ca` / `cafile` roots that [`apply_tls`] adds afterward +/// and under the built-in webpki roots (ordering is immaterial — the +/// rustls root store is a union). A missing, unreadable, or malformed +/// file yields an empty list, matching pnpm's silent treatment of a +/// missing `cafile` rather than failing the client build. +fn load_node_extra_ca_certs() -> Vec { + let Some(path) = std::env::var_os("NODE_EXTRA_CA_CERTS").filter(|value| !value.is_empty()) + else { + return Vec::new(); + }; + let Ok(bytes) = std::fs::read(&path) else { + return Vec::new(); + }; + Certificate::from_pem_bundle(&bytes).unwrap_or_default() +} + /// Apply [`TlsConfig`] onto a [`reqwest::ClientBuilder`]: register each /// CA, install the client identity, set `danger_accept_invalid_certs` /// when `strict_ssl: false`, and pin the outbound interface. Returns diff --git a/pacquet/crates/network/src/tests.rs b/pacquet/crates/network/src/tests.rs index 0349194685..88755fd89a 100644 --- a/pacquet/crates/network/src/tests.rs +++ b/pacquet/crates/network/src/tests.rs @@ -19,6 +19,7 @@ use super::{ ProxyError, ThrottledClient, TlsConfig, parse_proxy_url, }; use crate::proxy::{percent_decode_str, strip_userinfo}; +use pacquet_testing_utils::env_guard::EnvGuard; use reqwest::Url; fn list(entries: &[&str]) -> NoProxySetting { @@ -424,6 +425,48 @@ fn for_installs_strict_ssl_default_is_true() { .expect("strict-ssl unset builds"); } +#[test] +fn node_extra_ca_certs_is_loaded_and_failures_are_non_fatal() { + // `EnvGuard` serializes env-mutating tests process-wide and restores + // the prior value on drop — including on panic — so a failing + // `.expect()` below can't leak `NODE_EXTRA_CA_CERTS` into a sibling + // test. `for_installs` re-reads the var on each call. + let env = EnvGuard::snapshot(["NODE_EXTRA_CA_CERTS"]); + let fixture = concat!(env!("CARGO_MANIFEST_DIR"), "/tests/fixtures/test-ca.pem"); + + let build = || { + ThrottledClient::for_installs( + &ProxyConfig::default(), + &TlsConfig::default(), + &PerRegistryTls::default(), + &NetworkSettings::default(), + ) + }; + + // Empty value: nothing to add. + env.set("NODE_EXTRA_CA_CERTS", ""); + assert!(super::load_node_extra_ca_certs().is_empty()); + + // A valid PEM bundle parses into one trust root, and a client built + // with it succeeds. + env.set("NODE_EXTRA_CA_CERTS", fixture); + assert_eq!(super::load_node_extra_ca_certs().len(), 1); + build().expect("NODE_EXTRA_CA_CERTS pointing at a valid PEM builds"); + + // A readable file that isn't valid PEM: ignored → empty. + let bad = + std::env::temp_dir().join(format!("pacquet-node-extra-ca-{}.pem", std::process::id())); + std::fs::write(&bad, b"not a certificate").expect("write temp ca bundle"); + env.set("NODE_EXTRA_CA_CERTS", &bad); + assert!(super::load_node_extra_ca_certs().is_empty()); + let _ = std::fs::remove_file(&bad); + + // A nonexistent file: unreadable, ignored → empty. + env.set("NODE_EXTRA_CA_CERTS", "/pacquet/does-not-exist.pem"); + assert!(super::load_node_extra_ca_certs().is_empty()); + // `env` restores NODE_EXTRA_CA_CERTS on drop. +} + #[test] fn for_installs_local_address_pinned() { use std::net::Ipv4Addr; diff --git a/pacquet/crates/network/src/tls.rs b/pacquet/crates/network/src/tls.rs index cb8a5c416c..ed18e7935b 100644 --- a/pacquet/crates/network/src/tls.rs +++ b/pacquet/crates/network/src/tls.rs @@ -16,6 +16,14 @@ //! internally), emits no `ERR_PNPM_*` codes for malformed TLS //! material, silently ignores a missing `cafile`, and consults no //! environment variables. Pacquet mirrors each of those choices. +//! +//! One deliberate exception sits *outside* this struct: pacquet honors +//! the `NODE_EXTRA_CA_CERTS` environment variable as an additional +//! trust root (see `load_node_extra_ca_certs` in `lib.rs`). pnpm-on- +//! Node already trusts that bundle implicitly via Node's TLS runtime, +//! so a native port must read it explicitly to preserve real-world +//! parity. It is applied at the client-builder layer, never folded +//! into this `.npmrc`-only `TlsConfig`. use crate::auth::nerf_dart; use std::{collections::HashMap, net::IpAddr};