Skip to content

Phase 3.5: --prepare-system-drive flag for AppContainer host setup#389

Merged
MGudgin merged 1 commit into
mainfrom
user/gudge/downlevel_phase3_5
May 23, 2026
Merged

Phase 3.5: --prepare-system-drive flag for AppContainer host setup#389
MGudgin merged 1 commit into
mainfrom
user/gudge/downlevel_phase3_5

Conversation

@MGudgin

@MGudgin MGudgin commented May 22, 2026

Copy link
Copy Markdown
Member

Summary

Adds wxc-exec --prepare-system-drive / --unprepare-system-drive — a one-time, host-wide step that grants the well-known AppContainer SIDs metadata-only access on the system-drive root, so AppContainer-isolated tools (cmd, pwsh, node) don't hit ERROR_ACCESS_DENIED when their startup paths lstat or GetFileAttributes on C:\. Self-elevates via UAC.

SeChangeNotifyPrivilege (bypass-traverse) covers direct file opens, but not explicit metadata reads of each path component — and those are routine: if exist C:\, PowerShell drive enumeration, node.js's realpathSync in the module loader.

What it grants

Mask 0x00120088 = FILE_READ_ATTRIBUTES | FILE_READ_EA | READ_CONTROL | SYNCHRONIZE, no inheritance, on C:\ for S-1-15-2-1 (ALL APPLICATION PACKAGES) and S-1-15-2-2 (ALL RESTRICTED APPLICATION PACKAGES). The container can newly probe C:\ existence and read its attributes/SD; it still cannot enumerate children (no FILE_LIST_DIRECTORY) or read file contents (no FILE_READ_DATA).

Apply refuses if a same-SID Allow ACE with a different (mask, type, inheritance) already exists — avoids silently coalescing via SetEntriesInAclW(GRANT_ACCESS). Unprep is tuple-precise: only the ACE prepare would have written is removed, so same-SID ACEs from other tools survive.

Scope and known limits

Grants on the system-drive root only. Empirically:

  • ✅ Fully unblocks cmd.exe.
  • ⚠ Unblocks pwsh except Remove-Item -Recurse walks to C:\Users.
  • ⚠ Unblocks node for AppContainer policies rooted directly under C:\ (e.g. C:\temp); workloads under C:\Users\<user> still hit realpathSync on C:\Users.

Extending coverage to C:\Users (and the profile dir) is out of scope here and a candidate for a follow-up.

Validation

cargo fmt --check                                              # clean
cargo check --workspace --all-targets                          # clean
cargo clippy --workspace --all-targets -- -D warnings          # clean
cargo test -p wxc_common --lib -- --test-threads=1             # 350/350 pass

Six tests under system_drive_prep::tests cover the apply/revoke round-trip, tuple-precise revoke of same-SID priors, scan-before-apply refusal on conflicting-mask priors, drive-root validation, the MXC_PREPARE_PATH_OVERRIDE debug seam, and CommandLineToArgvW-compatible argument quoting.

Manual end-to-end (Windows 11 25H2, pwsh 7.6.2, node v24.13.0):

  • Elevated Test-Phase35-Elevated-v2.ps1 15/15 PASS, including seeded inheritable (OI)(CI)(R) AAP ACE preserved across our precise revoke.
  • T3 cmd / pwsh / node configs on a C:\temp workspace (mkdir + write + recursive enumerate + cleanup) all exit 42 with prep applied. Without prep, cmd's if exist C:\ returns HIDDEN (exit 99); with prep it flips to SEEN (exit 42).

References

Copilot AI review requested due to automatic review settings May 22, 2026 20:08

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

Adds a Windows host-preparation pathway to wxc-exec that can persistently grant (and precisely revoke) minimal metadata-read rights for the well-known AppContainer group SIDs on the system drive root, enabling common toolchains inside AppContainer to successfully stat C:\ without enabling enumeration or data reads.

Changes:

  • Adds wxc-exec --prepare-system-drive and --unprepare-system-drive CLI flags (plus hidden internal elevation helper flags) and routes them to a new Windows-only implementation.
  • Introduces wxc_common::system_drive_prep with a UAC self-elevation flow and tuple-precise revoke behavior, plus unit tests.
  • Refactors filesystem_dacl to expose reusable apply_explicit_ace and adds revoke_specific_aces_for_sid for precise ACE removal; adds PowerShell hand-test helpers and new documentation.
Show a summary per file
File Description
src/wxc/src/main.rs Adds new top-level CLI flags and early-exit dispatch to the host-prep entry points.
src/wxc_common/src/system_drive_prep.rs Implements the Windows-only prepare/unprepare logic, including UAC relaunch, DACL changes, and tests.
src/wxc_common/src/lib.rs Exposes the new system_drive_prep module on Windows builds.
src/wxc_common/src/filesystem_dacl.rs Refactors ACE-apply into apply_explicit_ace and adds tuple-precise revoke helper for SID-specific ACEs.
scripts/host-prep/Prepare-SystemDriveForAppContainer.ps1 Adds an elevated PowerShell script that mirrors the prepare behavior for hand testing / operators.
scripts/host-prep/Unprepare-SystemDriveForAppContainer.ps1 Adds an elevated PowerShell script to precisely remove only the ACEs authored by prepare.
docs/host-prep.md Documents the new flags, the exact rights granted, and validation/diagnostic behavior.
.github/copilot-instructions.md Adds docs/host-prep.md to the “Core references” list for future contributor discovery.

Copilot's findings

  • Files reviewed: 8/8 changed files
  • Comments generated: 2

