From 3ffe4bec10ee935ea99a887f61966baf8156d499 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:03:13 +0200 Subject: [PATCH 01/14] docs: add network policy spec for VM-to-VM and host isolation Specs the Linux firewall design that makes VM-to-VM reachability and host isolation contracts rather than accidents of the host's iptables policy. Two install-owned chains jumped from position 1, per-VM rules moved inside them, and IPv6 closed on ember links. --- docs/NETWORK-POLICY-SPEC.md | 458 ++++++++++++++++++++++++++++++++++++ 1 file changed, 458 insertions(+) create mode 100644 docs/NETWORK-POLICY-SPEC.md diff --git a/docs/NETWORK-POLICY-SPEC.md b/docs/NETWORK-POLICY-SPEC.md new file mode 100644 index 0000000..e8fe527 --- /dev/null +++ b/docs/NETWORK-POLICY-SPEC.md @@ -0,0 +1,458 @@ +# Network Policy Spec (Linux) + +VM-to-VM reachability and host isolation for the Linux backend. + +## Problem + +Today's Linux rule set is three appended rules per VM +(`network/nat.rs::add_rules`): + +``` +-t nat -A POSTROUTING -s /32 -o -m comment --comment ember: -j MASQUERADE +-A FORWARD -i -o -m comment --comment ember: -j ACCEPT +-A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED -m comment --comment ember: -j ACCEPT +``` + +Each VM sits on its own /30 with the host TAP address as its default +gateway, so VM-to-VM traffic has to be routed by the host between two +TAP devices. Nothing in the rule set above permits that, and nothing +denies it either. Whether two VMs can talk is decided by the host's +`FORWARD` policy: on a box with no firewall (policy `ACCEPT`) it +already works, on a box where docker, firewalld, or ufw set `FORWARD +DROP` it does not. The behavior is an accident of the host +configuration. + +Host reachability is the same story in reverse. The guest's gateway is +a host address, ember adds no `INPUT` rules anywhere, so a guest can +reach the host at its TAP address and at every other address the host +owns, unless the host's own firewall happens to drop it. "VMs cannot +reach the host" is not a property ember currently provides. + +This spec replaces both accidents with a contract that holds +regardless of what else is in the host's firewall: + +> An ember VM can reach the internet and the other VMs of its own +> installation. It cannot reach the host. + +## Goals + +- VM-to-VM traffic works within one installation. +- VM-to-host traffic is denied, for every host address, not just the + gateway. +- Outbound internet access keeps working (unchanged masquerade). +- Host-to-VM traffic keeps working (`ember ssh`, `exec`, `cp`). +- The policy does not depend on the host's `INPUT`/`FORWARD` policy or + on rule ordering relative to other tools. +- Cross-installation VM-to-VM traffic stays denied, preserving the + contract that `tests/isolation.rs` guards. +- Rules already written by an older ember binary stay deletable. + +## Non-goals + +- Blocking the host's LAN. A guest can still reach other machines on + the host's network, as it can today. Only the host itself becomes + unreachable. +- IPv6 connectivity of any kind. ember is IPv4-only, and this spec + closes IPv6 on ember links rather than policing it. +- Per-VM network policy knobs. The design leaves room for them (see + Open decisions) but does not add any. +- nftables. ember shells out to `iptables`, and that stays. + +## Design + +### Install-owned chains + +All policy moves into two chains owned by the installation, jumped to +from position 1 of the built-in chains: + +``` +iptables -N ember--input +iptables -N ember--forward +iptables -I INPUT 1 -j ember--input +iptables -I FORWARD 1 -j ember--forward +``` + +Position 1 is what makes the policy a contract rather than a +suggestion. Appending to `INPUT` cannot work: a pre-existing +`-A INPUT -s 10.0.0.0/8 -j ACCEPT`, which is a common "trust the LAN" +rule and which matches the default `10.100.0.0/16` guest range, would +match first and the host block would silently not apply. Appending to +`FORWARD` has the mirror problem, a mid-chain `REJECT` from ufw or +firewalld is reached before our `ACCEPT`. + +Inserting at the top is only acceptable because both chains are +transparent to everything that is not an ember VM. Every rule inside +them matches on an ember TAP interface, and a chain that matches +nothing falls off its end and returns to the built-in chain at the +rule right after our jump. Non-ember traffic sees no behavior change. + +### Chain names + +`ember--input` and `ember--forward`, where `` is +`GlobalConfig::instance_namespace()`. Legacy installs with no +instance id get `ember-input` and `ember-forward`. + +Lowercase-with-dashes matches every other install-scoped ember +resource name (`ember-aaaa-pool` for dm-thin, `emaaaa-` for TAPs, +`ember:aaaa` for the iptables comment) and keeps `iptables-save | +grep ember` useful. It deviates from netfilter's uppercase convention +(`DOCKER-USER`, `LIBVIRT_FWO`) on purpose, house consistency wins over +domain convention here because the instance id is lowercase hex and a +mixed-case name reads worse than either. iptables caps chain names at +28 characters, `ember-a3f4-forward` is 18. + +Naming lives in the networking subsystem next to `tap::prefix` and +`nat::comment`, and derives from the namespace the same way. + +### Interface wildcards + +iptables matches an interface name ending in `+` as a prefix. Since +every TAP of an installation shares the prefix from `tap::prefix`, one +wildcard rule covers all of them: + +``` +-i ema3f4-+ matches every TAP of install a3f4 +-i em-+ matches every TAP of a legacy install +``` + +The trailing dash keeps this away from physical NICs named `em1`, +`em2`. It also keeps installs apart, `em-+` does not match +`emaaaa-...` and vice versa. Two legacy installs on one host still +share a prefix, which is the pre-existing collision that instance ids +exist to fix, and this spec does not change it. + +### Chain contents + +`ember--input`, static, two rules, no per-VM state: + +``` +-A ember--input -i em-+ -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-A ember--input -i em-+ -j DROP +``` + +The established accept is mandatory, not a convenience. When the host +opens a connection to a guest, the guest's reply packets arrive on +`INPUT` from the TAP, so a bare `DROP` would break `ember ssh`, +`exec`, and `cp`. With the accept in front, host-initiated flows work +and guest-initiated flows to any host address hit the `DROP`. + +`ember--forward`, static: + +``` +-A ember--forward -i em-+ -o em-+ -j ACCEPT +-A ember--forward -i em-+ -j DROP +``` + +One rule delivers VM-to-VM in both directions. A packet from VM A to +VM B matches with `i=tapA, o=tapB`, and B's reply matches with +`i=tapB, o=tapA`, so no conntrack state rule is needed. Because the +rule is scoped to this install's prefix on both sides, cross-install +traffic does not match it. + +The terminal `DROP` is what makes the contract absolute. Without it, +traffic out of an ember TAP that matches none of our accepts falls +through to the host's `FORWARD` rules, and whether a VM can reach +docker0, another install's TAPs, or a libvirt bridge would again +depend on the host policy. With it, a VM's forwarded traffic can only +go to a sibling TAP or out the WAN interface. + +`ember--forward`, per VM, unchanged in shape from today apart from +living in the chain and dropping the comment match: + +``` +-I ember--forward 1 -i -o -j ACCEPT +-I ember--forward 1 -i -o -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +``` + +These stay per-VM rather than being wildcarded because the WAN +interface is captured at VM start and persisted in `NetworkInfo`. A +laptop that moves from ethernet to wifi between two VM starts gets a +correct rule for each VM, and teardown deletes exactly what setup +added. + +The comment match is dropped for rules inside our chains. Its only +job was scoping `-D` so one install cannot delete another's rules +(`nat.rs::comment`), and an install-owned chain does that +structurally. + +### Rule order is correct by construction + +The chains hold accepts and exactly one terminal drop, so ordering +needs no comparison logic and no rewriting: + +- Accepts are added with `-C` then `-I 1`, so they land above + the drop. +- The single drop is added with `-C` then `-A `, so it lands at + the bottom. + +Accepts commute with each other, so their relative order is +irrelevant, and the drop is always last. This holds no matter what +subset of rules already exists, which is what makes `ensure` below a +handful of idempotent calls instead of a diff-and-rebuild. + +### NAT stays where it is + +The `POSTROUTING` masquerade rule keeps its current form, its comment +tag, and its home in the shared `nat` table. It is a per-address +translation rather than a policy decision, ordering collisions there +are rare because masquerade rules are source-scoped, and the comment +already solves cross-install scoping for it. Giving `nat` a third +ember chain is possible but buys little (see Open decisions). + +Masquerade only matches `-o `, so VM-to-VM traffic is not +SNATed. VM B sees VM A's real guest address. + +### Lifecycle + +**Ensure at VM start.** `LinuxNetwork::setup` calls +`policy::ensure(namespace)` right after `enable_ip_forwarding()`, for +the same reason that call is there: iptables state does not survive a +reboot, and re-asserting cheap host-wide prep on every VM start is how +ember already recovers. Creating the chains only at `ember init` +would mean a reboot silently drops the host block, and silently losing +a security property is the failure mode this spec exists to remove. + +`ensure` is idempotent: create each chain, tolerating "Chain already +exists"; `-C || -I 1` each accept; `-C || -A` the drop; `-C || -I 1` +each jump. Two concurrent `vm start` runs can duplicate a rule, which +is harmless, and `iptables_delete` already loops to remove duplicates. + +**Teardown at deinit.** `NetworkBackend` gains + +```rust +/// Remove host-wide firewall state owned by this installation. +/// Default implementation is a no-op for backends that keep no such +/// state (macOS vmnet). +fn deinit(&self, config: &GlobalConfig) -> Result<()> { Ok(()) } +``` + +The Linux implementation deletes the two jumps, flushes, and deletes +the two chains, all scoped to this install's names so a second +install is untouched. `src/cli/deinit.rs` calls it before +`storage.deinit`, best-effort with a warning on failure, since a +leftover chain should not block a deinit. Deinit already refuses to +run while VMs are registered, so no per-VM rules are in the chain by +then. + +**Reconcile does nothing new.** Idle chains with no matching TAPs have +no effect, so pruning them when the last VM stops would be churn for +nothing. Rules for dead VMs are already cleaned by +`network::cleanup`. + +### IPv6 + +The stock kernel brings up IPv6 on the TAP, so both ends get a +link-local address and a guest can reach the host over IPv6 while the +IPv4 block is in force. Rather than mirroring every rule into +`ip6tables`, `tap::create` disables the stack on the interface it just +created: + +``` +sysctl -w net.ipv6.conf..disable_ipv6=1 +``` + +The host then has no IPv6 address on the link and ignores inbound +IPv6, which closes host access and, since IPv6 forwarding is off and +unaddressed, VM-to-VM over IPv6 too. A kernel built without IPv6 has +no such sysctl, so a missing key is ignored. This matches ember being +IPv4-only by design and keeps the rule surface half the size. + +### Legacy and upgrade + +The legacy path is the hot path, not a compat afterthought. The +reference install this spec was written against has a `config.json` +with no `instance_id`, three running VMs on `em-`-prefixed TAPs, and +docker on the host, which is both why VM-to-VM does not work there +(docker sets `FORWARD DROP`) and why `ember-input` / `ember-forward` +and the `em-+` wildcard need as much care as the tagged names. + +A VM started by an older binary has its two `FORWARD` rules in the +built-in chain, tagged with the comment. After an upgrade, teardown +must delete them from there, while newly started VMs get rules in the +chain. `NetworkInfo` records which applies: + +```rust +/// iptables chain holding this VM's FORWARD rules. `None` means the +/// rules were added by a binary that appended them to the built-in +/// FORWARD chain with a comment match, and must be deleted from +/// there. +#[serde(default)] +pub firewall_chain: Option, +``` + +`serde(default)` makes an old `vm.json` deserialize to `None` and take +the legacy delete path. This is the same trick `NetworkInfo.wan_iface` +already uses, and it pins the chain name per VM so a later rename does +not orphan rules. + +`nat::add_rules` and `remove_rules` grow past a comfortable positional +argument count, so both take one struct built at setup and rebuilt at +teardown from `NetworkInfo`: + +```rust +pub struct VmRules<'a> { + /// `None` selects the legacy shape: rules in the built-in FORWARD + /// chain, with the comment match. + pub chain: Option<&'a str>, + pub tap_device: &'a str, + pub guest_ip: &'a str, + pub wan_iface: &'a str, + pub comment: &'a str, +} +``` + +`comment` stays needed in both modes, for the masquerade rule in the +new mode and for all three rules in the legacy mode. + +### Landing order hazard + +The terminal `DROP` in `ember--forward` must not exist until the +per-VM egress accepts live inside that chain. A half-landed change +where the chain is jumped from `FORWARD` position 1 with its drop in +place while per-VM accepts are still appended to the built-in chain +kills all VM egress, because the drop is reached first. Either land +the chain work and the per-VM move together, or add the drop last. + +## Code changes + +`crates/ember-linux/src/network/`: + +- **`iptables.rs`** (new, small). The exec layer: `run`, `delete` + (the existing duplicate-tolerant loop), `exists` (`-C`), + `new_chain`, `delete_chain`, `flush_chain`. Every invocation gains + `-w 5`. That flag is missing today, which means two concurrent + `ember vm start` runs can already fail on the xtables lock, and this + spec adds more calls per start. +- **`nat.rs`**. Keeps per-VM rules, takes `VmRules`, emits into the + chain or the legacy shape. Loses the exec helpers to `iptables.rs`. +- **`policy.rs`** (new). Chain names, `ensure`, `deinit`, the static + rule set. The boundary: `iptables.rs` knows how to run iptables, + `nat.rs` owns per-VM rules, `policy.rs` owns install-scoped chains + and the policy in them. +- **`tap.rs`**. Disable IPv6 on the device after bringing it up. + +Outside networking: + +- `network_backend.rs`: call `policy::ensure`, pass `VmRules`, put the + chain name in `NetworkInfo`. +- `network.rs::cleanup`: rebuild `VmRules` from `NetworkInfo`. +- `ember-core/src/state/vm.rs`: `NetworkInfo.firewall_chain`. +- `ember-core/src/backend.rs`: `NetworkBackend::deinit` with a default + no-op body. +- `src/cli/deinit.rs`: call it. +- `src/cli/info.rs`: print the two chain names, cheap diagnostic next + to the existing dm-thin pool line. +- `docs/SPEC.md`: rewrite the Networking rule listing. + +Rough size: 350 to 450 lines including tests, most of it in +`policy.rs` and its unit tests. + +## Consequences and accepted limitations + +- **The host is unreachable from a guest, including the gateway + address.** A guest pinging its default gateway, or running + traceroute, sees nothing. Traffic is dropped rather than rejected, + so guest connections to the host hang until timeout instead of + failing fast. `REJECT --reject-with icmp-admin-prohibited` is a + one-line change if the hang turns out to be more annoying than the + silence is principled. +- **A host-run DNS resolver breaks guest DNS.** + `dns::detect_nameservers` filters loopback, but a host running + dnsmasq or pihole bound to its LAN address hands the guest a + nameserver that is a host address, which the block now drops. Worth + a warning at VM start: compare the detected nameservers against the + host's own addresses and say so out loud rather than letting DNS + fail mysteriously. +- **VM egress now works on hosts where it silently did not.** Moving + the egress accept into a chain at `FORWARD` position 1 punches + through mid-chain rejects from ufw or firewalld. This is a bug fix + against ember's documented promise of outbound access, and it is + also ember overriding the host admin's firewall. Called out as a + decision below. +- **A WAN interface change breaks a running VM's egress explicitly + rather than accidentally.** Its egress accept and masquerade both + name the old interface, so the terminal drop now stops the traffic. + Today it would fall through to a permissive host policy and leave + the guest sending unSNATed private-source packets out the new + interface, which fails upstream anyway. Not theoretical: the + reference install's WAN interface is `wg0-mullvad`, which comes and + goes with the VPN. +- **Inbound from other host bridges is one-way blocked.** A docker + container connecting to a VM is not matched by our chain on the way + in, but the VM's replies hit the terminal drop, so the connection + does not work. Consistent with the contract, worth knowing. +- **A host firewall flush while VMs run degrades the policy until the + next VM start.** Rules are re-asserted at start, not continuously. + Reporting policy health from `ember info` would close the + observability gap. + +## Open decisions + +1. **Should VM-to-host be overridable?** Running a service on the + host and hitting it from a VM is a common dev workflow, and this + spec makes it impossible. The escape hatch would be an install-wide + `ember init --allow-host-access` persisted on `GlobalConfig` and + read by `policy::ensure`, which then omits the drop or accepts the + TAP gateway address only. Not specced, since the ask was to block + the host. +2. **Is punching through the host's `FORWARD` rules acceptable?** The + alternative is leaving egress appended at the bottom of the + built-in chain, which keeps ember deferential but makes egress and + VM-to-VM behave inconsistently on restrictive hosts. +3. **Should masquerade move into an `ember--postrouting` chain + too?** Uniform scoping and immunity to `POSTROUTING` ordering, at + the cost of a third chain and lifecycle. It does not let the + comment machinery retire, since legacy deletes need it regardless. +4. **Per-VM opt-out.** A `network.isolated: true` in the VM config + would be a per-VM drop inserted above the sibling accept. Cheap to + add later, out of scope now. + +## Testing + +Unit, pure functions, matching how `nat.rs` and `tap.rs` are tested +today: + +- Chain name derivation for a tagged install and a legacy install, + with the 28-character budget asserted. +- The static rule vectors for both chains, locking the established + accept ahead of the drop and the wildcard form of both interface + matches. +- `VmRules` rendering in chain mode and legacy mode, locking that + legacy mode reproduces today's byte-for-byte rule including the + comment, and that chain mode omits the comment on `FORWARD` rules + and keeps it on masquerade. + +Integration, `#[ignore]`, root plus iptables, in a new +`tests/network_policy.rs`: + +- Two VMs of one install, ping and a TCP connect from A to B succeed. +- From inside a VM, `ping -c1 -W1 ` and a TCP connect to the + host's TAP address and to the host's LAN address all fail. +- `ember ssh` into a VM still works, which is the regression guard for + the established accept. +- Egress still works, one outbound connectivity check from a guest. +- Deinit removes both chains and both jumps, and a second install's + chains survive it. This extends the `tests/isolation.rs` family. +- Cross-install: install A's VM cannot reach install B's VM. + +## Alternatives considered + +**A shared bridge, like the macOS vmnet model.** Put every TAP on one +Linux bridge, switch the allocator to `allocate_single`, and VM-to-VM +becomes native L2 with no forwarding rules at all. Rejected as too +large a change for the ask: it replaces the documented /30 +point-to-point model, needs a migration path for running VMs, and +still needs the whole `INPUT` design for host blocking, since the +bridge address is a host address. It also gives up a property worth +keeping, with point-to-point links the host routes every packet +between VMs, which is what makes a future per-VM policy knob a +one-rule change. + +**Appending to the built-in chains.** Rejected, see Install-owned +chains. Ordering makes it a policy that silently might not apply. + +**Per-VM-pair forward rules.** O(n^2) rules and churn on every start +and stop, where one wildcard rule does the job. + +**An `ip6tables` mirror instead of disabling IPv6 on the TAP.** +Doubles the rule surface to police a stack ember never configures. From a0d20561e338ae38218960a903d6f737b4e2de57 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:11:39 +0200 Subject: [PATCH 02/14] net: route VM firewall rules through install-owned chains VM-to-VM reachability and host isolation were both accidents of the host's iptables policy: nothing permitted forwarding between two TAPs, nothing denied it, and nothing stopped a guest reaching the host at its gateway address. Which way it went depended on whether something else on the host had set FORWARD DROP. Each installation now owns 'ember--input' and 'ember--forward', jumped to from position 1 of the built-in chains, holding one contract: a VM reaches the internet and its siblings, and nothing else. Appending could not deliver that, a pre-existing 'trust the LAN' ACCEPT in INPUT or a mid-chain REJECT in FORWARD is reached first. Jumping in at the top is safe because every rule in the chains matches an ember TAP, so other traffic falls through unchanged. Rule order needs no bookkeeping: the chains hold ACCEPTs plus one terminal DROP, ACCEPTs are inserted at the front and the DROP is appended, so the DROP stays last whatever subset already exists. Per-VM forwarding rules move into the install's chain, where the chain is the scope and the comment match becomes redundant. Masquerade stays in the shared nat table with its comment, unchanged in shape, so rules written before and after this change stay mutually deletable. NetworkInfo records which chain a VM's rules went into, so a VM started by an older binary is still torn down from the built-in FORWARD chain. A Rule type now backs every iptables call, with one definition serving the add, check and delete paths. iptables compares full rule text on -D, so the previous duplicate spelling of each rule was a standing invitation to leak rules. Every invocation also takes the xtables lock, which was missing and could fail two concurrent VM starts. --- crates/ember-core/src/state/vm.rs | 11 + crates/ember-linux/src/network.rs | 27 +- crates/ember-linux/src/network/iptables.rs | 310 ++++++++++++++ crates/ember-linux/src/network/nat.rs | 463 +++++++++++++-------- crates/ember-linux/src/network/policy.rs | 335 +++++++++++++++ crates/ember-linux/src/network/tap.rs | 23 + crates/ember-linux/src/network_backend.rs | 25 +- crates/ember-macos/src/network.rs | 3 + crates/ember-macos/src/vm.rs | 1 + 9 files changed, 996 insertions(+), 202 deletions(-) create mode 100644 crates/ember-linux/src/network/iptables.rs create mode 100644 crates/ember-linux/src/network/policy.rs diff --git a/crates/ember-core/src/state/vm.rs b/crates/ember-core/src/state/vm.rs index bc457af..96a143c 100644 --- a/crates/ember-core/src/state/vm.rs +++ b/crates/ember-core/src/state/vm.rs @@ -61,6 +61,16 @@ pub struct NetworkInfo { /// even if the default route changes between start and stop. #[serde(default)] pub wan_iface: Option, + /// iptables chain holding this VM's forwarding rules (Linux only). + /// + /// `None` means the rules were appended to the built-in FORWARD + /// chain, tagged with the per-installation comment, which is what + /// binaries predating the install-owned policy chains wrote. + /// Teardown of such a VM has to delete them from there, so where a + /// VM's rules live is recorded per VM rather than derived from the + /// current config. + #[serde(default)] + pub firewall_chain: Option, } /// SSH connection configuration for a VM. @@ -529,6 +539,7 @@ mod tests { netmask: "255.255.255.252".to_string(), guest_mac: Some("AA:FC:00:00:00:01".to_string()), wan_iface: Some("eth0".to_string()), + firewall_chain: Some("ember-a3f4-forward".to_string()), }); vm.status = VmStatus::Running; vm.pid = Some(42); diff --git a/crates/ember-linux/src/network.rs b/crates/ember-linux/src/network.rs index dfa27f5..51c652e 100644 --- a/crates/ember-linux/src/network.rs +++ b/crates/ember-linux/src/network.rs @@ -1,6 +1,8 @@ pub mod dns; pub mod ip; +pub mod iptables; pub mod nat; +pub mod policy; pub mod tap; pub mod wan; @@ -10,19 +12,24 @@ use ember_core::state::vm::NetworkInfo; /// Best-effort cleanup of networking resources for a VM (Linux only). /// -/// The iptables comment is derived via [`nat::comment`] from the -/// install's namespace so the `-D` calls only match this -/// installation's rules even when another ember install on the same -/// host has rules for the same TAP/IP. +/// Rules are removed in the shape they were added: `net_info` records +/// both the WAN interface captured at start and the chain the VM's +/// forwarding rules went into, so a default-route change or a binary +/// upgrade between start and stop cannot turn the `-D` calls into +/// silent no-ops. The iptables comment comes from [`nat::comment`] so +/// deletions in shared chains only ever match this installation's +/// rules. pub fn cleanup(store: &StateStore, config: &GlobalConfig, vm_name: &str, net_info: &NetworkInfo) { let wan_iface = net_info.wan_iface.clone().or_else(|| wan::detect().ok()); if let Some(wan_iface) = wan_iface { - let _ = nat::remove_rules( - &net_info.tap_device, - &net_info.guest_ip, - &wan_iface, - &nat::comment(config.instance_namespace()), - ); + nat::VmRules { + chain: net_info.firewall_chain.as_deref(), + tap_device: &net_info.tap_device, + guest_ip: &net_info.guest_ip, + wan_iface: &wan_iface, + comment: &nat::comment(config.instance_namespace()), + } + .remove(); } let _ = tap::delete(&net_info.tap_device); let _ = ip::release(store, vm_name); diff --git a/crates/ember-linux/src/network/iptables.rs b/crates/ember-linux/src/network/iptables.rs new file mode 100644 index 0000000..96b0f8f --- /dev/null +++ b/crates/ember-linux/src/network/iptables.rs @@ -0,0 +1,310 @@ +//! Thin wrapper around the `iptables` binary. +//! +//! Every iptables call in ember goes through here, so that the +//! xtables lock is always taken and so that "the thing you named +//! isn't there" is told apart from a real failure in exactly one +//! place. + +use std::process::{Command, Output}; + +use ember_core::error::{Error, Result}; + +/// Seconds to wait for the xtables lock. +/// +/// iptables exits rather than blocking when another process holds the +/// lock, and two concurrent `ember vm start` runs each insert rules. +/// Five seconds is far longer than a handful of insertions needs, and +/// still fails loudly if something is wedged holding the lock. +const LOCK_WAIT_SECS: &str = "5"; + +/// Where a rule goes in its chain. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Placement { + /// Insert at position 1. Used for every ACCEPT, so it lands above + /// the terminal DROP of an ember chain, and for chain jumps, so + /// they land above whatever else the host keeps in INPUT/FORWARD. + Front, + /// Append. Used for a chain's single terminal DROP, which has to + /// stay last, and for rules in shared chains where ember has no + /// business jumping ahead of the host's own rules. + Back, +} + +/// One iptables rule, with the add and delete paths sharing a single +/// definition. +/// +/// iptables compares the full rule text when deleting, so a `-D` +/// whose arguments differ in any way from the `-A` that created it +/// silently no-ops and the rule leaks. Building both paths from one +/// value removes that failure mode by construction. +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Rule { + table: Option<&'static str>, + chain: String, + args: Vec, + placement: Placement, +} + +impl Rule { + /// A rule in the `filter` table, which is iptables' default. + pub fn filter(chain: impl Into, args: &[&str]) -> Self { + Self { + table: None, + chain: chain.into(), + args: args.iter().map(|s| s.to_string()).collect(), + placement: Placement::Back, + } + } + + /// A rule in the `nat` table. + pub fn nat(chain: impl Into, args: &[&str]) -> Self { + Self { + table: Some("nat"), + ..Self::filter(chain, args) + } + } + + /// Place this rule at the front of its chain instead of appending. + pub fn at_front(mut self) -> Self { + self.placement = Placement::Front; + self + } + + /// Add the rule, unconditionally. Fails if it is already there + /// only in the sense that a duplicate is created, which is why + /// callers on idempotent paths use [`ensure`](Self::ensure). + pub fn add(&self) -> Result<()> { + run(&self.add_args()).map(|_| ()) + } + + /// Add the rule unless an identical one already exists. + pub fn ensure(&self) -> Result<()> { + if self.exists()? { + return Ok(()); + } + self.add() + } + + /// Whether an identical rule is already present. + /// + /// A missing chain reports `false` rather than an error: the rule + /// is not there, which is all the caller asked. + pub fn exists(&self) -> Result { + match run(&self.check_args()) { + Ok(_) => Ok(true), + Err(Error::Network(msg)) if is_absent(&msg) => Ok(false), + Err(e) => Err(e), + } + } + + /// Remove every copy of the rule. + /// + /// `iptables -D` deletes one match at a time, so we loop until + /// iptables reports nothing left. Duplicates are possible when two + /// `vm start` runs raced on an [`ensure`](Self::ensure), and a + /// leftover copy would outlive the VM. Idempotent: a rule (or + /// chain) that was never there is not an error. + pub fn remove(&self) -> Result<()> { + let args = self.delete_args(); + loop { + match run(&args) { + Ok(_) => continue, + Err(Error::Network(msg)) if is_absent(&msg) => return Ok(()), + Err(e) => return Err(e), + } + } + } + + /// The `iptables` arguments that [`add`](Self::add) runs. + /// Crate-visible so rule shape can be asserted without touching + /// the host. + pub(crate) fn add_args(&self) -> Vec { + match self.placement { + Placement::Front => self.args_with(&["-I", &self.chain, "1"]), + Placement::Back => self.args_with(&["-A", &self.chain]), + } + } + + pub(crate) fn check_args(&self) -> Vec { + self.args_with(&["-C", &self.chain]) + } + + pub(crate) fn delete_args(&self) -> Vec { + self.args_with(&["-D", &self.chain]) + } + + /// Full argument vector: lock wait, table selection, the verb the + /// caller wants, then the rule body. + fn args_with(&self, verb: &[&str]) -> Vec { + let mut out: Vec = vec!["-w".into(), LOCK_WAIT_SECS.into()]; + if let Some(table) = self.table { + out.push("-t".into()); + out.push(table.into()); + } + out.extend(verb.iter().map(|s| s.to_string())); + out.extend(self.args.iter().cloned()); + out + } +} + +/// Create a chain in the `filter` table if it isn't there already. +pub fn ensure_chain(chain: &str) -> Result<()> { + match run(&args(&["-N", chain])) { + Ok(_) => Ok(()), + // iptables has no "create if absent", so an existing chain + // comes back as a plain error we have to recognize by text. + Err(Error::Network(msg)) if msg.contains("Chain already exists") => Ok(()), + Err(e) => Err(e), + } +} + +/// Flush and delete a chain in the `filter` table. +/// +/// Idempotent. A chain that doesn't exist is not an error. The chain +/// must already be unreferenced, iptables refuses to delete a chain +/// that something still jumps to. +pub fn remove_chain(chain: &str) -> Result<()> { + for verb in [["-F", chain], ["-X", chain]] { + match run(&args(&verb)) { + Ok(_) => {} + Err(Error::Network(msg)) if is_absent(&msg) => {} + Err(e) => return Err(e), + } + } + Ok(()) +} + +/// Argument vector for a command that isn't a rule operation. +fn args(verb: &[&str]) -> Vec { + let mut out: Vec = vec!["-w".into(), LOCK_WAIT_SECS.into()]; + out.extend(verb.iter().map(|s| s.to_string())); + out +} + +/// True when iptables is saying the rule or chain we named isn't +/// there. +/// +/// Both messages mean "nothing to do" on an idempotent path. The +/// first comes from `-C`/`-D` against a missing rule, the second from +/// naming a chain that doesn't exist. The strings are matched loosely +/// because their wording differs between the legacy and nft backends. +fn is_absent(stderr: &str) -> bool { + stderr.contains("does a matching rule exist") || stderr.contains("No chain/target/match") +} + +fn run(args: &[String]) -> Result { + let output = Command::new("iptables") + .args(args) + .output() + .map_err(|e| Error::CommandExec { + command: "iptables".into(), + source: e, + })?; + + if output.status.success() { + return Ok(output); + } + + // Errors carry iptables' own stderr because callers match on it + // to recognize the absent-rule and existing-chain cases. + Err(Error::Network( + String::from_utf8_lossy(&output.stderr).trim().to_string(), + )) +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn append_is_the_default_placement() { + let rule = Rule::filter("FORWARD", &["-i", "tap0", "-j", "ACCEPT"]); + assert_eq!( + rule.add_args(), + ["-w", "5", "-A", "FORWARD", "-i", "tap0", "-j", "ACCEPT"] + ); + } + + /// Front placement is what keeps ACCEPTs above a chain's terminal + /// DROP without any rule-order comparison. + #[test] + fn front_placement_inserts_at_position_one() { + let rule = Rule::filter("ember-forward", &["-i", "tap0", "-j", "ACCEPT"]).at_front(); + assert_eq!( + rule.add_args(), + [ + "-w", + "5", + "-I", + "ember-forward", + "1", + "-i", + "tap0", + "-j", + "ACCEPT" + ] + ); + } + + #[test] + fn table_selection_precedes_the_verb() { + let rule = Rule::nat("POSTROUTING", &["-j", "MASQUERADE"]); + assert_eq!( + rule.add_args(), + [ + "-w", + "5", + "-t", + "nat", + "-A", + "POSTROUTING", + "-j", + "MASQUERADE" + ] + ); + } + + /// The add, check and delete forms must differ only in the verb. + /// Anything else and `-D` stops matching what `-A` created. + #[test] + fn check_and_delete_mirror_the_rule_body() { + let rule = Rule::nat("POSTROUTING", &["-s", "10.0.0.2/32", "-j", "MASQUERADE"]).at_front(); + let body = ["-s", "10.0.0.2/32", "-j", "MASQUERADE"]; + + assert_eq!(rule.check_args()[..5], ["-w", "5", "-t", "nat", "-C"]); + assert_eq!(rule.check_args()[6..], body); + assert_eq!(rule.delete_args()[..5], ["-w", "5", "-t", "nat", "-D"]); + assert_eq!(rule.delete_args()[6..], body); + } + + /// Placement is an add-time concern only. Deleting must not care, + /// or a front-placed rule would be undeletable. + #[test] + fn placement_does_not_leak_into_delete() { + let body = ["-i", "tap0", "-j", "ACCEPT"]; + let back = Rule::filter("FORWARD", &body); + let front = Rule::filter("FORWARD", &body).at_front(); + assert_eq!(back.delete_args(), front.delete_args()); + } + + #[test] + fn every_invocation_waits_for_the_xtables_lock() { + let rule = Rule::filter("FORWARD", &["-j", "ACCEPT"]); + for v in [rule.add_args(), rule.check_args(), rule.delete_args()] { + assert_eq!(v[..2], ["-w", "5"], "missing lock wait in {v:?}"); + } + assert_eq!(args(&["-N", "ember-input"])[..2], ["-w", "5"]); + } + + #[test] + fn absent_recognizes_both_iptables_wordings() { + assert!(is_absent( + "iptables: Bad rule (does a matching rule exist in that chain?)." + )); + assert!(is_absent("iptables: No chain/target/match by that name.")); + assert!(!is_absent("iptables: Chain already exists.")); + assert!(!is_absent( + "iptables v1.8.13 (nf_tables): Permission denied (you must be root)" + )); + } +} diff --git a/crates/ember-linux/src/network/nat.rs b/crates/ember-linux/src/network/nat.rs index a2f8d0f..4769f20 100644 --- a/crates/ember-linux/src/network/nat.rs +++ b/crates/ember-linux/src/network/nat.rs @@ -1,27 +1,37 @@ -//! iptables NAT/masquerade rule management. +//! Per-VM iptables rules. //! -//! Each VM gets three iptables rules for outbound network connectivity: +//! Three rules give one guest outbound connectivity: //! -//! 1. **POSTROUTING MASQUERADE** — rewrites guest source IP for outbound traffic -//! 2. **FORWARD (outbound)** — allows traffic from TAP device to WAN interface -//! 3. **FORWARD (inbound)** — allows established/related return traffic from WAN to TAP +//! 1. **POSTROUTING MASQUERADE** rewrites the guest source IP on the +//! way out, so the guest's private address never leaves the host. +//! 2. **FORWARD (outbound)** permits traffic from the VM's TAP to the +//! WAN interface. +//! 3. **FORWARD (inbound)** permits established and related return +//! traffic back from the WAN interface to the TAP. //! -//! Rules are added on VM start and removed on VM stop/delete. The `remove_rules` -//! function is idempotent — it silently ignores errors when rules don't exist. - -use std::process::Command; +//! Rules 2 and 3 live in the installation's own FORWARD chain (see +//! [`super::policy`]), which is where the install-wide policy that +//! decides VM-to-VM and host reachability lives. Rule 1 stays in the +//! shared `nat` POSTROUTING chain, because address translation is not +//! a policy decision and has no ordering interaction with anything the +//! host keeps there. +//! +//! Rules are added on VM start and removed on stop, delete, and +//! crash recovery. Removal is idempotent. -use ember_core::error::{Error, Result}; +use super::iptables::Rule; +use ember_core::error::Result; /// iptables comment that scopes rule cleanup to one ember install. /// /// `Some(ns)` → `ember:{ns}`, embedded via `-m comment --comment` in -/// every rule so `-D` only matches *this* install's rules. `None` -/// returns the empty string, which [`with_comment`] uses as the -/// signal to omit the `-m comment` match entirely — older binaries -/// added rules without a comment match, so emitting one on legacy -/// installs would make `iptables -D` silently no-op and rules would -/// accumulate forever. Empty preserves the original rule shape. +/// every rule that lives in a chain ember does not own, so `-D` only +/// matches *this* install's rules. `None` returns the empty string, +/// which [`with_comment`] uses as the signal to omit the `-m comment` +/// match entirely. Older binaries added rules without a comment +/// match, so emitting one on legacy installs would make `iptables -D` +/// silently no-op and rules would accumulate forever. Empty preserves +/// the original rule shape. pub fn comment(instance_id: Option<&str>) -> String { match instance_id { None => String::new(), @@ -29,128 +39,113 @@ pub fn comment(instance_id: Option<&str>) -> String { } } -/// Add iptables NAT and forwarding rules for a VM. -/// -/// Creates three rules that together give the guest outbound internet access -/// through the host's WAN interface via masquerading (SNAT): -/// -/// ```text -/// -t nat -A POSTROUTING -s /32 -o [-m comment --comment ] -j MASQUERADE -/// -A FORWARD -i -o [-m comment --comment ] -j ACCEPT -/// -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED [-m comment --comment ] -j ACCEPT -/// ``` +/// The iptables rules belonging to one VM. /// -/// `comment` is a per-installation tag (e.g. `ember:a3f4`) embedded in -/// every rule via the `comment` match. It lets cleanup scope deletions -/// to this installation's rules and lets users grep `iptables-save` for -/// "rules ember put here". An empty `comment` skips the match entirely -/// so rules added by older ember binaries (which never tagged anything) -/// stay byte-for-byte identical and remain matchable by `remove_rules`. -pub fn add_rules(tap_device: &str, guest_ip: &str, wan_iface: &str, comment: &str) -> Result<()> { - let guest_cidr = format!("{guest_ip}/32"); - - iptables(&with_comment( - &[ - "-t", - "nat", - "-A", - "POSTROUTING", - "-s", - &guest_cidr, - "-o", - wan_iface, - ], - comment, - &["-j", "MASQUERADE"], - ))?; - - iptables(&with_comment( - &["-A", "FORWARD", "-i", tap_device, "-o", wan_iface], - comment, - &["-j", "ACCEPT"], - ))?; +/// One value describes both the add and the remove path, so a rule +/// can never be deleted in a shape that differs from how it was +/// added. Built from live allocation data at VM start and rebuilt from +/// the persisted [`NetworkInfo`](ember_core::state::vm::NetworkInfo) +/// at teardown. +pub struct VmRules<'a> { + /// FORWARD chain the VM's two forwarding rules live in. + /// + /// `None` selects the legacy shape: rules appended straight to the + /// built-in FORWARD chain and tagged with `comment`, which is what + /// binaries predating [`super::policy`] wrote. Teardown of a VM + /// started by such a binary has to delete them from there, so this + /// is persisted per VM rather than derived from the current + /// config. + pub chain: Option<&'a str>, + pub tap_device: &'a str, + pub guest_ip: &'a str, + pub wan_iface: &'a str, + /// Per-installation tag from [`comment`]. Empty on legacy + /// installs. + pub comment: &'a str, +} - iptables(&with_comment( - &[ - "-A", - "FORWARD", - "-i", - wan_iface, - "-o", - tap_device, - "-m", - "conntrack", - "--ctstate", - "RELATED,ESTABLISHED", - ], - comment, - &["-j", "ACCEPT"], - ))?; +impl VmRules<'_> { + /// Add every rule, skipping any that is already present. + /// + /// Idempotent so a retried VM start cannot leave duplicates + /// behind. + pub fn add(&self) -> Result<()> { + for rule in self.rules() { + rule.ensure()?; + } + Ok(()) + } - Ok(()) -} + /// Remove every rule, best effort. + /// + /// Called from stop, delete, and crash recovery, where a rule that + /// is already gone is the normal case and a failure to remove one + /// must not abort cleanup of the rest. + pub fn remove(&self) { + for rule in self.rules() { + let _ = rule.remove(); + } + } -/// Remove iptables NAT and forwarding rules for a VM. -/// -/// Mirrors [`add_rules`] but uses `-D` (delete) instead of `-A` (append). -/// Idempotent — silently ignores errors when rules don't exist. The -/// `comment` argument must match the value passed to [`add_rules`]; -/// iptables compares the full rule including the comment match, so a -/// wrong tag turns the delete into a no-op rather than removing -/// another install's rule. -pub fn remove_rules( - tap_device: &str, - guest_ip: &str, - wan_iface: &str, - comment: &str, -) -> Result<()> { - let guest_cidr = format!("{guest_ip}/32"); + /// The rules, in the order `add` applies them. + fn rules(&self) -> Vec { + let guest_cidr = format!("{}/32", self.guest_ip); - let _ = iptables_delete(&with_comment( - &[ - "-t", - "nat", - "-D", + // The masquerade rule keeps the same shape in both modes: it + // has always lived in the shared POSTROUTING chain with the + // comment as its only scoping, so rules written before and + // after the policy chains existed are byte-for-byte identical + // and stay mutually deletable. + let masquerade = Rule::nat( "POSTROUTING", - "-s", - &guest_cidr, - "-o", - wan_iface, - ], - comment, - &["-j", "MASQUERADE"], - )); - - let _ = iptables_delete(&with_comment( - &["-D", "FORWARD", "-i", tap_device, "-o", wan_iface], - comment, - &["-j", "ACCEPT"], - )); + &with_comment( + &["-s", &guest_cidr, "-o", self.wan_iface], + self.comment, + &["-j", "MASQUERADE"], + ), + ); - let _ = iptables_delete(&with_comment( - &[ - "-D", - "FORWARD", + let outbound = &["-i", self.tap_device, "-o", self.wan_iface]; + let inbound = &[ "-i", - wan_iface, + self.wan_iface, "-o", - tap_device, + self.tap_device, "-m", "conntrack", "--ctstate", "RELATED,ESTABLISHED", - ], - comment, - &["-j", "ACCEPT"], - )); + ]; - Ok(()) + let (outbound, inbound) = match self.chain { + // Inside a chain ember owns, the chain itself is the + // scope, so the comment match would be noise. Front + // placement keeps both ACCEPTs above the chain's terminal + // DROP without having to inspect rule order. + Some(chain) => ( + Rule::filter(chain, &[outbound.as_slice(), &["-j", "ACCEPT"]].concat()).at_front(), + Rule::filter(chain, &[inbound.as_slice(), &["-j", "ACCEPT"]].concat()).at_front(), + ), + None => ( + Rule::filter( + "FORWARD", + &with_comment(outbound, self.comment, &["-j", "ACCEPT"]), + ), + Rule::filter( + "FORWARD", + &with_comment(inbound, self.comment, &["-j", "ACCEPT"]), + ), + ), + }; + + vec![masquerade, outbound, inbound] + } } /// Splice `-m comment --comment ` between rule head and tail -/// when `comment` is non-empty. Empty comment yields the unwrapped -/// rule, matching what older ember binaries emitted byte-for-byte — -/// crucial because iptables compares full rules during `-D`. +/// when `comment` is non-empty. An empty comment yields the unwrapped +/// rule, matching what older ember binaries emitted byte-for-byte, +/// which matters because iptables compares full rules during `-D`. fn with_comment<'a>(head: &[&'a str], comment: &'a str, tail: &[&'a str]) -> Vec<&'a str> { let mut out = Vec::with_capacity(head.len() + tail.len() + 4); out.extend_from_slice(head); @@ -161,74 +156,24 @@ fn with_comment<'a>(head: &[&'a str], comment: &'a str, tail: &[&'a str]) -> Vec out } -/// Enable IPv4 forwarding via sysctl. -/// -/// This is required once before any VM can route traffic through the host. -/// Safe to call multiple times — sysctl is idempotent. -pub fn enable_ip_forwarding() -> Result<()> { - let output = Command::new("sysctl") - .args(["-w", "net.ipv4.ip_forward=1"]) - .output() - .map_err(|e| Error::CommandExec { - command: "sysctl".into(), - source: e, - })?; - Error::check_command("sysctl", output)?; - Ok(()) -} - -/// Run an iptables command, returning an error on failure. -fn iptables(args: &[&str]) -> Result<()> { - let output = Command::new("iptables") - .args(args) - .output() - .map_err(|e| Error::CommandExec { - command: "iptables".into(), - source: e, - })?; - Error::check_command("iptables", output)?; - Ok(()) -} - -/// Run an iptables delete command, removing ALL matching instances. -/// -/// `iptables -D` only removes the first match. If the same rule was -/// added multiple times (e.g. a test VM and a manual VM both at the -/// same IP), we need to loop until all copies are gone. -/// -/// Silently ignores "rule doesn't exist" errors for idempotent cleanup. -fn iptables_delete(args: &[&str]) -> Result<()> { - loop { - let output = - Command::new("iptables") - .args(args) - .output() - .map_err(|e| Error::CommandExec { - command: "iptables".into(), - source: e, - })?; - - if output.status.success() { - // Deleted one instance — loop to catch duplicates. - continue; - } +#[cfg(test)] +mod tests { + use super::*; - let stderr = String::from_utf8_lossy(&output.stderr); - if stderr.contains("does a matching rule exist") || stderr.contains("No chain/target/match") - { - // No more matching rules — done. - return Ok(()); + fn tagged(chain: Option<&'static str>) -> VmRules<'static> { + VmRules { + chain, + tap_device: "ema3f4-550e840", + guest_ip: "10.100.0.2", + wan_iface: "en0", + comment: "ember:a3f4", } - return Err(Error::Network(format!( - "iptables failed: {}", - stderr.trim() - ))); } -} -#[cfg(test)] -mod tests { - use super::*; + /// The exact `iptables` invocations `add` would run. + fn invocations(rules: &VmRules<'_>) -> Vec> { + rules.rules().iter().map(|r| r.add_args()).collect() + } #[test] fn comment_for_new_install_tags_namespace() { @@ -276,4 +221,152 @@ mod tests { ] ); } + + #[test] + fn every_vm_owns_exactly_three_rules() { + assert_eq!(tagged(Some("ember-a3f4-forward")).rules().len(), 3); + assert_eq!(tagged(None).rules().len(), 3); + } + + /// Masquerade must not move or change shape between modes: rules + /// written before and after the policy chains existed have to stay + /// mutually deletable. + #[test] + fn masquerade_is_identical_in_both_modes() { + let chained = invocations(&tagged(Some("ember-a3f4-forward"))); + let legacy = invocations(&tagged(None)); + assert_eq!(chained[0], legacy[0]); + assert_eq!( + chained[0], + [ + "-w", + "5", + "-t", + "nat", + "-A", + "POSTROUTING", + "-s", + "10.100.0.2/32", + "-o", + "en0", + "-m", + "comment", + "--comment", + "ember:a3f4", + "-j", + "MASQUERADE" + ] + ); + } + + /// Inside an ember-owned chain the chain is the scope, so the + /// comment match is dropped, and both ACCEPTs are inserted at the + /// front so they sit above the chain's terminal DROP. + #[test] + fn chain_mode_forward_rules_are_untagged_and_front_placed() { + let rules = invocations(&tagged(Some("ember-a3f4-forward"))); + assert_eq!( + rules[1], + [ + "-w", + "5", + "-I", + "ember-a3f4-forward", + "1", + "-i", + "ema3f4-550e840", + "-o", + "en0", + "-j", + "ACCEPT" + ] + ); + assert_eq!( + rules[2], + [ + "-w", + "5", + "-I", + "ember-a3f4-forward", + "1", + "-i", + "en0", + "-o", + "ema3f4-550e840", + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + "-j", + "ACCEPT" + ] + ); + } + + /// Locked: a VM started by a binary that predates the policy + /// chains has its rules appended to the built-in FORWARD chain and + /// tagged with the comment. Teardown has to reproduce that exactly + /// or `iptables -D` no-ops and the rules leak. + #[test] + fn legacy_mode_forward_rules_are_appended_to_builtin_chain() { + let rules = invocations(&tagged(None)); + assert_eq!( + rules[1], + [ + "-w", + "5", + "-A", + "FORWARD", + "-i", + "ema3f4-550e840", + "-o", + "en0", + "-m", + "comment", + "--comment", + "ember:a3f4", + "-j", + "ACCEPT" + ] + ); + assert_eq!( + rules[2], + [ + "-w", + "5", + "-A", + "FORWARD", + "-i", + "en0", + "-o", + "ema3f4-550e840", + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + "-m", + "comment", + "--comment", + "ember:a3f4", + "-j", + "ACCEPT" + ] + ); + } + + /// A legacy install has no namespace, so its rules carry no + /// comment match at all. + #[test] + fn legacy_install_rules_carry_no_comment_match() { + let rules = VmRules { + comment: "", + ..tagged(None) + }; + for invocation in invocations(&rules) { + assert!( + !invocation.contains(&"comment".to_string()), + "unexpected comment match: {invocation:?}" + ); + } + } } diff --git a/crates/ember-linux/src/network/policy.rs b/crates/ember-linux/src/network/policy.rs new file mode 100644 index 0000000..210d225 --- /dev/null +++ b/crates/ember-linux/src/network/policy.rs @@ -0,0 +1,335 @@ +//! Install-owned firewall chains: what an ember VM is allowed to +//! reach. +//! +//! An installation owns two chains, `ember--input` and +//! `ember--forward`, jumped to from position 1 of the built-in +//! INPUT and FORWARD chains. Together they deliver one contract: +//! +//! > A VM can reach the internet and the other VMs of its own +//! > installation. It cannot reach the host. +//! +//! Position 1 is what makes that a contract instead of a suggestion. +//! Appending cannot work: a pre-existing `-A INPUT -s 10.0.0.0/8 -j +//! ACCEPT`, a common "trust the LAN" rule that matches the default +//! guest range, would match first and the host block would silently +//! not apply. Appending to FORWARD has the mirror problem, a mid-chain +//! REJECT from ufw or firewalld is reached before our ACCEPT, which is +//! why VM-to-VM traffic does not work on a host running docker today. +//! +//! Jumping in at the top is only acceptable because both chains are +//! transparent to everything that is not an ember VM. Every rule in +//! them matches on an ember TAP interface, and a packet that matches +//! nothing falls off the end of the chain and resumes in the built-in +//! chain right after our jump. Non-ember traffic sees no change. +//! +//! Rule order inside the chains needs no comparison logic: the chains +//! hold ACCEPTs plus exactly one terminal DROP, ACCEPTs are inserted at +//! the front and the DROP is appended, so the DROP is always last no +//! matter which subset of rules already exists. That is what keeps +//! [`ensure`] a handful of idempotent calls rather than a +//! diff-and-rebuild. + +use std::process::Command; + +use ember_core::error::{Error, Result}; + +use super::iptables::{self, Rule}; +use super::tap; + +/// The chains one installation owns. +pub struct Chains { + pub input: String, + /// Also holds the per-VM forwarding rules from + /// [`super::nat::VmRules`], which is why the name is persisted on + /// each VM's `NetworkInfo`. + pub forward: String, +} + +/// Derive an installation's chain names. +/// +/// `Some(ns)` → `ember-{ns}-input`, `None` → `ember-input` for installs +/// that predate instance ids. Lowercase-with-dashes matches every other +/// install-scoped ember resource name (`ember-aaaa-pool`, `emaaaa-` +/// TAPs, `ember:aaaa` comments) and keeps `iptables-save | grep ember` +/// useful, at the price of deviating from netfilter's uppercase +/// convention. iptables caps chain names at 28 characters, which the +/// longest form here (`ember-ffff-forward`, 18) sits well inside. +pub fn chains(instance_id: Option<&str>) -> Chains { + match instance_id { + None => Chains { + input: "ember-input".to_string(), + forward: "ember-forward".to_string(), + }, + Some(id) => Chains { + input: format!("ember-{id}-input"), + forward: format!("ember-{id}-forward"), + }, + } +} + +/// Make the host ready to run this installation's VMs. +/// +/// Idempotent, and called on every VM start rather than once at +/// `ember init`, because iptables state does not survive a reboot. +/// Creating the chains only at init time would mean a reboot silently +/// drops the host block, and silently losing a security property is +/// the failure mode this module exists to remove. +pub fn ensure(instance_id: Option<&str>) -> Result<()> { + enable_ip_forwarding()?; + + let chains = chains(instance_id); + let taps = tap::wildcard(instance_id); + + // Order matters twice over. A jump to a chain that does not exist + // is an error, and a jump installed before the chain is populated + // would expose an empty (so fully permissive) chain to live + // traffic for as long as it takes to add the rules. + iptables::ensure_chain(&chains.input)?; + iptables::ensure_chain(&chains.forward)?; + for rule in static_rules(&chains, &taps) { + rule.ensure()?; + } + for rule in jumps(&chains) { + rule.ensure()?; + } + Ok(()) +} + +/// Remove this installation's chains and the jumps into them. +/// +/// Scoped to this install's chain names, so a second install on the +/// same host is untouched. Callers reach this through +/// `NetworkBackend::deinit`, which runs only once no VMs are +/// registered, so the forward chain holds no per-VM rules by then. +pub fn deinit(instance_id: Option<&str>) -> Result<()> { + let chains = chains(instance_id); + + // Jumps first. iptables refuses to delete a chain that anything + // still references. + for rule in jumps(&chains) { + rule.remove()?; + } + iptables::remove_chain(&chains.input)?; + iptables::remove_chain(&chains.forward)?; + Ok(()) +} + +/// The install-wide rules, the ones that hold no per-VM state. +/// +/// `taps` is the interface wildcard from [`tap::wildcard`], so one +/// rule covers every VM of the installation and none of anyone else's. +fn static_rules(chains: &Chains, taps: &str) -> Vec { + vec![ + // Return traffic for host-initiated connections. Mandatory, + // not a convenience: when the host opens a connection to a + // guest, the guest's replies arrive here from the TAP, so a + // bare DROP below would break `ember ssh`, `exec` and `cp`. + Rule::filter( + &chains.input, + &[ + "-i", + taps, + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + "-j", + "ACCEPT", + ], + ) + .at_front(), + // Everything else a guest sends to a host address, which is + // every address the host owns and not just the TAP gateway. + Rule::filter(&chains.input, &["-i", taps, "-j", "DROP"]), + // VM to VM, both directions in one rule: A to B matches with + // in=tapA out=tapB, B's reply matches with in=tapB out=tapA, + // so no conntrack state is needed. Scoped to this install's + // prefix on both sides, so it never covers another install's + // VMs. + Rule::filter(&chains.forward, &["-i", taps, "-o", taps, "-j", "ACCEPT"]).at_front(), + // Terminal DROP. Without it, forwarded traffic that matches + // none of our ACCEPTs would fall through to the host's own + // FORWARD rules, and whether a VM could reach docker0, another + // install's TAPs or a libvirt bridge would once again depend on + // the host's configuration. The per-VM egress ACCEPT from + // `nat::VmRules` is inserted at the front, so it is always + // reached before this. + Rule::filter(&chains.forward, &["-i", taps, "-j", "DROP"]), + ] +} + +/// The jumps from the built-in chains into ours. +fn jumps(chains: &Chains) -> Vec { + vec![ + Rule::filter("INPUT", &["-j", &chains.input]).at_front(), + Rule::filter("FORWARD", &["-j", &chains.forward]).at_front(), + ] +} + +/// Enable IPv4 forwarding via sysctl. +/// +/// Required before any VM can route traffic through the host, whether +/// out to the internet or across to a sibling VM. Safe to call +/// repeatedly. +fn enable_ip_forwarding() -> Result<()> { + let output = Command::new("sysctl") + .args(["-w", "net.ipv4.ip_forward=1"]) + .output() + .map_err(|e| Error::CommandExec { + command: "sysctl".into(), + source: e, + })?; + Error::check_command("sysctl", output)?; + Ok(()) +} + +#[cfg(test)] +mod tests { + use super::*; + + fn invocations(rules: &[Rule]) -> Vec> { + rules.iter().map(|r| r.add_args()).collect() + } + + #[test] + fn chain_names_embed_the_namespace() { + let c = chains(Some("a3f4")); + assert_eq!(c.input, "ember-a3f4-input"); + assert_eq!(c.forward, "ember-a3f4-forward"); + } + + /// Installs predating instance ids get unprefixed names. They are + /// the only names such an install will ever use, so they have to + /// stay stable. + #[test] + fn legacy_chain_names_are_unprefixed() { + let c = chains(None); + assert_eq!(c.input, "ember-input"); + assert_eq!(c.forward, "ember-forward"); + } + + #[test] + fn chain_names_fit_the_iptables_budget() { + let c = chains(Some("ffff")); + for name in [c.input, c.forward] { + assert!(name.len() <= 28, "chain name too long for iptables: {name}"); + } + } + + /// The established-accept has to be reachable before the DROP, and + /// front placement is what guarantees that without inspecting rule + /// order. If this inverts, `ember ssh` breaks. + #[test] + fn host_block_accepts_established_before_dropping() { + let c = chains(Some("a3f4")); + let rules = invocations(&static_rules(&c, "ema3f4-+")); + assert_eq!( + rules[0], + [ + "-w", + "5", + "-I", + "ember-a3f4-input", + "1", + "-i", + "ema3f4-+", + "-m", + "conntrack", + "--ctstate", + "RELATED,ESTABLISHED", + "-j", + "ACCEPT" + ] + ); + assert_eq!( + rules[1], + [ + "-w", + "5", + "-A", + "ember-a3f4-input", + "-i", + "ema3f4-+", + "-j", + "DROP" + ] + ); + } + + /// One rule carries VM-to-VM in both directions, and the terminal + /// DROP is appended so per-VM ACCEPTs inserted later still land + /// above it. + #[test] + fn sibling_traffic_is_accepted_and_the_rest_dropped() { + let c = chains(Some("a3f4")); + let rules = invocations(&static_rules(&c, "ema3f4-+")); + assert_eq!( + rules[2], + [ + "-w", + "5", + "-I", + "ember-a3f4-forward", + "1", + "-i", + "ema3f4-+", + "-o", + "ema3f4-+", + "-j", + "ACCEPT" + ] + ); + assert_eq!( + rules[3], + [ + "-w", + "5", + "-A", + "ember-a3f4-forward", + "-i", + "ema3f4-+", + "-j", + "DROP" + ] + ); + } + + /// Every static rule matches on an ember TAP, which is what makes + /// jumping in at position 1 transparent to the rest of the host. + #[test] + fn no_static_rule_matches_non_ember_traffic() { + let c = chains(Some("a3f4")); + for rule in invocations(&static_rules(&c, "ema3f4-+")) { + let i = rule.iter().position(|a| a == "-i").expect("no -i match"); + assert_eq!(rule[i + 1], "ema3f4-+", "unscoped rule: {rule:?}"); + } + } + + /// The jumps have to go in at position 1, or the host's own rules + /// could accept guest traffic before our chains ever see it. + #[test] + fn jumps_go_in_at_the_top_of_the_builtin_chains() { + let c = chains(Some("a3f4")); + let rules = invocations(&jumps(&c)); + assert_eq!( + rules[0], + ["-w", "5", "-I", "INPUT", "1", "-j", "ember-a3f4-input"] + ); + assert_eq!( + rules[1], + ["-w", "5", "-I", "FORWARD", "1", "-j", "ember-a3f4-forward"] + ); + } + + /// Two installs must derive disjoint chain names and disjoint + /// interface matches, or one install's policy would govern the + /// other's VMs. + #[test] + fn installs_do_not_share_chains_or_interface_matches() { + let a = chains(Some("aaaa")); + let b = chains(Some("bbbb")); + assert_ne!(a.input, b.input); + assert_ne!(a.forward, b.forward); + assert_ne!(tap::wildcard(Some("aaaa")), tap::wildcard(Some("bbbb"))); + } +} diff --git a/crates/ember-linux/src/network/tap.rs b/crates/ember-linux/src/network/tap.rs index 3574596..777b8d9 100644 --- a/crates/ember-linux/src/network/tap.rs +++ b/crates/ember-linux/src/network/tap.rs @@ -168,6 +168,20 @@ pub fn prefix(instance_id: Option<&str>) -> String { } } +/// iptables interface match covering every TAP device of an +/// installation. +/// +/// [`prefix`] followed by iptables' `+` wildcard, which matches any +/// interface whose name starts with what precedes it. The trailing dash +/// in the prefix is what keeps this away from physical NICs named `em1` +/// or `em2`, and what keeps installs apart: `em-+` does not match +/// `emaaaa-...` and vice versa. Two legacy installs on one host do +/// share `em-+`, which is the same collision their shared TAP prefix +/// already has. +pub fn wildcard(instance_id: Option<&str>) -> String { + format!("{}+", prefix(instance_id)) +} + /// List TAP devices on the system whose name starts with `prefix`. /// /// Parses the output of `ip -o link show type tun`. Pass the @@ -254,6 +268,15 @@ mod tests { assert!(p.len() + 7 <= 15); } + #[test] + fn wildcard_matches_one_installs_devices() { + assert_eq!(wildcard(Some("a3f4")), "ema3f4-+"); + assert_eq!(wildcard(None), "em-+"); + // The trailing dash is the whole reason a physical `em1` + // cannot match, so it must survive into the wildcard. + assert!(wildcard(Some("a3f4")).contains('-')); + } + /// Locked at 3 chars: legacy hosts have `em-` TAP names /// persisted in their `vm.json`, and the orphan sweep + delete /// paths reference that exact form. diff --git a/crates/ember-linux/src/network_backend.rs b/crates/ember-linux/src/network_backend.rs index d637361..f058315 100644 --- a/crates/ember-linux/src/network_backend.rs +++ b/crates/ember-linux/src/network_backend.rs @@ -56,19 +56,29 @@ impl NetworkBackend for LinuxNetwork { return Err(e); } - // Enable IP forwarding (idempotent). - if let Err(e) = network::nat::enable_ip_forwarding() { + // Enable IP forwarding and put this install's policy chains in + // place. Idempotent, and done on every start because iptables + // state does not survive a reboot. + let chains = network::policy::chains(ns); + if let Err(e) = network::policy::ensure(ns) { let _ = network::tap::delete(&tap_name); let _ = network::ip::release(&self.store, &vm.name); return Err(e); } - // Add iptables NAT/forwarding rules tagged with this install's - // comment so cleanup can scope to *this* installation. + // Per-VM rules: masquerade in the shared nat table, tagged with + // this install's comment, and the two forwarding rules inside + // the install's own chain. let comment = network::nat::comment(ns); - if let Err(e) = - network::nat::add_rules(&tap_name, &allocation.guest_ip, &wan_iface, &comment) - { + let rules = network::nat::VmRules { + chain: Some(&chains.forward), + tap_device: &tap_name, + guest_ip: &allocation.guest_ip, + wan_iface: &wan_iface, + comment: &comment, + }; + if let Err(e) = rules.add() { + rules.remove(); let _ = network::tap::delete(&tap_name); let _ = network::ip::release(&self.store, &vm.name); return Err(e); @@ -81,6 +91,7 @@ impl NetworkBackend for LinuxNetwork { netmask: allocation.netmask, guest_mac: None, wan_iface: Some(wan_iface), + firewall_chain: Some(chains.forward), }) } diff --git a/crates/ember-macos/src/network.rs b/crates/ember-macos/src/network.rs index 598e9ef..a5b25b2 100644 --- a/crates/ember-macos/src/network.rs +++ b/crates/ember-macos/src/network.rs @@ -136,6 +136,9 @@ impl NetworkBackend for MacosNetwork { netmask: VMNET_NETMASK.to_string(), guest_mac: None, wan_iface: None, + // vmnet keeps no host firewall state, so there is no + // chain to record. + firewall_chain: None, }) } diff --git a/crates/ember-macos/src/vm.rs b/crates/ember-macos/src/vm.rs index aace367..d16c3fb 100644 --- a/crates/ember-macos/src/vm.rs +++ b/crates/ember-macos/src/vm.rs @@ -206,6 +206,7 @@ impl VmBackend for MacosVm { netmask: String::new(), guest_mac: Some(mac), wan_iface: None, + firewall_chain: None, } }; From f82fb896123dac4bfaf7992a2e34a9c634fb0c76 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:12:43 +0200 Subject: [PATCH 03/14] net: disable IPv6 on TAP devices ember configures IPv4 only, but the kernel gives both ends of a TAP link a v6 link-local address, so a guest can reach the host over IPv6 while the v4 policy holds the host to be unreachable. Every TAP on this host has such an address today. Turning the stack off on the device closes that with one sysctl write per TAP, instead of a second parallel set of ip6tables rules to keep in sync. Done before the link comes up, so no link-local address is ever assigned. --- crates/ember-linux/src/network/tap.rs | 38 +++++++++++++++++++++++++-- 1 file changed, 36 insertions(+), 2 deletions(-) diff --git a/crates/ember-linux/src/network/tap.rs b/crates/ember-linux/src/network/tap.rs index 777b8d9..85646cb 100644 --- a/crates/ember-linux/src/network/tap.rs +++ b/crates/ember-linux/src/network/tap.rs @@ -95,7 +95,14 @@ pub fn create(name: &str, host_ip: &str) -> Result<()> { // fd can now be closed — the device persists in the kernel. drop(tun_fd); - // 5. Assign IP address: `ip addr add dev ` + // 5. Close the IPv6 stack on the link, before it comes up and the + // kernel would assign a link-local address. + if let Err(e) = disable_ipv6(name) { + let _ = delete(name); + return Err(e); + } + + // 6. Assign IP address: `ip addr add dev ` let output = Command::new("ip") .args(["addr", "add", host_ip, "dev", name]) .output() @@ -108,7 +115,7 @@ pub fn create(name: &str, host_ip: &str) -> Result<()> { Error::check_command("ip addr add", output)?; } - // 6. Bring the interface up: `ip link set up` + // 7. Bring the interface up: `ip link set up` let output = Command::new("ip") .args(["link", "set", name, "up"]) .output() @@ -124,6 +131,33 @@ pub fn create(name: &str, host_ip: &str) -> Result<()> { Ok(()) } +/// Turn off IPv6 on a TAP device. +/// +/// ember configures IPv4 only, but the kernel would otherwise give both +/// ends of the link a v6 link-local address, and a guest could then +/// reach the host over IPv6 while the v4 rules in [`super::policy`] +/// hold the host to be unreachable. Disabling the stack on the device +/// closes that without a second, parallel set of ip6tables rules to +/// keep in sync. +/// +/// Call before the link comes up, so no link-local address is ever +/// assigned. We write the sysctl directly rather than shelling out +/// because a kernel built without IPv6 has no such knob, and a missing +/// file tells us that apart from a genuine write failure while +/// `sysctl`'s exit status would not. +fn disable_ipv6(name: &str) -> Result<()> { + let path = format!("/proc/sys/net/ipv6/conf/{name}/disable_ipv6"); + match std::fs::write(&path, "1") { + Ok(()) => Ok(()), + // No IPv6 in this kernel, so nothing to close. + Err(e) if e.kind() == std::io::ErrorKind::NotFound => Ok(()), + Err(e) => Err(Error::Io { + path: path.into(), + source: e, + }), + } +} + /// Delete a TAP device by name. /// /// Equivalent to `ip link delete `. From 69fd1426aca6c88179ece5e535e1239df0920841 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:14:00 +0200 Subject: [PATCH 04/14] cli: remove firewall chains on deinit The install's chains outlive every VM, so something has to delete them when the install goes away. NetworkBackend gains a per-install deinit alongside the per-VM teardown, defaulting to a no-op for backends that keep no host-wide state. Runs before storage teardown, best-effort, a leftover chain is not worth refusing to tear the install down. 'ember info' now names the two chains, so 'iptables -S ' is one copy-paste away when the policy needs inspecting. --- crates/ember-core/src/backend.rs | 16 ++++++++++++++-- crates/ember-linux/src/network_backend.rs | 6 ++++++ crates/ember-linux/src/platform.rs | 2 ++ src/cli/deinit.rs | 9 ++++++++- 4 files changed, 30 insertions(+), 3 deletions(-) diff --git a/crates/ember-core/src/backend.rs b/crates/ember-core/src/backend.rs index e85ed50..33cd2e2 100644 --- a/crates/ember-core/src/backend.rs +++ b/crates/ember-core/src/backend.rs @@ -337,11 +337,23 @@ pub trait NetworkBackend { /// Tear down networking for a VM. /// - /// Linux: removes iptables rules (matched by per-installation - /// comment), deletes TAP device, releases IP. + /// Linux: removes the VM's iptables rules, deletes its TAP device, + /// releases its IP. /// macOS: no-op (vmnet cleans up automatically). fn teardown(&self, vm: &VmMetadata, config: &GlobalConfig) -> Result<()>; + /// Remove host-wide network state owned by this installation. + /// + /// Called from `ember deinit`, which refuses to run while any VM is + /// registered, so no per-VM state is left to consider. + /// + /// Linux: removes the installation's firewall chains. + /// + /// Default: no-op, for backends that keep no host-wide state. + fn deinit(&self, _config: &GlobalConfig) -> Result<()> { + Ok(()) + } + /// Discover the guest's IP address from its MAC address. /// /// Only meaningful on platforms where the guest IP is dynamically assigned diff --git a/crates/ember-linux/src/network_backend.rs b/crates/ember-linux/src/network_backend.rs index f058315..b47ed90 100644 --- a/crates/ember-linux/src/network_backend.rs +++ b/crates/ember-linux/src/network_backend.rs @@ -105,4 +105,10 @@ impl NetworkBackend for LinuxNetwork { } Ok(()) } + + /// Remove this installation's firewall chains and the jumps into + /// them. + fn deinit(&self, config: &GlobalConfig) -> Result<()> { + network::policy::deinit(config.instance_namespace()) + } } diff --git a/crates/ember-linux/src/platform.rs b/crates/ember-linux/src/platform.rs index 418c9d4..e2ac0d0 100644 --- a/crates/ember-linux/src/platform.rs +++ b/crates/ember-linux/src/platform.rs @@ -113,6 +113,8 @@ impl Platform for LinuxPlatform { if let Some(ref wan_iface) = config.wan_iface { extra.push(("WAN iface", wan_iface.clone())); } + let chains = crate::network::policy::chains(config.instance_namespace()); + extra.push(("Firewall", format!("{}, {}", chains.input, chains.forward))); extra } diff --git a/src/cli/deinit.rs b/src/cli/deinit.rs index 3342157..62456aa 100644 --- a/src/cli/deinit.rs +++ b/src/cli/deinit.rs @@ -8,7 +8,7 @@ use std::path::Path; use clap::Args; -use crate::backend::create_storage; +use crate::backend::{create_storage, Network, NetworkBackend}; use ember_core::config::GlobalConfig; use ember_core::state::store::StateStore; use ember_core::state::vm; @@ -46,6 +46,13 @@ pub fn run(args: &DeinitArgs, state_dir: &Path) -> anyhow::Result<()> { ); } + // Networking first: it is cheap, and a failure here should not + // leave the storage pool destroyed. Best-effort, since a leftover + // firewall chain is not worth refusing to tear the install down. + if let Err(e) = Network::new(store.clone()).deinit(&config) { + eprintln!("Warning: failed to remove firewall rules: {e}"); + } + let storage = create_storage(&config); storage.deinit(args.purge)?; From e2d038188bdd14d335874dbc1ebea100eb948ab0 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:16:15 +0200 Subject: [PATCH 05/14] net: drop host addresses from guest DNS servers A host running its own resolver (dnsmasq, or a pihole bound to the LAN address) hands out a nameserver that guests can no longer reach now that VM-to-host traffic is blocked, so every guest query would time out. Host addresses join loopback and IPv6 in the unreachable-from-the-guest filter, which falls through to the next detection source and warns about what it dropped. Working DNS via a public resolver beats a correct-looking nameserver that answers nothing, and the warning says why the internal resolver is not being used. --- crates/ember-linux/src/network/dns.rs | 135 ++++++++++++++++++++++---- 1 file changed, 116 insertions(+), 19 deletions(-) diff --git a/crates/ember-linux/src/network/dns.rs b/crates/ember-linux/src/network/dns.rs index 3ec9398..b97e7dd 100644 --- a/crates/ember-linux/src/network/dns.rs +++ b/crates/ember-linux/src/network/dns.rs @@ -30,32 +30,85 @@ const MAX_NAMESERVERS: usize = 2; /// 3. `/etc/resolv.conf` — direct resolv.conf /// 4. Fallback to 1.1.1.1 + 8.8.8.8 /// -/// Filters out IPv6 addresses (VMs only have IPv4) and loopback -/// addresses (unreachable from the guest). +/// Filters out IPv6 addresses (VMs only have IPv4), loopback +/// addresses, and the host's own addresses. All three are unreachable +/// from a guest, the last because [`super::policy`] blocks VM-to-host +/// traffic, so a host running its own resolver (dnsmasq or a pihole +/// bound to the LAN address) would otherwise hand every guest a +/// nameserver that answers nothing. pub fn detect_nameservers(wan_iface: &str) -> Vec { - // Try interface-specific DNS via resolvectl (most accurate). - if let Some(servers) = resolvectl_dns(wan_iface) { - if !servers.is_empty() { - return servers; - } + let host = host_addresses(); + let mut blocked = Vec::new(); + + // Sources in order of accuracy, each dropping anything the guest + // can't reach and falling through to the next if nothing survives. + let mut take = |servers: Option>| -> Option> { + let (reachable, unreachable) = partition_reachable(servers.unwrap_or_default(), &host); + blocked.extend(unreachable); + (!reachable.is_empty()).then_some(reachable) + }; + + let mut servers = take(resolvectl_dns(wan_iface)); + if servers.is_none() { + servers = take(parse_resolv_conf(Path::new( + "/run/systemd/resolve/resolv.conf", + ))); } + if servers.is_none() { + servers = take(parse_resolv_conf(Path::new("/etc/resolv.conf"))); + } + + let servers = + servers.unwrap_or_else(|| FALLBACK_NAMESERVERS.iter().map(|s| s.to_string()).collect()); - // Fall back to systemd-resolved upstream config. - if let Some(servers) = parse_resolv_conf(Path::new("/run/systemd/resolve/resolv.conf")) { - if !servers.is_empty() { - return servers; - } + if !blocked.is_empty() { + eprintln!( + "Warning: DNS server(s) {} belong to the host, which guests cannot reach.", + blocked.join(", ") + ); + eprintln!(" Using {} instead.", servers.join(", ")); } + servers +} + +/// Split nameservers into those a guest can reach and those it can't +/// because they are host addresses. +fn partition_reachable(servers: Vec, host: &[String]) -> (Vec, Vec) { + servers + .into_iter() + .partition(|server| !host.iter().any(|addr| addr == server)) +} - // Fall back to /etc/resolv.conf. - if let Some(servers) = parse_resolv_conf(Path::new("/etc/resolv.conf")) { - if !servers.is_empty() { - return servers; - } +/// The host's own IPv4 addresses, including TAP gateways. +/// +/// Empty if the addresses can't be listed, which leaves detection +/// exactly as permissive as it was before this filter existed rather +/// than failing a VM start over a diagnostic. +fn host_addresses() -> Vec { + let Ok(output) = Command::new("ip") + .args(["-4", "-o", "addr", "show"]) + .output() + else { + return Vec::new(); + }; + if !output.status.success() { + return Vec::new(); } + parse_host_addresses(&String::from_utf8_lossy(&output.stdout)) +} - // Last resort: hardcoded public DNS. - FALLBACK_NAMESERVERS.iter().map(|s| s.to_string()).collect() +/// Pull the addresses out of `ip -4 -o addr show` output, whose lines +/// look like `2: enp7s0 inet 192.168.0.23/24 brd ... scope global`. +fn parse_host_addresses(output: &str) -> Vec { + output + .lines() + .filter_map(|line| { + let mut fields = line.split_whitespace(); + fields.find(|f| *f == "inet")?; + let cidr = fields.next()?; + Some(cidr.split('/').next()?.to_string()) + }) + .collect() } /// Query DNS servers for a specific interface via `resolvectl dns`. @@ -206,6 +259,50 @@ mod tests { assert!(parse_resolv_conf(Path::new("/nonexistent/resolv.conf")).is_none()); } + #[test] + fn host_addresses_parsed_from_ip_output() { + let output = "\ +1: lo inet 127.0.0.1/8 scope host lo\\ valid_lft forever preferred_lft forever +2: enp7s0 inet 192.168.0.23/24 brd 192.168.0.255 scope global dynamic enp7s0\\ valid_lft 5000sec +8: em-eefd28d inet 10.100.0.1/30 brd 10.100.0.3 scope global em-eefd28d\\ valid_lft forever +"; + assert_eq!( + parse_host_addresses(output), + ["127.0.0.1", "192.168.0.23", "10.100.0.1"] + ); + } + + #[test] + fn host_addresses_tolerates_empty_output() { + assert!(parse_host_addresses("").is_empty()); + } + + /// A host-run resolver is unreachable from a guest, so it has to be + /// dropped rather than handed over and left to time out. + #[test] + fn nameservers_on_host_addresses_are_dropped() { + let host = ["192.168.0.23".to_string(), "10.100.0.1".to_string()]; + let (reachable, blocked) = partition_reachable( + vec![ + "192.168.0.23".to_string(), + "1.1.1.1".to_string(), + "10.100.0.1".to_string(), + ], + &host, + ); + assert_eq!(reachable, ["1.1.1.1"]); + assert_eq!(blocked, ["192.168.0.23", "10.100.0.1"]); + } + + #[test] + fn nameservers_off_host_are_all_kept() { + let host = ["192.168.0.23".to_string()]; + let servers = vec!["9.9.9.9".to_string(), "1.1.1.1".to_string()]; + let (reachable, blocked) = partition_reachable(servers.clone(), &host); + assert_eq!(reachable, servers); + assert!(blocked.is_empty()); + } + #[test] fn detect_nameservers_returns_something() { // Use a bogus interface — should fall back to resolv.conf or hardcoded. From 52bc5cafae34325de904dfe2d567d6d28e3d1738 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:21:28 +0200 Subject: [PATCH 06/14] tests: cover the firewall policy contract Placement is what the structural test pins down, not just presence: a terminal DROP that drifts above the per-VM ACCEPTs, or a jump that stops being first in INPUT, turns the contract back into a coin flip while every rule is still technically there. It also checks that stopping a VM leaves the install's policy standing and that deinit removes it. The connectivity test boots two VMs and uses real packets: a sibling is reachable, the host is not at any of its addresses, and egress still works. An isolation test guards that one install's deinit leaves another install's chains alone. docs/SPEC.md gets the rule listing, the chain lifecycle, the IPv6 step in VM start, and the host-address DNS filter. --- docs/SPEC.md | 89 ++++++++-- tests/isolation.rs | 82 +++++++++ tests/network_policy.rs | 370 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 528 insertions(+), 13 deletions(-) create mode 100644 tests/network_policy.rs diff --git a/docs/SPEC.md b/docs/SPEC.md index 731fbcf..f760ae5 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -103,7 +103,9 @@ src/ │ ├── mod.rs │ ├── tap.rs # TAP device via ioctl (nix crate) │ ├── ip.rs # IP allocation from pool -│ ├── nat.rs # iptables NAT/masquerade rules +│ ├── iptables.rs # iptables invocation: rules, chains, locking +│ ├── nat.rs # Per-VM masquerade and forwarding rules +│ ├── policy.rs # Install-owned chains: VM-to-VM, host isolation │ ├── dns.rs # Host DNS nameserver detection for guests │ └── wan.rs # WAN interface auto-detection ├── image/ @@ -278,7 +280,7 @@ The `forked_from` field in VM metadata tracks the origin snapshot path (e.g., `< 1. Load VM metadata from state store 2. Create TAP device + allocate IP -3. Configure iptables NAT rules +3. Assert the installation's firewall chains, add the VM's iptables rules 4. Spawn: `firecracker --api-sock --log-path --level Info` 5. Wait for API socket (poll 10ms, timeout 5s) 6. Configure via API: @@ -333,12 +335,16 @@ When no kernel is specified, `stock` is used as the default and auto-downloaded ### Model: TAP + NAT per VM -Each VM gets an isolated point-to-point link: +Each VM gets a point-to-point link: ``` Host: em- (TAP) 10.100.0.1/30 ←→ Guest: eth0 10.100.0.2/30 ``` +Traffic between two VMs is routed by the host across their TAPs, since +each sits on its own /30. What is and isn't permitted across those links +is the firewall policy below. + ### IP Allocation - Configurable base subnet (default: `10.100.0.0/16`) @@ -347,24 +353,81 @@ Host: em- (TAP) 10.100.0.1/30 ←→ Guest: eth0 10.100.0.2/30 - Supports ~16384 concurrent VMs with a /16 - Allocations tracked in state store, released on VM delete +### Firewall Policy + +One contract, independent of whatever else the host keeps in its +firewall: **a VM reaches the internet and the other VMs of its own +installation, and nothing else.** In particular it cannot reach the +host, at any host address. + +Each installation owns two chains, entered from position 1 of the +built-in chains. Position 1 is what makes the policy hold: a +pre-existing `-A INPUT -s 10.0.0.0/8 -j ACCEPT` would otherwise match +guest traffic before the host block, and a mid-chain `REJECT` from ufw +or firewalld would be reached before the forwarding rules. Both chains +are transparent to non-ember traffic, since every rule in them matches +an ember TAP interface and anything else falls through to the built-in +chain right after the jump. + +``` +-A INPUT -j ember--input +-A FORWARD -j ember--forward + +# ember--input: host isolation. +-i em-+ -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +-i em-+ -j DROP + +# ember--forward: install-wide. +-i em-+ -o em-+ -j ACCEPT # VM to VM, both directions +-i em-+ -j DROP # terminal + +# ember--forward: per VM. +-i -o -j ACCEPT +-i -o -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT +``` + +The established-accept in the input chain is mandatory. Replies to +host-initiated connections arrive on INPUT from the TAP, so without it +`ember ssh`, `exec` and `cp` would break. + +Rule order needs no bookkeeping. Each chain holds ACCEPTs plus exactly +one terminal DROP, ACCEPTs are inserted at the front and the DROP is +appended, so the DROP is always last whatever subset of rules already +exists. + +Chains are asserted on every VM start, not created once at `ember +init`, because iptables state does not survive a reboot. They are +removed by `ember deinit`. The chain a VM's rules went into is recorded +on its `NetworkInfo`, so rules written by an older binary (appended +straight to FORWARD with a `-m comment --comment ember:` tag) are +still deleted from where they actually are. + +Masquerade stays in the shared `nat` POSTROUTING chain with its comment +tag. It is an address translation rather than a policy decision, and +keeping its shape unchanged means rules written before and after the +policy chains existed remain mutually deletable. + ### Setup (per VM start) 1. Create TAP device via ioctl (`/dev/net/tun`, IFF_TAP | IFF_NO_PI) -2. `ip addr add /30 dev em-` + `ip link set up` -3. Enable IP forwarding: `sysctl net.ipv4.ip_forward=1` -4. iptables rules: - ``` - -t nat -A POSTROUTING -s /32 -o -j MASQUERADE - -A FORWARD -i -o -j ACCEPT - -A FORWARD -i -o -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT - ``` +2. Disable IPv6 on the device (`net.ipv6.conf..disable_ipv6=1`), + before the link comes up so no link-local address is ever assigned. + ember is IPv4-only, and a v6 link-local pair would let a guest reach + the host past the IPv4 policy. +3. `ip addr add /30 dev em-` + `ip link set up` +4. Enable IP forwarding: `sysctl net.ipv4.ip_forward=1` +5. Create the installation's chains and jumps if absent (idempotent) +6. Add the per-VM masquerade and forwarding rules ### Cleanup (per VM stop/delete) -1. `iptables -D` (same rules with delete flag) +1. `iptables -D` for the per-VM rules, in the chain recorded on the VM 2. `ip link delete em-` 3. Release IP allocation +The installation's chains outlive its VMs and are removed by `ember +deinit`. + ### WAN Interface Detection Runs `ip route get 8.8.8.8` and parses the `dev ` field. Auto-detected during `ember init` and cached in the global config (`config.json` `wan_iface` field). Can be overridden with `ember init --wan-iface `. At VM start time, falls back to re-detection if not cached. @@ -380,7 +443,7 @@ Detection order (scoped to the WAN interface to avoid unreachable servers): 3. `/etc/resolv.conf` — direct resolv.conf parsing 4. Fallback: `1.1.1.1`, `8.8.8.8` -Filters out IPv6 addresses (VMs only have IPv4) and loopback addresses (unreachable from the guest). Returns at most 2 servers (kernel `ip=` parameter limit). +Filters out IPv6 addresses (VMs only have IPv4), loopback addresses, and the host's own addresses. All three are unreachable from a guest, the last because the firewall policy blocks VM-to-host traffic, so a host running its own resolver (dnsmasq, or a pihole bound to the LAN address) would otherwise hand every guest a nameserver that answers nothing. A dropped server falls through to the next detection source and is reported as a warning. Returns at most 2 servers (kernel `ip=` parameter limit). ### Rootfs Injection diff --git a/tests/isolation.rs b/tests/isolation.rs index c76234c..c27960b 100644 --- a/tests/isolation.rs +++ b/tests/isolation.rs @@ -188,6 +188,88 @@ fn legacy_config_without_instance_id_keeps_working() { ); } +/// `deinit` must only remove firewall chains belonging to *this* +/// install. Create install A's chains manually, tear down install B, +/// and verify A's chains are still there. Chains are host-global and +/// named per install, so an unscoped delete would take down the +/// developer's live policy from inside a test run. +#[test] +#[ignore = "requires root + iptables"] +fn deinit_does_not_touch_other_installs_chains() { + let tmp = tempfile::tempdir().unwrap(); + let storage_b = tmp.path().join("dm-thin-b"); + let state_b = tmp.path().join("state-b"); + + let _cleanup_b = common::linux::DmThinCleanup { + state_dir: state_b.clone(), + }; + + // Stand in for install A: the chain names `instance_id` "aaaa" + // would derive, created directly so the test doesn't have to boot a + // VM to bring them into existence. + const CHAINS_A: [&str; 2] = ["ember-aaaa-input", "ember-aaaa-forward"]; + struct ChainCleanup; + impl Drop for ChainCleanup { + fn drop(&mut self) { + for chain in CHAINS_A { + let _ = std::process::Command::new("iptables") + .args(["-w", "5", "-X", chain]) + .status(); + } + } + } + let _chain_cleanup = ChainCleanup; + for chain in CHAINS_A { + let status = std::process::Command::new("iptables") + .args(["-w", "5", "-N", chain]) + .status() + .expect("failed to run iptables -N"); + assert!(status.success(), "failed to create test chain {chain}"); + } + + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "init", + "--storage", + "dm-thin", + "--storage-path", + storage_b.to_str().unwrap(), + "--size", + "200M", + "--instance-id", + "bbbb", + ]); + assert!( + output.status.success(), + "init B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let output = common::ember(&[ + "--state-dir", + state_b.to_str().unwrap(), + "deinit", + "--purge", + ]); + assert!( + output.status.success(), + "deinit B failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + for chain in CHAINS_A { + let check = std::process::Command::new("iptables") + .args(["-w", "5", "-S", chain]) + .output() + .expect("failed to run iptables -S"); + assert!( + check.status.success(), + "install B's deinit deleted install A's chain '{chain}'" + ); + } +} + /// Reconcile (run at the start of every command) must only sweep /// TAP devices belonging to *this* install's prefix. Create a TAP /// device manually with install A's prefix, then run a reconcile- diff --git a/tests/network_policy.rs b/tests/network_policy.rs new file mode 100644 index 0000000..f78cedc --- /dev/null +++ b/tests/network_policy.rs @@ -0,0 +1,370 @@ +//! Firewall policy contract for the Linux backend. +//! +//! Two promises are under test: +//! +//! 1. A VM can reach the other VMs of its own installation. +//! 2. A VM cannot reach the host, at any of the host's addresses. +//! +//! Both used to be accidents of whatever else was in the host's +//! iptables policy, which is why the structural test below pins down +//! rule *placement* and not just rule presence. A terminal DROP that +//! drifts above the per-VM ACCEPTs, or a jump that stops being first in +//! INPUT, turns the contract back into a coin flip while every rule is +//! still technically there. +//! +//! Gated `#[ignore]` and Linux-only because they touch real iptables, +//! TAP and hypervisor state. Run explicitly with: +//! +//! ```text +//! sudo cargo test --test network_policy -- --ignored --test-threads=1 +//! ``` + +#![cfg(target_os = "linux")] +#![allow(clippy::zombie_processes)] + +#[allow(dead_code)] +mod common; + +use std::path::Path; +use std::process::Command; + +// --------------------------------------------------------------------------- +// iptables helpers +// --------------------------------------------------------------------------- + +fn iptables(args: &[&str]) -> Result { + let output = Command::new("iptables") + .arg("-w") + .arg("5") + .args(args) + .output() + .expect("failed to run iptables"); + if output.status.success() { + Ok(String::from_utf8_lossy(&output.stdout).to_string()) + } else { + Err(String::from_utf8_lossy(&output.stderr).trim().to_string()) + } +} + +/// Rules in a chain, in order, as `iptables -S` prints them. Empty when +/// the chain does not exist. +fn rules(chain: &str) -> Vec { + match iptables(&["-S", chain]) { + Ok(out) => out + .lines() + .filter(|l| l.starts_with("-A")) + .map(|l| l.to_string()) + .collect(), + Err(_) => Vec::new(), + } +} + +fn chain_exists(chain: &str) -> bool { + iptables(&["-S", chain]).is_ok() +} + +/// Removes an installation's chains, so a failed assertion cannot leave +/// rules behind on the developer's host. +struct ChainCleanup { + input: String, + forward: String, +} + +impl Drop for ChainCleanup { + fn drop(&mut self) { + for (builtin, chain) in [("INPUT", &self.input), ("FORWARD", &self.forward)] { + while iptables(&["-D", builtin, "-j", chain]).is_ok() {} + let _ = iptables(&["-F", chain]); + let _ = iptables(&["-X", chain]); + } + } +} + +// --------------------------------------------------------------------------- +// State helpers +// --------------------------------------------------------------------------- + +fn read_json(path: &Path) -> serde_json::Value { + let raw = std::fs::read_to_string(path) + .unwrap_or_else(|e| panic!("failed to read {}: {e}", path.display())); + serde_json::from_str(&raw).expect("malformed json") +} + +/// The install's instance id, which every chain and TAP name derives +/// from. +fn instance_id(state_dir: &str) -> String { + read_json(&Path::new(state_dir).join("config.json"))["instance_id"] + .as_str() + .expect("config has no instance_id") + .to_string() +} + +/// A running VM's persisted network info. +fn network_info(state_dir: &str, vm: &str) -> serde_json::Value { + read_json(&Path::new(state_dir).join("vms").join(vm).join("vm.json"))["network"].clone() +} + +fn field(value: &serde_json::Value, key: &str) -> String { + value[key] + .as_str() + .unwrap_or_else(|| panic!("network info has no {key}: {value}")) + .to_string() +} + +// --------------------------------------------------------------------------- +// Structural test: one alpine VM, no guest tooling needed +// --------------------------------------------------------------------------- + +/// The chains exist, they are entered first, the per-VM rules live +/// inside them, the terminal DROP stays last, and `deinit` takes it all +/// away again. +#[test] +#[ignore = "requires root + firecracker + a ZFS pool"] +fn policy_chains_are_placed_correctly_and_removed_on_deinit() { + let env = common::TestEnv::with_running_vm("netpolicy", "polvm"); + let state = env.state().to_string(); + + let id = instance_id(&state); + let input_chain = format!("ember-{id}-input"); + let forward_chain = format!("ember-{id}-forward"); + let taps = format!("em{id}-+"); + let _cleanup = ChainCleanup { + input: input_chain.clone(), + forward: forward_chain.clone(), + }; + + let net = network_info(&state, "polvm"); + let tap = field(&net, "tap_device"); + let wan = field(&net, "wan_iface"); + assert_eq!( + field(&net, "firewall_chain"), + forward_chain, + "the VM must record which chain its rules went into, or an \ + upgraded binary cannot delete them" + ); + + // ── Chains and jumps ───────────────────────────────────────── + + assert!(chain_exists(&input_chain), "{input_chain} was not created"); + assert!( + chain_exists(&forward_chain), + "{forward_chain} was not created" + ); + + for (builtin, chain) in [("INPUT", &input_chain), ("FORWARD", &forward_chain)] { + let first = rules(builtin).first().cloned().unwrap_or_default(); + assert_eq!( + first, + format!("-A {builtin} -j {chain}"), + "the jump into {chain} must be the first rule in {builtin}, \ + otherwise a pre-existing ACCEPT can match guest traffic first" + ); + } + + // ── Static policy ──────────────────────────────────────────── + + let forward = rules(&forward_chain); + assert!( + forward.contains(&format!("-A {forward_chain} -i {taps} -o {taps} -j ACCEPT")), + "missing the VM-to-VM rule in {forward_chain}: {forward:#?}" + ); + assert_eq!( + forward.last().cloned().unwrap_or_default(), + format!("-A {forward_chain} -i {taps} -j DROP"), + "the terminal DROP must be the last rule in {forward_chain}, or it \ + shadows the per-VM ACCEPTs above it: {forward:#?}" + ); + + let input = rules(&input_chain); + let established = input + .iter() + .position(|r| r.contains("RELATED,ESTABLISHED")) + .unwrap_or_else(|| panic!("no established-accept in {input_chain}: {input:#?}")); + let drop = input + .iter() + .position(|r| r.ends_with("-j DROP")) + .unwrap_or_else(|| panic!("no host block in {input_chain}: {input:#?}")); + assert!( + established < drop, + "established traffic must be accepted before the host block, or \ + `ember ssh` breaks: {input:#?}" + ); + + // ── Per-VM rules ───────────────────────────────────────────── + + assert!( + forward.contains(&format!("-A {forward_chain} -i {tap} -o {wan} -j ACCEPT")), + "missing the VM's egress rule in {forward_chain}: {forward:#?}" + ); + assert!( + !rules("FORWARD").iter().any(|r| r.contains(&tap)), + "per-VM rules must live in {forward_chain}, not the built-in FORWARD chain" + ); + + // ── Stop: per-VM rules go, the install's policy stays ───────── + + let output = common::ember(&["--state-dir", &state, "vm", "stop", "polvm"]); + assert!( + output.status.success(), + "vm stop failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let forward = rules(&forward_chain); + assert!( + !forward.iter().any(|r| r.contains(&tap)), + "the stopped VM's rules were left behind: {forward:#?}" + ); + assert!( + forward.iter().any(|r| r.ends_with("-j DROP")), + "the install's policy must outlive its VMs: {forward:#?}" + ); + + // ── Deinit: everything goes ────────────────────────────────── + + let output = common::ember(&["--state-dir", &state, "vm", "delete", "polvm"]); + assert!( + output.status.success(), + "vm delete failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + let output = common::ember(&["--state-dir", &state, "deinit"]); + assert!( + output.status.success(), + "deinit failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + assert!(!chain_exists(&input_chain), "{input_chain} survived deinit"); + assert!( + !chain_exists(&forward_chain), + "{forward_chain} survived deinit" + ); + for builtin in ["INPUT", "FORWARD"] { + assert!( + !rules(builtin).iter().any(|r| r.contains("ember-")), + "a jump into an ember chain survived deinit in {builtin}" + ); + } +} + +// --------------------------------------------------------------------------- +// Connectivity test: two ubuntu VMs, real traffic +// --------------------------------------------------------------------------- + +/// Run a command inside a VM and return the exit status it had *in the +/// guest*. +/// +/// The status is echoed and parsed back rather than taken from `ember +/// exec`, because the negative assertions below depend on telling "the +/// host was unreachable" apart from "we never got to try". Those look +/// identical if the exec path's own failure is read as the command +/// failing. A missing marker panics instead of reporting a status. +fn guest_status(state: &str, vm: &str, command: &str) -> i32 { + let script = format!("{command} >/dev/null 2>&1; echo EXIT:$?"); + let output = common::ember(&["--state-dir", state, "exec", vm, "--", "sh", "-c", &script]); + let stdout = String::from_utf8_lossy(&output.stdout); + stdout + .lines() + .rev() + .find_map(|line| line.trim().strip_prefix("EXIT:")?.parse::().ok()) + .unwrap_or_else(|| { + panic!( + "`{command}` never ran in '{vm}'\nstdout: {stdout}\nstderr: {}", + String::from_utf8_lossy(&output.stderr) + ) + }) +} + +/// Whether a guest can reach `target`, retrying while the peer VM is +/// still booting. Only useful for the positive direction: a blocked +/// target would burn the whole window. +fn reachable_within(state: &str, vm: &str, target: &str, tries: u32) -> bool { + for _ in 0..tries { + if guest_status(state, vm, &format!("ping -c1 -W5 {target}")) == 0 { + return true; + } + std::thread::sleep(std::time::Duration::from_secs(3)); + } + false +} + +/// The contract itself, with real packets: siblings reachable, host +/// not, internet still fine. +#[test] +#[ignore = "requires root + firecracker + docker + internet; boots two VMs"] +fn vms_reach_each_other_but_not_the_host() { + let env = common::TestEnv::with_running_ssh_vm("netpolicyconn", "vma"); + let state = env.state().to_string(); + + let id = instance_id(&state); + let _cleanup = ChainCleanup { + input: format!("ember-{id}-input"), + forward: format!("ember-{id}-forward"), + }; + + // A second VM in the same install, so the two are siblings. + let kernel = common::linux::ensure_kernel(); + let output = common::ember(&[ + "--state-dir", + &state, + "vm", + "create", + "vmb", + "--image", + "ubuntu-slim", + "--kernel", + kernel.to_str().unwrap(), + "--cpus", + "1", + "--memory", + "512M", + ]); + assert!( + output.status.success(), + "creating the second VM failed: {}", + String::from_utf8_lossy(&output.stderr) + ); + + let net_a = network_info(&state, "vma"); + let net_b = network_info(&state, "vmb"); + let guest_b = field(&net_b, "guest_ip"); + let host_ip_a = field(&net_a, "host_ip"); + + // Sibling reachability. The two VMs are on different /30 links, so + // this only works if the host forwards between their TAPs. Retried, + // because `vm create` returns once Firecracker is up rather than + // once the guest has finished booting. + assert!( + reachable_within(&state, "vma", &guest_b, 20), + "VM A could not reach sibling VM B at {guest_b}" + ); + + // The host, at the guest's own default gateway. This is the address + // a guest is most likely to poke at, and the one it must not reach. + assert_ne!( + guest_status(&state, "vma", &format!("ping -c1 -W5 {host_ip_a}")), + 0, + "VM A reached the host at its gateway {host_ip_a}" + ); + + // Every other host address is equally off limits, including the + // gateway of a sibling's link. + let host_ip_b = field(&net_b, "host_ip"); + assert_ne!( + guest_status(&state, "vma", &format!("ping -c1 -W5 {host_ip_b}")), + 0, + "VM A reached the host at {host_ip_b}, the block must cover every \ + host address and not just the VM's own gateway" + ); + + // Egress still works. The terminal DROP is the most likely thing to + // have broken it. + assert_eq!( + guest_status(&state, "vma", "ping -c1 -W10 1.1.1.1"), + 0, + "VM A lost outbound connectivity" + ); + + common::stop_and_delete_vm(&state, "vmb"); +} From c32bb304cd14b3000466bfaf7cad7a87586d5619 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 13:25:32 +0200 Subject: [PATCH 07/14] net: adopt running VMs into the policy chain A VM running when the chains first appear has its forwarding rules in the built-in FORWARD chain, below the jump, so the chain's terminal DROP cuts it off the instant another VM start creates the chain. The VM keeps running and silently loses its network. Reconcile now moves such a VM's rules into the chain and records where they went, adding before deleting so the VM is never ruleless. The masquerade rule is excluded from the move: its shape is identical in both modes, so shifting the full set would delete it right after re-adding it and leave the VM without NAT. VmRules exposes the forwarding pair separately for exactly this reason. One-shot per VM. Once every record names a chain, the check is free. --- crates/ember-linux/src/network/nat.rs | 90 ++++++++++++++++---- crates/ember-linux/src/network/policy.rs | 23 +++++- crates/ember-linux/src/reconcile.rs | 101 ++++++++++++++++++++++- docs/NETWORK-POLICY-SPEC.md | 24 ++++++ docs/SPEC.md | 1 + 5 files changed, 215 insertions(+), 24 deletions(-) diff --git a/crates/ember-linux/src/network/nat.rs b/crates/ember-linux/src/network/nat.rs index 4769f20..b7253ca 100644 --- a/crates/ember-linux/src/network/nat.rs +++ b/crates/ember-linux/src/network/nat.rs @@ -87,24 +87,53 @@ impl VmRules<'_> { } } + /// Add only the two forwarding rules. + /// + /// Pairs with [`remove_forwarding`](Self::remove_forwarding) to move + /// a VM's forwarding rules from one chain to another without + /// touching the masquerade rule, whose shape is identical in both + /// modes and would otherwise be deleted right after being re-added. + pub fn add_forwarding(&self) -> Result<()> { + for rule in self.forwarding() { + rule.ensure()?; + } + Ok(()) + } + + /// Remove only the two forwarding rules, best effort. + pub fn remove_forwarding(&self) { + for rule in self.forwarding() { + let _ = rule.remove(); + } + } + /// The rules, in the order `add` applies them. fn rules(&self) -> Vec { - let guest_cidr = format!("{}/32", self.guest_ip); + let mut rules = vec![self.masquerade()]; + rules.extend(self.forwarding()); + rules + } - // The masquerade rule keeps the same shape in both modes: it - // has always lived in the shared POSTROUTING chain with the - // comment as its only scoping, so rules written before and - // after the policy chains existed are byte-for-byte identical - // and stay mutually deletable. - let masquerade = Rule::nat( + /// Source NAT for the guest's address. + /// + /// Identical in both modes: it has always lived in the shared + /// POSTROUTING chain with the comment as its only scoping, so rules + /// written before and after the policy chains existed are + /// byte-for-byte identical and stay mutually deletable. + fn masquerade(&self) -> Rule { + let guest_cidr = format!("{}/32", self.guest_ip); + Rule::nat( "POSTROUTING", &with_comment( &["-s", &guest_cidr, "-o", self.wan_iface], self.comment, &["-j", "MASQUERADE"], ), - ); + ) + } + /// Outbound and return-path rules for the guest's TAP. + fn forwarding(&self) -> [Rule; 2] { let outbound = &["-i", self.tap_device, "-o", self.wan_iface]; let inbound = &[ "-i", @@ -117,16 +146,16 @@ impl VmRules<'_> { "RELATED,ESTABLISHED", ]; - let (outbound, inbound) = match self.chain { + match self.chain { // Inside a chain ember owns, the chain itself is the // scope, so the comment match would be noise. Front // placement keeps both ACCEPTs above the chain's terminal // DROP without having to inspect rule order. - Some(chain) => ( + Some(chain) => [ Rule::filter(chain, &[outbound.as_slice(), &["-j", "ACCEPT"]].concat()).at_front(), Rule::filter(chain, &[inbound.as_slice(), &["-j", "ACCEPT"]].concat()).at_front(), - ), - None => ( + ], + None => [ Rule::filter( "FORWARD", &with_comment(outbound, self.comment, &["-j", "ACCEPT"]), @@ -135,10 +164,8 @@ impl VmRules<'_> { "FORWARD", &with_comment(inbound, self.comment, &["-j", "ACCEPT"]), ), - ), - }; - - vec![masquerade, outbound, inbound] + ], + } } } @@ -354,6 +381,37 @@ mod tests { ); } + /// The forwarding set must exclude masquerade. Moving a VM's rules + /// between chains adds the new set and removes the old one, and + /// masquerade has the same shape in both, so including it would + /// delete the rule right after re-adding it and leave the VM + /// without NAT. + #[test] + fn forwarding_set_excludes_masquerade() { + let rules = tagged(Some("ember-a3f4-forward")); + for rule in rules.forwarding() { + let invocation = rule.add_args(); + assert!( + !invocation.contains(&"MASQUERADE".to_string()) + && !invocation.contains(&"nat".to_string()), + "masquerade must not be part of the forwarding set: {invocation:?}" + ); + } + assert_eq!(rules.rules().len(), rules.forwarding().len() + 1); + } + + /// Moving rules between chains only works if the two modes really + /// target different chains for the same VM. + #[test] + fn the_two_modes_target_different_chains() { + let in_chain = tagged(Some("ember-a3f4-forward")); + let outside = tagged(None); + for (a, b) in in_chain.forwarding().iter().zip(outside.forwarding()) { + assert_ne!(a.add_args(), b.add_args()); + assert_ne!(a.delete_args(), b.delete_args()); + } + } + /// A legacy install has no namespace, so its rules carry no /// comment match at all. #[test] diff --git a/crates/ember-linux/src/network/policy.rs b/crates/ember-linux/src/network/policy.rs index 210d225..4e43710 100644 --- a/crates/ember-linux/src/network/policy.rs +++ b/crates/ember-linux/src/network/policy.rs @@ -104,14 +104,29 @@ pub fn ensure(instance_id: Option<&str>) -> Result<()> { pub fn deinit(instance_id: Option<&str>) -> Result<()> { let chains = chains(instance_id); + // Every step is attempted even after one fails, and only the first + // error is reported. Bailing early would leave one chain behind + // because the other could not be removed, and the caller's only + // recourse is a warning either way. + let mut failure = None; + // Jumps first. iptables refuses to delete a chain that anything // still references. for rule in jumps(&chains) { - rule.remove()?; + if let Err(e) = rule.remove() { + failure = failure.or(Some(e)); + } + } + for chain in [&chains.input, &chains.forward] { + if let Err(e) = iptables::remove_chain(chain) { + failure = failure.or(Some(e)); + } + } + + match failure { + Some(e) => Err(e), + None => Ok(()), } - iptables::remove_chain(&chains.input)?; - iptables::remove_chain(&chains.forward)?; - Ok(()) } /// The install-wide rules, the ones that hold no per-VM state. diff --git a/crates/ember-linux/src/reconcile.rs b/crates/ember-linux/src/reconcile.rs index 03bcde8..b959d43 100644 --- a/crates/ember-linux/src/reconcile.rs +++ b/crates/ember-linux/src/reconcile.rs @@ -6,7 +6,11 @@ //! is still alive. If dead, mark the VM as Stopped and clean up its //! network resources (TAP device, iptables rules, IP allocation). //! -//! 2. Find orphaned TAP devices belonging to *this* installation +//! 2. Move the forwarding rules of VMs that were started before the +//! installation had policy chains into the chain, so the chain's +//! terminal DROP doesn't cut them off. +//! +//! 3. Find orphaned TAP devices belonging to *this* installation //! (matched against [`network::tap::prefix`] for the install's //! namespace) and delete them. Other ember installs use distinct //! prefixes, so reconciliation here never touches their devices. @@ -49,6 +53,8 @@ pub fn run(state_dir: &Path) { // Track TAP devices that belong to legitimately running VMs. let mut active_tap_devices = HashSet::new(); + // Running VMs whose forwarding rules predate the policy chains. + let mut unchained = Vec::new(); // Phase 1: Reconcile VMs whose processes have died. for metadata in vms { @@ -77,6 +83,9 @@ pub fn run(state_dir: &Path) { // Process is alive — this VM is genuinely running. if let Some(ref net) = metadata.network { active_tap_devices.insert(net.tap_device.clone()); + if net.firewall_chain.is_none() { + unchained.push(metadata.clone()); + } } } else { // Process is dead — clean up and mark stopped. @@ -93,12 +102,19 @@ pub fn run(state_dir: &Path) { } } - // Phase 2: Clean up orphaned TAP devices belonging to this install. - // Without a config we have no way to scope the listing safely, so - // skip — leaving an orphan is preferable to deleting a foreign one. + // Without a config we have no way to scope host-global names safely, + // so skip the rest — leaving an orphan is preferable to deleting a + // foreign one. let Some(cfg) = config else { return; }; + + // Phase 2: Adopt running VMs whose rules predate the policy chains. + for metadata in unchained { + adopt_into_policy_chain(&store, &cfg, &metadata); + } + + // Phase 3: Clean up orphaned TAP devices belonging to this install. let prefix = network::tap::prefix(cfg.instance_namespace()); let system_devices = match network::tap::list_devices_with_prefix(&prefix) { Ok(devs) => devs, @@ -116,6 +132,83 @@ pub fn run(state_dir: &Path) { } } +/// Move a running VM's forwarding rules into the install's policy +/// chain. +/// +/// A VM started before the install had policy chains has its rules +/// appended to the built-in FORWARD chain, which is below the jump into +/// our chain, so the chain's terminal DROP would cut the VM off the +/// moment the chain appears. Rather than leave the user with a live VM +/// that has silently lost its network until they restart it, we re-add +/// its rules inside the chain and delete the ones outside. +/// +/// The masquerade rule is deliberately untouched. Its shape is +/// identical in both modes, so adding and then removing the full set +/// would delete it and break the VM's outbound NAT. +/// +/// Best effort. On failure the VM keeps working exactly as badly as it +/// would have anyway, and the warning says what to do about it. +fn adopt_into_policy_chain(store: &StateStore, config: &GlobalConfig, metadata: &vm::VmMetadata) { + let Some(net) = metadata.network.as_ref() else { + return; + }; + let Some(wan_iface) = net + .wan_iface + .clone() + .or_else(|| network::wan::detect().ok()) + else { + return; + }; + + let ns = config.instance_namespace(); + if let Err(e) = network::policy::ensure(ns) { + eprintln!("Warning: could not set up firewall chains: {e}"); + return; + } + let chains = network::policy::chains(ns); + + let comment = network::nat::comment(ns); + let in_chain = network::nat::VmRules { + chain: Some(&chains.forward), + tap_device: &net.tap_device, + guest_ip: &net.guest_ip, + wan_iface: &wan_iface, + comment: &comment, + }; + let outside = network::nat::VmRules { + chain: None, + ..in_chain + }; + + // Add before removing, so the VM is never without a rule. A brief + // duplicate ACCEPT is harmless. + if let Err(e) = in_chain.add_forwarding() { + eprintln!( + "Warning: VM '{}' still has its firewall rules outside '{}' \ + and may have lost network access ({e}). Restart it with \ + 'ember vm stop {} && ember vm start {}'.", + metadata.name, chains.forward, metadata.name, metadata.name + ); + return; + } + outside.remove_forwarding(); + + // Record where the rules now live, so teardown deletes them from + // the chain rather than from the built-in one. + let result = vm::update(store, &metadata.name, |m| { + if let Some(ref mut net) = m.network { + net.firewall_chain = Some(chains.forward.clone()); + } + Ok(()) + }); + if let Err(e) = result { + eprintln!( + "Warning: failed to record the firewall chain for VM '{}': {e}", + metadata.name + ); + } +} + /// Mark a VM as Stopped, clearing its PID and network info. fn mark_stopped(store: &StateStore, metadata: &vm::VmMetadata) { let result = vm::update(store, &metadata.name, |m| { diff --git a/docs/NETWORK-POLICY-SPEC.md b/docs/NETWORK-POLICY-SPEC.md index e8fe527..69466c1 100644 --- a/docs/NETWORK-POLICY-SPEC.md +++ b/docs/NETWORK-POLICY-SPEC.md @@ -304,6 +304,30 @@ pub struct VmRules<'a> { `comment` stays needed in both modes, for the masquerade rule in the new mode and for all three rules in the legacy mode. +### Adopting VMs that are already running + +A VM running at the moment the chains first appear is the sharp edge of +the upgrade. Its rules are in the built-in FORWARD chain, which is +below the jump, so the chain's terminal DROP cuts it off the instant +another VM start creates the chain. The VM keeps running and silently +loses its network. + +Reconcile, which runs at the start of every command, therefore moves +such a VM's forwarding rules into the chain: add the in-chain pair, +delete the built-in pair, then record `firewall_chain` on the VM so +teardown deletes from the right place. Add before delete, so the VM is +never without a rule. A momentary duplicate ACCEPT is harmless. + +The masquerade rule must be excluded from this move. Its shape is +identical in both modes, so a naive add-then-remove of the full rule +set would delete it immediately after re-adding it and leave the VM +with no NAT. That is why `VmRules` exposes the forwarding pair +separately from the full set. + +The work is one-shot per VM. After it runs, every VM's record points at +a chain and the check costs nothing, so reconcile does not re-assert +rules for VMs that already have them. + ### Landing order hazard The terminal `DROP` in `ember--forward` must not exist until the diff --git a/docs/SPEC.md b/docs/SPEC.md index f760ae5..e476782 100644 --- a/docs/SPEC.md +++ b/docs/SPEC.md @@ -511,6 +511,7 @@ pub struct VmMetadata { On every privileged command invocation (skipped for `init`, `version`, read-only queries, and SSH-client commands), lightweight reconciliation runs (`state/reconcile.rs`): - For each VM in Running or Paused state, check if PID is alive (`kill(pid, 0)`) - Dead process → mark Stopped, cleanup TAP + iptables + IP allocation +- A running VM whose forwarding rules are outside the installation's policy chain → move them in, so the chain's terminal DROP doesn't cut off a VM that was started before the chain existed - Orphaned `em-*` TAP devices without running VM → delete Reconciliation can also be triggered manually via `ember reconcile`. From 8815ba355e8d7a3c728ee4f8ad2b2e58268ec6c8 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 14:20:17 +0200 Subject: [PATCH 08/14] dm-thin: drop redundant borrows in format arguments clippy 1.97 flags these four as useless_borrows_in_formatting, which turns the CI lint step red on the current stable toolchain. Unrelated to the surrounding branch, it just happens to be what is standing between it and a green run. --- crates/ember-linux/src/dm_thin_storage.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/ember-linux/src/dm_thin_storage.rs b/crates/ember-linux/src/dm_thin_storage.rs index 1396dda..067b4e8 100644 --- a/crates/ember-linux/src/dm_thin_storage.rs +++ b/crates/ember-linux/src/dm_thin_storage.rs @@ -224,17 +224,17 @@ impl DmThinStorage { pool::PoolMode::ReadWrite => Ok(()), pool::PoolMode::ReadOnly => Err(Error::Pool(format!( "dm-thin pool '{}' is read-only — run `thin_check` and `thin_repair` to recover", - &self.pool_name + self.pool_name ))), pool::PoolMode::OutOfDataSpace => Err(Error::Pool(format!( "dm-thin pool '{}' is out of data space ({}/{} blocks used) — run `ember storage grow --size ` to extend it", - &self.pool_name, + self.pool_name, status.used_data_blocks, status.total_data_blocks, ))), pool::PoolMode::Failed => Err(Error::Pool(format!( "dm-thin pool '{}' has failed — inspect dmesg and `thin_check` the metadata device", - &self.pool_name + self.pool_name ))), } } @@ -622,7 +622,7 @@ impl StorageBackend for DmThinStorage { let _ = fs::remove_dir(&self.storage_path); } } - println!("dm-thin pool '{}' torn down.", &self.pool_name); + println!("dm-thin pool '{}' torn down.", self.pool_name); Ok(()) } From 534435b4b65bade26fbb01a118e654d4a64fc057 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 15:12:27 +0200 Subject: [PATCH 09/14] docs: record that VM-to-host access stays non-overridable No escape hatch for now. Keeps the shape of one on record in case reaching a host service from a VM turns out to be needed. --- docs/NETWORK-POLICY-SPEC.md | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/docs/NETWORK-POLICY-SPEC.md b/docs/NETWORK-POLICY-SPEC.md index 69466c1..f087915 100644 --- a/docs/NETWORK-POLICY-SPEC.md +++ b/docs/NETWORK-POLICY-SPEC.md @@ -412,13 +412,12 @@ Rough size: 350 to 450 lines including tests, most of it in ## Open decisions -1. **Should VM-to-host be overridable?** Running a service on the - host and hitting it from a VM is a common dev workflow, and this - spec makes it impossible. The escape hatch would be an install-wide - `ember init --allow-host-access` persisted on `GlobalConfig` and - read by `policy::ensure`, which then omits the drop or accepts the - TAP gateway address only. Not specced, since the ask was to block - the host. +1. **Should VM-to-host be overridable?** Decided: no, not for now. The + host block has no escape hatch, and reaching a service on the host + from a VM is simply not possible. If that turns out to be needed, the + shape would be an install-wide `ember init --allow-host-access` + persisted on `GlobalConfig` and read by `policy::ensure`, which would + then omit the drop or accept the TAP gateway address only. 2. **Is punching through the host's `FORWARD` rules acceptable?** The alternative is leaving egress appended at the bottom of the built-in chain, which keeps ember deferential but makes egress and From 524695642a8a7146d58cc2d8ea5fdcc237007d9b Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 16:16:47 +0200 Subject: [PATCH 10/14] tests: scope the post-deinit chain check to this install The check scanned INPUT and FORWARD for the substring 'ember-', which matches any install's jump, not just the one the test tore down. A developer's own install has its chains in those same built-in chains and they outlive its VMs by design, so the assertion failed on every machine that actually runs ember. --- tests/network_policy.rs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/tests/network_policy.rs b/tests/network_policy.rs index f78cedc..8d0ad85 100644 --- a/tests/network_policy.rs +++ b/tests/network_policy.rs @@ -240,10 +240,14 @@ fn policy_chains_are_placed_correctly_and_removed_on_deinit() { !chain_exists(&forward_chain), "{forward_chain} survived deinit" ); - for builtin in ["INPUT", "FORWARD"] { + // Scoped to this install's chain names. The developer's own install + // has its chains in INPUT and FORWARD too, and they legitimately + // outlive its VMs, so anything looser than an exact name here fails + // on every machine that actually runs ember. + for (builtin, chain) in [("INPUT", &input_chain), ("FORWARD", &forward_chain)] { assert!( - !rules(builtin).iter().any(|r| r.contains("ember-")), - "a jump into an ember chain survived deinit in {builtin}" + !rules(builtin).iter().any(|r| r.contains(chain.as_str())), + "the jump into {chain} survived deinit in {builtin}" ); } } From d6079cb82dd9594e79caf5f808d0dffebd244bbb Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 16:23:35 +0200 Subject: [PATCH 11/14] tests: stop VMs on the way out, and size the second VM for the pool Two ways the connectivity test wedged the machine rather than failing. A Firecracker process that outlives the test holds its zvol open, so the harness's pool teardown blocks in 'zpool destroy' in uninterruptible sleep until someone kills the process by hand. A panic anywhere past VM start hit this. Both tests now stop their VMs from a Drop guard, ordered to run before the pool is destroyed. The second VM also ran the ubuntu-slim image, while the harness sizes its pool for exactly one such rootfs. It only has to answer pings, which the guest kernel does by itself, so alpine at 128M does the job. --- tests/network_policy.rs | 49 +++++++++++++++++++++++++++++++++++------ 1 file changed, 42 insertions(+), 7 deletions(-) diff --git a/tests/network_policy.rs b/tests/network_policy.rs index 8d0ad85..df0cffc 100644 --- a/tests/network_policy.rs +++ b/tests/network_policy.rs @@ -80,6 +80,25 @@ impl Drop for ChainCleanup { } } +/// Stops the test's VMs on the way out, including on a panic. +/// +/// Without this, a Firecracker process outlives the test and holds its +/// zvol open, and the harness's pool teardown blocks in `zpool destroy` +/// in uninterruptible sleep until the process is killed by hand. Must +/// be declared *after* the `TestEnv` so it drops before the pool does. +struct VmCleanup { + state: String, + names: Vec<&'static str>, +} + +impl Drop for VmCleanup { + fn drop(&mut self) { + for name in &self.names { + common::stop_and_delete_vm(&self.state, name); + } + } +} + // --------------------------------------------------------------------------- // State helpers // --------------------------------------------------------------------------- @@ -128,10 +147,14 @@ fn policy_chains_are_placed_correctly_and_removed_on_deinit() { let input_chain = format!("ember-{id}-input"); let forward_chain = format!("ember-{id}-forward"); let taps = format!("em{id}-+"); - let _cleanup = ChainCleanup { + let _chain_cleanup = ChainCleanup { input: input_chain.clone(), forward: forward_chain.clone(), }; + let _vm_cleanup = VmCleanup { + state: state.clone(), + names: vec!["polvm"], + }; let net = network_info(&state, "polvm"); let tap = field(&net, "tap_device"); @@ -302,12 +325,26 @@ fn vms_reach_each_other_but_not_the_host() { let state = env.state().to_string(); let id = instance_id(&state); - let _cleanup = ChainCleanup { + let _chain_cleanup = ChainCleanup { input: format!("ember-{id}-input"), forward: format!("ember-{id}-forward"), }; + let _vm_cleanup = VmCleanup { + state: state.clone(), + names: vec!["vma", "vmb"], + }; + + // A second VM in the same install, so the two are siblings. Alpine + // rather than the ubuntu-slim image vma runs: this VM only has to + // answer pings, which the guest kernel does on its own, and the + // harness sizes its pool for exactly one ubuntu rootfs. + let output = common::ember(&["--state-dir", &state, "image", "pull", "alpine:latest"]); + assert!( + output.status.success(), + "pulling alpine failed: {}", + String::from_utf8_lossy(&output.stderr) + ); - // A second VM in the same install, so the two are siblings. let kernel = common::linux::ensure_kernel(); let output = common::ember(&[ "--state-dir", @@ -316,13 +353,13 @@ fn vms_reach_each_other_but_not_the_host() { "create", "vmb", "--image", - "ubuntu-slim", + "alpine:latest", "--kernel", kernel.to_str().unwrap(), "--cpus", "1", "--memory", - "512M", + "128M", ]); assert!( output.status.success(), @@ -369,6 +406,4 @@ fn vms_reach_each_other_but_not_the_host() { 0, "VM A lost outbound connectivity" ); - - common::stop_and_delete_vm(&state, "vmb"); } From 800fad59dbc7ef208261bfbd520478fd2bb46f12 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 16:35:47 +0200 Subject: [PATCH 12/14] tests: assert per-VM rules in the install chain, and stop leaking chains Two problems the vm suite surfaced. The networking test asserted that the built-in FORWARD chain mentions the VM's TAP. Per-VM rules now live in the install's own chain, and FORWARD holds only the jump into it, so the assertion looked for them in the one place they are deliberately absent. It now reads the chain name from the VM's inspect output, and also checks the rules are gone from that chain after a stop. Every test that starts a VM creates the install's chains, and nothing removed them: no test runs 'ember deinit', so each run left two chains and two jumps on the developer's machine and they piled up one pair per run. The pool cleanup guard now removes them, keyed off the instance id in the install's own config. Installs with no instance id are skipped, since their chain names are shared with the developer's real install and there is no way to tell whose chains those are. --- tests/common/linux.rs | 53 +++++++++++++++++++++++++++++++++++++++++ tests/init.rs | 1 + tests/network_policy.rs | 26 -------------------- tests/vm.rs | 23 +++++++++++++++--- 4 files changed, 74 insertions(+), 29 deletions(-) diff --git a/tests/common/linux.rs b/tests/common/linux.rs index 817e953..6e71e8d 100644 --- a/tests/common/linux.rs +++ b/tests/common/linux.rs @@ -88,6 +88,50 @@ pub fn destroy_pool(pool: &str) { let _ = Command::new("zpool").args(["destroy", "-f", pool]).status(); } +/// Remove the firewall chains an installation created, if any. +/// +/// An install's chains are host-global and outlive its VMs by design, so +/// a test that starts a VM and never runs `ember deinit` leaves two +/// chains plus two jumps behind on the developer's machine, and they +/// accumulate one pair per run. +/// +/// Reads the instance id from the install's own config, and does nothing +/// when it is absent or empty. An install predating instance ids shares +/// its chain names with every other such install, including the +/// developer's real one, so there is no safe way to tell whose chains +/// those are. +pub fn remove_install_chains(state_dir: &Path) { + let Ok(raw) = std::fs::read_to_string(state_dir.join("config.json")) else { + return; + }; + let Ok(config) = serde_json::from_str::(&raw) else { + return; + }; + let Some(id) = config["instance_id"].as_str().filter(|s| !s.is_empty()) else { + return; + }; + + for (builtin, chain) in [ + ("INPUT", format!("ember-{id}-input")), + ("FORWARD", format!("ember-{id}-forward")), + ] { + // The jump can exist more than once if rule insertion raced, and + // iptables refuses to delete a chain anything still references. + while Command::new("iptables") + .args(["-w", "5", "-D", builtin, "-j", &chain]) + .status() + .map(|s| s.success()) + .unwrap_or(false) + {} + let _ = Command::new("iptables") + .args(["-w", "5", "-F", &chain]) + .status(); + let _ = Command::new("iptables") + .args(["-w", "5", "-X", &chain]) + .status(); + } +} + /// RAII guard: destroys ZFS pool and detaches loop device on drop. /// /// Use this in tests to ensure cleanup happens even on panic. The @@ -99,10 +143,17 @@ pub struct PoolCleanup { pub pool: String, pub dev: String, pub backing_file: PathBuf, + /// State directory of the install, so its firewall chains can be + /// removed on drop. + pub state_dir: PathBuf, } impl Drop for PoolCleanup { fn drop(&mut self) { + // Before the pool, because a `zpool destroy` that blocks would + // otherwise strand the chains too. + remove_install_chains(&self.state_dir); + destroy_pool(&self.pool); // `zpool destroy -f` can fail (a still-running firecracker @@ -527,6 +578,7 @@ pub fn setup_pool_and_init( pool: pool.clone(), dev: loop_dev.clone(), backing_file: img, + state_dir: state_dir.clone(), }; let output = super::ember(&[ @@ -661,6 +713,7 @@ pub fn setup_pool_init_and_build_ubuntu( pool: pool.clone(), dev: loop_dev.clone(), backing_file: img, + state_dir: state_dir.clone(), }; let output = super::ember(&[ diff --git a/tests/init.rs b/tests/init.rs index bfbf32c..9154273 100644 --- a/tests/init.rs +++ b/tests/init.rs @@ -166,6 +166,7 @@ fn init_custom_dataset_name() { pool: pool.clone(), dev: loop_dev.clone(), backing_file: img, + state_dir: state_dir.clone(), }; let output = common::ember(&[ diff --git a/tests/network_policy.rs b/tests/network_policy.rs index df0cffc..3dc0687 100644 --- a/tests/network_policy.rs +++ b/tests/network_policy.rs @@ -63,23 +63,6 @@ fn chain_exists(chain: &str) -> bool { iptables(&["-S", chain]).is_ok() } -/// Removes an installation's chains, so a failed assertion cannot leave -/// rules behind on the developer's host. -struct ChainCleanup { - input: String, - forward: String, -} - -impl Drop for ChainCleanup { - fn drop(&mut self) { - for (builtin, chain) in [("INPUT", &self.input), ("FORWARD", &self.forward)] { - while iptables(&["-D", builtin, "-j", chain]).is_ok() {} - let _ = iptables(&["-F", chain]); - let _ = iptables(&["-X", chain]); - } - } -} - /// Stops the test's VMs on the way out, including on a panic. /// /// Without this, a Firecracker process outlives the test and holds its @@ -147,10 +130,6 @@ fn policy_chains_are_placed_correctly_and_removed_on_deinit() { let input_chain = format!("ember-{id}-input"); let forward_chain = format!("ember-{id}-forward"); let taps = format!("em{id}-+"); - let _chain_cleanup = ChainCleanup { - input: input_chain.clone(), - forward: forward_chain.clone(), - }; let _vm_cleanup = VmCleanup { state: state.clone(), names: vec!["polvm"], @@ -324,11 +303,6 @@ fn vms_reach_each_other_but_not_the_host() { let env = common::TestEnv::with_running_ssh_vm("netpolicyconn", "vma"); let state = env.state().to_string(); - let id = instance_id(&state); - let _chain_cleanup = ChainCleanup { - input: format!("ember-{id}-input"), - forward: format!("ember-{id}-forward"), - }; let _vm_cleanup = VmCleanup { state: state.clone(), names: vec!["vma", "vmb"], diff --git a/tests/vm.rs b/tests/vm.rs index 64c1dba..0b89685 100644 --- a/tests/vm.rs +++ b/tests/vm.rs @@ -949,15 +949,20 @@ fn networking_ssh_and_internet() { "expected MASQUERADE rule for {guest_ip} in NAT table:\n{nat_rules}" ); - // -- Verify FORWARD chain rules -- + // -- Verify forwarding rules landed in the install's own chain -- + // They deliberately do not go in the built-in FORWARD chain, which + // holds only the jump into this chain. See docs/NETWORK-POLICY-SPEC.md. + let firewall_chain = network["firewall_chain"] + .as_str() + .expect("expected firewall_chain string"); let iptables_fwd = std::process::Command::new("iptables") - .args(["-S", "FORWARD"]) + .args(["-S", firewall_chain]) .output() .expect("failed to run iptables"); let fwd_rules = String::from_utf8_lossy(&iptables_fwd.stdout); assert!( fwd_rules.contains(tap_device), - "expected FORWARD rules mentioning {tap_device}:\n{fwd_rules}" + "expected rules mentioning {tap_device} in {firewall_chain}:\n{fwd_rules}" ); // -- Ping guest from host -- @@ -1065,6 +1070,18 @@ fn networking_ssh_and_internet() { "MASQUERADE rule for {guest_ip} should be gone after stop:\n{nat_rules_after}" ); + // The install's chain outlives its VMs, but the VM's own rules in it + // must not. + let iptables_fwd_after = std::process::Command::new("iptables") + .args(["-S", firewall_chain]) + .output() + .expect("failed to run iptables"); + let fwd_rules_after = String::from_utf8_lossy(&iptables_fwd_after.stdout); + assert!( + !fwd_rules_after.contains(tap_device), + "rules for {tap_device} should be gone from {firewall_chain} after stop:\n{fwd_rules_after}" + ); + // -- Delete VM -- let del_output = common::ember(&["--state-dir", state, "vm", "delete", "netvm"]); assert!( From 9c227e7e1b8ddd5eaa9e38ac96df08655f392669 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 16:42:11 +0200 Subject: [PATCH 13/14] tests: silence expected iptables errors during chain cleanup Most calls in the cleanup path are expected to fail, since a test that never started a VM has no chains to remove. Letting iptables write to the inherited stderr buried the actual test output under a screenful of 'No chain/target/match'. --- tests/common/linux.rs | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/tests/common/linux.rs b/tests/common/linux.rs index 6e71e8d..d34dd8f 100644 --- a/tests/common/linux.rs +++ b/tests/common/linux.rs @@ -111,6 +111,10 @@ pub fn remove_install_chains(state_dir: &Path) { return; }; + // `output()` rather than `status()` throughout: most of these calls + // are expected to fail, because a test that never started a VM has no + // chains to remove, and letting iptables write "No chain/target/match" + // to the inherited stderr buries the actual test output. for (builtin, chain) in [ ("INPUT", format!("ember-{id}-input")), ("FORWARD", format!("ember-{id}-forward")), @@ -119,16 +123,15 @@ pub fn remove_install_chains(state_dir: &Path) { // iptables refuses to delete a chain anything still references. while Command::new("iptables") .args(["-w", "5", "-D", builtin, "-j", &chain]) - .status() - .map(|s| s.success()) + .output() + .map(|o| o.status.success()) .unwrap_or(false) {} - let _ = Command::new("iptables") - .args(["-w", "5", "-F", &chain]) - .status(); - let _ = Command::new("iptables") - .args(["-w", "5", "-X", &chain]) - .status(); + for verb in ["-F", "-X"] { + let _ = Command::new("iptables") + .args(["-w", "5", verb, &chain]) + .output(); + } } } From d2d8e1f81dd9621f4607fb05950875955b372275 Mon Sep 17 00:00:00 2001 From: Aljoscha Krettek Date: Tue, 11 Aug 2026 16:46:38 +0200 Subject: [PATCH 14/14] docs: record the FORWARD position-1 decision Entering FORWARD at position 1 is accepted, along with its consequence that a host admin's mid-chain DROP or REJECT no longer governs ember VM traffic. Notes the escape hatch that remains: ensure only checks that its jump exists, not where it sits, so a rule inserted above the jump survives later VM starts. Splits the section into what is decided and what is still open. --- docs/NETWORK-POLICY-SPEC.md | 38 ++++++++++++++++++++++--------------- 1 file changed, 23 insertions(+), 15 deletions(-) diff --git a/docs/NETWORK-POLICY-SPEC.md b/docs/NETWORK-POLICY-SPEC.md index f087915..f3e5270 100644 --- a/docs/NETWORK-POLICY-SPEC.md +++ b/docs/NETWORK-POLICY-SPEC.md @@ -55,7 +55,7 @@ regardless of what else is in the host's firewall: - IPv6 connectivity of any kind. ember is IPv4-only, and this spec closes IPv6 on ember links rather than policing it. - Per-VM network policy knobs. The design leaves room for them (see - Open decisions) but does not add any. + Still open) but does not add any. - nftables. ember shells out to `iptables`, and that stays. ## Design @@ -410,23 +410,31 @@ Rough size: 350 to 450 lines including tests, most of it in Reporting policy health from `ember info` would close the observability gap. -## Open decisions - -1. **Should VM-to-host be overridable?** Decided: no, not for now. The - host block has no escape hatch, and reaching a service on the host - from a VM is simply not possible. If that turns out to be needed, the - shape would be an install-wide `ember init --allow-host-access` - persisted on `GlobalConfig` and read by `policy::ensure`, which would - then omit the drop or accept the TAP gateway address only. -2. **Is punching through the host's `FORWARD` rules acceptable?** The - alternative is leaving egress appended at the bottom of the - built-in chain, which keeps ember deferential but makes egress and - VM-to-VM behave inconsistently on restrictive hosts. -3. **Should masquerade move into an `ember--postrouting` chain +## Decisions + +1. **VM-to-host is not overridable.** The host block has no escape + hatch, and reaching a service on the host from a VM is simply not + possible. If that turns out to be needed, the shape would be an + install-wide `ember init --allow-host-access` persisted on + `GlobalConfig` and read by `policy::ensure`, which would then omit the drop + or accept the TAP gateway address only. +2. **Entering `FORWARD` at position 1 is accepted**, with the + consequence that a host admin's mid-chain `DROP` or `REJECT` no + longer governs ember VM traffic. The alternative, leaving egress + appended at the bottom of the built-in chain, keeps ember deferential + but makes egress and VM-to-VM behave inconsistently on restrictive + hosts, which is the inconsistency this design exists to remove. An + admin who does want to restrain ember VMs still can: `policy::ensure` only + checks that its jump exists, not where it sits, so a rule inserted + above the jump survives every subsequent VM start. + +## Still open + +1. **Should masquerade move into an `ember--postrouting` chain too?** Uniform scoping and immunity to `POSTROUTING` ordering, at the cost of a third chain and lifecycle. It does not let the comment machinery retire, since legacy deletes need it regardless. -4. **Per-VM opt-out.** A `network.isolated: true` in the VM config +2. **Per-VM opt-out.** A `network.isolated: true` in the VM config would be a per-VM drop inserted above the sibling accept. Cheap to add later, out of scope now.