Skip to content

[Bubblewrap/LXC] Address common network policy gaps - model 1 - #631

Closed
Darren Hoehna (dhoehna) wants to merge 9 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-net-model1-ipv6-cidr-port-proto
Closed

[Bubblewrap/LXC] Address common network policy gaps - model 1#631
Darren Hoehna (dhoehna) wants to merge 9 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-net-model1-ipv6-cidr-port-proto

Conversation

@dhoehna

@dhoehna Darren Hoehna (dhoehna) commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

Linked work item: AB#62830559 — [Bubblewrap/LXC] Address common network policy gaps - model 1

Adds IPv6, CIDR, port and protocol filtering to the shared iptables enforcement path (lxc_common::network_iptables), used by the LXC backend and by Bubblewrap's host-level filtering.

How each ask is handled

1. IPv6 + CIDR parsing — fix dual-stack bypass

Working end to end from a config file.

resolve_host is now dual-stack. A host-list entry may be an IPv4/IPv6 literal, an IPv4/IPv6 CIDR block, or a hostname; hostnames resolve to both A and AAAA results. Each destination is routed by address family to either the iptables or the ip6tables chain, which are created and torn down in parallel with matching base rules, default policy and FORWARD hook.

Previously resolve_host kept IPv4 only and dropped every CIDR entry of either family, because a string containing / fails the IpAddr parse and then fails DNS. A dropped entry emits no rule, so it fell through to the default policy: under defaultPolicy: allow a blockedHosts entry was not enforced over IPv6 (the bypass this work item describes), and under defaultPolicy: block an allowedHosts entry was not honoured over IPv6, which broke reachability instead.

CIDR prefixes are validated (<= 32 for IPv4, <= 128 for IPv6). An out-of-range or malformed prefix produces no rule and logs a warning rather than being handed to iptables, which would reject the command and fail the whole apply.

2. Port filtering — add --dport

Implemented in the enforcement path; not reachable from a config file yet.

EgressRule.ports is a Vec<PortSpec>, where a PortSpec is a single port or an inclusive range. TCP/UDP rules emit --dport 443 or --dport 8000:8999; a range whose start equals its end normalizes to a single port. ICMP carries no ports, so the port dimension collapses and no --dport is emitted. A range with start greater than end is rejected before any chain is created, so a deny rule is never silently dropped and there is no partial state to roll back.

3. Protocol filtering — add -p tcp/udp/icmp

Implemented in the enforcement path; not reachable from a config file yet.

EgressRule.protocols emits -p tcp, -p udp, or -p icmp, using ipv6-icmp on the ip6tables chain because ip6tables rejects icmp. Destinations, protocols and ports expand as a cross-product, one rule per combination.

Reachability caveat

Asks 2 and 3 are enforced by ContainerPolicy.egress_rules, and nothing populates that field. wire::Network has no egress surface and carries deny_unknown_fields, so an egressRules key in a config is rejected at parse time; config_parser.rs has no egress branch. Repo-wide, EgressRule and PortSpec appear only in models.rs and network_iptables.rs, and every construction site outside those types is inside #[cfg(test)]. The parser is owned by the schema work (AB#62830582). Until it lands, port and protocol filtering are exercised by unit tests only.

The internal PortSpec shape is also interim: the policy spec expresses ranges as flat ports[].port + ports[].endPort, and reconciling to that belongs with the schema work.

Robustness

  • ip6tables is probed once per apply. On IPv4-only hosts the v6 chain is skipped and the number of unapplied IPv6 rules is logged, instead of failing the whole setup.
  • A failed apply tears down partially created chains and FORWARD hooks, so a retry does not trip over "chain already exists".
  • EgressRule defaults to action: deny.
  • Unresolvable hosts are warned and skipped, since failing name resolution is environmental rather than a malformed configuration.
  • Without a discovered veth the FORWARD hook is skipped rather than applying host-wide rules.

Docs

docs/lxc-support/lxc-backend.md stated that firewall mode was IPv4-only and that rule cleanup depended on a removeRulesOnExit field. Neither was true. That section now matches the code.

Tests

tests/configs/lxc_network_ipv6_cidr.json (IPv6 literals, IPv6 and IPv4 CIDRs) and tests/configs/lxc_network_invalid_cidr.json (out-of-range and malformed prefixes warned and skipped without failing setup), both run by run_lxc_all_tests.sh. tests/configs/bubblewrap_network_ipv6_cidr.json exists but is not yet wired to a script.

Validation

cargo fmt --all -- --check and cargo clippy -p wxc_common -p lxc_common --all-targets -- -D warnings clean. cargo test -p wxc_common 399 passed; cargo test -p lxc_common 61 passed (Linux).

Coupling

