[LXC] Scrub and apply proxy env vars through the shared helper - #788
[LXC] Scrub and apply proxy env vars through the shared helper#788Darren Hoehna (dhoehna) wants to merge 2 commits into
Conversation
LXC did not scrub proxy environment variables from caller-supplied env, so a caller could point a sandboxed process at an egress path the network policy never authorized, or disable the cooperative proxy outright. Add `apply_proxy_env` to `wxc_common::proxy_env`, the LXC entry point. It delegates to `apply_cooperative_proxy_env` so LXC scrubs and sets exactly the same key set as Bubblewrap and WSLc rather than maintaining a parallel list that can drift. With the proxy disabled the vars are still stripped. It returns `true` unconditionally, including for an empty env: the return value tells the caller to emit `--clear-env`, and an empty vector must still stop `lxc-attach` inheriting the MXC host process environment, which carries both proxy vars and credentials. Add `FTP_PROXY`/`ftp_proxy` to `PROXY_ENV_KEYS`. Both spellings of every family are now present, and the doc comment records why the lower-case duplicates are kept. Tests are black-box integration tests in `tests/proxy_env_spec.rs`, written against the public API by an author who did not see the implementation. All 22 pass; 7 of 7 seeded mutants are caught with no survivors. This is slice 1 of the work previously attempted in PR 632, re-cut from main so each slice is reviewable on its own. 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 a shared LXC proxy-environment helper while extending managed proxy variables to include FTP.
Changes:
- Adds
apply_proxy_envwith unconditional clean-environment signaling. - Scrubs
FTP_PROXYvariants. - Adds black-box 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/proxy_env.rs |
Implements LXC proxy environment hygiene. |
src/core/wxc_common/tests/proxy_env_spec.rs |
Tests proxy scrubbing, setting, ordering, and redaction. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| //! (a) LXC backend -- calls `apply_proxy_env`, uses the returned bool to | ||
| //! decide whether to pass `--clear-env` to `lxc-attach`. |
There was a problem hiding this comment.
Good catch, and correct on both counts. I verified before changing anything: �pply_proxy_env has no call site in the backend yet, and �ttach_run pushes --clear-env only under if !env.is_empty() at lxc_bindings.rs:90.
Fixed in 22098a2. The header now says the LXC integration is planned rather than existing.
Your comment also surfaced something worth more than the wording fix, so I recorded it in the same place. The empty-env case is a genuine divergence: apply_proxy_env returns true even for an empty env, deliberately, so lxc-attach cannot inherit the MXC host environment along with its proxy vars and credentials. Current code does the opposite there, and lxc_bindings.rs:743 pins that with a test. Whoever wires this in has to change both, and that is now written down in the header instead of being found late.
No assertions changed. As you say, these tests validate the helper contract -- that is what they are for. This PR is slice 1 of the work previously attempted in PR 632, re-cut from main so each slice is reviewable on its own; the runner integration is a later slice.
The test module header described client (a) in the present tense, which read as though the LXC backend already calls `apply_proxy_env`. It does not: the helper has no call site yet, and `attach_run` still derives `--clear-env` solely from `env` being non-empty (`lxc_bindings.rs:90`). Record the divergence while it is cheap to see. `apply_proxy_env` returns `true` even for an empty env so the MXC host environment cannot leak into the container, whereas current code emits no `--clear-env` in that case and pins the behavior with a test at `lxc_bindings.rs:743`. The integration slice has to update both. Comment only. No assertion changed; the tests validate the helper contract, which is what they are for. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7155573c-8938-4622-abf7-4594fb17eb3d
|
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
Adds
apply_proxy_env, the LXC entry point for cooperative proxy environment hygiene, and bringsFTP_PROXYinto the managed key set. This is roadmap requirement 4 of the "deny-all-except-proxy" network policy (model 2).When a sandbox's egress is routed through a cooperative proxy, the workload is handed
HTTP_PROXY/HTTPS_PROXYand cooperating clients route through it. That only holds if the workload cannot quietly opt out.apply_proxy_envscrubs every caller-supplied proxy variable before setting the configured ones, so a workload cannot pre-disable the proxy with its ownHTTP_PROXY, or carve an exemption withNO_PROXY.It delegates to the existing
apply_cooperative_proxy_envrather than keeping a second key list, so LXC scrubs and sets exactly what Bubblewrap and WSLc already do and the two cannot drift apart.Why it is this small
This is the first slice of the re-cut of #632, which is being closed. That PR reached 3,593 insertions across 17 files and collected 63 review comments, 52 of them substantive design or correctness objections. At that size an individual correctness fix could not be verified in isolation — the same failure that ended #633.
So model 2 is landing as a series of independently reviewable PRs. Reviewability is the invariant that wins here; where it conflicts with delivering model 2 quickly, it wins. This slice is deliberately confined to pure, host-independent string handling: no firewall code, no root, no container.
Still to come, each on its own: the proxy URL and address model, loopback proxy validation, the
iptablesandip6tablesenforcement core, runner and signal-cleanup lifecycle integration, and docs plus integration scripts.The
--clear-envcontractapply_proxy_envreturnstrueunconditionally, including when the resulting vector is empty. That return value tells the caller to emit--clear-env, and an empty environment must still stoplxc-attachinheriting the MXC host process environment, which carries both proxy variables and credentials.This is worth stating explicitly because the abandoned branch shipped the opposite once: returning
falsewhen nothing was scrubbed dropped--clear-envand leaked the host environment. A mutation test pins it (see below).FTP_PROXYFTP_PROXYandftp_proxyjoinPROXY_ENV_KEYS. Matching runs throughis_managed_proxy_key, which is case-insensitive, so listing both spellings is redundant for that path — but every other family already lists both, and a consumer doing a case-sensitivecontainsover the slice would otherwise see a partial set.seatbelt_runner.rskeeps its own case-sensitive copy of this list and does exactly that; it is out of scope here and left alone.Tests
The spec tests were written by a sub-agent that had not read the implementation, from the documented contract alone, per the
blackbox-unit-testsskill. I wrote the implementation, so I could not author them. They were derived against the unit-testing knowledge corpus, and each names the client it protects: the LXC backend, the Bubblewrap backend, the WSLc backend, or security review.They live in
tests/proxy_env_spec.rsas an integration test, so they can only reach the public API — the module's internals are not visible to them.Mutation testing, not the green run, is the evidence they work. 7 seeded mutants, 7 caught, 0 survivors. Each mutant is a defect this
slice could plausibly ship:
returns_false_when_env_empty-- the--clear-envleak PR 632 shipped onceno_scrub_when_disabled-- vars survive with the proxy offcase_sensitive_key_match-- lower-case spellings stop being scrubbeddrops_ftp_from_scrub_setneutralizer_carries_urlskips_neutralizing_no_proxyreverses_caller_orderVerification
cargo test -p wxc_common --test proxy_env_spec-- 22 passed, 0 failedcargo test -p wxc_common --lib-- 566 passed, 0 failedcargo fmt -p wxc_common -- --check-- cleancargo clippy -p wxc_common --tests-- cleanMicrosoft Reviewers: Open in CodeFlow
Refs AB#62830341.