mirror of
https://github.com/pnpm/pnpm.git
synced 2026-06-28 01:45:30 -04:00
feat(pacquet): honor NODE_EXTRA_CA_CERTS for custom CA trust (#12508)
* 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.
This commit is contained in:
committed by
GitHub
parent
511e74200d
commit
2dd79e372c
1
Cargo.lock
generated
1
Cargo.lock
generated
@@ -3983,6 +3983,7 @@ dependencies = [
|
||||
"derive_more",
|
||||
"miette 7.6.0",
|
||||
"mockito",
|
||||
"pacquet-testing-utils",
|
||||
"pipe-trait",
|
||||
"pretty_assertions",
|
||||
"reqwest 0.13.4",
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -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<Client, ForInstallsError> {
|
||||
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<Certificate> {
|
||||
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
|
||||
|
||||
@@ -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;
|
||||
|
||||
@@ -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};
|
||||
|
||||
Reference in New Issue
Block a user