From c10dc84035f0cc468f9b1b6f6bada4e7a99f84d6 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 12:44:03 -0400 Subject: [PATCH 1/3] Trust platform and SSL_CERT_FILE roots (WIP) Co-Authored-By: Claude Opus 5.5 (1M context) From c510914b01e27fa552a07f4bd4043be5ff2e8b46 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 13:11:51 -0400 Subject: [PATCH 2/3] Trust the OS store and SSL_CERT_FILE for HTTPS Every HTTPS call (patch API, public proxy, blob and registry fetches, telemetry, self-update) trusted only the bundled Mozilla roots. Behind a TLS-inspecting corporate proxy, whose private CA no setting could add, every request failed with UnknownIssuer. All production reqwest clients are now built by one constructor, utils::http::client_builder(), which adds the platform trust store to the bundled roots. rustls-native-certs reads the macOS keychain, the Windows store or the Linux bundle, and honors SSL_CERT_FILE and SSL_CERT_DIR. The store is read once per process and unusable roots are skipped. Verification is never relaxed. Tests run a local TLS server signed by a private CA: the handshake succeeds only when SSL_CERT_FILE names that CA. A text ratchet keeps new reqwest::Client constructors out of production code. Fixes #1107 Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 69 +++++ Cargo.toml | 7 + crates/socket-patch-cli/CLI_CONTRACT.md | 2 +- crates/socket-patch-core/Cargo.toml | 3 + crates/socket-patch-core/src/api/client.rs | 4 +- crates/socket-patch-core/src/telemetry.rs | 2 +- .../socket-patch-core/src/update/download.rs | 2 +- .../socket-patch-core/src/update/release.rs | 2 +- crates/socket-patch-core/src/utils/http.rs | 267 ++++++++++++++++++ .../src/vendor/registry_fetch.rs | 4 +- crates/socket-patch-core/tests/tls-ca/ca.pem | 12 + .../socket-patch-core/tests/tls-ca/leaf.key | 5 + .../socket-patch-core/tests/tls-ca/leaf.pem | 12 + docs/configuration.md | 5 + 14 files changed, 388 insertions(+), 8 deletions(-) create mode 100644 crates/socket-patch-core/tests/tls-ca/ca.pem create mode 100644 crates/socket-patch-core/tests/tls-ca/leaf.key create mode 100644 crates/socket-patch-core/tests/tls-ca/leaf.pem diff --git a/Cargo.lock b/Cargo.lock index b2e66a1ef..df00e81c9 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -293,6 +293,22 @@ dependencies = [ "unicode-segmentation", ] +[[package]] +name = "core-foundation" +version = "0.10.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b2a6cd9ae233e7f62ba4e9353e81a88df7fc8a5987b8d445b4d90c879bd156f6" +dependencies = [ + "core-foundation-sys", + "libc", +] + +[[package]] +name = "core-foundation-sys" +version = "0.8.7" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "773648b94d0e5d620f64f280777445740e61fe701025087ec8b57f45c791888b" + [[package]] name = "core_detect" version = "1.0.0" @@ -1249,6 +1265,12 @@ version = "1.70.2" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "384b8ab6d37215f3c5301a95a4accb5d64aa607f1fcb26a11b5303878451b4fe" +[[package]] +name = "openssl-probe" +version = "0.2.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "7c87def4c32ab89d880effc9e097653c8da5d6ef28e6b539d313baaacfbafcbe" + [[package]] name = "parking_lot" version = "0.12.5" @@ -1591,6 +1613,18 @@ dependencies = [ "zeroize", ] +[[package]] +name = "rustls-native-certs" +version = "0.8.4" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "dab5152771c58876a2146916e53e35057e1a4dfa2b9df0f0305b07f611fdea4d" +dependencies = [ + "openssl-probe", + "rustls-pki-types", + "schannel", + "security-framework", +] + [[package]] name = "rustls-pki-types" version = "1.14.0" @@ -1651,6 +1685,15 @@ dependencies = [ "sdd", ] +[[package]] +name = "schannel" +version = "0.1.29" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "91c1b7e4904c873ef0710c1f407dde2e6287de2bebc1bbbf7d430bb7cbffd939" +dependencies = [ + "windows-sys 0.61.2", +] + [[package]] name = "scopeguard" version = "1.2.0" @@ -1663,6 +1706,29 @@ version = "3.0.10" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "490dcfcbfef26be6800d11870ff2df8774fa6e86d047e3e8c8a76b25655e41ca" +[[package]] +name = "security-framework" +version = "3.7.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "b7f4bc775c73d9a02cde8bf7b2ec4c9d12743edf609006c7facc23998404cd1d" +dependencies = [ + "bitflags 2.11.0", + "core-foundation", + "core-foundation-sys", + "libc", + "security-framework-sys", +] + +[[package]] +name = "security-framework-sys" +version = "2.17.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6ce2691df843ecc5d231c0b14ece2acc3efb62c0a398c7e1d875f3983ce020e3" +dependencies = [ + "core-foundation-sys", + "libc", +] + [[package]] name = "self-replace" version = "1.5.0" @@ -1944,6 +2010,8 @@ dependencies = [ "rayon", "regex", "reqwest", + "rustls", + "rustls-native-certs", "same-file", "self-replace", "semver", @@ -1957,6 +2025,7 @@ dependencies = [ "tempfile", "thiserror 2.0.18", "tokio", + "tokio-rustls", "tokio-util", "toml_edit", "uuid", diff --git a/Cargo.toml b/Cargo.toml index 68f7fa37f..9e9a6ccf0 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -28,6 +28,13 @@ sha2 = "=0.10.9" sha1 = "=0.10.6" hex = "=0.4.3" reqwest = { version = "=0.12.28", features = ["rustls-tls", "json"], default-features = false } +# Platform trust store (+ SSL_CERT_FILE / SSL_CERT_DIR) for every HTTPS +# client, alongside reqwest's bundled webpki roots. `rustls` (already in +# the graph through reqwest) only screens each native root before it is +# handed to reqwest; no crypto provider feature is needed for that. +rustls-native-certs = "=0.8.4" +rustls = { version = "=0.23.45", default-features = false, features = ["std"] } +tokio-rustls = { version = "=0.26.4", default-features = false, features = ["ring", "tls12"] } tokio = { version = "=1.50.0", features = ["full"] } tokio-util = "=0.7.18" futures-util = { version = "=0.3.32", default-features = false, features = ["std"] } diff --git a/crates/socket-patch-cli/CLI_CONTRACT.md b/crates/socket-patch-cli/CLI_CONTRACT.md index bb4592f4b..b094cf1f6 100644 --- a/crates/socket-patch-cli/CLI_CONTRACT.md +++ b/crates/socket-patch-cli/CLI_CONTRACT.md @@ -1128,7 +1128,7 @@ Exactly three keys are honored, each slotting **below** the env var and **above* Contract properties: - **Read-only pledge**: socket-patch never creates, modifies, or deletes this file; socket-cli owns it. There is no `socket-patch login`/`config` subcommand — use `socket login`. -- Other socket-cli keys (`apiProxy`, `enforcedOrgs`, `skipAskToPersistDefaultOrg`) and unknown keys are ignored. Non-string or empty values for the three honored keys count as unset. For an HTTP forward proxy use the standard `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY` vars, which the HTTP client honors; socket-cli's `apiProxy` is deliberately not mapped (and is unrelated to `--proxy-url`, which is the public patch *endpoint*). +- Other socket-cli keys (`apiProxy`, `enforcedOrgs`, `skipAskToPersistDefaultOrg`) and unknown keys are ignored. Non-string or empty values for the three honored keys count as unset. For an HTTP forward proxy use the standard `HTTP_PROXY`/`HTTPS_PROXY`/`NO_PROXY` vars, which the HTTP client honors. Every HTTPS client (patch API, public proxy, blob/artifact and registry fetches, telemetry, self-update) trusts the bundled webpki (Mozilla) roots plus the platform trust store; `SSL_CERT_FILE` / `SSL_CERT_DIR`, when set, replace the platform store with that bundle/directory, so a TLS-inspecting proxy's private CA can be trusted. Verification is never disabled. socket-cli's `apiProxy` is deliberately not mapped (and is unrelated to `--proxy-url`, which is the public patch *endpoint*). - Missing file / unresolvable data dir: silent (the normal case). Present but unreadable or undecodable (not base64(JSON), with a plain-JSON leniency fallback): a one-shot stderr warning naming the path, then treated as absent — never fatal, and `--json` stdout stays clean (all diagnostics are stderr-only). - The file is read lazily at most once per process, only when a key is still unresolved after flag + env. - The telemetry endpoint resolver shares the same `apiBaseUrl` chain as API-client construction (`resolve_api_base_url`), so telemetry can never target a different host than the client. diff --git a/crates/socket-patch-core/Cargo.toml b/crates/socket-patch-core/Cargo.toml index b331a0734..b48287751 100644 --- a/crates/socket-patch-core/Cargo.toml +++ b/crates/socket-patch-core/Cargo.toml @@ -26,6 +26,8 @@ sha2 = { workspace = true } sha1 = { workspace = true } hex = { workspace = true } reqwest = { workspace = true } +rustls-native-certs = { workspace = true } +rustls = { workspace = true } tokio = { workspace = true } # CancellationToken for the in-memory hosted engine (`hosted::memory`). tokio-util = { workspace = true } @@ -84,3 +86,4 @@ tempfile = { workspace = true } tokio = { workspace = true, features = ["full", "test-util"] } serial_test = { workspace = true } wiremock = { workspace = true } +tokio-rustls = { workspace = true } diff --git a/crates/socket-patch-core/src/api/client.rs b/crates/socket-patch-core/src/api/client.rs index d12a0821c..745c8033f 100644 --- a/crates/socket-patch-core/src/api/client.rs +++ b/crates/socket-patch-core/src/api/client.rs @@ -1992,7 +1992,7 @@ fn api_client(api_token: Option<&str>, timeouts: &ApiTimeouts) -> reqwest::Clien } timeouts - .apply(reqwest::Client::builder().default_headers(default_headers)) + .apply(crate::utils::http::client_builder().default_headers(default_headers)) .build() .expect("failed to build reqwest client") } @@ -2009,7 +2009,7 @@ fn plain_client(timeouts: &ApiTimeouts) -> reqwest::Client { HeaderValue::from_static(USER_AGENT_VALUE), ); timeouts - .apply(reqwest::Client::builder().default_headers(headers)) + .apply(crate::utils::http::client_builder().default_headers(headers)) .build() .expect("failed to build plain reqwest client") } diff --git a/crates/socket-patch-core/src/telemetry.rs b/crates/socket-patch-core/src/telemetry.rs index 8a2998e03..bca253c54 100644 --- a/crates/socket-patch-core/src/telemetry.rs +++ b/crates/socket-patch-core/src/telemetry.rs @@ -325,7 +325,7 @@ fn prepare_send(event: PatchTelemetryEvent, auth: &TelemetryAuth) -> PreparedSen async fn send_telemetry_event(prepared: PreparedSend) { let PreparedSend { event, url, bearer } = prepared; - let client = match reqwest::Client::builder() + let client = match crate::utils::http::client_builder() .connect_timeout(std::time::Duration::from_secs(2)) .timeout(std::time::Duration::from_secs(5)) .build() diff --git a/crates/socket-patch-core/src/update/download.rs b/crates/socket-patch-core/src/update/download.rs index 23df6ea01..a61099de7 100644 --- a/crates/socket-patch-core/src/update/download.rs +++ b/crates/socket-patch-core/src/update/download.rs @@ -60,7 +60,7 @@ fn download_client( endpoints: &UpdateEndpoints, timeouts: &UpdateTimeouts, ) -> Result { - reqwest::Client::builder() + crate::utils::http::client_builder() .user_agent(crate::constants::USER_AGENT) .connect_timeout(timeouts.connect) .timeout(timeouts.download) diff --git a/crates/socket-patch-core/src/update/release.rs b/crates/socket-patch-core/src/update/release.rs index a04e47a60..73f62817b 100644 --- a/crates/socket-patch-core/src/update/release.rs +++ b/crates/socket-patch-core/src/update/release.rs @@ -264,7 +264,7 @@ fn metadata_client( timeouts: &UpdateTimeouts, redirects: reqwest::redirect::Policy, ) -> Result { - reqwest::Client::builder() + crate::utils::http::client_builder() .user_agent(crate::constants::USER_AGENT) .connect_timeout(timeouts.connect) .timeout(timeouts.metadata) diff --git a/crates/socket-patch-core/src/utils/http.rs b/crates/socket-patch-core/src/utils/http.rs index c96c65801..65a77b898 100644 --- a/crates/socket-patch-core/src/utils/http.rs +++ b/crates/socket-patch-core/src/utils/http.rs @@ -1,5 +1,66 @@ //! Small shared HTTP primitives. +use std::sync::OnceLock; + +/// The one constructor behind every production `reqwest::Client`. +/// +/// Trust roots are reqwest's bundled webpki (Mozilla) roots **plus** the +/// configured platform roots ([`configured_roots`]): the OS trust store, or +/// the bundle that `SSL_CERT_FILE` / `SSL_CERT_DIR` name. That is the rule +/// curl, npm, pip and cargo already follow, so a TLS-inspecting corporate +/// proxy whose CA the OS (or `SSL_CERT_FILE`) trusts works here too. +/// Certificate verification is never relaxed: an issuer neither set +/// trusts still fails the handshake. +/// +/// Callers add their own headers, timeouts and redirect policy. A text +/// ratchet test (`no_client_is_built_outside_client_builder`) fails if +/// production code builds a client any other way. +pub fn client_builder() -> reqwest::ClientBuilder { + with_roots(reqwest::Client::builder(), configured_roots()) +} + +fn with_roots( + mut builder: reqwest::ClientBuilder, + roots: &[reqwest::Certificate], +) -> reqwest::ClientBuilder { + for cert in roots { + builder = builder.add_root_certificate(cert.clone()); + } + builder +} + +/// The platform roots, loaded once per process: the native store is read +/// from disk / the keychain, which is too slow to repeat for every client. +fn configured_roots() -> &'static [reqwest::Certificate] { + static ROOTS: OnceLock> = OnceLock::new(); + ROOTS.get_or_init(load_configured_roots) +} + +/// Read the platform trust store through `rustls-native-certs`, which uses +/// `SSL_CERT_FILE` / `SSL_CERT_DIR` when set and the OS store (keychain on +/// macOS, the system store on Windows, the distro bundle on Linux) +/// otherwise. Native stores often carry a few certificates rustls can't +/// parse; each root is screened on its own and an unusable one is skipped, +/// since reqwest would otherwise refuse to build the whole client. A store +/// that can't be read at all leaves only the bundled webpki roots. +fn load_configured_roots() -> Vec { + let loaded = rustls_native_certs::load_native_certs(); + if crate::utils::env_compat::is_debug_enabled() { + for err in &loaded.errors { + eprintln!("[socket-patch] could not read platform trust roots: {err}"); + } + } + loaded + .certs + .into_iter() + .filter(|der| { + let mut probe = rustls::RootCertStore::empty(); + probe.add(der.clone()).is_ok() + }) + .filter_map(|der| reqwest::Certificate::from_der(der.as_ref()).ok()) + .collect() +} + /// Why [`read_capped_typed`] gave up — typed so a caller can tell a body cut /// off mid-transfer (a transport failure, worth a retry) from a cap breach /// (the same bytes would breach it again) without matching on wording. @@ -123,4 +184,210 @@ mod tests { 16 ); } + + // ── Trust roots (#1107) ─────────────────────────────────────────── + // + // `tests/tls-ca/` holds a private CA (`ca.pem`) and a leaf for + // 127.0.0.1 it signed (`leaf.pem` + `leaf.key`, P-256, valid for a + // century): the shape of a TLS-inspecting proxy's re-signed traffic. + + const TLS_FIXTURES: &str = concat!(env!("CARGO_MANIFEST_DIR"), "/tests/tls-ca"); + + fn pem_blocks(pem: &str) -> Vec> { + use base64::Engine as _; + let mut out = Vec::new(); + let mut body: Option = None; + for line in pem.lines() { + let line = line.trim(); + if line.starts_with("-----BEGIN") { + body = Some(String::new()); + } else if line.starts_with("-----END") { + let b = body.take().expect("END without BEGIN"); + out.push(base64::engine::general_purpose::STANDARD.decode(b).unwrap()); + } else if let Some(b) = body.as_mut() { + b.push_str(line); + } + } + out + } + + /// Serve `ok` over TLS with the CA-signed leaf on 127.0.0.1; returns + /// the base URL. + async fn spawn_tls_server() -> String { + use std::sync::Arc; + use tokio::io::{AsyncReadExt, AsyncWriteExt}; + use tokio_rustls::rustls; + + let read = |name: &str| std::fs::read_to_string(format!("{TLS_FIXTURES}/{name}")).unwrap(); + let certs = pem_blocks(&read("leaf.pem")) + .into_iter() + .map(rustls::pki_types::CertificateDer::from) + .collect::>(); + let key = + rustls::pki_types::PrivateKeyDer::Pkcs8(pem_blocks(&read("leaf.key")).remove(0).into()); + let config = rustls::ServerConfig::builder_with_provider(Arc::new( + rustls::crypto::ring::default_provider(), + )) + .with_safe_default_protocol_versions() + .unwrap() + .with_no_client_auth() + .with_single_cert(certs, key) + .unwrap(); + let acceptor = tokio_rustls::TlsAcceptor::from(Arc::new(config)); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0").await.unwrap(); + let addr = listener.local_addr().unwrap(); + tokio::spawn(async move { + while let Ok((tcp, _)) = listener.accept().await { + let acceptor = acceptor.clone(); + tokio::spawn(async move { + let Ok(mut tls) = acceptor.accept(tcp).await else { + return; + }; + let mut buf = Vec::new(); + let mut chunk = [0u8; 1024]; + while !buf.windows(4).any(|w| w == b"\r\n\r\n") { + match tls.read(&mut chunk).await { + Ok(0) | Err(_) => return, + Ok(n) => buf.extend_from_slice(&chunk[..n]), + } + } + let _ = tls + .write_all( + b"HTTP/1.1 200 OK\r\ncontent-length: 2\r\nconnection: close\r\n\r\nok", + ) + .await; + let _ = tls.shutdown().await; + }); + } + }); + format!("https://{addr}/") + } + + /// Run `f` with `SSL_CERT_FILE` set to `file` and `SSL_CERT_DIR` + /// cleared, restoring both afterwards. + fn with_cert_env(file: &std::path::Path, f: impl FnOnce() -> T) -> T { + let saved = ( + std::env::var_os("SSL_CERT_FILE"), + std::env::var_os("SSL_CERT_DIR"), + ); + std::env::set_var("SSL_CERT_FILE", file); + std::env::remove_var("SSL_CERT_DIR"); + let out = f(); + match saved.0 { + Some(v) => std::env::set_var("SSL_CERT_FILE", v), + None => std::env::remove_var("SSL_CERT_FILE"), + } + if let Some(v) = saved.1 { + std::env::set_var("SSL_CERT_DIR", v); + } + out + } + + fn client_with(roots: &[reqwest::Certificate]) -> reqwest::Client { + // `no_proxy`: a sandbox HTTPS_PROXY must not intercept loopback. + with_roots(reqwest::Client::builder(), roots) + .no_proxy() + .build() + .unwrap() + } + + #[tokio::test] + #[serial_test::serial] + async fn client_trusts_a_private_ca_named_by_ssl_cert_file() { + let url = spawn_tls_server().await; + let ca = std::path::Path::new(TLS_FIXTURES).join("ca.pem"); + let roots = with_cert_env(&ca, load_configured_roots); + assert_eq!(roots.len(), 1, "SSL_CERT_FILE's one CA is loaded"); + let resp = client_with(&roots).get(&url).send().await.unwrap(); + assert_eq!(resp.status(), 200); + assert_eq!(resp.text().await.unwrap(), "ok"); + } + + #[tokio::test] + #[serial_test::serial] + async fn client_without_the_private_ca_fails_with_unknown_issuer() { + let url = spawn_tls_server().await; + let tmp = tempfile::tempdir().unwrap(); + let empty = tmp.path().join("empty.pem"); + std::fs::write(&empty, "").unwrap(); + let roots = with_cert_env(&empty, load_configured_roots); + assert!(roots.is_empty()); + let err = client_with(&roots).get(&url).send().await.unwrap_err(); + assert!( + format!("{err:?}").contains("UnknownIssuer"), + "verification must stay on: {err:?}" + ); + } + + #[test] + #[serial_test::serial] + fn unparsable_platform_roots_are_skipped_not_fatal() { + let tmp = tempfile::tempdir().unwrap(); + let bundle = tmp.path().join("bundle.pem"); + let ca = std::fs::read_to_string(format!("{TLS_FIXTURES}/ca.pem")).unwrap(); + // A syntactically valid PEM block whose DER is not a certificate. + std::fs::write( + &bundle, + format!("-----BEGIN CERTIFICATE-----\nAAAA\n-----END CERTIFICATE-----\n{ca}"), + ) + .unwrap(); + let roots = with_cert_env(&bundle, load_configured_roots); + assert_eq!(roots.len(), 1, "only the usable root survives"); + client_with(&roots); + } + + /// Ratchet: production code builds every `reqwest::Client` through + /// [`client_builder`], so each one gets the same trust roots. + #[test] + fn no_client_is_built_outside_client_builder() { + let crates = std::path::Path::new(env!("CARGO_MANIFEST_DIR")).join(".."); + let forbidden = regex::Regex::new( + r"(^|[^A-Za-z0-9_])(Client::builder|Client::new|ClientBuilder::new)\(", + ) + .unwrap(); + let mut offenders = Vec::new(); + for krate in ["socket-patch-core", "socket-patch-cli", "socket-patch-node"] { + let src = crates.join(krate).join("src"); + for entry in walkdir::WalkDir::new(&src) { + let entry = entry.unwrap(); + let path = entry.path(); + let name = path.file_name().unwrap().to_string_lossy(); + if path.extension().is_none_or(|e| e != "rs") + || name == "tests.rs" + || name.ends_with("_tests.rs") + || path.components().any(|c| { + let c = c.as_os_str(); + c == "tests" || c == "test_support" + }) + || path.ends_with("utils/http.rs") + { + continue; + } + let text = std::fs::read_to_string(path).unwrap(); + let mut in_test_mod = false; + let mut prev_cfg_test = false; + for (i, line) in text.lines().enumerate() { + if in_test_mod { + if line == "}" { + in_test_mod = false; + } + continue; + } + if prev_cfg_test && line.starts_with("mod ") && line.ends_with('{') { + in_test_mod = true; + continue; + } + prev_cfg_test = line.trim() == "#[cfg(test)]"; + if forbidden.is_match(line) { + offenders.push(format!("{}:{}: {}", path.display(), i + 1, line.trim())); + } + } + } + } + assert!( + offenders.is_empty(), + "build reqwest clients with utils::http::client_builder():\n{}", + offenders.join("\n") + ); + } } diff --git a/crates/socket-patch-core/src/vendor/registry_fetch.rs b/crates/socket-patch-core/src/vendor/registry_fetch.rs index dc3f3cbd9..c676e1e8c 100644 --- a/crates/socket-patch-core/src/vendor/registry_fetch.rs +++ b/crates/socket-patch-core/src/vendor/registry_fetch.rs @@ -45,7 +45,7 @@ pub type RegistryClient = reqwest::Client; pub fn build_registry_client() -> RegistryClient { registry_client_builder(USER_AGENT) .build() - .unwrap_or_else(|_| reqwest::Client::new()) + .expect("failed to build registry HTTP client") } /// The one builder behind every registry client (npm-family, PyPI, Go, @@ -55,7 +55,7 @@ pub fn build_registry_client() -> RegistryClient { /// artifact that keeps streaming is never cut off while a stalled host /// still fails the fetch. pub(crate) fn registry_client_builder(user_agent: &str) -> reqwest::ClientBuilder { - registry_timeouts().apply(reqwest::Client::builder().user_agent(user_agent)) + registry_timeouts().apply(crate::utils::http::client_builder().user_agent(user_agent)) } fn registry_timeouts() -> ApiTimeouts { diff --git a/crates/socket-patch-core/tests/tls-ca/ca.pem b/crates/socket-patch-core/tests/tls-ca/ca.pem new file mode 100644 index 000000000..d44f26572 --- /dev/null +++ b/crates/socket-patch-core/tests/tls-ca/ca.pem @@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIBxjCCAW2gAwIBAgIUGwVSNNK9SEGdmPh3N0MCMGY+PK4wCgYIKoZIzj0EAwIw +MDEuMCwGA1UEAwwlc29ja2V0LXBhdGNoIHRlc3QgaW5zcGVjdGluZy1wcm94eSBD +QTAgFw0yNjEwMDkxNjQ0MjRaGA8yMTI2MDkxNTE2NDQyNFowMDEuMCwGA1UEAwwl +c29ja2V0LXBhdGNoIHRlc3QgaW5zcGVjdGluZy1wcm94eSBDQTBZMBMGByqGSM49 +AgEGCCqGSM49AwEHA0IABMSimDghXzMADKRhi28e2eV8CElHc/HrKGHtrKQrBets +/15/DWbWabloRgbpEQfoaCuhh/EsqmctTjhpi/ihBdajYzBhMB0GA1UdDgQWBBTY +C8DduFw/Q2eRQGuvRPYUeYV9yTAfBgNVHSMEGDAWgBTYC8DduFw/Q2eRQGuvRPYU +eYV9yTAPBgNVHRMBAf8EBTADAQH/MA4GA1UdDwEB/wQEAwIBBjAKBggqhkjOPQQD +AgNHADBEAiAZUp+qvV/UMLGyY7xoOxPgXm1groy4RZI8Qu6C6mZgBgIgKLmxP7Ue +tYIgMGUGx79RfOiLTjhN5Yzod+A6Fc5Gs2E= +-----END CERTIFICATE----- diff --git a/crates/socket-patch-core/tests/tls-ca/leaf.key b/crates/socket-patch-core/tests/tls-ca/leaf.key new file mode 100644 index 000000000..0bef62834 --- /dev/null +++ b/crates/socket-patch-core/tests/tls-ca/leaf.key @@ -0,0 +1,5 @@ +-----BEGIN PRIVATE KEY----- +MIGHAgEAMBMGByqGSM49AgEGCCqGSM49AwEHBG0wawIBAQQgqBT1g6D/E+yP1j90 +rq08yqgWXWCV2BZhVp83aSv6152hRANCAAS+rpWwJDCduZePlhnE+QKK1XB7kFh7 +Hn4lQzckdN3Q5ywySItC6smwsD8HHlupso6juIk42DIcpwDoQUsQ+9my +-----END PRIVATE KEY----- diff --git a/crates/socket-patch-core/tests/tls-ca/leaf.pem b/crates/socket-patch-core/tests/tls-ca/leaf.pem new file mode 100644 index 000000000..5d197178f --- /dev/null +++ b/crates/socket-patch-core/tests/tls-ca/leaf.pem @@ -0,0 +1,12 @@ +-----BEGIN CERTIFICATE----- +MIIB3DCCAYGgAwIBAgIUKzzYpnLQQZIDNvSswWmpHoAQ1+kwCgYIKoZIzj0EAwIw +MDEuMCwGA1UEAwwlc29ja2V0LXBhdGNoIHRlc3QgaW5zcGVjdGluZy1wcm94eSBD +QTAgFw0yNjEwMDkxNjQ0MjRaGA8yMTI2MDkxNTE2NDQyNFowFDESMBAGA1UEAwwJ +MTI3LjAuMC4xMFkwEwYHKoZIzj0CAQYIKoZIzj0DAQcDQgAEvq6VsCQwnbmXj5YZ +xPkCitVwe5BYex5+JUM3JHTd0OcsMkiLQurJsLA/Bx5bqbKOo7iJONgyHKcA6EFL +EPvZsqOBkjCBjzAMBgNVHRMBAf8EAjAAMA4GA1UdDwEB/wQEAwIHgDATBgNVHSUE +DDAKBggrBgEFBQcDATAaBgNVHREEEzARhwR/AAABgglsb2NhbGhvc3QwHQYDVR0O +BBYEFGwk6plxQ1A1e+ZmoP4rZ42rrF33MB8GA1UdIwQYMBaAFNgLwN24XD9DZ5FA +a69E9hR5hX3JMAoGCCqGSM49BAMCA0kAMEYCIQDAfvPyBuM8QFTgP+2PGv8GFxZ5 +vafgESaeXSY79Xa1AwIhAKNSr2EmWxF+L8j7HltS/Ai+NtD755WcGEdwFzxrAG7N +-----END CERTIFICATE----- diff --git a/docs/configuration.md b/docs/configuration.md index c1b388a5e..6a8e1cbb7 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -40,6 +40,11 @@ Empty environment values are treated as unset. - `--proxy-url` / `SOCKET_PROXY_URL` selects the public patch API endpoint. It is distinct from an HTTP forward proxy: use `HTTP_PROXY`, `HTTPS_PROXY`, and `NO_PROXY` for those. +- HTTPS connections trust the bundled Mozilla (webpki) roots plus the operating + system's trust store (macOS keychain, Windows certificate store, the distro + bundle on Linux). `SSL_CERT_FILE` / `SSL_CERT_DIR` replace the system store + with the named bundle or directory, which is how to trust a TLS-inspecting + proxy's private CA. Certificate verification is never disabled. - `--offline` prohibits network access. `scan` and `get` require the patch API and refuse offline operation. Other commands can use locally available state; missing records or artifacts can still prevent completion. From 442e1e972fe1a58be591b7c7cf727fec9c1f76f1 Mon Sep 17 00:00:00 2001 From: Mikola Lysenko Date: Fri, 9 Oct 2026 14:18:43 -0400 Subject: [PATCH 3/3] Read platform roots only on an unknown issuer CI's scan benchmark showed 5-10 ms more CPU per run: every process read the whole platform store (hundreds of PEM files on Linux) and every client re-parsed those roots. client_builder() now hands reqwest one shared rustls config, built once per process. Its verifier checks the bundled webpki roots first and reads the platform store (or SSL_CERT_FILE / SSL_CERT_DIR) only after an UnknownIssuer, then once per process. Other verification errors are returned unchanged, so trust is still the union of both sets and verification is never relaxed. Building a client no longer parses any roots. The load-error log line flagged by CodeQL is gone. Co-Authored-By: Claude Opus 5.5 (1M context) --- Cargo.lock | 1 + Cargo.toml | 10 +- crates/socket-patch-core/Cargo.toml | 1 + crates/socket-patch-core/src/utils/http.rs | 255 ++++++++++++++++----- 4 files changed, 212 insertions(+), 55 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index df00e81c9..8b447a267 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -2030,6 +2030,7 @@ dependencies = [ "toml_edit", "uuid", "walkdir", + "webpki-roots", "windows-sys 0.59.0", "wiremock", "zip", diff --git a/Cargo.toml b/Cargo.toml index 9e9a6ccf0..0666823de 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -29,11 +29,13 @@ sha1 = "=0.10.6" hex = "=0.4.3" reqwest = { version = "=0.12.28", features = ["rustls-tls", "json"], default-features = false } # Platform trust store (+ SSL_CERT_FILE / SSL_CERT_DIR) for every HTTPS -# client, alongside reqwest's bundled webpki roots. `rustls` (already in -# the graph through reqwest) only screens each native root before it is -# handed to reqwest; no crypto provider feature is needed for that. +# client, alongside the bundled webpki roots (utils::http::client_builder). +# `rustls` and `webpki-roots` are the exact builds reqwest's `rustls-tls` +# already pulls in (ring provider); the shared client config is built +# from them directly. rustls-native-certs = "=0.8.4" -rustls = { version = "=0.23.45", default-features = false, features = ["std"] } +rustls = { version = "=0.23.45", default-features = false, features = ["std", "ring", "tls12"] } +webpki-roots = "=1.0.6" tokio-rustls = { version = "=0.26.4", default-features = false, features = ["ring", "tls12"] } tokio = { version = "=1.50.0", features = ["full"] } tokio-util = "=0.7.18" diff --git a/crates/socket-patch-core/Cargo.toml b/crates/socket-patch-core/Cargo.toml index b48287751..9b8d4f550 100644 --- a/crates/socket-patch-core/Cargo.toml +++ b/crates/socket-patch-core/Cargo.toml @@ -28,6 +28,7 @@ hex = { workspace = true } reqwest = { workspace = true } rustls-native-certs = { workspace = true } rustls = { workspace = true } +webpki-roots = { workspace = true } tokio = { workspace = true } # CancellationToken for the in-memory hosted engine (`hosted::memory`). tokio-util = { workspace = true } diff --git a/crates/socket-patch-core/src/utils/http.rs b/crates/socket-patch-core/src/utils/http.rs index 65a77b898..a7e03d580 100644 --- a/crates/socket-patch-core/src/utils/http.rs +++ b/crates/socket-patch-core/src/utils/http.rs @@ -1,64 +1,180 @@ //! Small shared HTTP primitives. -use std::sync::OnceLock; +use std::sync::{Arc, OnceLock}; + +use rustls::client::danger::{HandshakeSignatureValid, ServerCertVerified, ServerCertVerifier}; +use rustls::client::WebPkiServerVerifier; +use rustls::crypto::CryptoProvider; +use rustls::pki_types::{CertificateDer, ServerName, UnixTime}; +use rustls::{CertificateError, DigitallySignedStruct, RootCertStore, SignatureScheme}; /// The one constructor behind every production `reqwest::Client`. /// -/// Trust roots are reqwest's bundled webpki (Mozilla) roots **plus** the -/// configured platform roots ([`configured_roots`]): the OS trust store, or -/// the bundle that `SSL_CERT_FILE` / `SSL_CERT_DIR` name. That is the rule +/// Trust roots are the bundled webpki (Mozilla) roots **plus** the +/// platform roots ([`load_configured_roots`]): the OS trust store, or the +/// bundle that `SSL_CERT_FILE` / `SSL_CERT_DIR` name. That is the rule /// curl, npm, pip and cargo already follow, so a TLS-inspecting corporate /// proxy whose CA the OS (or `SSL_CERT_FILE`) trusts works here too. /// Certificate verification is never relaxed: an issuer neither set -/// trusts still fails the handshake. +/// trusts still fails the handshake ([`BundledThenPlatform`]). /// -/// Callers add their own headers, timeouts and redirect policy. A text -/// ratchet test (`no_client_is_built_outside_client_builder`) fails if -/// production code builds a client any other way. +/// The TLS config is built once per process and shared, so a client costs +/// no more to build than reqwest's own default. Callers add their own +/// headers, timeouts and redirect policy. A text ratchet test +/// (`no_client_is_built_outside_client_builder`) fails if production code +/// builds a client any other way. pub fn client_builder() -> reqwest::ClientBuilder { - with_roots(reqwest::Client::builder(), configured_roots()) + static CONFIG: OnceLock = OnceLock::new(); + let config = CONFIG.get_or_init(|| tls_config(Box::new(load_configured_roots))); + reqwest::Client::builder().use_preconfigured_tls(config.clone()) +} + +type RootLoader = Box Vec> + Send + Sync>; + +/// The rustls config every client shares: ring (the provider reqwest's +/// `rustls-tls` already builds), TLS 1.2 and 1.3, HTTP/1.1 ALPN (reqwest is +/// built without HTTP/2), and the [`BundledThenPlatform`] verifier. +fn tls_config(platform_roots: RootLoader) -> rustls::ClientConfig { + let provider = Arc::new(rustls::crypto::ring::default_provider()); + let verifier = BundledThenPlatform::new(provider.clone(), platform_roots); + let mut config = rustls::ClientConfig::builder_with_provider(provider) + .with_safe_default_protocol_versions() + .expect("ring supports TLS 1.2 and 1.3") + .dangerous() + .with_custom_certificate_verifier(Arc::new(verifier)) + .with_no_client_auth(); + config.alpn_protocols = vec![b"http/1.1".to_vec()]; + config +} + +/// Full webpki verification against the bundled roots first and, only +/// when that fails with `UnknownIssuer`, against the platform roots. The +/// result is the union of both trust sets, while the platform store (a +/// keychain query, or a few hundred PEM files on Linux) is read only by a +/// run that actually meets a certificate the bundled roots don't know, and +/// then once per process. Every other verification failure (expiry, wrong +/// host, bad signature) is returned unchanged. +struct BundledThenPlatform { + provider: Arc, + bundled: Arc, + platform: OnceLock>>, + platform_roots: RootLoader, +} + +impl std::fmt::Debug for BundledThenPlatform { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.debug_struct("BundledThenPlatform") + .field("platform_loaded", &self.platform.get().is_some()) + .finish_non_exhaustive() + } } -fn with_roots( - mut builder: reqwest::ClientBuilder, - roots: &[reqwest::Certificate], -) -> reqwest::ClientBuilder { - for cert in roots { - builder = builder.add_root_certificate(cert.clone()); +impl BundledThenPlatform { + fn new(provider: Arc, platform_roots: RootLoader) -> Self { + let bundled = RootCertStore { + roots: webpki_roots::TLS_SERVER_ROOTS.to_vec(), + }; + BundledThenPlatform { + bundled: verifier_for(bundled, &provider) + .expect("the bundled webpki roots form a valid verifier"), + provider, + platform: OnceLock::new(), + platform_roots, + } + } + + /// The platform-root verifier, loaded on first use. Native stores often + /// carry a few certificates rustls can't parse; those are skipped. An + /// empty or unreadable store yields `None` (bundled roots only). + fn platform(&self) -> Option<&WebPkiServerVerifier> { + self.platform + .get_or_init(|| { + let mut store = RootCertStore::empty(); + store.add_parsable_certificates((self.platform_roots)()); + if store.is_empty() { + return None; + } + verifier_for(store, &self.provider) + }) + .as_deref() } - builder } -/// The platform roots, loaded once per process: the native store is read -/// from disk / the keychain, which is too slow to repeat for every client. -fn configured_roots() -> &'static [reqwest::Certificate] { - static ROOTS: OnceLock> = OnceLock::new(); - ROOTS.get_or_init(load_configured_roots) +fn verifier_for( + roots: RootCertStore, + provider: &Arc, +) -> Option> { + WebPkiServerVerifier::builder_with_provider(Arc::new(roots), provider.clone()) + .build() + .ok() +} + +impl ServerCertVerifier for BundledThenPlatform { + fn verify_server_cert( + &self, + end_entity: &CertificateDer<'_>, + intermediates: &[CertificateDer<'_>], + server_name: &ServerName<'_>, + ocsp_response: &[u8], + now: UnixTime, + ) -> Result { + let verify = |v: &WebPkiServerVerifier| { + v.verify_server_cert(end_entity, intermediates, server_name, ocsp_response, now) + }; + match verify(&self.bundled) { + Err(rustls::Error::InvalidCertificate(CertificateError::UnknownIssuer)) => { + match self.platform() { + Some(platform) => verify(platform), + None => Err(rustls::Error::InvalidCertificate( + CertificateError::UnknownIssuer, + )), + } + } + verdict => verdict, + } + } + + fn verify_tls12_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + rustls::crypto::verify_tls12_signature( + message, + cert, + dss, + &self.provider.signature_verification_algorithms, + ) + } + + fn verify_tls13_signature( + &self, + message: &[u8], + cert: &CertificateDer<'_>, + dss: &DigitallySignedStruct, + ) -> Result { + rustls::crypto::verify_tls13_signature( + message, + cert, + dss, + &self.provider.signature_verification_algorithms, + ) + } + + fn supported_verify_schemes(&self) -> Vec { + self.provider + .signature_verification_algorithms + .supported_schemes() + } } /// Read the platform trust store through `rustls-native-certs`, which uses /// `SSL_CERT_FILE` / `SSL_CERT_DIR` when set and the OS store (keychain on /// macOS, the system store on Windows, the distro bundle on Linux) -/// otherwise. Native stores often carry a few certificates rustls can't -/// parse; each root is screened on its own and an unusable one is skipped, -/// since reqwest would otherwise refuse to build the whole client. A store -/// that can't be read at all leaves only the bundled webpki roots. -fn load_configured_roots() -> Vec { - let loaded = rustls_native_certs::load_native_certs(); - if crate::utils::env_compat::is_debug_enabled() { - for err in &loaded.errors { - eprintln!("[socket-patch] could not read platform trust roots: {err}"); - } - } - loaded - .certs - .into_iter() - .filter(|der| { - let mut probe = rustls::RootCertStore::empty(); - probe.add(der.clone()).is_ok() - }) - .filter_map(|der| reqwest::Certificate::from_der(der.as_ref()).ok()) - .collect() +/// otherwise. A store that can't be read leaves the bundled roots only. +fn load_configured_roots() -> Vec> { + rustls_native_certs::load_native_certs().certs } /// Why [`read_capped_typed`] gave up — typed so a caller can tell a body cut @@ -283,9 +399,19 @@ mod tests { out } - fn client_with(roots: &[reqwest::Certificate]) -> reqwest::Client { + /// A client using the shared verifier design with `roots` standing in + /// for the platform store; `loads` counts platform-store reads. + fn client_with( + roots: Vec>, + loads: Arc, + ) -> reqwest::Client { + let config = tls_config(Box::new(move || { + loads.fetch_add(1, std::sync::atomic::Ordering::SeqCst); + roots.clone() + })); // `no_proxy`: a sandbox HTTPS_PROXY must not intercept loopback. - with_roots(reqwest::Client::builder(), roots) + reqwest::Client::builder() + .use_preconfigured_tls(config) .no_proxy() .build() .unwrap() @@ -298,9 +424,18 @@ mod tests { let ca = std::path::Path::new(TLS_FIXTURES).join("ca.pem"); let roots = with_cert_env(&ca, load_configured_roots); assert_eq!(roots.len(), 1, "SSL_CERT_FILE's one CA is loaded"); - let resp = client_with(&roots).get(&url).send().await.unwrap(); - assert_eq!(resp.status(), 200); - assert_eq!(resp.text().await.unwrap(), "ok"); + let loads = Arc::default(); + let client = client_with(roots, Arc::clone(&loads)); + for _ in 0..2 { + let resp = client.get(&url).send().await.unwrap(); + assert_eq!(resp.status(), 200); + assert_eq!(resp.text().await.unwrap(), "ok"); + } + assert_eq!( + loads.load(std::sync::atomic::Ordering::SeqCst), + 1, + "the platform store is read once, on the first unknown issuer" + ); } #[tokio::test] @@ -312,16 +447,21 @@ mod tests { std::fs::write(&empty, "").unwrap(); let roots = with_cert_env(&empty, load_configured_roots); assert!(roots.is_empty()); - let err = client_with(&roots).get(&url).send().await.unwrap_err(); + let err = client_with(roots, Arc::default()) + .get(&url) + .send() + .await + .unwrap_err(); assert!( format!("{err:?}").contains("UnknownIssuer"), "verification must stay on: {err:?}" ); } - #[test] + #[tokio::test] #[serial_test::serial] - fn unparsable_platform_roots_are_skipped_not_fatal() { + async fn unparsable_platform_roots_are_skipped_not_fatal() { + let url = spawn_tls_server().await; let tmp = tempfile::tempdir().unwrap(); let bundle = tmp.path().join("bundle.pem"); let ca = std::fs::read_to_string(format!("{TLS_FIXTURES}/ca.pem")).unwrap(); @@ -332,8 +472,21 @@ mod tests { ) .unwrap(); let roots = with_cert_env(&bundle, load_configured_roots); - assert_eq!(roots.len(), 1, "only the usable root survives"); - client_with(&roots); + let resp = client_with(roots, Arc::default()) + .get(&url) + .send() + .await + .unwrap(); + assert_eq!(resp.status(), 200, "the usable root still verifies"); + } + + /// The production builder accepts the shared rustls config (reqwest + /// refuses a config from a different rustls build at `build()`), and + /// building clients never reads the platform store. + #[test] + fn production_builder_builds() { + client_builder().build().unwrap(); + client_builder().build().unwrap(); } /// Ratchet: production code builds every `reqwest::Client` through