From 8223abd473f826e47b500fd902d761d5423520ef Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 17:04:18 -0700 Subject: [PATCH 1/4] [LXC] Pin the proxy hostname instead of rewriting the URL host Model 2 needs the sandbox and the firewall to agree on exactly one proxy endpoint. Otherwise the sandbox re-resolves the hostname itself and, under round-robin or split-horizon DNS, reaches an address the firewall never authorized. PR 632 solved this by rewriting the proxy URL's host to the resolved IP. Review rejected that (comment 3724788051): an `https://`-scheme proxy would then be contacted at an IP literal, so SNI and certificate validation fail unless the proxy certificate carries an IP SAN. Add `ProxyHostPin` and `ProxyAddress::host_pin` instead. These express the mapping as a hosts-file pin, so the hostname stays in the URL and TLS identity is preserved while the endpoint is still forced. `host_pin` returns `None` when the address is already an IP literal, because there is then nothing to resolve. `hosts_line` writes the address bare: a hosts file takes an unbracketed IPv6 literal, unlike a URL host component. Also fix `to_url`. It hardcoded `127.0.0.1` whenever no original URL was recorded, regardless of the actual address. That is reachable: `unix_proxy_coordinator.rs:234` builds a `ProxyAddress` from the configured bind address with no original URL, so a proxy bound to a non-loopback address reported an endpoint it was not listening on -- the same class of defect as the objection above. Every existing caller passes `127.0.0.1`, so their output is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- src/core/wxc_common/src/models.rs | 87 ++++++++++++++++++++++++++++++- 1 file changed, 85 insertions(+), 2 deletions(-) diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index 0a703c89d..0b23c2102 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -378,6 +378,39 @@ impl From for NetworkEnforcementMode { } } +/// A hostname-to-IP mapping that makes a sandbox resolve the proxy to exactly +/// the address the firewall authorized. +/// +/// This exists instead of rewriting the proxy URL's host to the resolved IP. +/// Rewriting the host breaks TLS for an `https://`-scheme proxy: the client +/// then contacts an IP literal, so SNI and certificate validation fail unless +/// the proxy certificate carries an IP SAN. Pinning the name resolution +/// instead keeps the hostname in the URL, so TLS identity is preserved, while +/// still guaranteeing the sandbox and the firewall agree on one endpoint. +/// +/// Without a pin the sandbox re-resolves the hostname itself, and under +/// round-robin or split-horizon DNS it can select an address the firewall +/// never allowed. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct ProxyHostPin { + /// The proxy hostname as it appears in the URL handed to the sandbox. + pub hostname: String, + /// The address the host resolved that hostname to, and the only address + /// the firewall authorizes for it. + pub ip: String, +} + +impl ProxyHostPin { + /// Render this pin as a `/etc/hosts` line. + /// + /// The address is written bare. A hosts file takes an unbracketed IPv6 + /// literal, unlike a URL host component, so this must not reuse the + /// bracketing applied by [`ProxyAddress::to_url`]. + pub fn hosts_line(&self) -> String { + format!("{} {}", self.ip, self.hostname) + } +} + #[derive(Debug, Clone)] pub struct ProxyAddress { pub address: String, @@ -413,12 +446,62 @@ impl ProxyAddress { } /// Returns the proxy URL. Uses the original URL if one was provided, - /// otherwise constructs `http://127.0.0.1:{port}` for localhost proxies. + /// otherwise constructs one from this address and port. + /// + /// The constructed form names [`Self::address`] rather than assuming + /// loopback. A proxy bound to a non-loopback address is reachable through + /// [`ProxyAddress::new`], and reporting `127.0.0.1` for it would hand the + /// sandbox a different endpoint from the one the firewall authorized. pub fn to_url(&self) -> String { if let Some(url) = &self.original_url { return url.clone(); } - format!("http://127.0.0.1:{}", self.port) + format!( + "http://{}:{}", + Self::bracket_if_ipv6(&self.address), + self.port + ) + } + + /// Returns the pin required for a sandbox to resolve this proxy's hostname + /// to `ip`, or `None` when no pin is needed or possible. + /// + /// `None` is returned when the address is already an IP literal, since + /// there is nothing to resolve, and when it is empty. Callers treat `None` + /// as "no hosts entry required", not as an error. + /// + /// The URL is deliberately left untouched. See [`ProxyHostPin`] for why + /// rewriting the host to `ip` instead would break TLS. + pub fn host_pin(&self, ip: &str) -> Option { + let hostname = Self::unbracket(&self.address); + if hostname.is_empty() || hostname.parse::().is_ok() { + return None; + } + + Some(ProxyHostPin { + hostname: hostname.to_string(), + ip: Self::unbracket(ip).to_string(), + }) + } + + /// Wraps `host` in `[` `]` when it is a bare IPv6 literal, so it is valid + /// as a URL host component. Any other host, including one that is already + /// bracketed, is returned unchanged. + fn bracket_if_ipv6(host: &str) -> std::borrow::Cow<'_, str> { + if host.starts_with('[') { + return std::borrow::Cow::Borrowed(host); + } + match host.parse::() { + Ok(std::net::IpAddr::V6(_)) => std::borrow::Cow::Owned(format!("[{host}]")), + _ => std::borrow::Cow::Borrowed(host), + } + } + + /// Strips one pair of surrounding `[` `]` from a bracketed IPv6 literal. + fn unbracket(host: &str) -> &str { + host.strip_prefix('[') + .and_then(|rest| rest.strip_suffix(']')) + .unwrap_or(host) } } From 0050a1b4ed6a43bcfd5e8e25f3673f0e95328aa8 Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 17:20:15 -0700 Subject: [PATCH 2/4] Drop a dead bracket guard and document why unbracketing is load-bearing Mutation testing surfaced an equivalent mutant: deleting the `starts_with('[')` early return from `bracket_if_ipv6` changed no observable behavior. Verified why, rather than assuming the tests were weak -- `IpAddr::from_str` rejects brackets, so `[::1]` already fell through the catch-all arm unchanged and could never be bracketed twice. The guard was dead code. Remove it and record the reason. The mirror case is NOT dead, and mutation proves it: replacing `Self::unbracket(&self.address)` in `host_pin` with the raw field fails a test. Unbracketing there is what lets a bracketed IPv6 literal be classified as a literal instead of pinned as though it were a hostname. Say so in the doc comment, which previously described it as mere normalization. Comment and dead-code only. All 566 library tests and 19 spec tests pass unchanged, and the seeded-mutant suite now runs 9 for 9 with no survivors. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- src/core/wxc_common/src/models.rs | 22 +- .../wxc_common/tests/proxy_address_spec.rs | 303 ++++++++++++++++++ 2 files changed, 318 insertions(+), 7 deletions(-) create mode 100644 src/core/wxc_common/tests/proxy_address_spec.rs diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index 0b23c2102..df1f3307c 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -466,10 +466,18 @@ impl ProxyAddress { /// Returns the pin required for a sandbox to resolve this proxy's hostname /// to `ip`, or `None` when no pin is needed or possible. /// - /// `None` is returned when the address is already an IP literal, since - /// there is nothing to resolve, and when it is empty. Callers treat `None` + /// `None` is returned when the address is empty, and when it is an IP + /// literal, since there is then nothing to resolve. Callers treat `None` /// as "no hosts entry required", not as an error. /// + /// The address is unbracketed before that classification so a bracketed + /// IPv6 literal is recognized as a literal rather than mistaken for a + /// hostname: `IpAddr::from_str` rejects brackets, so `[::1]` would + /// otherwise be pinned as though it were a name. + /// + /// `ip` is recorded bare for the same reason [`ProxyHostPin::hosts_line`] + /// writes it bare -- a hosts file takes an unbracketed IPv6 literal. + /// /// The URL is deliberately left untouched. See [`ProxyHostPin`] for why /// rewriting the host to `ip` instead would break TLS. pub fn host_pin(&self, ip: &str) -> Option { @@ -485,12 +493,12 @@ impl ProxyAddress { } /// Wraps `host` in `[` `]` when it is a bare IPv6 literal, so it is valid - /// as a URL host component. Any other host, including one that is already - /// bracketed, is returned unchanged. + /// as a URL host component. + /// + /// An already-bracketed literal needs no special case. `IpAddr::from_str` + /// does not accept brackets, so a bracketed host fails to parse and falls + /// through unchanged rather than being bracketed twice. fn bracket_if_ipv6(host: &str) -> std::borrow::Cow<'_, str> { - if host.starts_with('[') { - return std::borrow::Cow::Borrowed(host); - } match host.parse::() { Ok(std::net::IpAddr::V6(_)) => std::borrow::Cow::Owned(format!("[{host}]")), _ => std::borrow::Cow::Borrowed(host), diff --git a/src/core/wxc_common/tests/proxy_address_spec.rs b/src/core/wxc_common/tests/proxy_address_spec.rs new file mode 100644 index 000000000..4b6a64bca --- /dev/null +++ b/src/core/wxc_common/tests/proxy_address_spec.rs @@ -0,0 +1,303 @@ +// Copyright (c) Microsoft Corporation. +// Licensed under the MIT License. + +//! Black-box contract tests for `wxc_common::models::ProxyAddress` and +//! `ProxyHostPin`. +//! +//! These live in the integration-test directory on purpose: from here only the +//! crate's public API is visible, so the tests exercise the same surface the +//! real callers do and cannot accidentally couple to a private helper. They +//! were written from the documented contract without reading the +//! implementation, so a bug baked into the code cannot silently teach the tests +//! to expect it. +//! +//! Why this surface matters. The "deny-all-except-proxy" network policy +//! requires the sandbox and the firewall to agree on exactly one proxy +//! endpoint. If the URL handed to the sandbox names a different endpoint than +//! the firewall authorized -- or if the sandbox is left free to re-resolve a +//! hostname under round-robin or split-horizon DNS -- that is a policy bypass. +//! `to_url` decides the endpoint string the sandbox receives, and `host_pin` / +//! `hosts_line` express the resolved mapping as a hosts-file pin so the +//! hostname stays in the URL and TLS identity is preserved. +//! +//! Client status, verified against the tree on the day these tests were +//! written: +//! +//! * The URL surface (`new`, `from_url`, `to_url`, and the `original_url` +//! field) has live callers today. `appcontainer_runner::inject_proxy_vars` +//! turns `to_url()` into the `HTTP_PROXY` / `HTTPS_PROXY` values injected into +//! the sandboxed process, `proxy_coordinator` uses it to launch the elevated +//! shim, `unix_proxy_coordinator` logs it, `config_parser` produces addresses +//! via `from_url`, and `wsl_container_runner` reads the `original_url` field +//! directly. +//! * The pin surface (`host_pin`, `hosts_line`, `ProxyHostPin`) has no callers +//! yet. It is planned wiring for the firewall / hosts-file consumer, so the +//! tests below name that consumer as planned, not present. +//! +//! Test list (the scenarios these tests are meant to cover, enumerated before +//! the assertions were written): +//! 1. `to_url` with no original URL constructs `http://{address}:{port}` from +//! the struct's own address -- for loopback, for a non-loopback address +//! that must not be rewritten to loopback, and for bare and +//! already-bracketed IPv6 literals. +//! 2. `to_url` with an original URL returns it verbatim -- including a +//! trailing slash, credentials, a path and query, and an `https` scheme. +//! 3. The two constructors differ only in whether they record `original_url`. +//! 4. `host_pin` returns a pin for a hostname and `None` for every IP literal +//! and for the empty address, stripping brackets from the supplied IP. +//! 5. `host_pin` does not disturb `to_url`. +//! 6. `hosts_line` writes `{ip} {hostname}` with the address bare, which is +//! the deliberate asymmetry against `to_url`'s bracketing of IPv6. + +use wxc_common::models::{ProxyAddress, ProxyHostPin}; + +// Protects `appcontainer_runner::inject_proxy_vars` and the proxy coordinators, +// which build the sandbox's proxy URL from an address created with `new`. A +// loopback bind address is the common case for the builtin test proxy. +#[test] +fn to_url_constructs_http_url_for_loopback_when_no_original_url() { + let addr = ProxyAddress::new("127.0.0.1".to_string(), 8080); + + assert_eq!(addr.to_url(), "http://127.0.0.1:8080"); +} + +// Protects `appcontainer_runner::inject_proxy_vars`. A proxy bound to a +// non-loopback address is constructible via `new`, and the sandbox must be told +// that exact endpoint. Reporting `127.0.0.1` here would hand the sandbox a +// different endpoint than the firewall authorized -- a policy bypass. +#[test] +fn to_url_preserves_non_loopback_address_and_does_not_assume_loopback() { + let addr = ProxyAddress::new("10.1.2.3".to_string(), 3128); + + assert_eq!(addr.to_url(), "http://10.1.2.3:3128"); +} + +// Protects every client that turns a `new`-built address into a URL when the +// proxy is bound to an IPv6 address. An unbracketed IPv6 literal is not a valid +// URL host component, so the constructed URL must bracket it. +#[test] +fn to_url_brackets_bare_ipv6_literal() { + let addr = ProxyAddress::new("2001:db8::1".to_string(), 8080); + + assert_eq!(addr.to_url(), "http://[2001:db8::1]:8080"); +} + +// Protects the same URL-building clients against a double-bracketing bug when +// the address is already in bracketed form. +#[test] +fn to_url_does_not_double_bracket_already_bracketed_ipv6() { + let addr = ProxyAddress::new("[2001:db8::1]".to_string(), 8080); + + assert_eq!(addr.to_url(), "http://[2001:db8::1]:8080"); +} + +// Protects `wsl_container_runner` and the env-var injection path, which forward +// the operator-supplied proxy URL unchanged. When an original URL was recorded +// via `from_url`, `to_url` must return it byte for byte. +#[test] +fn to_url_returns_original_url_verbatim() { + let addr = ProxyAddress::from_url( + "http://proxy.example.com:8080", + "proxy.example.com".to_string(), + 8080, + ); + + assert_eq!(addr.to_url(), "http://proxy.example.com:8080"); +} + +// Protects `wsl_container_runner` against the trailing-slash mangling an earlier +// implementation exhibited. The original URL must pass through exactly, slash +// and all. +#[test] +fn to_url_preserves_trailing_slash_in_original_url() { + let addr = ProxyAddress::from_url( + "http://proxy.example.com:8080/", + "proxy.example.com".to_string(), + 8080, + ); + + assert_eq!(addr.to_url(), "http://proxy.example.com:8080/"); +} + +// Protects `wsl_container_runner` and the env-var injection path for a +// fully-specified URL. Credentials, path, and query must all survive verbatim; +// dropping the credentials would silently change how the proxy authenticates. +#[test] +fn to_url_preserves_credentials_path_and_query_in_original_url() { + let addr = ProxyAddress::from_url( + "http://user:pass@proxy.example.com:8080/path?token=abc", + "proxy.example.com".to_string(), + 8080, + ); + + assert_eq!( + addr.to_url(), + "http://user:pass@proxy.example.com:8080/path?token=abc" + ); +} + +// Protects the whole reason `ProxyHostPin` exists instead of rewriting the host +// to an IP: an `https` proxy must keep its hostname and scheme so the client's +// SNI and certificate validation still work. The original URL passes through +// unchanged, including the `https` scheme. +#[test] +fn to_url_preserves_https_scheme_original_url_verbatim() { + let addr = ProxyAddress::from_url( + "https://proxy.example.com:8443", + "proxy.example.com".to_string(), + 8443, + ); + + assert_eq!(addr.to_url(), "https://proxy.example.com:8443"); +} + +// Protects `wsl_container_runner`, which reads the `original_url` field +// directly. `from_url` must record the original string and `new` must leave it +// empty; that single difference is what selects passthrough versus construction +// in `to_url`. +#[test] +fn from_url_records_original_url_and_new_does_not() { + let from_url = ProxyAddress::from_url( + "http://proxy.example.com:8080", + "proxy.example.com".to_string(), + 8080, + ); + let constructed = ProxyAddress::new("127.0.0.1".to_string(), 8080); + + assert_eq!( + from_url.original_url, + Some("http://proxy.example.com:8080".to_string()) + ); + assert_eq!(constructed.original_url, None); +} + +// Protects the planned firewall / hosts-file consumer. A hostname address +// requires resolution, so `host_pin` must return a mapping carrying the +// hostname and the supplied IP unchanged when neither is bracketed. +#[test] +fn host_pin_returns_pin_for_hostname() { + let addr = ProxyAddress::new("proxy.example.com".to_string(), 8080); + + assert_eq!( + addr.host_pin("10.0.0.5"), + Some(ProxyHostPin { + hostname: "proxy.example.com".to_string(), + ip: "10.0.0.5".to_string(), + }) + ); +} + +// Protects the planned firewall / hosts-file consumer. When the resolved IP is +// supplied in bracketed IPv6 form, the pin must strip the brackets, because a +// hosts file takes a bare address. +#[test] +fn host_pin_strips_brackets_from_ipv6_ip_argument() { + let addr = ProxyAddress::new("proxy.example.com".to_string(), 8080); + + assert_eq!( + addr.host_pin("[2001:db8::1]"), + Some(ProxyHostPin { + hostname: "proxy.example.com".to_string(), + ip: "2001:db8::1".to_string(), + }) + ); +} + +// Protects the planned firewall / hosts-file consumer. An IPv4 literal is +// already an endpoint, so there is nothing to resolve and no hosts entry is +// required; `None` means "no entry", not an error. +#[test] +fn host_pin_returns_none_for_ipv4_literal() { + let addr = ProxyAddress::new("127.0.0.1".to_string(), 8080); + + assert_eq!(addr.host_pin("10.0.0.5"), None); +} + +// Protects the planned firewall / hosts-file consumer. A bare IPv6 literal is +// likewise already an endpoint and needs no hosts entry. +#[test] +fn host_pin_returns_none_for_bare_ipv6_literal() { + let addr = ProxyAddress::new("2001:db8::1".to_string(), 8080); + + assert_eq!(addr.host_pin("10.0.0.5"), None); +} + +// Protects the planned firewall / hosts-file consumer. A bracketed IPv6 literal +// is still an IP literal and must be treated the same as the bare form. +#[test] +fn host_pin_returns_none_for_bracketed_ipv6_literal() { + let addr = ProxyAddress::new("[2001:db8::1]".to_string(), 8080); + + assert_eq!(addr.host_pin("10.0.0.5"), None); +} + +// Protects the planned firewall / hosts-file consumer. An empty address names +// nothing to resolve, so no hosts entry is required. +#[test] +fn host_pin_returns_none_for_empty_address() { + let addr = ProxyAddress::new(String::new(), 8080); + + assert_eq!(addr.host_pin("10.0.0.5"), None); +} + +// Protects both the URL clients and the planned pin consumer. Computing a pin +// is documented not to alter the URL, so `to_url` must return the same verbatim +// original after `host_pin` as before it. +#[test] +fn host_pin_does_not_change_to_url() { + let addr = ProxyAddress::from_url( + "https://proxy.example.com:8443", + "proxy.example.com".to_string(), + 8443, + ); + + let before = addr.to_url(); + let pin = addr.host_pin("10.0.0.5"); + + assert!(pin.is_some(), "a hostname address should yield a pin"); + assert_eq!(addr.to_url(), before); + assert_eq!(addr.to_url(), "https://proxy.example.com:8443"); +} + +// Protects the planned firewall / hosts-file consumer. A hosts line is +// "{ip} {hostname}" -- address first, then hostname, separated by a single +// space. +#[test] +fn hosts_line_writes_ip_then_hostname() { + let pin = ProxyHostPin { + hostname: "proxy.example.com".to_string(), + ip: "10.0.0.5".to_string(), + }; + + assert_eq!(pin.hosts_line(), "10.0.0.5 proxy.example.com"); +} + +// Protects the planned firewall / hosts-file consumer. A hosts file takes an +// unbracketed IPv6 literal, so `hosts_line` must write the address bare -- the +// deliberate opposite of how `to_url` renders IPv6. +#[test] +fn hosts_line_writes_ipv6_address_without_brackets() { + let pin = ProxyHostPin { + hostname: "proxy.example.com".to_string(), + ip: "2001:db8::1".to_string(), + }; + + assert_eq!(pin.hosts_line(), "2001:db8::1 proxy.example.com"); +} + +// Protects both surfaces at once by pinning the asymmetry the contract calls +// out explicitly: for the very same IPv6 literal, the URL host component is +// bracketed while the hosts-file line is bare. A well-meaning refactor that +// unified the two would break exactly one of them, and this test names which. +#[test] +fn ipv6_is_bracketed_in_url_but_bare_in_hosts_line() { + let url = ProxyAddress::new("2001:db8::1".to_string(), 8080).to_url(); + let line = ProxyHostPin { + hostname: "proxy.example.com".to_string(), + ip: "2001:db8::1".to_string(), + } + .hosts_line(); + + assert_eq!(url, "http://[2001:db8::1]:8080"); + assert_eq!(line, "2001:db8::1 proxy.example.com"); +} From c7cca5c9b1397dc19a41202d98daf4d19896f58b Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 17:31:20 -0700 Subject: [PATCH 3/4] [LXC] Make an unpinnable proxy address unrepresentable Review on PR 789 pointed out that ProxyHostPin's fields were public Strings, so a caller could set ip to "[::1]", to the empty string, or to text containing a newline, and hosts_line() would emit it verbatim. That is an injection into /etc/hosts: a newline ends the record and starts a second, unauthorized mapping. The type exists to guarantee the sandbox and the firewall agree on one endpoint, so a value that denotes two mappings defeats its whole purpose. The fields are now private and the address is an IpAddr, so no such value can be constructed. IpAddr also renders IPv6 bare, which is what a hosts file requires -- the difference from to_url, which brackets, is now structural instead of a convention a caller has to remember. host_pin returns Result, WxcError>. Ok(None) keeps its single meaning: the address is an IP literal, so there is nothing to resolve. An empty or malformed hostname is now Err, not None. Folding it into None would have told the caller "no hosts entry required", so a malformed address would silently skip the pin and let the sandbox re-resolve the name -- failing open, which is the defect review objected to elsewhere in this work. Tests are updated in a separate commit by the author who did not write this implementation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- src/core/wxc_common/src/models.rs | 100 ++++++++++++++++++++++-------- 1 file changed, 75 insertions(+), 25 deletions(-) diff --git a/src/core/wxc_common/src/models.rs b/src/core/wxc_common/src/models.rs index df1f3307c..a893cb62b 100644 --- a/src/core/wxc_common/src/models.rs +++ b/src/core/wxc_common/src/models.rs @@ -2,6 +2,9 @@ // Licensed under the MIT License. use serde::{Deserialize, Serialize}; +use std::net::IpAddr; + +use crate::error::WxcError; /// Selects which containment backend to use for script execution. #[derive(Debug, Default, Clone, Serialize, Deserialize, PartialEq, Eq)] @@ -391,21 +394,38 @@ impl From for NetworkEnforcementMode { /// Without a pin the sandbox re-resolves the hostname itself, and under /// round-robin or split-horizon DNS it can select an address the firewall /// never allowed. +/// +/// The fields are private and the address is an [`IpAddr`], so a pin that does +/// not denote exactly one mapping cannot be constructed. This matters because +/// [`Self::hosts_line`] is written to a hosts file: a newline or space in +/// either field would inject additional entries, letting an attacker redirect +/// names the policy never mentioned. Construct one with +/// [`ProxyAddress::host_pin`]. #[derive(Debug, Clone, PartialEq, Eq)] pub struct ProxyHostPin { - /// The proxy hostname as it appears in the URL handed to the sandbox. - pub hostname: String, - /// The address the host resolved that hostname to, and the only address - /// the firewall authorizes for it. - pub ip: String, + hostname: String, + ip: IpAddr, } impl ProxyHostPin { + /// The proxy hostname as it appears in the URL handed to the sandbox. + pub fn hostname(&self) -> &str { + &self.hostname + } + + /// The address the hostname is pinned to, and the only address the + /// firewall authorizes for it. + pub fn ip(&self) -> IpAddr { + self.ip + } + /// Render this pin as a `/etc/hosts` line. /// /// The address is written bare. A hosts file takes an unbracketed IPv6 - /// literal, unlike a URL host component, so this must not reuse the - /// bracketing applied by [`ProxyAddress::to_url`]. + /// literal, unlike a URL host component, and [`IpAddr`]'s `Display` is + /// already that bare form -- so the difference from + /// [`ProxyAddress::to_url`], which brackets, is structural rather than a + /// convention a caller could forget. pub fn hosts_line(&self) -> String { format!("{} {}", self.ip, self.hostname) } @@ -464,32 +484,62 @@ impl ProxyAddress { } /// Returns the pin required for a sandbox to resolve this proxy's hostname - /// to `ip`, or `None` when no pin is needed or possible. + /// to `ip`. /// - /// `None` is returned when the address is empty, and when it is an IP - /// literal, since there is then nothing to resolve. Callers treat `None` - /// as "no hosts entry required", not as an error. + /// `Ok(None)` means no pin is needed: the address is already an IP + /// literal, so there is nothing to resolve and nothing a sandbox could + /// resolve differently. /// - /// The address is unbracketed before that classification so a bracketed - /// IPv6 literal is recognized as a literal rather than mistaken for a - /// hostname: `IpAddr::from_str` rejects brackets, so `[::1]` would - /// otherwise be pinned as though it were a name. + /// `Err` means a pin is needed but cannot be produced, because the address + /// is empty or contains characters that are not valid in a hostname. This + /// is deliberately an error rather than `None`. Returning `None` would tell + /// the caller "no hosts entry required", so a malformed address would + /// silently skip the pin and let the sandbox re-resolve the name freely -- + /// failing open, which is the defect review objected to elsewhere in this + /// work. A proxy whose endpoint cannot be pinned must not run. /// - /// `ip` is recorded bare for the same reason [`ProxyHostPin::hosts_line`] - /// writes it bare -- a hosts file takes an unbracketed IPv6 literal. + /// Taking an [`IpAddr`] rather than a string means the caller has already + /// resolved the name, and makes an unparseable address unrepresentable + /// here. /// /// The URL is deliberately left untouched. See [`ProxyHostPin`] for why /// rewriting the host to `ip` instead would break TLS. - pub fn host_pin(&self, ip: &str) -> Option { + pub fn host_pin(&self, ip: IpAddr) -> Result, WxcError> { let hostname = Self::unbracket(&self.address); - if hostname.is_empty() || hostname.parse::().is_ok() { - return None; + + // An IP literal needs no pin. Unbracket first so a bracketed IPv6 + // literal is recognized as a literal rather than mistaken for a + // hostname: `IpAddr::from_str` rejects brackets, so `[::1]` would + // otherwise be pinned as though it were a name. + if hostname.parse::().is_ok() { + return Ok(None); + } + + if !Self::is_pinnable_hostname(hostname) { + return Err(WxcError::NetworkProxy(format!( + "proxy address {:?} cannot be pinned to {}: not a valid hostname", + self.address, ip + ))); } - Some(ProxyHostPin { + Ok(Some(ProxyHostPin { hostname: hostname.to_string(), - ip: Self::unbracket(ip).to_string(), - }) + ip, + })) + } + + /// Whether `host` is safe to write as the name column of a hosts file + /// entry. + /// + /// Rejects the empty string and anything outside the letter, digit, `-`, + /// and `.` set. That set excludes whitespace and newlines, which is the + /// property that matters: either would end the record and inject a second, + /// unauthorized mapping. + fn is_pinnable_hostname(host: &str) -> bool { + !host.is_empty() + && host + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-' || c == '.') } /// Wraps `host` in `[` `]` when it is a bare IPv6 literal, so it is valid @@ -499,8 +549,8 @@ impl ProxyAddress { /// does not accept brackets, so a bracketed host fails to parse and falls /// through unchanged rather than being bracketed twice. fn bracket_if_ipv6(host: &str) -> std::borrow::Cow<'_, str> { - match host.parse::() { - Ok(std::net::IpAddr::V6(_)) => std::borrow::Cow::Owned(format!("[{host}]")), + match host.parse::() { + Ok(IpAddr::V6(_)) => std::borrow::Cow::Owned(format!("[{host}]")), _ => std::borrow::Cow::Borrowed(host), } } From 1ba5a1cbf450fbaa4f6600a331782025d52c79bb Mon Sep 17 00:00:00 2001 From: Darren Hoehna Date: Sat, 8 Aug 2026 17:47:17 -0700 Subject: [PATCH 4/4] [LXC] Update proxy address spec for the unpinnable-address fix 22 black-box tests against the new host_pin contract, written by an author who has not read models.rs. The empty address moved from Ok(None) to Err, so the test that covered it was rewritten to match on all three arms by name. Asserting is_err() || is_none() would have passed either way, and the whole point of the change is that those two answers are not interchangeable: Ok(None) tells the caller no hosts entry is needed, which is how a malformed address ends up unpinned and the firewall bypassed. Added coverage for the injection strings review called out -- a hostname carrying a newline or a space must be Err and must never reach hosts_line. Dropped the test that stripped brackets from the ip argument; ip is an IpAddr now, so there is no textual form to strip and the behavior no longer exists. Mutation harness: 11 mutants, 11 caught by a failing test, 0 survivors. Four of them removed the last call to a private helper, which the crate's deny-warnings turns into a build failure -- real detection, but by the compiler, which proves nothing about the tests. The harness now suppresses those lints for the mutated build so the suite has to answer for itself. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d --- .../wxc_common/tests/proxy_address_spec.rs | 234 +++++++++++++----- 1 file changed, 170 insertions(+), 64 deletions(-) diff --git a/src/core/wxc_common/tests/proxy_address_spec.rs b/src/core/wxc_common/tests/proxy_address_spec.rs index 4b6a64bca..b5e40fe41 100644 --- a/src/core/wxc_common/tests/proxy_address_spec.rs +++ b/src/core/wxc_common/tests/proxy_address_spec.rs @@ -30,9 +30,24 @@ //! shim, `unix_proxy_coordinator` logs it, `config_parser` produces addresses //! via `from_url`, and `wsl_container_runner` reads the `original_url` field //! directly. -//! * The pin surface (`host_pin`, `hosts_line`, `ProxyHostPin`) has no callers -//! yet. It is planned wiring for the firewall / hosts-file consumer, so the -//! tests below name that consumer as planned, not present. +//! * The pin surface (`host_pin`, `hosts_line`, `ProxyHostPin`) still has no +//! callers. It is planned wiring for the firewall / hosts-file consumer, so +//! the tests below name that consumer as planned, not present. `ProxyHostPin` +//! has no public constructor -- the only way to obtain one is `host_pin` on a +//! hostname -- so the tests build pins that way through the `pin_for` helper. +//! +//! `host_pin` returns `Result, WxcError>`, and the three +//! arms are the whole point of the type after PR 789: +//! +//! * `Ok(None)` -- and only this -- means the address is an IP literal (bare or +//! bracketed), so there is nothing to resolve and no pin is needed. +//! * `Ok(Some(pin))` means the address is a hostname and the pin is required. +//! * `Err(_)` means a pin is required but impossible: the address is empty or +//! holds characters invalid in a hostname (notably whitespace or a newline, +//! the hosts-file injection vectors). Conflating this with `Ok(None)` would +//! fail open -- the caller would skip a required pin and let the sandbox +//! re-resolve the name freely -- so the tests assert the specific arm, not +//! merely `is_err` or `is_none`. //! //! Test list (the scenarios these tests are meant to cover, enumerated before //! the assertions were written): @@ -43,14 +58,31 @@ //! 2. `to_url` with an original URL returns it verbatim -- including a //! trailing slash, credentials, a path and query, and an `https` scheme. //! 3. The two constructors differ only in whether they record `original_url`. -//! 4. `host_pin` returns a pin for a hostname and `None` for every IP literal -//! and for the empty address, stripping brackets from the supplied IP. +//! 4. `host_pin` returns `Ok(Some)` for a hostname (with a hyphen accepted and +//! the typed IP read back through `ip()`), `Ok(None)` for every IP literal, +//! and `Err` for the empty address and for hostnames carrying a newline or +//! a space. //! 5. `host_pin` does not disturb `to_url`. //! 6. `hosts_line` writes `{ip} {hostname}` with the address bare, which is //! the deliberate asymmetry against `to_url`'s bracketing of IPv6. +use std::net::IpAddr; + use wxc_common::models::{ProxyAddress, ProxyHostPin}; +// `ProxyHostPin` has no public constructor: the only way to obtain one is +// `ProxyAddress::host_pin` on a hostname address, which must return +// `Ok(Some(pin))`. This helper centralizes that construction and fails the +// test with a precise message if either non-`Ok(Some)` arm comes back, so the +// pin-shape tests can read like ordinary value assertions. +fn pin_for(address: &str, ip: IpAddr) -> ProxyHostPin { + match ProxyAddress::new(address.to_string(), 8080).host_pin(ip) { + Ok(Some(pin)) => pin, + Ok(None) => panic!("expected a pin for hostname {address:?}, got Ok(None)"), + Err(_) => panic!("expected a pin for hostname {address:?}, got Err"), + } +} + // Protects `appcontainer_runner::inject_proxy_vars` and the proxy coordinators, // which build the sandbox's proxy URL from an address created with `new`. A // loopback bind address is the common case for the builtin test proxy. @@ -172,72 +204,141 @@ fn from_url_records_original_url_and_new_does_not() { } // Protects the planned firewall / hosts-file consumer. A hostname address -// requires resolution, so `host_pin` must return a mapping carrying the -// hostname and the supplied IP unchanged when neither is bracketed. +// requires resolution, so `host_pin` must return `Ok(Some(pin))` carrying the +// hostname and the typed IP it was handed. #[test] fn host_pin_returns_pin_for_hostname() { - let addr = ProxyAddress::new("proxy.example.com".to_string(), 8080); + let ip: IpAddr = "10.0.0.5".parse().unwrap(); - assert_eq!( - addr.host_pin("10.0.0.5"), - Some(ProxyHostPin { - hostname: "proxy.example.com".to_string(), - ip: "10.0.0.5".to_string(), - }) - ); + let pin = pin_for("proxy.example.com", ip); + + assert_eq!(pin.hostname(), "proxy.example.com"); + assert_eq!(pin.ip(), ip); } -// Protects the planned firewall / hosts-file consumer. When the resolved IP is -// supplied in bracketed IPv6 form, the pin must strip the brackets, because a -// hosts file takes a bare address. +// Protects the planned firewall / hosts-file consumer against an over-strict +// validator. Hyphens and dots are legal in a hostname, so a label containing a +// hyphen must still pin rather than being rejected as invalid. #[test] -fn host_pin_strips_brackets_from_ipv6_ip_argument() { - let addr = ProxyAddress::new("proxy.example.com".to_string(), 8080); +fn host_pin_accepts_hostname_with_hyphen() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); - assert_eq!( - addr.host_pin("[2001:db8::1]"), - Some(ProxyHostPin { - hostname: "proxy.example.com".to_string(), - ip: "2001:db8::1".to_string(), - }) - ); + let pin = pin_for("my-proxy.example.com", ip); + + assert_eq!(pin.hostname(), "my-proxy.example.com"); } -// Protects the planned firewall / hosts-file consumer. An IPv4 literal is -// already an endpoint, so there is nothing to resolve and no hosts entry is -// required; `None` means "no entry", not an error. +// Protects the planned firewall / hosts-file consumer. `ip()` now returns a +// typed `IpAddr`, so a pin built for a hostname with a resolved IPv6 address +// must return that exact address through the accessor -- not a string, and not a +// lossy reformatting. #[test] -fn host_pin_returns_none_for_ipv4_literal() { - let addr = ProxyAddress::new("127.0.0.1".to_string(), 8080); +fn host_pin_ip_accessor_returns_typed_ipv6_address() { + let ip: IpAddr = "2001:db8::1".parse().unwrap(); - assert_eq!(addr.host_pin("10.0.0.5"), None); + let pin = pin_for("proxy.example.com", ip); + + assert_eq!(pin.ip(), ip); +} + +// Protects the planned firewall / hosts-file consumer. An IPv4 literal address +// is already an endpoint, so there is nothing to resolve: the one and only +// `Ok(None)` case ("no pin needed"), which must not be confused with `Err` +// ("pin needed but impossible"). +#[test] +fn host_pin_returns_ok_none_for_ipv4_literal() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + + match ProxyAddress::new("127.0.0.1".to_string(), 8080).host_pin(ip) { + Ok(None) => {} + Ok(Some(_)) => panic!("an IPv4 literal needs no pin; expected Ok(None), got Ok(Some)"), + Err(_) => panic!("an IPv4 literal needs no pin; expected Ok(None), got Err"), + } } // Protects the planned firewall / hosts-file consumer. A bare IPv6 literal is // likewise already an endpoint and needs no hosts entry. #[test] -fn host_pin_returns_none_for_bare_ipv6_literal() { - let addr = ProxyAddress::new("2001:db8::1".to_string(), 8080); +fn host_pin_returns_ok_none_for_bare_ipv6_literal() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); - assert_eq!(addr.host_pin("10.0.0.5"), None); + match ProxyAddress::new("2001:db8::1".to_string(), 8080).host_pin(ip) { + Ok(None) => {} + Ok(Some(_)) => panic!("a bare IPv6 literal needs no pin; expected Ok(None), got Ok(Some)"), + Err(_) => panic!("a bare IPv6 literal needs no pin; expected Ok(None), got Err"), + } } // Protects the planned firewall / hosts-file consumer. A bracketed IPv6 literal -// is still an IP literal and must be treated the same as the bare form. +// is unbracketed before classification, so `[::1]` is still an IP literal and +// must be `Ok(None)`, never treated as a hostname to pin. #[test] -fn host_pin_returns_none_for_bracketed_ipv6_literal() { - let addr = ProxyAddress::new("[2001:db8::1]".to_string(), 8080); +fn host_pin_returns_ok_none_for_bracketed_ipv6_literal() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + + match ProxyAddress::new("[::1]".to_string(), 8080).host_pin(ip) { + Ok(None) => {} + Ok(Some(_)) => { + panic!("a bracketed IPv6 literal needs no pin; expected Ok(None), got Ok(Some)") + } + Err(_) => panic!("a bracketed IPv6 literal needs no pin; expected Ok(None), got Err"), + } +} - assert_eq!(addr.host_pin("10.0.0.5"), None); +// Protects the planned firewall / hosts-file consumer, and pins the security fix +// from PR 789. An empty address is a pin that is REQUIRED but impossible, so it +// must be `Err`, never `Ok(None)`. If these two arms were swapped the caller +// would read "no hosts entry needed", skip the pin, and let the sandbox +// re-resolve the name freely -- failing open and defeating the firewall. +#[test] +fn host_pin_returns_err_for_empty_address() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + + match ProxyAddress::new(String::new(), 8080).host_pin(ip) { + Err(_) => {} + Ok(None) => { + panic!("empty address must be Err (pin required but impossible), not Ok(None); Ok(None) fails open") + } + Ok(Some(_)) => panic!("empty address cannot yield a pin; expected Err, got Ok(Some)"), + } } -// Protects the planned firewall / hosts-file consumer. An empty address names -// nothing to resolve, so no hosts entry is required. +// Protects the planned firewall / hosts-file consumer against hosts-file +// injection, the defect PR 789 fixed. A newline would end the hosts record and +// begin a second, unauthorized mapping, so an address carrying one must be `Err` +// and never reach `hosts_line`. #[test] -fn host_pin_returns_none_for_empty_address() { - let addr = ProxyAddress::new(String::new(), 8080); +fn host_pin_returns_err_for_hostname_with_newline() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + let injected = "proxy.example.com\n10.0.0.1 evil.example.com"; + + match ProxyAddress::new(injected.to_string(), 8080).host_pin(ip) { + Err(_) => {} + Ok(None) => { + panic!("a newline-bearing address must be Err, not Ok(None); Ok(None) fails open") + } + Ok(Some(_)) => { + panic!("a newline-bearing address must be Err; Ok(Some) would inject a hosts record") + } + } +} - assert_eq!(addr.host_pin("10.0.0.5"), None); +// Protects the planned firewall / hosts-file consumer against hosts-file +// injection. A space splits one hosts record into an address and a second, +// unauthorized name, so an address containing whitespace must be `Err`. +#[test] +fn host_pin_returns_err_for_hostname_with_space() { + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + + match ProxyAddress::new("proxy.example.com evil".to_string(), 8080).host_pin(ip) { + Err(_) => {} + Ok(None) => { + panic!("a space-bearing address must be Err, not Ok(None); Ok(None) fails open") + } + Ok(Some(_)) => { + panic!("a space-bearing address must be Err; Ok(Some) would inject a hosts record") + } + } } // Protects both the URL clients and the planned pin consumer. Computing a pin @@ -250,24 +351,27 @@ fn host_pin_does_not_change_to_url() { "proxy.example.com".to_string(), 8443, ); + let ip: IpAddr = "10.0.0.5".parse().unwrap(); let before = addr.to_url(); - let pin = addr.host_pin("10.0.0.5"); + match addr.host_pin(ip) { + Ok(Some(_)) => {} + Ok(None) => panic!("a hostname address should require a pin, got Ok(None)"), + Err(_) => panic!("a hostname address should pin cleanly, got Err"), + } - assert!(pin.is_some(), "a hostname address should yield a pin"); assert_eq!(addr.to_url(), before); assert_eq!(addr.to_url(), "https://proxy.example.com:8443"); } // Protects the planned firewall / hosts-file consumer. A hosts line is // "{ip} {hostname}" -- address first, then hostname, separated by a single -// space. +// space, with no trailing newline. #[test] fn hosts_line_writes_ip_then_hostname() { - let pin = ProxyHostPin { - hostname: "proxy.example.com".to_string(), - ip: "10.0.0.5".to_string(), - }; + let ip: IpAddr = "10.0.0.5".parse().unwrap(); + + let pin = pin_for("proxy.example.com", ip); assert_eq!(pin.hosts_line(), "10.0.0.5 proxy.example.com"); } @@ -277,26 +381,28 @@ fn hosts_line_writes_ip_then_hostname() { // deliberate opposite of how `to_url` renders IPv6. #[test] fn hosts_line_writes_ipv6_address_without_brackets() { - let pin = ProxyHostPin { - hostname: "proxy.example.com".to_string(), - ip: "2001:db8::1".to_string(), - }; + let ip: IpAddr = "2001:db8::1".parse().unwrap(); + + let pin = pin_for("proxy.example.com", ip); + let line = pin.hosts_line(); - assert_eq!(pin.hosts_line(), "2001:db8::1 proxy.example.com"); + assert!( + !line.contains('['), + "hosts line must not bracket IPv6: {line:?}" + ); + assert_eq!(line, "2001:db8::1 proxy.example.com"); } -// Protects both surfaces at once by pinning the asymmetry the contract calls -// out explicitly: for the very same IPv6 literal, the URL host component is +// Protects both surfaces at once by pinning the asymmetry the contract calls out +// explicitly: for the very same IPv6 literal, the URL host component is // bracketed while the hosts-file line is bare. A well-meaning refactor that // unified the two would break exactly one of them, and this test names which. #[test] fn ipv6_is_bracketed_in_url_but_bare_in_hosts_line() { + let ip: IpAddr = "2001:db8::1".parse().unwrap(); + let url = ProxyAddress::new("2001:db8::1".to_string(), 8080).to_url(); - let line = ProxyHostPin { - hostname: "proxy.example.com".to_string(), - ip: "2001:db8::1".to_string(), - } - .hosts_line(); + let line = pin_for("proxy.example.com", ip).hosts_line(); assert_eq!(url, "http://[2001:db8::1]:8080"); assert_eq!(line, "2001:db8::1 proxy.example.com");