[LXC] Pin the proxy hostname instead of rewriting the URL host - #789
[LXC] Pin the proxy hostname instead of rewriting the URL host#789Darren Hoehna (dhoehna) wants to merge 4 commits into
Conversation
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
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
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Pull request overview
Adds proxy hostname pinning while preserving TLS identity and corrects proxy URL generation.
Changes:
- Introduces
ProxyHostPinand IPv6-aware URL formatting. - Adds comprehensive proxy address contract tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
src/core/wxc_common/src/models.rs |
Adds host-pin modeling and correct URL construction. |
src/core/wxc_common/tests/proxy_address_spec.rs |
Tests URL preservation, IP handling, and hosts-file formatting. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| /// 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, |
There was a problem hiding this comment.
Good catch, and I agree -- this is a real hole, not a theoretical one. hosts_line is written into /etc/hosts, so a newline in either field ends the record and starts a second, unauthorized mapping. A type whose whole job is to make the sandbox and the firewall agree on one endpoint cannot allow a value that denotes two.
Fixed in c7cca5c (implementation) and 1ba5a1c (tests).
I took the strongest of the options you listed rather than the cheapest, since there is no caller yet and this only gets more expensive later:
- Both fields are private now, read through
hostname()andip().ProxyAddress::host_pinis the only way to obtain one. ipis anIpAddr, not aString. That makes the invalid state unrepresentable rather than merely rejected, and it drops the bracket-stripping the old code needed. It also lands the IPv6 requirement structurally:Display for IpAddrrenders bare, which is what a hosts file wants, so the asymmetry withto_url-- which brackets, because a URL host component requires it -- is no longer a convention someone can forget.- The hostname is validated against letters, digits,
-, and.. That set is chosen for what it excludes: whitespace and control characters, which are the injection vector.
One thing worth flagging, because it changed the signature: host_pin now returns Result<Option<ProxyHostPin>, WxcError>. An empty or malformed hostname used to return None, and I did not want to keep that. None means "no hosts entry required", so a malformed address would silently skip the pin, the sandbox would re-resolve the name freely, and the firewall could be bypassed. That is fail-open, which is the same defect flagged on the config path in the earlier iteration of this work. Ok(None) now means only "the address is an IP literal, nothing to resolve"; anything unpinnable is Err.
Tests: 22 black-box tests, written against the documented contract by an author who has not read models.rs. The empty-address test matches on all three arms by name -- is_err() || is_none() would have passed either way, and that distinction is the entire point of the change. Verified with a mutation harness of 11 mutants, including "accept any hostname" and "return Ok(None) instead of Err": 11 caught by a failing test, 0 survivors.
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<Option<ProxyHostPin>, 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
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
| fn unbracket(host: &str) -> &str { | ||
| host.strip_prefix('[') | ||
| .and_then(|rest| rest.strip_suffix(']')) | ||
| .unwrap_or(host) | ||
| } |
|
Superseded by #798. This work was split into six PRs on my initiative; it should have been one. Four of the six (#790, #792, #796, #797) were cumulatively stacked, so each one re-rendered the previous diff rather than reducing what a reviewer had to read, and they forced a merge order for no benefit. All of the changes here are in #798, cut fresh from |
What this changes
Model 2 ("deny-all-except-proxy") requires the sandbox and the firewall to
agree on exactly one proxy endpoint. If they disagree, the sandbox
re-resolves the proxy hostname itself and, under round-robin or split-horizon
DNS, can reach an address the firewall never authorized. That is a policy
bypass, not a cosmetic inconsistency.
This slice adds the data model for forcing that agreement, and fixes one
existing way the model could report the wrong endpoint.
1.
ProxyHostPin— force the endpoint without breaking TLSPR 632 solved this by rewriting the proxy URL's host to the resolved IP.
Review rejected that, and correctly (comment 3724788051):
ProxyHostPinis that alternative. It expresses the mapping as a hosts-filepin, so the hostname stays in the URL and TLS identity survives, while the
endpoint is still pinned to the one address the firewall allows.
Its fields are private and the address is an
IpAddr, so a pin that denotesanything other than exactly one mapping cannot be constructed. That is not
decoration:
hosts_lineis written into/etc/hosts, and a newline or spacein either field would end the record and inject a second, unauthorized
mapping. Review caught this on the first revision, when the fields were public
Strings.host_pinreturnsResult<Option<ProxyHostPin>, WxcError>, and the threeanswers are deliberately distinct:
Ok(None)— and only this — means the address is an IP literal, so there isnothing to resolve and no pin is needed.
Ok(Some(pin))means the address is a hostname and the pin is required.Errmeans a pin is required but cannot be produced.An empty or malformed hostname is
Err, neverNone. Folding it intoNonewould tell the caller "no hosts entry required", so a malformed address would
silently skip the pin, the sandbox would re-resolve the name freely, and the
firewall could be bypassed — fail-open, which is the defect objection
3724803457raised against the config path.hosts_linewrites the address bare. A hosts file takes an unbracketedIPv6 literal, unlike a URL host component — so
to_urlbrackets andhosts_linedoes not. That asymmetry is deliberate, andIpAddr'sDisplaymakes it structural rather than a convention a caller could forget.
2.
to_urlno longer assumes loopbackto_urlhardcodedhttp://127.0.0.1:{port}whenever no original URL wasrecorded, regardless of the actual address.
That is reachable, not theoretical.
unix_proxy_coordinator.rs:234builds aProxyAddressfrom 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. This is the same class of defect as the objection above — the
client being told a different endpoint from the authorized one.
Every existing caller passes
127.0.0.1, so their output is byte-identical.All 566 existing
wxc_commontests pass unchanged.Testing
Tests are black-box integration tests in
tests/proxy_address_spec.rs,written against the public API by an author who did not see the
implementation and was explicitly prohibited from reading it. Code-derived
tests inherit the code's own mistakes; contract-derived tests do not.
22 tests. Highlights of what they pin:
to_urlnames the configured address and does not assume loopback.original_urlpassthrough is verbatim, including a trailing slash -- anearlier implementation mangled that.
host_pinreturnsOk(None)for every IP-literal shape, and calling it doesnot perturb
to_url.Errand never reacheshosts_line. The empty-address test matches on all three arms by name,so it fails if
ErrandOk(None)are ever swapped --is_err() || is_none()would have passed either way, and that distinction is the wholepoint.
Mutation results
A green test run proves the tests ran, not that they would catch anything.
So each plausible defect was seeded and the suite re-run:
11 seeded mutants, 11 caught by a failing test, 0 survivors.
to_url_reverts_to_loopback-- the bug this PR fixesto_url_drops_ipv6_bracketsto_url_ignores_original_urlhost_pin_returns_some_for_ip_literalhost_pin_fails_open_on_bad_hostname--Ok(None)instead ofErrhost_pin_accepts_any_hostname-- the injection defect review foundhost_pin_accepts_empty_hostnamehost_pin_keeps_brackets_on_hostnamehosts_line_reverses_columnshosts_line_brackets_ipv6bracket_brackets_ipv4_tooTwo findings from running it, both of which would have been easy to wave
through:
An earlier mutant survived, and turned out to be equivalent rather than a
test gap: deleting an early-return guard from the private bracketing helper
changed nothing, because
IpAddr::from_strrejects brackets, so analready-bracketed literal fell through unchanged either way. That was dead
code in the implementation, so the guard is removed in
0050a1band the mutantreplaced with one that distinguishes real behavior.
Four mutants were initially caught by the compiler, not by the tests. The
crate denies warnings, so removing the last call to a private helper makes it
dead code and the build fails. That is real detection — such a defect cannot
ship — but it proves nothing about the suite, and counting it as a pass would
have overstated coverage. The harness now suppresses those lints for the
mutated build, so all 11 numbers above are failing tests.
The mirror case is genuinely load-bearing, and mutation proves it:
host_pin_keeps_brackets_on_hostnamefails a test, because unbracketing iswhat stops a bracketed IPv6 literal being pinned as though it were a hostname.
Verification
cargo test -p wxc_common-- 566 library tests passed, 0 failedcargo test -p wxc_common --test proxy_address_spec-- 22 passed, 0 failedcargo fmt -p wxc_common -- --check-- cleancargo clippy -p wxc_common --tests-- cleanWhy this is a small PR
This is slice 2 of 6 of the work previously attempted in PR 632, which was
closed and re-cut from
main. 632 reached 3,593 lines across 17 files in 33commits and accumulated 63 review comments, 52 of them substantive design
objections — the same way PR 633 died at 87 comments over 42 commits. Neither
was reviewable by the end.
The invariant for the re-cut is that reviewability wins. Slices land in
dependency order, each independently reviewable:
IPv6 parity is a launch requirement of slice 4, not a follow-up: 632's
single largest review theme was that an IPv4-only deny-all posture is entirely
evadable over IPv6.
Not in this PR
This slice is the data model only. Nothing calls
host_pinyet — writing thehosts entry and programming the matching firewall rule land in slices 4 and 5.
The
Nonereturn is what those callers will use to skip the hosts write for anIP-literal proxy.
Microsoft Reviewers: Open in CodeFlow
Refs AB#62830341.