EgressRule / Protocol / RuleAction are also introduced by the schema PR (AB#62830582) and by net-model-2 (AB#62830341); merge-time reconciliation is expected. Rule ordering here is the interim allow-before-deny; the spec's deny-precedence is owned by AB#62830341.

Microsoft Reviewers: Open in CodeFlow

…(AB#62830559)

- resolve_host returns dual-stack; add ip6tables v6 chain mirroring the v4 chain.
- CIDR (v4/v6) passthrough; per-rule --dport and -p tcp/udp/icmp via new EgressRule model field.
- Pure rule-builder helpers with unit tests; update legacy IPv6-drop tests to dual-stack.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 3b78bec0-e139-4cfd-9c10-092ef986d4f4
Copilot AI review requested due to automatic review settings July 10, 2026 22:56
@dhoehna
Darren Hoehna (dhoehna) requested a review from a team as a code owner July 10, 2026 22:56

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

This PR extends the LXC firewall enforcement path to support a richer, dual-stack egress network policy model (IPv4 + IPv6), including CIDR destinations and per-rule protocol/port filtering, while preserving the legacy allowed/blocked host lists.

Changes:

  • Added ContainerPolicy.egress_rules with supporting EgressRule, Protocol, and RuleAction types in wxc_common.
  • Updated the LXC iptables enforcement implementation to build parallel iptables + ip6tables chains and to preserve IPv6 literals/AAAA resolution results.
  • Refactored rule construction into helper functions returning argument vectors, with unit tests for the new rule-building behavior.

Reviewed changes

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

File Description
src/core/wxc_common/src/models.rs Introduces the egress_rules policy model and supporting enums/structs on ContainerPolicy.
src/backends/lxc/common/src/network_iptables.rs Implements dual-stack (iptables/ip6tables) rule application, CIDR/protocol/port handling, and adds unit tests for rule argument construction.

Comment thread src/backends/lxc/common/src/network_iptables.rs Outdated
Comment thread src/backends/lxc/common/src/network_iptables.rs Outdated
Comment thread src/backends/lxc/common/src/network_iptables.rs
Comment thread src/backends/lxc/common/src/network_iptables.rs
Comment thread src/backends/lxc/common/src/network_iptables.rs Outdated
Resolves the five Copilot reviewer comments:
- resolve_host doc comment now accurately describes CIDR validation (address
  parses + prefix in range; host bits are not required to be zero since
  iptables/ip6tables apply the mask).
- protocol_arg is address-family aware: IPv6 ICMP rules use `ipv6-icmp`
  (ip6tables rejects `icmp`).
- ICMP rules never emit `--dport`; the port dimension is collapsed for
  portless protocols so no invalid or duplicate rules are generated.
- FORWARD hook and its cleanup now match container egress by input interface
  (`-i <veth>`) instead of `-o <veth>`, and the delete matches the insert.

Adds unit tests for IPv4/IPv6 ICMP protocol naming and ICMP dport suppression.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@SohamDas2021

Copy link
Copy Markdown
Contributor

PR description only calls out LXC- "Adds IPv6, CIDR, port, and protocol filtering to the LXC iptables enforcement path.", but this is for Bwrap as well, please update?

Comment thread src/backends/lxc/common/src/network_iptables.rs Outdated
Comment thread src/backends/lxc/common/src/network_iptables.rs Outdated
Comment on lines +231 to +235
vec![
"-A", chain_name, "-p", "udp", "--dport", "53", "-j", "ACCEPT",
],
vec![
"-A", chain_name, "-p", "tcp", "--dport", "53", "-j", "ACCEPT",

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.

These ACCEPT udp/tcp port 53 to any destination, applied to both chains. Under default: Block, this is a standing DNS-tunneling exfiltration path- exactly the threat the GA doc’s default-deny targets and the PR now extends it to IPv6. Pre-existing for v4, but this is the natural place to tighten.
Suggest: scope the DNS ACCEPT to the container’s configured resolver(s), not 0.0.0.0/0  /  ::/0.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Agreed this is worth tightening, and thanks for flagging it. Two constraints on doing it inside this PR: (1) it's pre-existing v4 behavior, and (2) there is no resolver/nameserver field on ContainerPolicy today, so "the container's configured resolver(s)" isn't expressible yet — scoping the port-53 ACCEPT to specific resolvers needs a new config surface. The default-deny still drops all non-DNS egress and the ACCEPT is limited to udp/tcp dport 53, but I agree it remains a residual DNS-exfil channel.

I'd rather land resolver-scoping as a dedicated follow-up that (a) adds a dnsServers/resolver policy field and (b) scopes both the v4 and v6 port-53 ACCEPTs to it, instead of hard-coding a resolver here. Leaving this open to track it — I can file it as its own work item if you'd like.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up: partially addressed by #632, though not for every mode, so I want to be precise about what is and isn't closed.

Closed in proxy mode. #632 resolves the proxy host once on the host and injects that literal address into HTTP(S)_PROXY, so the container never needs a resolver. Port 53 is therefore not opened at all under deny-all-except-proxy (let allow_dns = !proxy_enabled;), and the standing DNS-tunnel path you described does not exist in that posture.

Still open in the mode this PR touches. defaultPolicy: block with hostname allow/block lists and no proxy still ACCEPTs udp/tcp 53 to any destination, because the container does its own name resolution and there is no resolver field on ContainerPolicy to scope the rule to. Scoping it needs a new policy surface (dnsServers or equivalent) threaded through wire → parser → ContainerPolicy → iptables — a schema change I'm holding until the GA schema lands in main, so it isn't something I can add here.

Happy to file that as a tracked work item now if you'd like it separate from #655.

Comment thread src/core/wxc_common/src/models.rs Outdated
.map(|ip| ip.to_string())
.collect(),
Err(_) => Vec::new(),
fn build_policy_rule_args(chain_name: &str, policy: &ContainerPolicy) -> FirewallRuleArgs {

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.

allow-before-deny ordering has no deny-precedence. build_policy_rule_args appends allowed before blocked, then egress_rules in author order. With
iptables first-match-wins, an IP in both lists is ACCEPTed, and egress_rules carry no deny priority, contrary to GA D4 (deny-wins). This is captured in #62830341, but since this PR introduces the mixed allow/deny model, flag it so the ordering is reconciled rather than assumed correct in the interim.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Flagged in 96af8f9: added a NOTE on build_policy_rule_args documenting that rules are emitted allow-list -> block-list -> egress_rules (author order) and applied first-match-wins, so this model-1 change does not implement deny-precedence — a destination present in both lists is ACCEPTed, and egress_rules carry no deny priority. Reconciling to the GA "deny-wins" ordering across the combined allow/deny model stays owned by net-model-2 (AB#62830341), as noted in the PR's Coupling section. Leaving this thread open so the interim behavior stays tracked until that reconciliation lands.

@dhoehna Darren Hoehna (dhoehna) Jul 30, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Follow-up: the deny-precedence reconciliation this thread was tracking is implemented in #632 (AB#62830341). Correcting an earlier version of this comment: #632 is still open, so it has not landed yet.

build_ordered_egress_rules there emits the block list before the allow list through a pure ordered builder, so under first-match-wins a destination present in both lists is DROPped rather than ACCEPTed — the inverse of the interim behavior the NOTE on this line documents. Locked in by ordered_egress_rules_put_deny_before_allow.

The NOTE is accurate for this branch. #631 and #632 edit the same function, so whichever lands second carries the merge and the deny-first builder replaces the note at that point. Nothing further to change here.

@SohamDas2021

Copy link
Copy Markdown
Contributor

Lets add a test config jsons for bwrap and lxc that exercises ipv6 filtering and CIDR ranges.

Darren Hoehna (dhoehna) and others added 2 commits July 15, 2026 13:10
…-proto

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: e5d2aa5b-7f04-4e4d-83d3-a02efe7020ab
… fail-closed egress default

- network_iptables: probe ip6tables once and skip the parallel v6 chain
  (with a warning) when the binary is absent or IPv6 is disabled, so
  IPv4-only hosts no longer fail the whole firewall setup.
- network_iptables: roll back partially-created chains/FORWARD hooks on
  any apply error so a retry does not hit "chain already exists" and leak
  state; share the teardown path with remove_firewall_rules.
- models: EgressRule default action is now Deny (fail-closed) so an
  under-specified egress rule cannot silently widen access.
- network_iptables: document the interim allow-before-deny ordering
  (deny-precedence reconciliation tracked by AB#62830341).
- tests: add lxc + bubblewrap network configs exercising IPv6 and CIDR
  host filtering.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@microsoft-github-policy-service 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 16, 2026
@dhoehna

Copy link
Copy Markdown
Contributor Author

Soham Das (@SohamDas2021) thanks for the review — addressed the two non-inline items in 96af8f9:

Re: "PR description only calls out LXC ... but this is for Bwrap as well"
Updated the PR description. The summary now states the change applies to the shared iptables/ip6tables enforcement path used by both the LXC and Bubblewrap backends (Bubblewrap reuses lxc_common::network_iptables::NetworkIptablesManager), and I added a "Review follow-ups" section documenting this round's fixes.

Re: "add test config jsons for bwrap and lxc that exercises ipv6 filtering and CIDR ranges"
Added two configs that exercise IPv6 literals + IPv4/IPv6 CIDR ranges in both the allow and block lists:

  • tests/configs/lxc_network_ipv6_cidr.json
  • tests/configs/bubblewrap_network_ipv6_cidr.json

Both validate against the dev schema (node scripts/versioning/validate-configs.js passes). Note they drive IPv6/CIDR through allowedHosts / blockedHosts (which this PR extends to retain IPv6 literals and pass CIDRs through by family) rather than the richer egressRules block — the JSON parser for egressRules is intentionally owned by the schema PR (AB#62830582), so it isn't wired into config parsing on this branch yet.

Also pushed the three inline fixes (ip6tables probe, partial-apply rollback, fail-closed EgressRule default) and flagged the allow-before-deny ordering in code; replies are on the individual threads.

@dhoehna

Copy link
Copy Markdown
Contributor Author

🧪 Local test re-verification — 2026-07-17

Re-ran the test suites locally at branch tip 96af8f9 on a dev workstation. All green (0 failures).

Windows host (x86_64-pc-windows-msvc, cargo 1.96.1):

  • cargo test -p wxc_common396 passed, 0 failed

Linux (WSL2 Ubuntu-24.04, x86_64-unknown-linux-gnu, cargo 1.97.0, isolated CARGO_TARGET_DIR):

  • cargo test -p lxc_common50 passed, 0 failed — the iptables/ip6tables rule-builder enforcement ran on Linux (the original Validation only cargo check-compiled it for the Linux target).

Note: the wxc_common count differs slightly from the original Validation because the branch advanced since it was written.

The 'SDK Integration Tests (linux)' job failed on the pre-existing proxy test 'should enforce allowedHosts at the proxy layer' because the allowed-host sentinel 'curl https://api.github.com/zen' hit a GitHub API 403 rate-limit on the shared runner IP (two earlier tests in the same job fetched the same URL successfully). This is environmental and unrelated to this PR, which only touches the iptables/ip6tables enforcement path (models.rs is additive-only; the proxy allowlist path is untouched). Empty commit to re-run CI on a fresh runner.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@dhoehna

Copy link
Copy Markdown
Contributor Author

🔁 PR processing — 2026-07-20 (branch tip df26d9a)

Review threads: all inline threads from the round-2 CHANGES_REQUESTED review are addressed and pushed (0b8560b, 96af8f9). Two threads remain intentionally open as tracked follow-ups: the DNS port-53 ACCEPT scoping (needs a resolver / dnsServers policy field that does not exist yet) and the allow-before-deny deny-precedence ordering (owned by net-model-2, AB#62830341, and documented in build_policy_rule_args).

Red check SDK Integration Tests (linux) — environmental, not a regression

It failed on the pre-existing proxy test should enforce allowedHosts at the proxy layer:

AssertionError: missing SENTINEL_OK in: curl: (22) The requested URL returned error: 403
BLOCKED_OK

The allowed-host sentinel curl -fsSL https://api.github.com/zen got an HTTP 403 from GitHub's API — the unauthenticated 60-requests/hour per-IP rate limit — on the shared runner. Evidence it is not caused by this PR:

  • The two preceding proxy tests in the same job fetched the same api.github.com/zen URL successfully (200 → PROXY_OK / BUILTIN_OK); the third hit tipped the per-IP limit, so the proxy allowlist and connectivity themselves are fine.
  • This PR's diff is 4 files (network_iptables.rs, models.rs, two new test configs). models.rs is additive-only (allowed_hosts is byte-for-byte unchanged; the new egress_rules field is not wired into JSON parsing on this branch yet), and the builtin-test-server proxy path (linux_test_proxy, bwrap_runner, SDK TS) is untouched.
  • The failing test file (linux-bubblewrap.test.ts) is not part of this PR.

I do not have actions:write on this repo to re-run the failed job, so I pushed an empty commit (df26d9a) to re-trigger CI on a fresh runner. Hardening that proxy test so its allowed-host sentinel does not depend on the rate-limited api.github.com/zen endpoint is worth a separate test-infra follow-up rather than bundling it into this networking PR.

Local re-validation (Windows, branch tip)

  • cargo fmt --all -- --check — clean
  • cargo test -p wxc_common396 passed, 0 failed

@dhoehna

Copy link
Copy Markdown
Contributor Author

✅ CI is green after the re-trigger — the SDK Integration Tests (linux) job passed on the fresh run (all 20 checks: 19 success, 1 skipped CodeQL, 0 failing). This confirms the earlier failure was the transient api.github.com/zen rate-limit and not a regression from this PR. Ready for re-review.

Darren Hoehna (dhoehna) added a commit to dhoehna/mxc that referenced this pull request Jul 30, 2026
…proxy, DNS closed

Firewall setup could fail partway and leave orphan chains, only filtered
IPv4, opened DNS to the whole internet, and hooked FORWARD on the wrong
direction. Each of those either breaks a retry or defeats the
deny-all-except-proxy posture this PR exists to add.

- Roll back partial state. apply_firewall_rules now wraps a fallible inner
  body and, on failure, flushes and deletes whatever was created via a
  shared teardown_chains. Previously remove_firewall_rules short-circuited
  on rules_applied == false, so a mid-setup failure left the chain behind
  and every later attempt failed on -N ("chain already exists").

- Mirror the chain into ip6tables. Without a v6 chain a dual-stack
  container reached the internet over IPv6 while the v4 chain dropped
  everything -- a straight bypass of model 2. All destination rules stay
  IPv4 (resolve_host keeps only A records), so the v6 chain carries the
  base rules plus the same closing stance. Guarded by an ip6tables -S
  probe: IPv4-only hosts warn and continue rather than failing a policy
  that worked before, and such a host has no v6 egress to leak.

- Keep DNS closed in proxy mode. The proxy host is now resolved once on
  the host and the resulting literal is injected into HTTP(S)_PROXY, so
  the container never needs a resolver. This also makes the ACCEPT rule
  and the container's proxy setting name the same endpoint; previously
  the container re-resolved the hostname and could pick a different
  address under round-robin or split-horizon DNS and be dropped by its
  own policy. An unresolvable proxy host now fails setup instead of
  silently producing a chain that drops everything.

- Hook FORWARD with -i, not -o. Container-originated packets arrive at
  the host on the host-side veth, so egress matches -i; -o matched
  traffic toward the container, leaving egress unfiltered. The teardown
  -D uses -i for the same reason, or the hook leaks. Same fix microsoft#631 made
  in 96af8f9.

- Proxy mode now closes with DROP regardless of defaultPolicy, so an
  explicit defaultPolicy=allow cannot reopen the chain.

Known gap, deliberately not addressed here: with defaultPolicy=allow and
blockedHosts, those hosts remain reachable over IPv6. Blocking them needs
AAAA resolution, which belongs with the IPv6/CIDR work in microsoft#631.

Tests: 392 pass on Linux (cargo test -p lxc_common -p wxc_common),
clippy -D warnings clean, rustfmt clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
tests/configs/lxc_network_ipv6_cidr.json was added in 96af8f9 but nothing
ever executed it. Add run_lxc_network_ipv6_cidr_test.sh and register it in
run_lxc_all_tests.sh so the IPv6-literal, IPv6-CIDR and IPv4-CIDR entries
are actually exercised.

The script asserts on lxc-exec output that no host or CIDR entry failed to
resolve, that no iptables/ip6tables rule was rejected (which is how a
v4/v6 address-family routing mistake surfaces, since a rejected rule aborts
firewall setup), that the default DROP policy was applied, and that the
IPv6 rules were not silently skipped.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
Copilot AI review requested due to automatic review settings July 31, 2026 17:56
@dhoehna

Copy link
Copy Markdown
Contributor Author

Re: "Lets add a test config jsons for bwrap and lxc that exercises ipv6 filtering and CIDR ranges" — following up, because my earlier reply overstated this.

Both config files went in with 96af8f9, but nothing ever ran them. tests/configs/ is consumed by explicit filename from the runner scripts, never globbed, so a config that no script names is inert. Adding the JSON satisfied the letter of the ask and not the point of it. That's on me.

Fixed for LXC in 1c794d7: new tests/scripts/run_lxc_network_ipv6_cidr_test.sh, registered in run_lxc_all_tests.sh, so tests/configs/lxc_network_ipv6_cidr.json is now actually executed. The config exercises IPv6 literals (2606:50c0:8000::153, fe80::1), IPv6 CIDRs (2606:50c0::/32, 2001:db8::/32) and IPv4 CIDRs (140.82.112.0/20, 10.0.0.0/8).

The script asserts four things on the run output:

Assertion Catches
no could not resolve host a CIDR or IPv6 literal that never parsed
no <ip[6]tables> ... failed: / Firewall setup failed: a rule the kernel rejected — this is where a v4/v6 address-family routing mistake surfaces, since run_firewall_command returns Err and aborts setup when a rule is rejected
Default network policy: DROP present the firewall path ran to completion
no IPv6 firewall rule(s) not applied the v6 half genuinely ran instead of being skipped for an unusable ip6tables

Grepping output rather than asserting on rule text is deliberate: run_firewall_command only logs on failure, so there is no per-rule success line to match against.

Bubblewrap is intentionally not covered here. tests/configs/bubblewrap_network_ipv6_cidr.json exists but is still not executed by any runner — I'm keeping this change LXC-only rather than widening it. Happy to wire the bwrap side up in a follow-up if you'd rather it land now; say the word.

The other item from your review — the description claiming this was LXC-only when Bubblewrap shares lxc_common::network_iptables::NetworkIptablesManager — was fixed in the description back in 96af8f9 and still reads correctly.

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

Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.

Suppressed comments (5)

src/backends/lxc/common/src/network_iptables.rs:292

  • Invalid destinations are silently converted into an empty rule set. For a deny rule under a default-allow policy, a typo such as an out-of-range CIDR prefix therefore drops the restriction without any error or warning. Since EgressRule.destinations accepts arbitrary strings, policy application should reject an invalid destination rather than treating it as no-op.
        let Some(family) = Self::destination_family(destination) else {
            return FirewallRuleArgs::default();
        };

src/core/wxc_common/src/models.rs:599

  • The new field is not included in Bubblewrap's needs_iptables_rules or bwrap_command host-rule checks, both of which inspect only allowed_hosts/blocked_hosts. A Bubblewrap request containing only egress_rules therefore never invokes this shared firewall path (and default-block may select --unshare-net instead), so the new port/protocol rules do not apply to Bubblewrap as claimed. Plumb this field through those checks and add an executed Bubblewrap test.
    pub egress_rules: Vec<EgressRule>,

tests/configs/bubblewrap_network_ipv6_cidr.json:4

  • This config is not referenced by any test script or by run_bwrap_all_tests.sh; repository search finds only the file itself. Consequently it does not exercise Bubblewrap IPv6/CIDR filtering as stated in the PR validation notes, and regressions in the Bubblewrap path remain undetected.
  "containment": "bubblewrap",

tests/configs/bubblewrap_network_ipv6_cidr.json:10

  • This configuration cannot be enforced by the Bubblewrap path described in the PR. Bubblewrap never calls set_veth_interface, while apply_firewall_rules_inner only installs a FORWARD hook when that field is set and otherwise returns success; with host lists present, Bubblewrap also omits --unshare-net. The sandbox therefore shares the host network while the newly built IPv4/IPv6 chains are unreachable, so this does not provide Bubblewrap IPv6/CIDR filtering.
    "defaultPolicy": "block",
    "enforcementMode": "firewall",

src/backends/lxc/common/src/network_iptables.rs:672

  • teardown_chains discards every ip6tables/iptables error, after which this method clears rules_applied and returns Ok(()). A transient failure (for example the xtables lock being held) can therefore leave a FORWARD hook and chain installed permanently, while both the caller and Drop believe cleanup completed and never retry. Propagate cleanup failure and retain the applied state until all owned hooks/chains are removed.
        self.teardown_chains(logger);

        self.rules_applied = false;
        Ok(())

Comment on lines +584 to +589
} else if !policy_rules.ipv6.is_empty() {
logger.log_line(&format!(
"Warning: {} IPv6 firewall rule(s) not applied because ip6tables \
is unavailable; IPv6 egress is unfiltered on this host.",
policy_rules.ipv6.len()
));
@@ -0,0 +1,29 @@
{
"version": "0.4.0-alpha",
"Firewall setup failed: {}. Cleaning up partial iptables state.",
e
));
self.teardown_chains(logger);
…k docs

Completes the remaining asks on the LXC network-policy work item.

Port ranges: EgressRule.ports becomes Vec<PortSpec>, an untagged enum
accepting a bare port number or an inclusive { start, end } object. The
LXC backend emits --dport start:end, normalizes start == end to a single
port, and skips selectors with start > end after logging a warning, since
passing them to iptables would fail the whole apply. ICMP port collapsing
is unchanged.

Tests: an integration test asserting invalid CIDR entries are warned and
skipped without failing firewall setup, and the IPv6/CIDR test now checks
each configured host individually and stays in sync with its config.

Docs: lxc-backend.md claimed firewall mode was IPv4-only and that cleanup
depended on a removeRulesOnExit field. Neither is true. It now describes
dual-stack chains, CIDR and hostname handling, the ip6tables-unavailable
and missing-veth fallbacks, and automatic teardown.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
Copilot AI review requested due to automatic review settings July 31, 2026 19:48

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

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

Suppressed comments (3)

src/backends/lxc/common/src/network_iptables.rs:615

  • When ip6tables is missing, this returns success while leaving IPv6 completely outside the policy. That also bypasses a defaultPolicy: block even when there are no explicit IPv6 rules, and a missing userspace binary does not prove the host IPv6 stack is disabled. Firewall setup should fail when IPv6 can be routed but cannot be filtered, or the sandbox must explicitly disable IPv6; warning and continuing is fail-open.
        } else if !policy_rules.ipv6.is_empty() {
            logger.log_line(&format!(
                "Warning: {} IPv6 firewall rule(s) not applied because ip6tables \
                 is unavailable; IPv6 egress is unfiltered on this host.",
                policy_rules.ipv6.len()

src/backends/lxc/common/src/network_iptables.rs:560

  • Rollback also runs when the first -N fails, before this attempt has created anything. Because chain names are deterministic and truncated to 20 sanitized characters, a collision or concurrent run can make -N report an existing active chain; teardown_chains then flushes/deletes that other sandbox's firewall state. Track which chain/hooks this apply actually created and roll back only those artifacts.
                logger.log_line(&format!(
                    "Firewall setup failed: {}. Cleaning up partial iptables state.",
                    e
                ));
                self.teardown_chains(logger);

src/backends/lxc/common/src/network_iptables.rs:401

  • Silently omitting an invalid port range can weaken a deny policy: with defaultPolicy: allow, a deny rule whose only selector has start > end produces no rule and the forbidden traffic remains allowed. Structured egress selectors should be rejected during validation/apply rather than logged and skipped.
    /// Invalid port ranges are skipped instead of being passed to iptables,
    /// which would reject the command and roll back the whole firewall apply.

Comment on lines +632 to +635
// Hook the chains into FORWARD for the container's egress traffic.
// Packets originating in the container arrive at the host on the
// host-side veth, so they match FORWARD by input interface (`-i`);
// `-o` would instead match traffic flowing toward the container.
pub allow_local_network: bool,
pub allowed_hosts: Vec<String>,
pub blocked_hosts: Vec<String>,
pub egress_rules: Vec<EgressRule>,
Skipping a malformed port selector is unsafe for a Deny rule: the traffic
it was meant to block falls through to the default policy, which silently
widens access under defaultPolicy: allow. Skipping an Allow rule is
fail-closed, so the previous behavior was asymmetric.

apply_firewall_rules now validates port selectors before creating any
chain, so a malformed policy fails before it mutates host firewall state
and there is nothing to roll back. This matches the sandbox policy spec,
which rejects a configuration a backend cannot enforce rather than running
it advisory.

PortSpec keeps its interim { start, end } shape; the spec expresses ranges
as flat port + endPort, and reconciling to that belongs with the schema
work that adds a parser for egress_rules.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
Copilot AI review requested due to automatic review settings July 31, 2026 20:05
@dhoehna

Copy link
Copy Markdown
Contributor Author

Scope note: more changes will arrive here after the GA network schema lands.

Everything in this PR that does not depend on the GA spec is done. What is deliberately deferred:

  • Port range shape. The policy spec expresses ranges as flat ports[].port + ports[].endPort (Kubernetes endPort style). This PR carries an interim internal PortSpec (443 or { start, end }) because there is no parser for egress_rules yet. Once the schema PR (AB#62830582) adds one, PortSpec should be reconciled to port + endPort rather than kept alongside it.
  • endPort is spec'd but implemented nowhere. EgressPortWire and EgressPort currently carry only protocol + port, both with deny_unknown_fields, so an endPort in a config is a hard parse error today. That gap is in the schema work, not here.
  • Rule ordering. Interim allow-before-deny; the spec's deny-precedence is owned by AB#62830341.
  • Ingress. ingress.hostLoopback is GA surface and is not touched by this PR.

One thing that did not need to wait: an inverted port range is now rejected before any chain is created, rather than being skipped. Skipping was fail-closed for an allow rule but fail-open for a deny rule, and the spec's position is that a configuration a backend cannot enforce is rejected rather than run advisory.

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

Copilot reviewed 9 out of 9 changed files in this pull request and generated 1 comment.

Suppressed comments (4)

src/backends/lxc/common/src/network_iptables.rs:626

  • This path knowingly continues with IPv6 egress unfiltered. The probe conflates a genuinely IPv4-only kernel with a missing ip6tables binary, permission/lock failures, and other transient errors; on an IPv6-capable host, defaultPolicy: block and IPv6 deny rules then fail open. Distinguish “IPv6 disabled” from tooling failure and reject the policy (or disable IPv6 for the sandbox) whenever requested IPv6 enforcement cannot be installed.
        if ipv6_enabled {
            Self::run_ip6tables_rule_args(&policy_rules.ipv6, logger)?;
        } else if !policy_rules.ipv6.is_empty() {
            logger.log_line(&format!(
                "Warning: {} IPv6 firewall rule(s) not applied because ip6tables \
                 is unavailable; IPv6 egress is unfiltered on this host.",
                policy_rules.ipv6.len()
            ));

tests/configs/lxc_network_ipv6_cidr.json:2

  • This test config is rejected before the firewall code runs: the parser currently accepts schema versions >=0.6, <=0.8 (config_parser.rs:224-266). Consequently this new test always fails at its default-policy assertion instead of exercising IPv6/CIDR rules. Use a supported version.
  "version": "0.4.0-alpha",

src/backends/lxc/common/src/network_iptables.rs:568

  • Rollback is unconditional even when the first -N failed because the chain already existed. Chain names are derived from only the first 20 sanitized container-name characters (lines 62-71), so a collision or concurrent reuse can make this path flush/delete another live sandbox's chain and remove its hook. Track which chains/hooks this apply actually created and tear down only those; never treat pre-existing state as owned by this manager.
                logger.log_line(&format!(
                    "Firewall setup failed: {}. Cleaning up partial iptables state.",
                    e
                ));
                self.teardown_chains(logger);

tests/configs/lxc_network_invalid_cidr.json:2

  • This config uses an unsupported schema version. validate_schema_version accepts only >=0.6, <=0.8 (config_parser.rs:224-266), so the invalid-CIDR test exits during parsing and never reaches the warning/setup behavior it intends to verify. Use a supported version.
  "version": "0.4.0-alpha",

Comment on lines +291 to +293
let Some(family) = Self::destination_family(destination) else {
return FirewallRuleArgs::default();
};
@dhoehna

Copy link
Copy Markdown
Contributor Author

Soham Das (@SohamDas2021) — coverage against the three asks in AB#62830559, plus the open questions I would like a decision on. Line numbers are on 2e1a249.

How each ask is covered

Ask 1 — "IPv6 + CIDR parsing – fix dual-stack bypass"

"Add ip6tables rules alongside iptables so IPv6 destinations are filtered. Today NetworkIptablesManager only resolves to IPv4 — on dual-stack hosts, traffic to the same destination over IPv6 bypasses the firewall entirely. Also add proper CIDR parsing (e.g. /20 subnet ranges) instead of single-IP resolution."

Covered, and reachable from a config file today.

  • resolve_host (network_iptables.rs:128-165) returns a ResolvedDestinations { ipv4, ipv6 } bucket. A CIDR string is routed through destination_family; a bare literal is binned by IpAddr variant; a hostname goes through to_socket_addrs() and both A and AAAA results are kept.
  • destination_family (:170-186) validates the prefix — IPv4 iff <= 32, IPv6 iff <= 128 — and rejects empty or malformed prefixes. The CIDR string is passed through verbatim in -d; host bits need not be zero because iptables applies the mask (:118-127).
  • A parallel ip6tables chain is created, populated and torn down alongside the v4 chain, with its own FORWARD hook (:596-603, :641-663).
  • Config path: allowedHosts / blockedHosts (wire.rs:274-289) → config_parser.rs:786-820build_policy_rule_args (:436-449).

On the bypass framing: it holds exactly for blockedHosts under defaultPolicy: allow — the v4 DROP was emitted, the v6 DROP was not, so IPv6 hit the default ACCEPT. Under defaultPolicy: block the same defect showed up as breakage rather than bypass: the v4 ACCEPT landed, the v6 ACCEPT did not, so IPv6 hit the default DROP. Worth noting the CIDR half was never IPv6-specific — a / fails the IpAddr parse and then fails DNS, so IPv4 CIDRs were dropped too. Both are fixed.

Ask 2 — "Port filtering – add --dport"

"Add --dport to iptables rules so allow/deny can be scoped to specific destination ports or port ranges. Today rules match all ports for a given IP — allowing an IP on port 443 also allows it on every other port."

Enforcement implemented, including ranges. Not reachable from a config file yet — see below.

  • PortSpec (models.rs:404-407) is a single port or an inclusive range; iptables_dport_arg (:417-424) emits 443 or 8000:8999, and normalizes Range { 443, 443 } to 443.
  • --dport is appended only when the protocol supports ports (network_iptables.rs:344-370, guard at :362-368).
  • validate_port_selectors (:409-419) rejects start > end at :591, before the first -N at :596 — so a malformed range fails with no host state to roll back.

Ask 3 — "Protocol filtering – add -p tcp/udp/icmp"

"Add -p tcp/udp/icmp to iptables rules so allow/deny can be scoped to a specific transport protocol. Today rules match all protocols — an allow rule intended for TCP 443 also permits UDP and ICMP to the same destination."

Enforcement implemented. Not reachable from a config file yet.

  • protocol_arg (:195-206) maps to tcp / udp / icmp, and to ipv6-icmp on the v6 chain because ip6tables rejects icmp.
  • Destinations x protocols x ports expand as a cross-product (:284-336). ICMP collapses the port dimension to a single portless rule (:315-323), so an ICMP rule with two ports emits one rule, not two.

The caveat on asks 2 and 3

Both are driven by ContainerPolicy.egress_rules, and nothing populates that field. wire::Network has no egress surface and carries deny_unknown_fields (wire.rs:273-274), so an egressRules key in a config is rejected at parse time. Repo-wide, EgressRule and PortSpec appear in only two files, and every construction site is inside #[cfg(test)]. The parser is owned by AB#62830582, so until it lands these two asks are enforced but not configurable.


Open questions

  1. endPort has two different failure modes. The spec documents it (sandbox-policy/v2/networking.md:159), but EgressPortWire (wire.rs:395-406) has no end_port and deny_unknown_fields, so endPort in a config is a hard parse error. EgressPort (mxc_engine/src/policy.rs:509-518) also lacks the field but has no deny_unknown_fields, so it silently ignores it. Which is intended for GA, and should the spec carry a "not yet implemented" note until a range shape lands?

  2. Interim PortSpec shape. This PR uses Single(u16) | Range { start, end }; the spec uses flat port + endPort. Is landing the interim shape acceptable, and where is the bridging parser tracked?

  3. Deny-precedence. This PR ships allow-before-deny with a NOTE at network_iptables.rs:423-434; spec decision D4 (networking.md:197-201) requires deny to win. build_ordered_egress_rules is not on this branch. OK to merge as a documented interim step behind AB#62830341, or should this gate?

  4. Ingress is not implemented anywhere. The spec says ingress.hostLoopback is enforced on LXC/bwrap via the iptables INPUT chain (networking.md:148, :181, :289). Grep for INPUT / hostLoopback in the LXC and Bubblewrap backends returns zero. The chain here is hooked only into FORWARD with -i <veth>. Is that a tracked GA gap? I did not see a roadmap item for it.

  5. Outbound ICMP is dropped under a block policy, both families. build_base_chain_rule_args (:219-240) emits exactly four rules — lo, ESTABLISHED,RELATED, UDP 53, TCP 53 — with no ICMP allowance, and takes no family parameter, so the identical set goes to both tables. Because the chain is egress-only, NDP and inbound PTB do not traverse it, so I do not think this is an RFC 4890 correctness bug. But outbound ICMPv6 (including ping6 and any error the container emits) is dropped. Do you want an ICMPv6 allow-list for error types 1-4, or is symmetry with IPv4 the intended posture? Either way I would like it recorded rather than incidental.

  6. Hostname re-resolution (roadmap Grant AppContainer access to NUL device for runtime stdio initialization #23) is untouched. Resolution happens once at policy install time (:158-165). Making it dual-stack does not change that — a DNS answer that moves after install still bypasses the rules. Flagging only so Grant AppContainer access to NUL device for runtime stdio initialization #23 isn't assumed covered by this PR.

  7. tests/configs/bubblewrap_network_ipv6_cidr.json is not wired to any script. Nothing in the repo references it. Should a bwrap runner land here mirroring the LXC one, or should the fixture wait?

  8. The LXC shell tests are not in CI. They require root plus liblxc (run_lxc_all_tests.sh:6-9) and drive the real lxc-exec; no pipeline invokes them. They also assert on emitted firewall rules rather than container reachability, by design. Is a Linux CI leg planned, and do you want reachability coverage before GA?

@dhoehna

Copy link
Copy Markdown
Contributor Author

Closing this in favour of #724.

This PR mixed two kinds of work:

  1. IPv6 + CIDR filtering — reachable from the current config schema (allowedHosts / blockedHosts) and working today.
  2. Port and protocol filtering — an EgressRule / PortSpec model added to models.rs plus the iptables --dport / -p machinery to consume it.

The second part was not reachable. Nothing populated ContainerPolicy.egress_rules: wire.rs had no corresponding field, so config_parser.rs had nothing to translate, and the vector was empty at runtime. Every construction site was inside #[cfg(test)]. Expressing a port needs structured egress rules in the schema, which is AB#62830582 — and the GA wire schema is not in main (landed in #676, reverted in #707).

Worse, the internal shape I guessed did not match the GA one. EgressRuleWire pairs {protocol, port} per selector; my model held independent protocols and ports vectors that were cross-producted. Filling those from a GA rule listing [{tcp,443},{udp,53}] would have emitted four rules — including tcp/53 and udp/443 — silently widening the policy. That is a fail-open bug, and it would only have been caught when the parser was eventually written.

So rather than keep speculative code in the tree, #724 is branched fresh from main and carries only the IPv6/CIDR work, its tests, and its test configs. models.rs, wire.rs, and config_parser.rs are untouched there.

Port and protocol filtering will come back after AB#62830582 lands, built against the real schema shape instead of a guess.

The open questions raised above are still open and are carried into #724.

@microsoft-github-policy-service microsoft-github-policy-service Bot removed the Needs-Attention Issue needs attention from Microsoft label Jul 31, 2026
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