Skip to content

[LXC] Filter IPv6 destinations and CIDR ranges in firewall mode (AB#62830559) - #724

Open
Darren Hoehna (dhoehna) wants to merge 14 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-net-ipv6-cidr
Open

[LXC] Filter IPv6 destinations and CIDR ranges in firewall mode (AB#62830559)#724
Darren Hoehna (dhoehna) wants to merge 14 commits into
microsoft:mainfrom
dhoehna:user/dahoehna/lxc-net-ipv6-cidr

Conversation

@dhoehna

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

Copy link
Copy Markdown
Contributor

Linked work item: AB#62830559 — LXC IPv6 + CIDR destination filtering in firewall mode

Summary

In firewall mode (enforcementMode: "firewall"), the LXC backend resolved allowedHosts / blockedHosts to IPv4 only, so IPv6 destinations were silently unfiltered and any CIDR entry (a string containing /) was discarded without becoming a rule. This PR resolves and programs both address families and forwards validated CIDRs verbatim to iptables / ip6tables, closing roadmap item 19 (IPv6 + CIDR parsing). Bubblewrap picks this up too, because it delegates to the same manager.

  • Hostnames now resolve to both A and AAAA records; IPv4 rules go to iptables, IPv6 rules to ip6tables, each with its own per-container chain and FORWARD hook.
  • Validated CIDRs (140.82.112.0/20, 2606:50c0::/32) are forwarded verbatim; the prefix must be ASCII digits within family range (<=32 v4, <=128 v6).
  • Empty or whitespace-only entries resolve to nothing, avoiding a Winsock ":0" lookup that would otherwise program the host's own interface addresses.
  • ip6tables is probed once: if IPv6 is disabled or the binary is missing, the IPv4 chain still applies and the count of unapplied IPv6 rules is logged.

Behavior changes to accept on review

  • A new fail-closed hard error when the host has active IPv6 but an unusable ip6tables. On main this silently succeeded with IPv4-only rules.
  • CIDR entries now take effect. On main a /-containing entry failed IP parse, then failed DNS, and produced no rule at all.
  • IPv6 destinations are now filtered where previously they were silently bypassed.
  • Invalid CIDR is skipped with a warning rather than treated as fatal.

Interim rule ordering

Allow-list rules are emitted before block-list rules, and iptables/ip6tables are first-match-wins, so a destination present in both lists is ACCEPTed. This allow-wins behavior already exists on main and is preserved, not introduced, here. GA deny-precedence is owned by AB#62830341 (net-model-2, PR #632) and is documented in a NOTE on build_policy_rules_logged. Callers must not assume deny-precedence.

Scope

Only the IPv6 + CIDR work is in scope. Port and protocol filtering are out of scope: the shipping allowedHosts / blockedHosts lists are flat host strings with nowhere to attach a port, and the GA egress.allow[]/deny[] schema is not in main. DNS hostnames are still accepted, and the GA schema migration is untouched, so this PR does not close roadmap item 15. It does not modify the roadmap document.

Validation

  • cargo test -p lxc_common (WSL Ubuntu-24.04) — 105 passed, 0 failed, 0 ignored (re-verified today).
  • cargo clippy -p lxc_common -- -D warnings (WSL Ubuntu-24.04) — clean (re-verified today).
  • Four integration configs and scripts (lxc_network_ipv6_cidr, lxc_network_invalid_cidr, lxc_network_dualstack_hostname, lxc_network_cidr_boundary) are wired into run_lxc_all_tests.sh. They need a root LXC host and were not executed in this pass.

AB#62830559

Microsoft Reviewers: Open in CodeFlow

…2830559)

Firewall mode resolved `allowedHosts` / `blockedHosts` to IPv4 only. On a
dual-stack host, traffic to the same destination over IPv6 bypassed the
firewall entirely, and any CIDR entry (v4 or v6) failed to parse as an
address, then failed DNS resolution, and was dropped.

Changes, all confined to the LXC backend:

- `resolve_host` returns IPv4 and IPv6 destinations separately. Hostnames
  resolve to both A and AAAA records; bare literals and validated CIDR
  blocks pass through in their own family.
- `destination_family` validates CIDR syntax and prefix length (<=32 for
  IPv4, <=128 for IPv6). Malformed entries are reported as unresolved and
  skipped rather than handed to iptables, which would reject them at apply
  time and abort setup for the whole policy.
- IPv4 rules go to `iptables`, IPv6 rules to `ip6tables`, with parallel
  per-container chains and FORWARD hooks.
- `ip6tables` is probed once. When it is missing or IPv6 is disabled in the
  kernel, the IPv4 chain is still applied and the number of unapplied IPv6
  rules is logged, instead of failing a policy that worked before
  dual-stack support.
- Setup failures after partial chain creation are rolled back, and teardown
  removes both families' hooks and chains.

Scope: this covers the IPv6 + CIDR item of AB#62830559 only. Port and
protocol filtering are not included -- they require structured egress
rules in the config schema (AB#62830582), which is not in main.

Tests: 8 new unit tests for family routing, CIDR pass-through, prefix and
syntax rejection, and allow/block ordering; 2 integration configs and
scripts wired into run_lxc_all_tests.sh.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd

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 dual-stack IPv4/IPv6 and CIDR firewall filtering for LXC.

Changes:

  • Resolves and validates IPv4/IPv6 destinations and CIDRs.
  • Programs and tears down parallel iptables/ip6tables chains.
  • Adds documentation, unit coverage, and LXC integration tests.

Reviewed changes

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

Show a summary per file
File Description
src/backends/lxc/common/src/network_iptables.rs Implements dual-stack firewall handling and rollback.
docs/lxc-support/lxc-backend.md Documents dual-stack behavior and limitations.
tests/configs/lxc_network_ipv6_cidr.json Adds valid IPv6/CIDR coverage.
tests/configs/lxc_network_invalid_cidr.json Adds malformed CIDR coverage.
tests/scripts/run_lxc_network_ipv6_cidr_test.sh Tests IPv6/CIDR firewall setup.
tests/scripts/run_lxc_network_invalid_cidr_test.sh Tests malformed CIDR handling.
tests/scripts/run_lxc_all_tests.sh Registers the new integration tests.

Comment on lines +419 to +423
logger.log_line(&format!(
"Firewall setup failed: {}. Cleaning up partial iptables state.",
e
));
self.teardown_chains(logger);
Comment on lines 458 to 464
for host in policy
.allowed_hosts
.iter()
.chain(policy.blocked_hosts.iter())
{
if Self::resolve_host(host).is_empty() {
logger.log_line(&format!("Warning: could not resolve host '{}'", host));
Comment on lines +470 to +475
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.",
Comment on lines +97 to +101
# The v6 half is the point of the test: if ip6tables is unusable the v6 rules
# are skipped with a warning, which would make this a v4-only run.
if echo "$OUTPUT" | grep -q "IPv6 firewall rule(s) not applied"; then
fail "IPv6 rules were skipped; ip6tables is unusable on this host."
fi
Comment on lines +52 to +54
if ! echo "$OUTPUT" | grep -q "Default network policy: DROP"; then
fail "default-deny policy was not applied."
fi
…#62830559)

Tests were written black-box from roadmap item 19, AB#62830559 and the public doc comments, without reading network_iptables.rs, so they pin the specified contract rather than the current implementation.

Unit tests (24 new, in two child modules of network_iptables): resolution/CIDR contract - family routing, CIDR passthrough, host bits not required to be zero, prefix bounds at 0/32 and 0/128, malformed syntax, IPv4-mapped IPv6, dual-stack hostname resolution; and rule generation - per-family bucketing, ACCEPT/DROP mapping, allow-before-block ordering in both families, family-agnostic base rules, chain-name cap.

E2E: lxc_network_dualstack_hostname covers hostnames with both A and AAAA records (the bypass this work item fixes) alongside mixed-family literals and CIDRs; lxc_network_cidr_boundary covers /0, /32, /128, non-zero host bits and the previously untested defaultPolicy=allow path. Both wired into run_lxc_all_tests.sh.

No change to wire.rs, models.rs, config_parser.rs, schemas/ or sdk/.

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 22:32

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 13 out of 13 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

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

  • If ip6tables is missing or its probe fails while the host still has IPv6 enabled, this path returns success and leaves IPv6 completely outside the firewall. That preserves the dual-stack bypass this PR is intended to close (including for defaultPolicy: block policies with no explicit IPv6 destinations). Please continue only after positively establishing that IPv6 is disabled; otherwise fail policy setup when the IPv6 chain cannot be installed.
        } 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.",

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

  • Rollback is also entered when the first -N fails because this chain already exists. In that case this manager created nothing, but teardown_chains flushes and deletes the pre-existing chain and hook. Since chain names use only the first 20 sanitized container-name characters, collisions or concurrent runs can therefore remove another active sandbox's firewall. Track which family chains/hooks were successfully created and roll back only those resources.
                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:464

  • Each hostname is resolved here for warning output and then resolved again inside build_policy_rule_args. DNS can change or fail between calls, so a blocked host can pass the first lookup but yield no rule on the second without any warning; this is also unnecessary duplicate DNS work. Refactor rule construction to resolve each entry once and use that same result for both logging and rule generation.
            if Self::resolve_host(host).is_empty() {
                logger.log_line(&format!("Warning: could not resolve host '{}'", host));

tests/scripts/run_lxc_network_cidr_boundary_test.sh:117

  • This test claims to validate rule programming rather than reachability, but it fails whenever the container's wget cannot reach GitHub. That makes the newly wired all-tests suite depend on external network availability even when firewall setup is correct. Ignore the workload exit here and let the subsequent firewall-log assertions determine success, as the other new network scripts do.
if [ "$STATUS" -ne 0 ]; then
    fail "lxc-exec exited with status $STATUS for boundary-valid prefixes."

tests/configs/lxc_network_ipv6_cidr.json:2

  • This fixture is rejected before reaching LXC because the parser's supported range starts at 0.6 (config_parser.rs:297-332); the existing LXC network fixture already uses 0.6.0-alpha. As written, the new integration test can never exercise IPv6/CIDR rule setup. Use a currently supported schema version.
  "version": "0.4.0-alpha",

tests/configs/lxc_network_invalid_cidr.json:2

  • This fixture is rejected before reaching LXC because the parser's supported range starts at 0.6 (config_parser.rs:297-332). Consequently, the script sees a schema-version error rather than the expected unresolved-CIDR warnings. Use a currently supported schema version.
  "version": "0.4.0-alpha",

tests/configs/lxc_network_dualstack_hostname.json:2

  • Schema version 0.4 is below the parser's supported range (config_parser.rs:297-332), so this fixture fails during config loading and never tests dual-stack hostname resolution. Use the same supported version as the existing LXC network fixture.
  "version": "0.4.0-alpha",

tests/configs/lxc_network_cidr_boundary.json:2

  • Schema version 0.4 is below the parser's supported range (config_parser.rs:297-332), so lxc-exec rejects this fixture before any boundary CIDRs are programmed. Use a currently supported schema version.
  "version": "0.4.0-alpha",

Comment on lines +49 to +52
run_test "LXC Network IPv6+CIDR" "$SCRIPT_DIR/run_lxc_network_ipv6_cidr_test.sh"
run_test "LXC Network Invalid CIDR" "$SCRIPT_DIR/run_lxc_network_invalid_cidr_test.sh"
run_test "LXC Network Dual-Stack Hostname" "$SCRIPT_DIR/run_lxc_network_dualstack_test.sh"
run_test "LXC Network CIDR Boundary" "$SCRIPT_DIR/run_lxc_network_cidr_boundary_test.sh"
…30559)

Mutation testing showed that inverting the DNS branch so AAAA records are pushed into the IPv4 bucket - the exact dual-stack bypass this work item fixes - left the suite green. The only hostname test used localhost, which resolves to 127.0.0.1 only on many hosts, so the v6 arm of the DNS path was never executed.

Adds a family-purity invariant asserting every destination in a bucket belongs to that bucket's family, exercised over well-known dual-stack names. It now kills that mutation. All 9 mutations tried against the module are caught.

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 22:46

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 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (4)

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

  • Rollback must not tear down chains that this invocation did not create. If the first -N fails because another active container owns the same chain (chain names truncate container IDs to 20 characters), this unconditional teardown flushes that existing chain; its FORWARD hook can then point at an empty chain, disabling the other container's policy. Track successful chain/hook creation per family and roll back only those resources; handle stale/pre-existing chains through an ownership-safe cleanup path.
                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:444

  • Treating every failed ip6tables -S probe as an IPv4-only host makes firewall enforcement fail open. A dual-stack host can have IPv6 enabled while the binary is missing, permissions are wrong, or the probe fails transiently; this path still returns success and leaves IPv6 completely unfiltered, preserving the bypass this PR is intended to close. Skip the v6 chain only after confirming IPv6 is disabled; otherwise fail setup when ip6tables is unusable.
        // Probe ip6tables once. On IPv4-only hosts (binary absent or IPv6
        // disabled in the kernel) enforce the v4 policy and skip the v6 chain
        // rather than failing setup for a policy that worked before dual-stack.
        let ipv6_enabled = Self::ip6tables_available(logger);

tests/scripts/run_lxc_network_cidr_boundary_test.sh:117

  • This makes the boundary test depend on successful external wget reachability even though the test explicitly says it validates rule programming, not reachability. On an offline runner, valid firewall setup still produces a nonzero command status and fails here. Capture the output while tolerating the workload exit, as the other new firewall tests do; the subsequent required log assertions still catch setup/config failures.
if [ "$STATUS" -ne 0 ]; then
    fail "lxc-exec exited with status $STATUS for boundary-valid prefixes."
fi

tests/scripts/run_lxc_network_dualstack_test.sh:149

  • These assertions can still pass when setup fails after the default rules are appended—for example, if inserting either FORWARD hook fails. The script discards lxc-exec's status and never checks the emitted Firewall setup failed:/iptables error, so it can report the dual-stack bypass closed even though no chain is hooked. Reject firewall setup errors before declaring success.
if ! grep -Fq "Creating iptables/ip6tables chain:" <<<"$OUTPUT"; then
    fail "iptables/ip6tables chain creation was not logged."
fi

… (AB#62830559)

A spec-derived test asserting that '10.0.0.0/+24' is rejected was failing. It was rewritten to assert the current behaviour instead of being left as a finding, which is the wrong resolution: whether MXC should accept a permissive prefix spelling in a security policy file is a design decision, not something to settle by editing the test.

The original assertion is restored verbatim and marked #[ignore] so the finding stays visible in test output pending a decision. The separate assertion that a leading '+' cannot smuggle an out-of-range prefix past the family bound check is kept as a passing test, since prefix bounds are unambiguous.

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 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

Copilot reviewed 13 out of 13 changed files in this pull request and generated no new comments.

Suppressed comments (7)

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

  • Rollback is unconditional even when the first -N failed, so it can delete firewall state owned by another active manager. Chain names are truncated to 20 sanitized characters, and concurrent runs for the same container necessarily share a name; the second run's creation failure reaches this cleanup and removes the first run's FORWARD hook/chain. Track which chains and hooks this invocation successfully created, and roll back only those resources.
                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:464

  • Each hostname is resolved here for the warning and then resolved again while building policy_rules. DNS results can change or the second lookup can transiently fail; under default-allow, a blocked hostname can therefore pass the first lookup (no warning) but emit no DROP rule on the second lookup. Resolve each entry once and reuse that exact result for both diagnostics and rule generation.
            if Self::resolve_host(host).is_empty() {
                logger.log_line(&format!("Warning: could not resolve host '{}'", host));

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

  • Returning success here leaves IPv6 completely unfiltered when the kernel supports IPv6 but the ip6tables binary is absent. This also omits the terminal IPv6 DROP for defaultPolicy: block, even when policy_rules.ipv6 is empty, so the dual-stack bypass remains open. Only skip safely after proving IPv6 is disabled; otherwise fail closed or disable IPv6 for the sandbox.
        } 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:183

  • u8::from_str accepts a leading +, so 10.0.0.0/+24 is treated as valid even though the documented contract rejects malformed/non-digit prefixes and the corresponding test is quarantined. Require ASCII digits before parsing so this typo follows the unresolved-host path.
            let prefix = prefix.parse::<u8>().ok()?;

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

  • Teardown invokes ip6tables unconditionally even when the availability probe skipped creation of the IPv6 chain. On a host where the binary exists but IPv6 is disabled, these -F/-X calls log failures during otherwise successful cleanup; the new invalid-CIDR integration test treats those messages as setup failure. Persist whether the IPv6 chain was created and only clean it up in that case.
        let _ = Self::run_iptables(&["-F", &self.chain_name], logger);
        let _ = Self::run_iptables(&["-X", &self.chain_name], logger);
        let _ = Self::run_ip6tables(&["-F", &self.chain_name], logger);
        let _ = Self::run_ip6tables(&["-X", &self.chain_name], logger);

src/backends/lxc/common/src/network_iptables_resolution_spec_tests.rs:216

  • This test still passes when no AAAA record is available, so an offline CI run never exercises the DNS branch that routes AAAA results to IPv6—the central regression this PR fixes. The integration test likewise skips external-hostname assertions when DNS is unavailable. Add an injectable/mock resolver or deterministic local dual-stack resolver so misfiling AAAA records fails on every run.
    if !saw_v6 {
        eprintln!(
            "WARNING: no AAAA record resolved for any of {hosts:?}; the IPv6 DNS \
             arm of resolve_host was not exercised by this run."
        );

tests/scripts/run_lxc_network_dualstack_test.sh:149

  • The script can pass when firewall setup fails while inserting either FORWARD hook: chain creation and the default policy are logged before hook insertion, and the nonzero executor status is discarded. Check the setup-failure diagnostics before declaring the dual-stack policy programmed.
if ! grep -Fq "Creating iptables/ip6tables chain:" <<<"$OUTPUT"; then
    fail "iptables/ip6tables chain creation was not logged."
fi

Darren Hoehna (dhoehna) and others added 2 commits July 31, 2026 16:40
Two defects found by a coverage audit of this branch, both caught by
spec-derived tests written black-box against the roadmap contract.

resolve_host("") fell through to DNS resolution, where format!("{}:0", host)
produces ":0". Winsock resolves that to every local interface address, so an
empty allowedHosts entry emitted rules for the host's own LAN and link-local
addresses. glibc rejects it, so this reproduced only on Windows -- it turned
CI red on windows/x64 and windows/arm64. config_parser assigns host lists
verbatim, so an empty string does reach resolve_host from a policy file.

destination_family validated the CIDR prefix with u8::from_str, which accepts
a leading '+'. 10.0.0.0/+24 was forwarded to iptables, which silently
canonicalizes it to 10.0.0.0/24, so a policy typo was applied instead of being
reported by the unresolved-host warning that run_lxc_network_invalid_cidr_test.sh
exists to guarantee. The prefix must now be ASCII digits, which also subsumes
the embedded-slash case. The test for this was previously quarantined pending
a bad-code/bad-test ruling; the ruling is bad code, so it is now un-ignored.

Also adds lifecycle tests pinning three behaviours a cargo-mutants run proved
were unpinned: a new manager reports no rules applied, a non-firewall
enforcement mode is a successful no-op, and the enforcement-mode gate is not
inverted. The last matters most -- an inverted gate would silently skip all
filtering while reporting success.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: ea24fae3-d643-4f19-a701-9e31b9f950fd
None of these four scripts had ever executed a single firewall assertion
since they were added. Two independent causes:

  - every config declared "version": "0.4.0-alpha", but the parser accepts
    >=0.6 <=0.8, so each run died at config parse
  - lxc-exec buffers diagnostics unless --debug is passed, so the log lines
    the scripts assert on were never emitted even after the version bump

Bumps the configs to 0.6.0-alpha, matching the sibling LXC configs, passes
--debug, and adds post-run iptables/ip6tables assertions that the
per-container chain is torn down rather than leaked.

Verified by running all four as root under WSL: each creates a real container,
programs real v4/v6 chains, and cleans up. Assertion liveness was confirmed by
flipping defaultPolicy in a config and observing exit 1 with
"FAIL: default-deny policy was not applied."

Also normalizes lxc_network_ipv6_cidr.json to LF; it was the only one of the
four committed with CRLF.

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 23:40

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 14 out of 14 changed files in this pull request and generated 1 comment.

Suppressed comments (8)

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

  • Each hostname is resolved here for the warning and then resolved again by build_policy_rule_args at line 485. DNS can change or fail between calls, so a blocked hostname may resolve successfully here but produce no DROP rule on the second call, with no warning; under default-allow that silently permits the destination. Resolve each host once and use the same ResolvedDestinations for both diagnostics and rule generation.
            if Self::resolve_host(host).is_empty() {

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

  • This treats a disabled IPv6 stack and an unavailable/failing ip6tables command as equivalent. If the kernel still has IPv6 enabled but the binary is missing, even defaultPolicy: block gets only an IPv4 DROP chain and all IPv6 egress remains unfiltered. Distinguish a genuinely disabled IPv6 stack; when IPv6 is active, fail setup or provide equivalent IPv6 enforcement instead of failing open.
        // Probe ip6tables once. On IPv4-only hosts (binary absent or IPv6
        // disabled in the kernel) enforce the v4 policy and skip the v6 chain
        // rather than failing setup for a policy that worked before dual-stack.
        let ipv6_enabled = Self::ip6tables_available(logger);

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

  • Rollback also runs when the first -N failed because this chain already belonged to another manager. Since chain names truncate container IDs to 20 characters (lines 70–77), distinct containers can collide; this teardown then flushes the existing chain and may remove its hook, silently disabling that container's firewall. Track which chains/hooks this attempt successfully created and roll back only those resources—never flush a chain whose creation failed.
                logger.log_line(&format!(
                    "Firewall setup failed: {}. Cleaning up partial iptables state.",
                    e
                ));
                self.teardown_chains(logger);

tests/configs/lxc_network_cidr_boundary.json:6

  • This fixture's script requires lxc-exec to exit zero, but the workload depends on an external API. On an offline runner, wget fails and the boundary test reports a prefix failure even though firewall programming succeeded (lxc-exec propagates the workload exit code). Use a local success command because this test explicitly does not verify reachability.
    "commandLine": "wget -qO- https://api.github.com/zen"

tests/scripts/run_lxc_network_ipv6_cidr_test.sh:50

  • run_lxc_all_tests.sh already requires UID 0, so invoking sudo here is unnecessary and makes this cleanup assertion silently pass when sudo is not installed: command-not-found is interpreted exactly like “chain absent.” Query both tables directly so the new teardown coverage remains effective on minimal root test hosts.
    if sudo -n iptables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "iptables chain '$CHAIN_NAME' was left behind after lxc-exec completed."
    fi
    if sudo -n ip6tables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "ip6tables chain '$CHAIN_NAME' was left behind after lxc-exec completed."

tests/scripts/run_lxc_network_invalid_cidr_test.sh:40

  • run_lxc_all_tests.sh already requires UID 0, so invoking sudo here is unnecessary and makes this cleanup assertion silently pass when sudo is not installed: command-not-found is interpreted exactly like “chain absent.” Query both tables directly so the new teardown coverage remains effective on minimal root test hosts.
    if sudo -n iptables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "iptables chain '$CHAIN_NAME' was left behind after lxc-exec completed."
    fi
    if sudo -n ip6tables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "ip6tables chain '$CHAIN_NAME' was left behind after lxc-exec completed."

tests/scripts/run_lxc_network_dualstack_test.sh:61

  • run_lxc_all_tests.sh already requires UID 0, so invoking sudo here is unnecessary and makes this cleanup assertion silently pass when sudo is not installed: command-not-found is interpreted exactly like “chain absent.” Query both tables directly so the new teardown coverage remains effective on minimal root test hosts.
    if sudo -n iptables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "iptables chain '$CHAIN_NAME' was left behind after lxc-exec completed."
    fi
    if sudo -n ip6tables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "ip6tables chain '$CHAIN_NAME' was left behind after lxc-exec completed."

tests/scripts/run_lxc_network_cidr_boundary_test.sh:57

  • run_lxc_all_tests.sh already requires UID 0, so invoking sudo here is unnecessary and makes this cleanup assertion silently pass when sudo is not installed: command-not-found is interpreted exactly like “chain absent.” Query both tables directly so the new teardown coverage remains effective on minimal root test hosts.
    if sudo -n iptables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "iptables chain '$CHAIN_NAME' was left behind after lxc-exec completed."
    fi
    if sudo -n ip6tables -S "$CHAIN_NAME" >/dev/null 2>&1; then
        fail "ip6tables chain '$CHAIN_NAME' was left behind after lxc-exec completed."

let policy = policy_with_enforcement_mode(mode.clone());
let mut logger = Logger::new(Mode::Buffer);

let _ = manager.apply_firewall_rules(&policy, &mut logger);
Copilot AI review requested due to automatic review settings August 4, 2026 19:47

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 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (6)

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

  • Rollback runs even when the first -N failed because this chain already existed. In that case this invocation created nothing, but teardown_chains still deletes the matching FORWARD hook and flushes/deletes the pre-existing v4/v6 chains, which can disable another live container's firewall (chain names are truncated to 20 container-name characters). Track which chains/hooks this apply actually created and roll back only those operations.
                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:461

  • An unavailable ip6tables binary does not prove that IPv6 is disabled. On a host with an active IPv6 stack but no binary, this path returns success without installing either the IPv6 destination rules or the terminal default policy, so defaultPolicy: block and IPv6 block-list entries remain bypassable. Only preserve the IPv4-only fallback after positively establishing that IPv6 traffic is unavailable; otherwise fail closed.
        // Probe ip6tables once. On IPv4-only hosts (binary absent or IPv6
        // disabled in the kernel) enforce the v4 policy and skip the v6 chain
        // rather than failing setup for a policy that worked before dual-stack.
        let ipv6_enabled = Self::ip6tables_available(logger);

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

  • Each hostname is resolved here for warning generation and then resolved again in build_policy_rule_args. DNS can change or transiently fail between those calls, so a successful first lookup followed by a failed second lookup silently emits no rule and no warning—widening a default-allow block policy. Resolve each entry once and use that same result for both logging and rule generation.
        for host in policy
            .allowed_hosts
            .iter()
            .chain(policy.blocked_hosts.iter())
        {
            if Self::resolve_host(host).is_empty() {

tests/scripts/run_lxc_network_dualstack_test.sh:159

  • The command status is intentionally discarded, but this script never checks for Firewall setup failed. A failure while adding the FORWARD hook occurs after both the chain-creation and default-policy messages, then rollback makes the cleanup assertion pass, so this test can report PASS without any firewall being installed. Reject firewall command/setup errors as the other new integration scripts do.
if ! grep -Fq "Creating iptables/ip6tables chain:" <<<"$OUTPUT"; then
    fail "iptables/ip6tables chain creation was not logged."
fi

src/backends/lxc/common/src/network_iptables_resolution_spec_tests.rs:215

  • This test still passes when no AAAA answer is available, so an offline CI run never exercises the DNS-to-IPv6 branch named by the test. The end-to-end script also skips external-hostname assertions when DNS is unavailable, allowing an AAAA-to-IPv4 regression to pass the full suite. Use an injected resolver or deterministic local dual-stack fixture and require a known IPv6 answer.
    if !saw_v6 {
        eprintln!(
            "WARNING: no AAAA record resolved for any of {hosts:?}; the IPv6 DNS \
             arm of resolve_host was not exercised by this run."
        );

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

  • The -o to -i change is what makes this chain govern container egress, but none of the new tests verifies the installed FORWARD rule: both spellings are accepted by iptables, and the scripts only inspect logs after teardown. A regression back to -o would therefore leave all destination rules ineffective while the suite passes. Add a command-executor assertion for the exact hook arguments or an in-flight firewall/reachability assertion.
        if let Some(ref iface) = self.veth_interface {
            Self::run_iptables(
                &["-I", "FORWARD", "-i", iface, "-j", &self.chain_name],
                logger,
            )?;

Darren Hoehna (dhoehna) and others added 3 commits August 4, 2026 13:10
Address four PR microsoft#724 review threads that are an interwoven refactor of
the same enforcement path in network_iptables.rs:

- Resolve each allow/block destination exactly once. The apply path
  previously resolved a host for the unresolved-host warning and then a
  second time inside rule construction; two lookups of the same name can
  disagree under DNS round-robin or a TTL expiry, so the installed rule
  need not match the logged one. build_policy_rules_logged now resolves
  once and reuses that result for both. The pure builders that resolve
  are gated to test-only.

- Extract enforcement_mode_uses_firewall as a pure predicate and test it
  directly, instead of the lifecycle test invoking apply_firewall_rules
  (which shells out to the host firewall) for the Firewall and Both cases.

- Fail closed when IPv6 is active but ip6tables is unusable. The old
  boolean probe skipped IPv6 for every ip6tables failure, marking the
  policy applied while IPv6 egress went unfiltered. classify_ip6tables_status
  now distinguishes a kernel with no active IPv6 (safe to skip) from an
  IPv6-capable host whose ip6tables is missing or broken (setup fails).
  host_has_active_ipv6 reads /proc/net/if_inet6, which the kernel
  populates only when the IPv6 stack is loaded and addresses exist.

- Track which per-family chains and FORWARD hooks each attempt created
  and roll back only those. Rollback previously tore down chains
  unconditionally, and since chain names truncate at 20 characters a
  partial-failure rollback could delete a chain belonging to a different
  container. teardown_created acts on the recorded CreatedResources.

Also log a positive confirmation when a FORWARD hook is installed, so the
E2E scripts can assert on it.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The four LXC network E2E scripts probed for a leftover chain with
`sudo -n iptables -S`. Under `sudo -n`, a host without passwordless sudo
fails the probe for a reason unrelated to whether the chain exists, so
the cleanup assertion could pass without ever having checked. The LXC
suite already requires root (run_lxc_all_tests.sh), so query iptables and
ip6tables directly instead.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
All four LXC network E2E scripts could report PASS while the per-container
chain was never hooked into FORWARD: the code emits a skipped-hook warning
that nothing checked, so an undiscovered veth silently enforced nothing.
Each script now fails on the "Skipping FORWARD hook" warning and requires
the positive "FORWARD hook installed" confirmation before reporting PASS.
This pairs with the confirmation log line added to the enforcement backend.

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

The boundary fixture ran `wget -qO- https://api.github.com/zen`, so the
test failed whenever the network or the remote host was down, independent
of the code under test. Replace it with the local success command `true`
so a non-zero lxc-exec status reflects a firewall-setup failure on the
boundary-valid prefixes rather than an unrelated outage, and note that in
the script's status check.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 20:13

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 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (4)

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

  • This probe also changes Bubblewrap behavior: bwrap_runner.rs:205-206 uses this same manager, so a Bubblewrap host-list policy now fails before spawning whenever IPv6 is active and ip6tables is unavailable. That contradicts this PR's LXC-only scope and the stated fallback to an IPv4 chain. Please either isolate the fail-closed behavior to LXC or include the Bubblewrap behavior, tests, and documentation in this PR.
        let ipv6_enabled = match Self::ip6tables_status(logger) {
            Ip6tablesStatus::Available => true,
            Ip6tablesStatus::KernelIpv6Disabled => false,
            Ip6tablesStatus::UnusableButIpv6Active => {

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

  • This warning is emitted only for KernelIpv6Disabled, but says IPv6 egress is unfiltered. That contradicts the preceding classification that there is no IPv6 egress to filter and can make operators interpret a safe skip as a security gap.
            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()
            ));

docs/lxc-support/lxc-backend.md:123

  • This description does not match the implementation. The code skips IPv6 only for KernelIpv6Disabled; when IPv6 is active and ip6tables is missing or broken, it fails setup before creating the IPv4 chain. Document the fail-closed behavior so operators know the actual dependency and outcome.
If `ip6tables` is unavailable or IPv6 is disabled in the host kernel, MXC applies the IPv4 chain, skips IPv6 rules, and logs a warning with the number of unapplied IPv6 rules. On such hosts, IPv6 egress is unfiltered.

tests/scripts/run_lxc_network_dualstack_test.sh:155

  • This test does not prove that either hostname produced an AAAA rule: resolving only an A record suppresses the unresolved-host warning, while the fixture's IPv6 literal/CIDR makes the later IPv6-chain checks pass. A regression that drops all hostname AAAA results therefore remains green. Add a deterministic resolver seam or inspect/log the programmed hostname destinations and require at least one AAAA result from each prevalidated dual-stack hostname.
for host in "${ASSERT_RESOLVED_HOSTS[@]}"; do
    if grep -Fq "Warning: could not resolve host '$host'" <<<"$OUTPUT"; then
        fail "host '$host' was not resolved."
    fi
done

The #[cfg(test)] build_policy_rule_args reimplemented allow-before-block
ordering as two separate loops, so the rulegen ordering specs asserted
against a duplicate that production never runs. A future change to emission
order in build_policy_rules_logged (the AB#62830341 deny-precedence work)
would have left those ordering tests green while shipping first-match-wins.

Make build_policy_rule_args a thin test-only shim that delegates to the
shipping build_policy_rules_logged with a throwaway buffer logger, so the
ordering assertions bind to production code again. No test cases added or
changed. Move the deny-precedence contract docstring onto the shipping
function it now guards.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 20:22

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 14 out of 14 changed files in this pull request and generated no new comments.

Suppressed comments (5)

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

  • /proc/net/if_inet6 normally contains the ::1 entry for lo even on an IPv4-only host with no forwardable IPv6 path. Treating any nonempty line as active IPv6 sends those hosts to UnusableButIpv6Active, so a missing ip6tables now aborts setup instead of preserving IPv4 operation. Ignore loopback-only entries (or inspect the container/veth path) before deciding that IPv6 egress is active.
            Ok(contents) => contents.lines().any(|line| !line.trim().is_empty()),

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

  • This fail-closed branch contradicts the PR's stated compatibility behavior that a missing ip6tables should still apply the IPv4 chain and log skipped IPv6 rules. It also affects Bubblewrap because bwrap_runner.rs:205-206 invokes this same manager, despite the PR being scoped to LXC. Decide whether fail-closed is the intended contract; then align the implementation, PR/docs, and Bubblewrap coverage.
        let ipv6_enabled = match Self::ip6tables_status(logger) {
            Ip6tablesStatus::Available => true,
            Ip6tablesStatus::KernelIpv6Disabled => false,
            Ip6tablesStatus::UnusableButIpv6Active => {

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

  • This warning is emitted only after KernelIpv6Disabled (the active-IPv6/unusable-tool case already returned), so saying IPv6 egress is unfiltered contradicts the status classification and can falsely suggest a security gap. Report that the kernel has no active IPv6 instead.
        } 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_resolution_spec_tests.rs:215

  • This allows the test to pass without ever exercising the AAAA filing arm. The integration script also skips external-host assertions when DNS is unavailable, so offline CI has no deterministic coverage of the core dual-stack-hostname regression. Inject/abstract resolution, or extract the address-filing helper, and feed fixed IPv4 and IPv6 addresses in a unit test.
    if !saw_v6 {
        eprintln!(
            "WARNING: no AAAA record resolved for any of {hosts:?}; the IPv6 DNS \
             arm of resolve_host was not exercised by this run."
        );

docs/lxc-support/lxc-backend.md:123

  • This documents skip-and-continue for an unavailable ip6tables, but the implementation returns an error before creating the IPv4 chain whenever /proc/net/if_inet6 is nonempty. Split the disabled-kernel case from the unavailable-tool case so users are not told firewall setup succeeds when it actually fails closed.
If `ip6tables` is unavailable or IPv6 is disabled in the host kernel, MXC applies the IPv4 chain, skips IPv6 rules, and logs a warning with the number of unapplied IPv6 rules. On such hosts, IPv6 egress is unfiltered.

Closes the testability gap flagged in the type doc comment: the function
was extracted to be pure so the fail-open vs fail-closed decision could
be unit-tested without a privileged Linux host, but had zero tests.

Nine tests, all derived from the documented contract only:
- Exhaustive 4-case truth table (both boolean inputs x all combinations)
- Invariant: working probe always yields Available
- Invariant: failed probe never yields Available
- Security invariant: UnusableButIpv6Active is reachable only when
  probe=false AND ipv6_active=true; unreachable under every other input
- Invariant: KernelIpv6Disabled reachable only when probe=false AND
  ipv6_active=false
- Discriminant-distinctness: all three variants are distinct under PartialEq

All 6 mutations (swap outcomes, fail-open collapse, fail-closed collapse,
probe invert, ipv6_active invert, always-Available) are caught.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 21:20

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 15 out of 15 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

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

  • Any /proc/net/if_inet6 read error is treated as positive proof that IPv6 is disabled. If procfs is hidden/unreadable while the ip6tables probe also fails, this selects the safe-skip branch and applies only IPv4 rules even though IPv6 may be usable, recreating the fail-open this classification is meant to prevent. Preserve an unknown/error state and fail closed unless IPv6 inactivity is positively established.
            Err(_) => false,

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

  • The PR description says that when ip6tables is absent the IPv4 chain is still applied and IPv6 rules are merely logged as unapplied. This branch instead aborts the entire policy whenever the host is considered IPv6-active. Please align the implementation and documented compatibility contract; this is a user-visible difference between a successful IPv4-only run and refusing to start the sandbox.
            Ip6tablesStatus::UnusableButIpv6Active => {
                return Err(
                    "ip6tables is unusable but the host has active IPv6; refusing to \
                     apply an IPv4-only policy that would leave IPv6 egress unfiltered"
                        .to_string(),

docs/lxc-support/lxc-backend.md:123

  • This documents behavior opposite to the implementation. UnusableButIpv6Active returns an error at network_iptables.rs:617-623; only KernelIpv6Disabled skips the IPv6 chain. Update the text so operators know that a missing/broken ip6tables fails setup when IPv6 is active.
If `ip6tables` is unavailable or IPv6 is disabled in the host kernel, MXC applies the IPv4 chain, skips IPv6 rules, and logs a warning with the number of unapplied IPv6 rules. On such hosts, IPv6 egress is unfiltered.

src/backends/lxc/common/src/network_iptables_resolution_spec_tests.rs:215

  • This test still passes when no AAAA record is returned, so CI can complete without executing the DNS-to-IPv6 branch that fixes the reported bypass; the integration script likewise skips external-hostname assertions offline. Use an injected resolver or deterministic local dual-stack DNS fixture so routing an AAAA result into the wrong bucket always fails the suite.
    if !saw_v6 {
        eprintln!(
            "WARNING: no AAAA record resolved for any of {hosts:?}; the IPv6 DNS \
             arm of resolve_host was not exercised by this run."
        );

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

  • This manager is also called directly by Bubblewrap (src/backends/bubblewrap/common/src/bwrap_runner.rs:205-206), so adding ip6tables execution changes that backend too. The PR only updates LXC tests/docs, while Bubblewrap's documentation still states firewall mode is IPv4-only. Either isolate the behavior to LXC or update and test the Bubblewrap contract in this change.
    /// Run an ip6tables command and return success/failure.
    fn run_ip6tables(args: &[&str], logger: &mut Logger) -> Result<bool, String> {
        Self::run_firewall_command("ip6tables", args, logger)

/// live, so a broken `ip6tables` is a real gap rather than a no-op.
fn host_has_active_ipv6() -> bool {
match std::fs::read_to_string("/proc/net/if_inet6") {
Ok(contents) => contents.lines().any(|line| !line.trim().is_empty()),
…eview)

Address test-honesty defects found by evidence-based review; every fix is
proven with mutation testing (before: mutant survives; after: mutant caught).

Task 1 -- DNS bucket test was vacuous.  The family split is factored into a
pure `bucket_resolved_addrs` and the AAAA-in-v6-bucket test now injects
addresses and asserts the v6 bucket is non-empty and family-pure, so deleting
the IPv6 result path fails it.  A separate live characterization asserts only
the purity invariant, with no warning-that-still-passes.

Task 2 -- failure to read IPv6 state was treated as confirmed inactivity.
Factored the parse/classify into pure `classify_host_ipv6_state` (file
content and read-error as input) and `ipv6_state_treated_as_active`.  A
NotFound read (IPv6 disabled) stays a confirmed negative; any other read error
is Unknown and treated as active so it fails closed instead of leaving IPv6
egress unfiltered.  Loopback-only `::1` on `lo` no longer counts as active.
Added spec tests for the whole mapping (previously untested).

Task 3 -- E2E scripts could count skips as passes.  The aggregate runner now
treats exit 77 as SKIPPED (never PASS) and flags a run that executed nothing;
each script honestly skips on missing root/iptables/ip6tables/LXC/binary.
Added assertions on the actual programmed destination rules (via a new
per-rule debug log) so deleting destination-rule emission fails the scripts,
and the dual-stack script now asserts a positive IPv6 rule exists, including a
hostname-derived AAAA rule when external DNS is available.

Task 4 -- corrected docs to describe the three-way ip6tables classification
(Available / KernelIpv6Disabled / UnusableButIpv6Active) and that an active
host with unusable ip6tables fails setup rather than skipping IPv6.

Runtime behavior of the shell E2E scripts is UNVERIFIED here (Windows; no
root/iptables/netns); scripts were syntax-checked with bash -n only.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot AI review requested due to automatic review settings August 4, 2026 22:09
Darren Hoehna (dhoehna) added a commit to dhoehna/mxc that referenced this pull request Aug 4, 2026
run_bwrap_network_firewall_test.sh calls skip_live() when root, bwrap,
iptables, or the built lxc-exec binary is missing.  skip_live printed
"SKIP" and then exited 0, and run_bwrap_all_tests.sh treated any zero
exit as a pass, so a host missing every prerequisite reported the
Bubblewrap firewall test as passing while none of the live behavior ran.
The comment above skip_live already claimed a skip "never reads as a
pass"; the code did the opposite.

skip_live now exits 77, the Automake convention for a skipped test, and
the runner classifies 77 separately: it is never counted as a pass, the
skipped tests are listed by name at the end, and a run in which
everything skipped and nothing passed prints an explicit warning that
nothing was verified.

Verified by extracting the edited run_test function and driving it with
child scripts exiting 0, 77, and 3: the result is passed=1, failed=1,
skipped=1.  Both scripts are bash -n clean and remain LF-only, so the
runner's CRLF guard still passes.  The live path needs Linux and root
and has not been executed here.

run_lxc_all_tests.sh has the same shape but is deliberately untouched --
PR microsoft#724 is changing that file, and editing it here would collide.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Copilot-Session: 52fb9e8c-4a48-435c-9964-8ec0fee653c6

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 16 out of 16 changed files in this pull request and generated 1 comment.

Suppressed comments (7)

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

  • NotFound does not prove that IPv6 is disabled: the same error occurs when /proc is not mounted or this path is hidden. On an IPv6-capable host with unusable ip6tables, that case is classified inactive and skips v6 filtering, defeating the intended fail-closed behavior. Use an independent kernel/IPv6 probe, or treat ambiguous absence as Unknown.
            Err(e) if e.kind() == std::io::ErrorKind::NotFound => HostIpv6State::Inactive,
            Err(_) => HostIpv6State::Unknown,

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

  • This fail-closed branch contradicts the PR description’s ip6tables row, which says a missing binary still applies the IPv4 chain and logs unapplied IPv6 rules. Here an active-IPv6 host returns an error before creating the IPv4 chain. The docs/tests support this safer behavior, so update the PR description to state the active-vs-disabled distinction.
        let ipv6_enabled = match Self::ip6tables_status(logger) {
            Ip6tablesStatus::Available => true,
            Ip6tablesStatus::KernelIpv6Disabled => false,
            Ip6tablesStatus::UnusableButIpv6Active => {
                return Err(

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

  • This resolver is also used by Bubblewrap (bwrap_runner.rs:205-206), so the change makes that backend dual-stack and adds the new ip6tables availability behavior despite the PR being scoped to LXC. docs/bwrap-support/bubblewrap-backend.md:187-191 still explicitly documents firewall mode as IPv4-only. Either isolate the LXC behavior or update Bubblewrap documentation and coverage in this PR.
    fn resolve_host(host: &str) -> ResolvedDestinations {

tests/configs/lxc_network_ipv6_cidr.json:6

  • This test only asserts firewall setup, but it runs an external wget with no script timeout. On an offline runner, or if the allowed range no longer covers the resolved endpoint, the suite can wait for wget’s network timeout even though reachability is explicitly out of scope. Use the same local true command as the boundary test.
    "commandLine": "wget -qO- https://api.github.com/zen"

tests/configs/lxc_network_invalid_cidr.json:6

  • After all three invalid entries are omitted, the default DROP rule blocks this external wget; because no script timeout is configured, the integration test can stall until wget’s own network timeout. The script only checks setup diagnostics, so use a local success command.
    "commandLine": "wget -qO- https://api.github.com/zen"

tests/configs/lxc_network_dualstack_hostname.json:6

  • The test validates host-side DNS resolution and generated firewall rules, not container reachability, yet this external wget has no configured timeout. Network outages can therefore make the suite hang or run slowly after all relevant assertions have already been determined. Use a local success command.
    "commandLine": "wget -qO- https://dns.google/"

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

  • These messages are emitted while building arguments, before either command runner executes. If a later iptables command fails and all state is rolled back, diagnostics still claim every destination was “Programmed.” Log after each successful command, or call these “Generated” rules and update the integration assertions accordingly.
            for rule in &rule_args.ipv4 {
                logger.log_line(&format!("Programmed iptables rule: {}", rule.join(" ")));
            }
            for rule in &rule_args.ipv6 {
                logger.log_line(&format!("Programmed ip6tables rule: {}", rule.join(" ")));


self.rules_applied = true;
Ok(true)
Ok(())
@@ -0,0 +1,33 @@
{
"version": "0.6.0-alpha",

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.

shall we use the latest schema version? 0.8?
Applies to all the configs you created

@@ -0,0 +1,158 @@
//! Spec-derived tests for the `ip6tables` usability classification and the

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.

We use inline mod tests throughout the repo and in src/backends/lxc/common/src/network_iptables.rs. Why create new files for these tests?

}
} else {
// Without a veth interface, we cannot safely scope rules to the container.
// Refuse to apply host-wide rules to avoid affecting all host traffic.

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.

Fyi, this seems to be pre-existing TODO, not related to this change.
Filed #755 to track and assigned to you.

ipv4: vec![host.to_string()],
ipv6: Vec::new(),
},
IpAddr::V6(_) => ResolvedDestinations {

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.

 ::ffff:a.b.c.d  is bucketed as V6 and gets an  ip6tables -d ::ffff:…  rule, but Linux emits mapped destinations as IPv4 on the wire, so that rule never matches — a mapped  blockedHosts  entry fails open under default-allow. Map these to their embedded IPv4 ( to_ipv4_mapped ) and emit an iptables rule instead?
Related test needs to be fixed as well

// there's nothing to remove the iptables `-D`/`-F`/`-X` calls just
// no-op.
mgr.rules_applied = true;
mgr.created = CreatedResources {

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.

Hardcoding v4_chain: true, v6_chain: true here (instead of the real CreatedResources) makes the signal-path teardown -F / -X the chain unconditionally. Chain names sanitize+truncate to 20 chars, so on a name collision a signal to container A flushes container B's live chain (-X fails but -F empties it) → B fails open. The apply/rollback path is correctly guarded by CreatedResources ; the signal path should be too.

HostIpv6State::Inactive
}
}
Err(e) if e.kind() == std::io::ErrorKind::NotFound => HostIpv6State::Inactive,

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.

Err(e) if …NotFound => Inactive treats an unmounted/unreadable /proc the same as IPv6-being-disabled (both ENOENT). The comment above over-claims certainty; a host without /proc would be misclassified as IPv6-inactive. Narrow, but worth distinguishing Unknown (fail-closed) from a genuine "IPv6 off" signal.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Needs-Author-Feedback Issue needs attention from issue or PR author

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants