[LXC] Address file system policy gaps by dhoehna · Pull Request #630 · microsoft/mxc

dhoehna · 2026-07-10T22:56:08Z

Linked work item: AB#62861419 — [LXC] Address file system policy gaps

Summary

Hardens LXC denied-path masking and adds a reusable most-specific-path-wins policy resolver.

Denied-path masking: replaces the is_file() heuristic (which follows symlinks, is TOCTOU-prone, and mis-classifies missing paths) with an explicit type: "file" | "dir" schema discriminator plus a symlink_metadata() fallback that never follows symlinks. A missing path with no explicit type now fails closed with an actionable error.
Most-specific-path-wins resolver: new path_specificity module in wxc_common — deeper paths override shallower ancestors; exact-path ties resolve most-restrictive-wins (deny > readonly > readwrite). Wired into LXC mount emission (shallowest → deepest so the deepest intent wins).

Model / schema

Kept denied_paths: Vec<String> for compatibility; added MaskKind + denied_path_kinds: HashMap<String, MaskKind> (default empty).
Wire deniedPaths now accepts a bare string or { path, type }. Regenerated JSON schema + TS wire types.

Validation

cargo fmt --all -- --check; cargo clippy for wxc_common and (linux target) lxc_common with -D warnings — clean
cargo test -p wxc_common — 398 passed
cargo check --target x86_64-unknown-linux-gnu for wxc_common and lxc_common --tests — pass

Independent of the sibling LXC branches (no shared model fields).

Microsoft Reviewers: Open in CodeFlow

…B#62861419) - Replace is_file() masking heuristic with explicit `type` schema field + symlink_metadata() fallback (no symlink follow; fail-closed on missing+untyped). - Add reusable most-specific-path-wins resolver in wxc_common (deny>ro>rw); wire LXC mounts to emit in specificity order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4

Copilot

Pull request overview

This PR hardens filesystem policy handling by extending the wire format for filesystem.deniedPaths and introducing a reusable “most-specific-path-wins” resolver to produce deterministic mount emission order for Linux backends.

Changes:

Extend deniedPaths to accept either a legacy string or an object { path, type: "file" | "dir" }, and plumb the parsed mask kind into ContainerPolicy.
Add wxc_common::path_specificity to resolve overlapping filesystem intents by specificity (deep overrides shallow; exact ties pick most restrictive).
Update LXC mount emission to use the new resolver and apply explicit denied-path mask kinds.

Reviewed changes

Copilot reviewed 9 out of 10 changed files in this pull request and generated 2 comments.

Show a summary per file

File	Description
src/core/wxc_common/src/wire.rs	Changes wire model so `deniedPaths` supports string or typed object entries.
src/core/wxc_common/src/ts_emit.rs	Updates TS emitter to correctly emit `anyOf` unions and `allOf` wrappers as TypeScript type aliases.
src/core/wxc_common/src/path_specificity.rs	Introduces the new most-specific-path-wins resolver and tests.
src/core/wxc_common/src/models.rs	Adds `MaskKind` and `denied_path_kinds` to `ContainerPolicy` for backend masking decisions.
src/core/wxc_common/src/lib.rs	Exposes the new `path_specificity` module.
src/core/wxc_common/src/filesystem_resolve.rs	Keeps compatibility by re-exporting the resolver from the new module.
src/core/wxc_common/src/config_parser.rs	Converts wire denied-path entries into `denied_paths` + `denied_path_kinds` in the domain policy.
src/backends/lxc/common/src/filesystem_mounts.rs	Switches mount emission ordering to the resolver and adds explicit denied-path masking logic.
sdk/src/generated/wire.ts	Regenerates TS wire types for the new `DeniedPath` union/object forms.
schemas/dev/mxc-config.schema.0.8.0-dev.json	Regenerates the dev schema to reflect the new denied-path shapes.

dhoehna · 2026-07-13T21:08:58Z

+fn observed_mask_path_kind(full_path: &str) -> Result<Option<ObservedMaskPathKind>, String> {
+    match std::fs::symlink_metadata(full_path) {
+        Ok(metadata) => {
+            let file_type = metadata.file_type();
+            if file_type.is_dir() {
+                Ok(Some(ObservedMaskPathKind::Dir))
+            } else if file_type.is_symlink() {
+                Ok(Some(ObservedMaskPathKind::Symlink))
+            } else {
+                Ok(Some(ObservedMaskPathKind::File))
+            }
+        }
+        Err(err) if err.kind() == std::io::ErrorKind::NotFound => Ok(None),
+        Err(err) => Err(format!(
+            "Unable to inspect denied path '{}': {}. Set deniedPaths entry to an object with explicit type \"file\" or \"dir\".",
+            full_path, err
+        )),
+    }
+}


Fixed in b7bfe39. observed_mask_path_kind now takes &Path and inspects the host path (Path::new(host_path)) directly instead of the container rootfs path. Denied paths are host paths, so classifying them off the rootfs path (which doesn't exist before mounts are applied) forced a spurious NotFound and demanded an explicit type. Passing &Path also drops the to_string_lossy() allocation.

dhoehna · 2026-07-13T21:09:00Z

+                let rootfs_base = format!("{}/{}/rootfs", container.lxc_path(), container.name());
+                let full_path = Path::new(&rootfs_base).join(container_path);
+                let explicit = policy.denied_path_kinds.get(host_path).copied();
+                let kind = resolve_mask_kind(
+                    host_path,
+                    explicit,
+                    observed_mask_path_kind(&full_path.to_string_lossy())?,
+                )?;


Fixed in b7bfe39. Mask-kind observation now uses the host path instead of .../rootfs/<container_path>, so host paths that don't yet exist under rootfs (including the temp paths substituted into tests/configs/lxc_filesystem_object.json by tests/scripts/run_lxc_object_test.sh) no longer hard-error.

I also added lookup_mask_kind, which tries an exact match first and then falls back to a trailing-separator–normalized comparison, so an explicit type isn't dropped when the resolved mount path and the configured denied_path_kinds key differ only by a trailing slash. Added unit tests for host-path observation (dir/file/missing) and the normalized lookup.

…h keys Address PR review feedback on denied-path masking: - `observed_mask_path_kind` now takes `&Path` and inspects the *host* path instead of the container rootfs path (`.../rootfs/<container_path>`). The rootfs path does not exist before mounts are applied, so the previous code returned `NotFound` for real host denied paths and spuriously demanded an explicit `type`, breaking existing LXC object-policy validation. Also drops the `to_string_lossy()` allocation by passing `&Path` directly. - Add `lookup_mask_kind` with trailing-separator normalization so an explicit `type` is not dropped when the resolved mount path and the configured `denied_path_kinds` key differ only by a trailing slash. - Add unit tests for host-path observation (dir/file/missing) and normalized kind lookup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SohamDas2021 · 2026-07-13T23:35:06Z

        }
      ]
    },
+    "DeniedPath": {


This will conflict with the bwrap implementation- https://github.com/microsoft/mxc/pull/640/changes#diff-1cd186aad0db50fef812edec1d51621e632e26ed32f8… as I had already mentioned the dependency in the task. There is no need to make this change again. I will merge the bwrap PR first

You may want to base your changes off user/sodas/bwrap-denied-path-type and reuse the schema changes

Schema/wire layer is shared across all backends. This is exactly the shared surface wxc_common exists to own.
You have to educate LXC runner to just use this instead.

I just had a chat with Gudge- "BTW I've blocked #640. I want to do schema work like that separately. And if we're adding file/dir, we should probably think about adding leaf/tree at the same time. They are both on my file system policy backlog."

Let's hold off to making the file vs dir distinction for now in the schema. You can continue to work on the remaining fs gaps.

Done — the file/dir type distinction is dropped from the schema entirely in b0a5766. The generated dev schema (schemas/dev/mxc-config.schema.0.8.0-dev.json) and SDK wire types (sdk/src/generated/wire.ts) are reverted to the #608 base, and deniedPaths is back to bare strings. Agreed on holding off per Gudge — I'll leave the file-vs-dir (and leaf/tree) design to the separate schema work and consume the shared schema/wire surface from #640 / user/sodas/bwrap-denied-path-type once it merges. This PR is now scoped to the remaining fs gap: the most-specific-path resolver. Codegen gates (check-schema-codegen / check-sdk-types-codegen / check-schema-versions / validate-configs) all pass.

SohamDas2021 · 2026-07-13T23:59:11Z

Since this might be touching common code, make sure all lxc and bwrap tests/configs pass

SohamDas2021 · 2026-07-13T23:44:02Z

+#[derive(Debug, Clone, Serialize, Deserialize)]
+#[cfg_attr(feature = "schema-gen", derive(schemars::JsonSchema))]
+#[serde(untagged)]
+pub enum DeniedPath {


Again all these changes are already made in my bwrap PR- #640. You would need to use that.

Removed. All wire.rs changes for the denied-path type union (DeniedPath, MaskKind, denied_path_kinds) are reverted to base in b0a5766, so this no longer duplicates the bwrap #640 wire surface. LXC will consume the shared wxc_common wire types from #640 once it lands rather than redefining them here.

SohamDas2021 · 2026-07-13T23:51:11Z

+    explicit: Option<MaskKind>,
+    observed: Option<ObservedMaskPathKind>,
+) -> Result<MaskKind, String> {
+    if let Some(kind) = explicit {


The Bubblewrap side ( denied_masks_as_file ) does the opposite: when the host path exists, host reality wins and a contradicting declared type is downgraded to a warning- specifically to avoid emitting an invalid mount (e.g. binding /dev/null over a real directory). Here, a config declaring type:"file" on a path that is actually a directory will emit /dev/null … create=file over it. For the same schema field to behave oppositely on LXC vs Bubblewrap is a cross-backend correctness inconsistency.

Can we replicate bwrap behavior here?

Resolved by removal — the divergent LXC mask-kind logic is gone in b0a5766. The denied-path branch is reverted to its original host-reality masking (/dev/null for a file, tmpfs for a directory) with no declared-type override, so there's no longer a cross-backend inconsistency with bwrap's denied_masks_as_file. When the shared file/dir surface arrives via #640, LXC will adopt bwrap's semantics (host reality wins; a contradicting declared type is downgraded to a warning) instead of reintroducing the divergent behavior here.

SohamDas2021 · 2026-07-13T23:56:02Z

+            .then_with(|| b.intent.cmp(&a.intent))
+            .then_with(|| a.sequence.cmp(&b.sequence))
+    });
+    candidates.dedup_by(|a, b| {


In Vec::dedup_by, when the closure returns true, a (the later element) is removed and b is kept; so, mutating a has no effect on the retained element. It's dead code. The "most-restrictive-wins" result only survives because the preceding sort_by already places the most-restrictive entry first (b). Either drop the mutation (rely on the sort) with a comment or mutate b . Please also add a direct test asserting the kept path string on an exact-key conflict across lists that differ only by trailing slash.

Fixed in b0a5766. Dropped the dead mutation of a and now rely on the preceding most-restrictive-first sort_by, with a comment explaining that Vec::dedup_by keeps the earlier element (b) and removes the later a. Added a direct test — exact_key_conflict_keeps_most_restrictive_path_string — asserting the kept path string when two lists collide on the same normalized key but differ only by a trailing slash. Per the module-rename thread this now lives in filesystem_resolve.rs. cargo test -p wxc_common passes (398 tests).

SohamDas2021 · 2026-07-13T23:56:36Z

@@ -0,0 +1,266 @@
+// Copyright (c) Microsoft Corporation.


Renaming a shared module that was introduced in #608, and leaving a permanent compat shim, is avoidable churn and increases conflict surface with the Bubblewrap branches. Prefer extending filesystem_resolve in place.

Reverted the rename. path_specificity.rs and the compat shim are deleted; the enhanced resolver (exact-key dedup + the new kept-path test) now extends filesystem_resolve in place, so #608's module keeps its name and there's no added conflict surface with the Bubblewrap branches. Consumers (LXC filesystem_mounts.rs and bwrap bwrap_command.rs) import from wxc_common::filesystem_resolve unchanged.

Address review feedback on microsoft#630: - Remove the denied-path file/dir `type` discriminator (wire DeniedPath union, MaskKind, denied_path_kinds, regenerated schema + TS wire types, ts_emit anyOf/allOf handling, and the LXC mask-kind logic). Per reviewer guidance the file-vs-dir schema distinction is deferred (to be designed alongside leaf/tree), and the wire change duplicates bwrap microsoft#640. - Keep the most-specific-path-wins resolver but extend `filesystem_resolve` in place instead of renaming it to `path_specificity` with a compat shim, avoiding churn / conflict surface with the bwrap branches. Delete path_specificity.rs and the shim. - Fix the `dedup_by` dead code: the retained element is the earlier `b`, so mutating `a` was a no-op. Rely on the most-restrictive-first sort (with a comment) and add a test asserting the kept path string on an exact-key conflict across lists that differ only by a trailing slash. - Revert LXC denied-path masking to its original behavior while still emitting mounts in resolver (shallow->deep) order. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

SohamDas2021 · 2026-07-14T20:06:00Z

The PR description is stale and still says that you update the schema.

SohamDas2021 · 2026-07-14T20:10:11Z

+/// of the deepest plan entry that is an ancestor of (or equal to) `path`, with
+/// most-restrictive-wins breaking an exact-depth tie. `None` if no entry covers
+/// the path.
+pub fn effective_intent(plan: &[ResolvedMount], path: &str) -> Option<FsIntent> {


unused? Only in tests? Remove

SohamDas2021 · 2026-07-14T20:11:27Z

+                ));
+                container.set_config_item("lxc.mount.entry", &mount_entry)?;
+            }
+            FsIntent::Denied => {


Denied file/dir masking gap unaddressed? still rootfs is_file()?

SohamDas2021 · 2026-07-14T20:13:24Z

+/// Exact same-path conflicts (equal [`PathKey`], including entries that differ
+/// only by a trailing separator) collapse to the single most-restrictive
+/// intent; the surviving entry keeps that intent's original path spelling.
+pub fn resolve_path_plan(


Redundant conflict collapse, duplicates normalize_object_conflicts? Upstream duplicate already run by every runner by calling normalize_object_conflicts

Copilot AI review requested due to automatic review settings July 10, 2026 22:56

dhoehna requested a review from a team as a code owner July 10, 2026 22:56

Copilot started reviewing on behalf of dhoehna July 10, 2026 22:56 View session

Copilot AI reviewed Jul 10, 2026

View reviewed changes

SohamDas2021 reviewed Jul 13, 2026

View reviewed changes

SohamDas2021 requested changes Jul 13, 2026

View reviewed changes

microsoft-github-policy-service Bot assigned dhoehna Jul 13, 2026

microsoft-github-policy-service Bot added the Needs-Author-Feedback Issue needs attention from issue or PR author label Jul 13, 2026

microsoft-github-policy-service Bot added Needs-Attention Issue needs attention from Microsoft and removed Needs-Author-Feedback Issue needs attention from issue or PR author labels Jul 14, 2026

SohamDas2021 requested changes Jul 14, 2026

View reviewed changes

microsoft-github-policy-service Bot added the Needs-Author-Feedback Issue needs attention from issue or PR author label Jul 14, 2026

Uh oh!

Conversation

dhoehna commented Jul 10, 2026 • edited by microsoft-github-policy-service Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Model / schema

Validation

Microsoft Reviewers: Open in CodeFlow

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SohamDas2021 Jul 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SohamDas2021 commented Jul 13, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SohamDas2021 commented Jul 14, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

dhoehna commented Jul 10, 2026 •

edited by microsoft-github-policy-service Bot

Loading

SohamDas2021 Jul 13, 2026 •

edited

Loading