Comment thread src/wxc_common/src/system_drive_prep.rs Outdated
Comment thread src/wxc_common/src/system_drive_prep.rs Outdated
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase3_5 branch 4 times, most recently from 611a6a1 to 82ab4e9 Compare May 22, 2026 22:53
MGudgin pushed a commit that referenced this pull request May 22, 2026
The `network_path_rejected_e2e` test (added in #295 / Phase 3) calls
`grant_appcontainer_access` on `\\someserver\share\foo` and previously
asserted the error was `NetworkPathRejected` or `PathNotFound`. On CI
agents whose DNS resolves `someserver` (or returns a non-`NotFound`
Win32 error such as `ERROR_BAD_NETPATH` or `ERROR_LOGON_FAILURE`),
`fs::canonicalize` lands in our `Win32 { reason: "canonicalize: …" }`
fallback, which the matcher did not accept — flaking the test.

Broaden the matcher to accept `Win32 { .. }` as well, and convert the
assertion into a `match` that prints the actual variant on failure so
future flakes are diagnostic. The test's intent ("we never silently
succeed on a UNC path") is preserved — the deterministic verification
of the classifier itself lives in
`ensure_local_canonical_prefix_rejects_unc_namespace` and
`ensure_local_canonical_prefix_rejects_raw_unc` above, both
unaffected.

No production behavior change. Discovered while shepherding #389
(Phase 3.5) through CI on an agent that hit this code path
differently than the dev-box agents used during Phase 3's CI run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase3_5 branch from 01e4745 to 82ab4e9 Compare May 22, 2026 23:40
MGudgin added a commit that referenced this pull request May 23, 2026
Add `src/rust-toolchain.toml` pinning the local Rust toolchain to the
same channel CI uses (`ms-prod-1.93` in
`.azure-pipelines/templates/*.Build.Job.yml`, equivalent to public
`1.93`). Without this pin, developers on a newer rustup toolchain
pass local `cargo clippy --all-targets --all-features --release --
-D warnings` and still hit CI failures on lints that were introduced,
downgraded, or differently-defaulted between releases — for example
`clippy::duplicated_attributes` (added in 1.93, weakened in 1.94)
recently bit phase 3.5 (#389) on CI despite running clean locally on
1.94.

The pin is honored automatically by rustup: any `cargo` command from
`src/` (or below) downloads the channel on first use and selects it
thereafter. Opt-out with `cargo +<channel> ...` or
`RUSTUP_TOOLCHAIN=<channel>` for one-off testing.

Also document the pin and the bump procedure in
`.github/copilot-instructions.md` § Prerequisites.

Testing
-------

- `cargo --version` from `src/` reports 1.93.1 after rustup auto-
  syncs the channel on first invocation.
- `cargo clippy --target x86_64-pc-windows-msvc --workspace
  --all-targets --all-features --profile release --locked --
  -D warnings` clean under 1.93 (this is the exact CI invocation
  from `.azure-pipelines/templates/Rust.Build.Job.yml`).

Co-authored-by: Gudge <gudge@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
AppContainer-isolated tools (cmd, pwsh, node) fail at startup or during
canonicalization on hosts where the well-known AppContainer SIDs have
no rights on `C:\`. SeChangeNotifyPrivilege bypass-traverse covers
direct opens but not explicit metadata reads of each path component,
and those are routine: `if exist C:\`, PowerShell drive enumeration,
node.js's `realpathSync` in the module loader.

Add `wxc-exec --prepare-system-drive` / `--unprepare-system-drive` to
add/remove a metadata-only allow ACE (mask `0x00120088` =
`FILE_READ_ATTRIBUTES | FILE_READ_EA | READ_CONTROL | SYNCHRONIZE`,
no inheritance) on the system-drive root for S-1-15-2-1 (ALL
APPLICATION PACKAGES) and S-1-15-2-2 (ALL RESTRICTED APPLICATION
PACKAGES). Self-elevates via UAC. Apply refuses if a same-SID Allow
ACE with a different `(mask, type, inheritance)` already exists rather
than silently coalescing via `SetEntriesInAclW(GRANT_ACCESS)`. Unprep
is tuple-precise: only the ACE prepare would have written is removed,
so same-SID ACEs from other tools survive.

The container can newly probe `C:\` existence, read its attributes,
and read its security descriptor; it still cannot enumerate children
(no `FILE_LIST_DIRECTORY`) or read file contents (no `FILE_READ_DATA`).

Elevation handshake: the parent passes the resolved target path and a
per-invocation `%TEMP%` log path to the elevated child via hidden
`--internal-target-path` / `--internal-log-path` flags, quoted per
`CommandLineToArgvW` rules (trailing backslashes doubled, so `C:\`
survives the spawn). Parent-owned log path also works under
over-the-shoulder UAC where the child runs as a different user.
Child runs hidden; the parent re-emits the log on non-zero exit.

Input validation: `target_path` normalizes `%SystemDrive%` (accepts
`<letter>:` and `<letter>:\`, rejects anything else with a clear
error). `validate_target_path` runs unconditionally before any ACL
operation — in both the elevated-child role and the already-elevated
direct invocation — so a tampered or misconfigured `%SystemDrive%`
can never silently steer an ACL write at the wrong directory.

Scope: grants on the system-drive root only. Empirically enough to
fully unblock cmd, unblock pwsh except for recursive deletes that
walk to `C:\Users`, and unblock node for AppContainer policies rooted
under `C:\` directly. Workloads rooted under `C:\Users\<user>` still
need additional ACEs on `C:\Users` and the profile dir — out of scope
here, tracked separately.

Testing
-------

- `cargo fmt --check`, `cargo check --workspace --all-targets`, and
  `cargo clippy --workspace --all-targets -- -D warnings` clean.
- `cargo test -p wxc_common --lib -- --test-threads=1`: 353/353 pass,
  including 9 tests under `system_drive_prep::tests` covering the
  apply/revoke round-trip, tuple-precise revoke of same-SID priors,
  scan-before-apply refusal on conflicting-mask priors, drive-root
  validation, the `MXC_PREPARE_PATH_OVERRIDE` debug seam,
  `target_path` normalization (canonical / trailing-backslash forms)
  and rejection of non-drive-root values (`C:\\Windows`,
  `\\\\server\\share`, etc.), and the `CommandLineToArgvW`-compatible
  argument quoter.
- Manual end-to-end on Windows 11 25H2 against real `C:\` (elevated
  PowerShell): apply lands two `(Rc,S,REA,RA)` ACEs visible via
  `icacls C:\`, is idempotent on re-apply, and unprep restores the
  pre-test SDDL byte-for-byte. With prep applied, T3 cmd/pwsh/node
  configs running on `C:\temp` workspaces (mkdir + write + recursive
  enumerate + cleanup) all complete exit 42.

See `docs/host-prep.md`; hand-test scripts in `scripts/host-prep/`.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MGudgin MGudgin force-pushed the user/gudge/downlevel_phase3_5 branch from 82ab4e9 to 66953fb Compare May 23, 2026 00:29
MGudgin pushed a commit that referenced this pull request May 23, 2026
The `network_path_rejected_e2e` test (added in #295 / Phase 3) calls
`grant_appcontainer_access` on `\\someserver\share\foo` and previously
asserted the error was `NetworkPathRejected` or `PathNotFound`. On CI
agents whose DNS resolves `someserver` (or returns a non-`NotFound`
Win32 error such as `ERROR_BAD_NETPATH` or `ERROR_LOGON_FAILURE`),
`fs::canonicalize` lands in our `Win32 { reason: "canonicalize: …" }`
fallback, which the matcher did not accept — flaking the test.

Broaden the matcher to accept `Win32 { .. }` as well, and convert the
assertion into a `match` that prints the actual variant on failure so
future flakes are diagnostic. The test's intent ("we never silently
succeed on a UNC path") is preserved — the deterministic verification
of the classifier itself lives in
`ensure_local_canonical_prefix_rejects_unc_namespace` and
`ensure_local_canonical_prefix_rejects_raw_unc` above, both
unaffected.

No production behavior change. Discovered while shepherding #389
(Phase 3.5) through CI on an agent that hit this code path
differently than the dev-box agents used during Phase 3's CI run.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
MGudgin added a commit that referenced this pull request May 23, 2026
The `network_path_rejected_e2e` test (added in #295 / Phase 3) calls
`grant_appcontainer_access` on `\\someserver\share\foo` and previously
asserted the error was `NetworkPathRejected` or `PathNotFound`. On CI
agents whose DNS resolves `someserver` (or returns a non-`NotFound`
Win32 error such as `ERROR_BAD_NETPATH` or `ERROR_LOGON_FAILURE`),
`fs::canonicalize` lands in our `Win32 { reason: "canonicalize: …" }`
fallback, which the matcher did not accept — flaking the test.

Broaden the matcher to accept `Win32 { .. }` as well, and convert the
assertion into a `match` that prints the actual variant on failure so
future flakes are diagnostic. The test's intent ("we never silently
succeed on a UNC path") is preserved — the deterministic verification
of the classifier itself lives in
`ensure_local_canonical_prefix_rejects_unc_namespace` and
`ensure_local_canonical_prefix_rejects_raw_unc` above, both
unaffected.

No production behavior change. Discovered while shepherding #389
(Phase 3.5) through CI on an agent that hit this code path
differently than the dev-box agents used during Phase 3's CI run.

Co-authored-by: Gudge <gudge@microsoft.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@MGudgin MGudgin merged commit 5657c7f into main May 23, 2026
18 checks passed
@MGudgin MGudgin deleted the user/gudge/downlevel_phase3_5 branch May 23, 2026 21:13
MGudgin pushed a commit that referenced this pull request May 23, 2026
This PR removes a couple of Powershell scripts introduced by PR #389 that are
not needed and fixes up docs/code comments that referenced them.
MGudgin pushed a commit that referenced this pull request May 27, 2026
This PR removes a couple of Powershell scripts introduced by PR #389 that are
not needed and fixes up docs/code comments that referenced them.
MGudgin added a commit that referenced this pull request May 27, 2026
This PR removes a couple of Powershell scripts introduced by PR #389 that are
not needed and fixes up docs/code comments that referenced them.

Co-authored-by: Gudge <gudge@microsoft.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants