[WSLC] Reject remaining policy fields the backend cannot honor - #1058
[WSLC] Reject remaining policy fields the backend cannot honor#1058Soham Das (SohamDas2021) wants to merge 3 commits into
Conversation
WSLc accepted four policy fields, carried them into the runner, and then
never read them. A caller asking for a UI lockdown, a firewall enforcement
mode, inbound local-network access, or policy preservation got a container
that silently did not have the posture they asked for. Close each hole with
an explicit policy_validation rejection.
Every rejection aborts before anything is created. All three entry surfaces
validate first: ScriptRunner::run ahead of execute, SandboxBackend::spawn
ahead of start_container, and the state-aware dispatcher ahead of each phase
body -- and connect_daemon() lives inside provision(), so a refused provision
never even spawns the daemon.
ui rejected on every phase, both surfaces
network.allowLocalNetwork rejected at state-aware provision (one-shot
already rejected it)
network.enforcementMode firewall/both rejected; capabilities accepted
lifecycle.preservePolicy rejected on one-shot (state-aware already
rejects the whole lifecycle section at parse)
ui is rejected by presence, not value. UiPolicy::default() is full lockdown,
so an explicitly supplied lockdown ui is indistinguishable by value from an
absent one -- a value-based check would let the single most restrictive
request a caller can write through unenforced. This uses the parse-derived
ContainerPolicy::ui_specified flag, mirroring IsolationSession.
enforcementMode and preservePolicy are rejected by value instead, because
their defaults honestly describe WSLc's behavior: an all-or-nothing container
network with nothing per-host to enforce, and auto-remove teardown. Refusing
those for mere presence would be dishonest.
destroyOnExit stays honored -- it selects WSLC_CONTAINER_FLAG_AUTO_REMOVE --
so only preservePolicy is refused. A blanket lifecycle rejection would have
broken the wslc_destroy_on_exit_{true,false} configs; a test pins both values
still passing.
The two allowLocalNetwork messages differ deliberately. One-shot points
callers at experimental.wslc portMappings, but WslcProvisionPhase has no
portMappings field at all, so repeating that advice on the state-aware
surface would be a lie.
Rejection ordering is filesystem -> ui -> network, documented in the policy.rs
module header and pinned by precedence tests.
No wire, schema, or parser-gating changes: this is a domain-model behavior
change only, so it lands while WSLc is still experimental and the nightly
WSLc suite exercises the new rejections before the surface moves.
Co-authored-by: Copilot App <[email protected]>
Copilot-Session: c23ddc86-3848-452e-8355-e11d2ffa9b7f
|
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 fail-closed WSLc validation for policy fields the backend cannot enforce.
Changes:
- Rejects unsupported UI, network enforcement, local-network, and lifecycle settings.
- Applies validation before one-shot, streaming, and state-aware execution.
- Adds regression tests and updates WSLc documentation.
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
src/core/wxc_common/src/models.rs |
Documents WSLc use of UI presence tracking. |
src/backends/wslc/common/src/wsl_container_runner.rs |
Adds one-shot policy validation. |
src/backends/wslc/common/src/state_aware.rs |
Tests state-aware validation hooks. |
src/backends/wslc/common/src/sandbox.rs |
Tests pre-start streaming validation. |
src/backends/wslc/common/src/policy.rs |
Implements shared WSLc policy rejection rules. |
docs/wsl/wslc-state-aware.md |
Updates the state-aware policy matrix. |
docs/wsl/wsl-container-getting-started.md |
Documents unsupported one-shot policy fields. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
src/backends/wslc/common/src/wsl_container_runner.rs:2600
- This test locks in incorrect behavior:
destroyOnExit=falseonly omitsWSLC_CONTAINER_FLAG_AUTO_REMOVE; the one-shotWslcSessionGuardstill terminates the session at the end of the run and reaps the session-scoped container. Update this case to expect a policy rejection forfalse, while retaining the acceptance check fortrue.
for destroy_on_exit in [true, false] {
cc7007a to
fa97faa
Compare
| /// The state-aware surface needs no counterpart: the parser rejects the whole | ||
| /// one-shot `lifecycle` section on state-aware requests. | ||
| fn reject_unsupported_lifecycle(request: &ExecutionRequest) -> Result<(), ScriptResponse> { | ||
| if !request.lifecycle.destroy_on_exit { |
| | Hyperlight | `wxc-exec.exe` | Windows | `backends/hyperlight/common/src/lib.rs` — Hyperlight + Unikraft micro-VM backend | | ||
| | IsolationSession | `wxc-exec.exe` | Windows | `backends/isolation_session/common/src/` — feature-gated behind `isolation_session`, experimental, uses the in-proc `Windows.AI.IsolationSession.Preview` `IsoSessionOps` API. Supports both one-shot (single-invocation lifecycle, via `ScriptRunner`) and state-aware (multi-invocation provision/start/exec/stop/deprovision, via `StatefulSandboxBackend`) modes. Rejects all filesystem policy (`readwritePaths`/`readonlyPaths`/`deniedPaths`) at every phase with `policy_validation` — the backend has no host-folder-sharing primitive. Likewise rejects any supplied `ui` policy at every phase on both surfaces (as `policy_validation` on the state-aware surface; one-shot discards the typed variant and surfaces `backend_error` with the reason in the message): the isolation session isolates the *host's* UI from contained code but does not deny it UI capabilities (window creation, GDI and the session's own clipboard all work inside it), so no `ui` posture is truthful here — there is no value combination that could be accepted instead, which is why there is no acknowledgment-style gate as there is for `network`. The check is presence-based via `ContainerPolicy::ui_specified` (twin of `network_specified`) because `UiPolicy`'s defaults are full lockdown, making an explicit lockdown `ui` indistinguishable by value from an absent one. An omitted `ui` is accepted and applies no restriction — the schema's default-deny reading does not hold on this backend. One-shot additionally rejects `lifecycle.destroyOnExit=false` and `lifecycle.preservePolicy=true` — the in-proc API has no session-lifetime knob, and the default `destroyOnExit=true` matches actual behavior so it is accepted; the state-aware parser already rejects the whole `lifecycle` section. The full per-phase honor matrix for both surfaces is in `docs/isolation-session/state-aware-rust.md`. The container's network is unrestricted (outbound open; a process inside can listen on a localhost-reachable port) and MXC has no primitive to filter or deny it, so provision (and one-shot) accept ONLY the canonical unrestricted-network acknowledgment — `network.defaultPolicy=allow` + `network.allowLocalNetwork=true`, no host rules, no proxy, default enforcement — and refuse anything else (including an absent policy, which defaults to the unenforceable deny) with `policy_validation`; post-provision phases reject any supplied network policy (fixed at provision, tracked via `ExecutionRequest.network_specified`) and inherit an absent one. State-aware provision accepts an optional `appId` (a packaged app must pass its Package Family Name in the `PFN:<pfn>` format, e.g. `PFN:Contoso.App_8wekyb3d8bbwe`; an unpackaged app may pass any string), carried verbatim inside the returned `sandboxId`; the one-shot surface takes no backend configuration at all (a stray `experimental.isolation_session` payload is accepted and ignored). Streams stdout/stderr, forwards stdin, and switches to ConPTY mode when wxc-exec's stdout is a TTY for `spawnSandbox` parity. | | ||
| | WSLc | `wxc-exec.exe` | Windows | `backends/wslc/common/src/` — feature-gated behind `wslc`, experimental, uses the WSLc SDK (`wslcsdk.dll`, loaded at runtime) to run Linux containers in a WSL2 VM. Supports both one-shot (`WSLContainerRunner`, via `ScriptRunner` + streaming `SandboxBackend`) and state-aware (`state_aware.rs` `WslcStateAwareRunner`, via `StatefulSandboxBackend`) modes. Because the WSLc SDK has **no cross-process re-attach**, state-aware keeps the session (VM) + container warm across separate `wxc-exec` phase processes behind a persistent per-user daemon (`wxc-wslc-daemon.exe`, `backends/wslc/daemon/`) that owns the live `WslcSession`/`WslcContainer` handles; phase processes are thin named-pipe clients (`daemon_client.rs`). The daemon runs all SDK calls on one apartment-affine worker thread (so exec is currently serialized across sandboxes — see `docs/wsl/wslc-state-aware.md`). Honors `readwritePaths`/`readonlyPaths` at provision (→ container volumes) + `network.defaultPolicy` (`Block`→`None`, `Allow`→`Bridged`; networking is all-or-nothing — no per-host filtering, since the container lacks `CAP_NET_ADMIN`); rejects `deniedPaths` nested under a mount and rejects proxy/host-filtering at provision. exec honors `network.proxy` **url-form only** (injected as `HTTP_PROXY`/`HTTPS_PROXY`); start/stop/deprovision reject all policy. ID prefix `wslc` (`wslc:<32-hex>`). Idle-timeout is env-overridable via `MXC_WSLC_DAEMON_IDLE_TIMEOUT_SECS`/`MXC_WSLC_DAEMON_IDLE_POLL_SECS`. See `docs/wsl/wslc-state-aware.md`. | | ||
| | WSLc | `wxc-exec.exe` | Windows | `backends/wslc/common/src/` — feature-gated behind `wslc`, experimental, uses the WSLc SDK (`wslcsdk.dll`, loaded at runtime) to run Linux containers in a WSL2 VM. Supports both one-shot (`WSLContainerRunner`, via `ScriptRunner` + streaming `SandboxBackend`) and state-aware (`state_aware.rs` `WslcStateAwareRunner`, via `StatefulSandboxBackend`) modes. Because the WSLc SDK has **no cross-process re-attach**, state-aware keeps the session (VM) + container warm across separate `wxc-exec` phase processes behind a persistent per-user daemon (`wxc-wslc-daemon.exe`, `backends/wslc/daemon/`) that owns the live `WslcSession`/`WslcContainer` handles; phase processes are thin named-pipe clients (`daemon_client.rs`). The daemon runs all SDK calls on one apartment-affine worker thread (so exec is currently serialized across sandboxes — see `docs/wsl/wslc-state-aware.md`). Honors `readwritePaths`/`readonlyPaths` at provision (→ container volumes) + `network.defaultPolicy` (`Block`→`None`, `Allow`→`Bridged`; networking is all-or-nothing — no per-host filtering, since the container lacks `CAP_NET_ADMIN`); rejects `deniedPaths` nested under a mount and rejects proxy/host-filtering at provision. exec honors `network.proxy` **url-form only** (injected as `HTTP_PROXY`/`HTTPS_PROXY`); start/stop/deprovision reject all policy. Rejects, on **both** surfaces and every phase, the fields it cannot honor: any supplied `ui` (presence-based via `ContainerPolicy::ui_specified`, since a WSLc container runs Linux while `ui` maps to Windows `JOB_OBJECT_UILIMIT_*` — an omitted `ui` is accepted and applies no restriction, so the schema's default-deny reading does not hold here) and `network.enforcementMode` other than `capabilities` (no `CAP_NET_ADMIN` for in-container rules). Provision/one-shot additionally reject `network.allowLocalNetwork=true` (all-or-nothing networking; the one-shot message points at `experimental.wslc.portMappings`, which the state-aware surface does not have), and one-shot rejects `lifecycle.preservePolicy=true` and `lifecycle.destroyOnExit=false` (the container is session-scoped and the session dies with the one-shot process, so `false` cannot be honored; only the default `true` matches actual behavior). Every rejection aborts before any container is created. ID prefix `wslc` (`wslc:<32-hex>`). Idle-timeout is env-overridable via `MXC_WSLC_DAEMON_IDLE_TIMEOUT_SECS`/`MXC_WSLC_DAEMON_IDLE_POLL_SECS`. See `docs/wsl/wslc-state-aware.md`. | |
There was a problem hiding this comment.
note: UI is meant to be cross platform. On Windows it might map to JOB_OBJECT_UILIMIT_ but on Linux we still want to see if there is something we can map it to.
| ### `ui` is not supported | ||
|
|
||
| A `ui` section is **rejected**. The section maps to Windows job-object UI limits | ||
| (`JOB_OBJECT_UILIMIT_*`); a WSLC container runs Linux inside the WSL2 VM, so | ||
| those limits have no analogue and nothing in the backend could apply them. | ||
|
|
There was a problem hiding this comment.
note: see my comment above about what UI is suppose to be for. It's ok for WSLC to not support it if it can't but I don't think we want to imply it's only for Windows when it's meant to be cross platform. We can probably remove that sentence from here actually.
| scoped to a session this process owns, terminating that session at the end of | ||
| the run reaps the container regardless of the AutoRemove flag, and the WSLC SDK | ||
| has no cross-process re-attach. Use the state-aware lifecycle if you need a | ||
| container to persist — its daemon holds the session open across phase processes. |
There was a problem hiding this comment.
quick question here if two apps use MXC and both you WSLC state-aware are the two daemons operating in separate processes and if so do they know anything about the others WSL containers? Just want to confirm they won't stamp on each others toes is all.
| `ui` is rejected by **presence, not value**, on every phase. A WSLc container runs Linux, so the | ||
| section's Windows job-object UI limits (`JOB_OBJECT_UILIMIT_*`) have no analogue inside it and no | ||
| phase could honor it. Presence is the only workable test because `UiPolicy`'s defaults are full |
There was a problem hiding this comment.
note: see comment on UI above.
| [`isolation-session/state-aware-rust.md`](isolation-session/state-aware-rust.md)) — | ||
| and accepts an omitted one without applying any UI restriction. The Windows | ||
| [`isolation-session/state-aware-rust.md`](isolation-session/state-aware-rust.md)), | ||
| while a WSLc container runs Linux and has no analogue of the Windows job-object |
There was a problem hiding this comment.
thought: "WSLc container", we probably want to start saying "WSL container" right?
| /// Refuses the `lifecycle` settings the one-shot surface cannot honour. | ||
| /// | ||
| /// Value-based rather than presence-based (unlike `ui`), because the defaults | ||
| /// genuinely match the behaviour: | ||
| /// | ||
| /// * `destroyOnExit: true` (the default) is honoured — it selects | ||
| /// `WSLC_CONTAINER_FLAG_AUTO_REMOVE` and [`StartedContainer::destroy`] stops | ||
| /// and deletes the container. | ||
| /// * `destroyOnExit: false` is refused. [`StartedContainer`] owns the | ||
| /// [`WslcSessionGuard`], whose `Drop` terminates the session — and with it | ||
| /// the session-scoped container — regardless of the flag, and the WSLC SDK | ||
| /// has no cross-process re-attach. The outcome is identical to `true`, so | ||
| /// accepting `false` would promise a container that is already gone. | ||
| /// * `preservePolicy: true` asks for filesystem and network policy to outlive | ||
| /// the run. WSLc installs no persistent host-side enforcement: `rw`/`ro` | ||
| /// paths become container volume mounts and the network posture is a | ||
| /// container networking mode, both of which are properties of the container | ||
| /// object itself and cannot be retained independently of it. There is | ||
| /// nothing to preserve, so the request is refused rather than silently | ||
| /// dropped. | ||
| /// | ||
| /// The state-aware surface needs no counterpart: the parser rejects the whole | ||
| /// one-shot `lifecycle` section on state-aware requests. |
There was a problem hiding this comment.
thought: Do we need these comments? In this case the code kind of speaks for itself really.
| // -- Accept-but-ignore closures -------------------------------------- | ||
| // | ||
| // Each of these fields used to be parsed, carried into the runner, and | ||
| // then never read — so a caller got a container that silently did not | ||
| // have the posture they asked for. They are now refused, and refused | ||
| // from `validate_runner`, which both `ScriptRunner::run` and | ||
| // `SandboxBackend::spawn` call *before* any container exists. | ||
|
|
||
| /// A WSLc container runs Linux; `ui` maps to Windows job-object UI limits | ||
| /// (`JOB_OBJECT_UILIMIT_*`) with no analogue inside it. | ||
| /// | ||
| /// Presence-based: `UiPolicy::default()` is full lockdown, so an | ||
| /// explicitly-supplied lockdown `ui` is indistinguishable by value from an | ||
| /// absent one. A value-based check would let the single most restrictive | ||
| /// request a caller can write through unenforced. |
There was a problem hiding this comment.
note: same thing for these comments on tests, the test seems like it explains themselves. The docs you have at the top already explain the reasoning.
| /// Consumed by IsolationSession and WSLc today, neither of which has a | ||
| /// UI-restriction primitive: both refuse a supplied UI policy rather than | ||
| /// accepting and dropping it. The other backends that do not enforce | ||
| /// `policy.ui` — LXC and Bubblewrap on Linux, Seatbelt on macOS, Windows | ||
| /// Sandbox — still accept and ignore it, so this flag being set does not | ||
| /// mean a UI policy was honored anywhere; it means only that the caller | ||
| /// supplied one. | ||
| #[serde(skip)] |
There was a problem hiding this comment.
note: probably don't need the full explanation, but as an aside, I'm starting to think we should have made these *_supplied booleans bit flags. Not for this PR but just for later.
| @@ -0,0 +1,20 @@ | |||
| { | |||
| "_comment": "Rejection fixture: the one-shot WSLc surface refuses lifecycle.destroyOnExit=false. The container is scoped to a session this process owns, and terminating that session reaps the container regardless of the AutoRemove flag, so `false` cannot be honored. Expects a rejection before any container is created -- the payload must never run.", | |||
| "version": "0.6.0-alpha", | |||
There was a problem hiding this comment.
question (non-blocking): I guess the version doesn't really matter here any more, as the behavior will be for all of them + stable after promotion.
| /// Reject any supplied UI policy. Presence-based, not value-based: the domain | ||
| /// `UiPolicy::default()` is full lockdown, so an explicitly-supplied lockdown | ||
| /// `ui` is indistinguishable from an absent one by value — the same blind spot | ||
| /// `network_specified` closes for the network policy. | ||
| /// | ||
| /// Shared by every WSLc phase and by the one-shot / streaming | ||
| /// `validate_runner`: the reason is the container's OS, not the lifecycle | ||
| /// phase, so there is no phase on either surface where a `ui` section could be | ||
| /// honoured. Runs after the filesystem check so a filesystem rejection keeps | ||
| /// precedence, and before the network checks. |
There was a problem hiding this comment.
issue: I think the comments in this file can probably be reduced, if the code is clear enough (in this case simply rejecting a specified policy + tests) we don't need to say what its doing. I think we can have copilot do an inventory to slim all these in the PR down or remove them to be honest.
📖 Description
.github/copilot-instructions.md.Summary
WSLc accepted five policy fields, carried them into the runner, and never read them. A caller asking for a UI lockdown, a firewall enforcement mode, inbound local-network access, policy preservation, or a container that outlives the run got a container that silently did not have the posture they asked for. Each is now an explicit
policy_validationrejection.uinetwork.allowLocalNetworknetwork.enforcementModefirewall/bothrejected;capabilitiesacceptedlifecycle.preservePolicylifecycle.destroyOnExitfalseaccepted, behaved astruefalserejected on one-shot;trueacceptedRejection means abort, not teardown. All three entry surfaces validate before anything is created:
ScriptRunner::runahead ofexecute,SandboxBackend::spawnahead ofstart_container, and the state-aware dispatcher ahead of each phase body — andconnect_daemon()lives insideprovision(), so a refused provision never even spawns the daemon.uiis rejected by presence, not value. The parser materializes a full-lockdownuiblock into every request, so an explicitly supplied lockdownuiis indistinguishable by value from an absent one — a value-based check would reject every WSLc config in the repo while letting the most restrictive request a caller can write through unenforced. This uses the parse-derivedContainerPolicy::ui_specifiedflag, mirroring IsolationSession.enforcementMode,preservePolicy, anddestroyOnExitare rejected by value instead, because their defaults honestly describe WSLc's behavior —destroyOnExitdefaults totrue, so bare configs are untouched.destroyOnExit: falsecorrects a claim this PR originally made. SelectingWSLC_CONTAINER_FLAG_AUTO_REMOVEis not the same as honoring the field:StartedContainerowns theWslcSessionGuard, whoseDropterminates the session — and with it the session-scoped container — regardless of the flag, and the WSLc SDK has no cross-process re-attach.trueandfalsetherefore produce an identical observable outcome, which the repo's own fixture had already admitted in a comment. Acceptingfalsepromised a container that was already gone, so it is now refused and that fixture converted into a rejection test.No wire, schema, or parser-gating changes — this is a domain-model behavior change only, so it lands while WSLc is still experimental.
Validation
wslc_common211 tests (with and withoutlink-wslcsdk),wxc_common983, cleanfmtand clippy onwslc_common/wxc_common/wxc --features wslc. All 43tests/configs/wslc*.jsonpushed through the real parser + validator: 26 accept, 17 reject. Of the rejections, 16 are pre-existing (13{{SANDBOX_ID}}placeholders, 3 named*_rejected_*) and 1 is the new fixture this PR adds. No config regressed into an unintended rejection.The full WSLc one-shot E2E suite was run on a WSL2 host and is green, including both lifecycle fixtures.
On E2E coverage, precisely. One rejection is now asserted positively end-to-end:
wslc_destroy_on_exit_false_rejected.jsonruns in the nightly WSLc suite and requires exit-1plus the rejection message, so the payload provably never runs. The other three are not — no fixture supplies auiblock, afirewallenforcement mode, orpreservePolicy. What the config corpus covers for those is the inverse and, for this change, the higher risk: that none of the existing configs regressed into an over-rejection. The positive path is covered by unit tests instead, including the near-miss acceptances (capabilities,allowLocalNetwork: false, absentui) that a gate flipping between value- and presence-based would break. Positive E2E fixtures for the remaining three are better added alongside the promotion in PR 3, where these fields move to the top-level surface and the configs have to be rewritten anyway.Related Issues
📋 Issue Type
GitHub Actions runs the PR validation build automatically. The ADO pipeline
(
MXC-PR-Build) is the Azure version of the PR pipeline, kept in parity with the GitHubActions build; it runs on merge to
main, and Microsoft reviewers with write access can trigger iton a PR with
/azp run. See docs/pull-requests.md.If the
dependency-feed-checkcheck fails on a new dependency, the crate must be added tothe feed before the PR can pass. See docs/pull-requests.md
for the steps.
Microsoft Reviewers: Open in CodeFlow