feat(net): per-VM egress networking (netns + TAP + nftables NAT)#64
Open
markovejnovic wants to merge 35 commits into
Open
feat(net): per-VM egress networking (netns + TAP + nftables NAT)#64markovejnovic wants to merge 35 commits into
markovejnovic wants to merge 35 commits into
Conversation
Pure config plumbing for per-VM egress networking: Hyper.Cfg.Network on the Elixir side (enabled?/uplink/clone_pool) and a matching Network struct + Tools.ip/Tools.nft on the Rust helper side. Absent [network] table means networking stays disabled (today's no-NIC behaviour). The "172.31.0.0/16" clone_pool default is shared verbatim between both languages so an unconfigured node and helper always agree.
…nit) Adds tools/network/args.rs producing pure ip/nft Command sequences (Which::Ip | Which::Nft, argv: Vec<String>) for prepare/teardown/host-init, unit-tested without root. Wires a Network subcommand into the Tool dispatch tree with typed PrepareArgs/TeardownArgs/HostInitArgs and noop run() stubs; real execution lands in Task 4.
…sequence tests nftables accept is a terminating verdict, so the metadata-IP drop rule must precede the broad clone-pool egress accept in the forward chain or a guest-initiated flow to 169.254.169.254 is accepted first. Also add full-Vec<Command> golden tests for prepare_commands and host_init_commands so a wrong flag, address, SNAT/DNAT target, or reordering fails the test instead of slipping past substring checks.
Replaces the Task 3 stubs with real privileged execution: each op resolves the Plan (prepare/teardown) or uplink+clone_pool (host-init) from the validated uid + Config::network(), then runs the args::*_commands argv sequence through a shared exec::run_all, stopping at the first non-zero exit. Refusal (uid out of range, malformed clone_pool CIDR, [network] absent) happens in prepare::resolve/plan_from before any command is ever spawned. host-init is idempotent by checking whether the hyper nft table already carries its masquerade rule before rebuilding the program.
…ing-disabled refusal host-init previously skipped its whole ruleset whenever the `hyper` nft table already contained "masquerade" — but masquerade is added by the 3rd of 7 commands, so a crash between commands 3 and 4 permanently strands the table without the forward-chain drop policy or the metadata-IP block: every later host-init sees masquerade and skips. Replace the presence check with delete-then-recreate: host_init_commands now leads with a failure-tolerant `nft delete table ip hyper` (ENOENT on first run is not an error) followed by the full rebuild, so the table is always either absent or complete, never partial. Also extract the `[network]`-absent refusal into a pure `resolve_network(Option<&Network>) -> Result<&Network, NetworkingDisabled>` shared by prepare/teardown/host_init, so the security-critical networking-disabled refusal is directly unit-testable without root or a live config, and both real call sites run the exact code path the test covers.
…bled BootSpec.resolve/2 now sets network_interfaces and appends the guest ip= cmdline fragment when Cfg.Network.enabled?(), deriving the NIC from vm_id. State.init/1 threads vm_id into the source map so resolve can find it. Disabled path (today's behavior) is unchanged and covered by a unit test; the enabled path needs a [network] config override and is deferred to the e2e/integration suite.
…nabled build_argv now takes an Option<&Path> netns and inserts --netns before the `--` firecracker separator when present. run() computes the path from Config::network().is_some() and the validated --id, never from caller input; this must match the ip netns add <id> path the network tool creates. Kept build_argv a pure function of its parameters (rather than reading global config) so the enabled/disabled cases are unit-testable directly, without needing a process-wide [network] config table in the test process.
Introduces a `prelaunch/1` seam in Daemon.init/1: reset any stale jail, then — gated on Hyper.Cfg.Network.enabled?/0 — prepare the VM's netns via the setuid helper before the jailer launches, since the jailer enters that netns via --netns and refuses to start if it doesn't already exist. clear_jail/1 now takes the full Opts (needs uid) and tears down the network alongside the chroot/cgroup removal wherever the jail is cleared (stale-reset on relaunch, and terminate/2), both best-effort. Disabled networking is behavior-identical to before.
…light Jailer.Checks gains network_ready/0: a no-op when [network] is absent, otherwise verifies the uplink iface exists under /sys/class/net and net.ipv4.ip_forward reads 1. Wired into Checks.run/0. Hyper.Node.test_system/0 now calls Hyper.SuidHelper.Network.host_init/0 once at startup (after the suidhelper presence check) when networking is enabled, reconciling the host-wide `hyper` nftables table idempotently. Rust host_init asserts ip_forward before touching nft, refusing with a message pointing at `sysctl -w net.ipv4.ip_forward=1` rather than setting it itself (that's the provision script's job). The parsing rule is a pure ip_forward_enabled/1 helper, tested in tests/tools/host_init.rs; the privileged /proc read stays in run_privileged.
Extends Plan.orphans/3 to Plan.orphans/4, folding /var/run/netns leaf names into the same (leaves ∪ rw ∪ netns) \ live candidate set that already feeds the two-strike confirm/2 grace. That grace is what keeps this safe: prelaunch/1 creates a VM's netns before it appears in gather_live/0, so an immediate-reap path would race a normally-booting VM. Folding netns into the same gated pipeline means it gets the same protection for free. Reaper.list_netns/0 lists /var/run/netns (gated on Cfg.Network.enabled?(), enoent-safe, mirrors list_cgroup_leaves/0). reap_one/1 calls the new SuidHelper.Network.teardown_orphan/1, which has no uid parameter since a crashed node never persisted one - the netns name alone (== vm_id) is enough. The Rust `network teardown-orphan` subcommand runs `ip netns del <vm_id>` directly, tolerating an ENOENT-shaped failure as success so it doesn't error out when it races an already-completed normal teardown.
Kernel ip= autoconfig sets the guest's address/route but never DNS, so unmodified OCI images boot with no resolver. BootSpec now appends hyper.resolver=<cfg> to the enabled-path cmdline (Hyper.Cfg.Network.resolver/0, default 1.1.1.1); the guest-agent's PID-1 setup() parses it back out of /proc/cmdline after mounting /proc and writes /etc/resolv.conf, best-effort like the existing mount tolerance.
resolver_from_cmdline uses split_whitespace() + strip_prefix() so a
param must be its own whitespace-delimited token, not a substring
match. Add regression tests so a future refactor to
cmdline.contains("hyper.resolver=") — which would match look-alike
tokens like "xhyper.resolver=..." or "hyper.resolverX=..." — cannot
silently reintroduce a DNS-hijack bug. Also add a first-wins case with
surrounding unrelated tokens; the empty-value case was already pinned
by empty_value_is_none.
…k] table disables not bricks host_init_commands only claimed the postrouting/forward hooks, so a guest packet addressed to a host-owned IP (its gateway, or any other host IP) was locally delivered straight to the kernel INPUT hook and past the host's default ACCEPT policy — reaching epmd, distribution, Postgres, gRPC. Add a hyper input chain (policy accept) with an explicit drop of all clone-pool-sourced traffic, so isolation now covers both hooks. Network.uplink had no serde default, and Config is a process-wide LazyLock that exits the whole helper on any parse failure — so a `[network]` table with clone_pool set but uplink omitted bricked every subcommand, not just networking. uplink is now Option<String>; Config::network() returns None when it's unset, matching the Elixir side's enabled?/0 exactly.
# Conflicts: # .github/scripts/provision-kvm-host.sh # native/suidhelper/Cargo.toml # native/suidhelper/src/config.rs # native/suidhelper/src/tools/mod.rs # native/suidhelper/tests/e2e/chroot_jail.rs # native/suidhelper/tests/e2e/jailer.rs
❌ 4 Tests Failed:
View the top 2 failed test(s) by shortest run time
View the full list of 2 ❄️ flaky test(s)
To view more test analytics, go to the Test Analytics Dashboard |
Test Results 4 files ± 0 204 suites +14 3m 47s ⏱️ + 1m 28s For more details on these failures, see this check. Results for commit 18bf73d. ± Comparison against base commit d57336c. This pull request removes 5 and adds 56 tests. Note that renamed tests count towards both.♻️ This comment has been updated with latest results. |
…efault The per-binary default_dmsetup/losetup/blockdev/ip/nft/thin_dump fns were one-line PathBuf wrappers used only by Tools::default. Inline them as "…".into() at the single call site and drop the fns. Kept default_work_dir and default_parent_cgroup (both document a keep-in-sync-with-Elixir invariant) and default_clone_pool (a real #[serde(default = …)] annotation).
The E2E only checked wget's exit code (implicitly 2xx). Capture the server response headers (wget -S, merged via 2>&1) and assert the status line is HTTP .. 200, so a redirect or proxy-injected page can't pass.
Networking was gated on a [network] table being present — absent meant VMs
booted with no NIC (the pre-feature behaviour). Remove that opt-out: every VM
now gets a NIC, and a node refuses to start unless networking is configured
(the Jailer network preflight and Hyper.Node.init_network both hard-fail with
:network_not_configured), joining kvm/firecracker as a host requirement.
- Cfg.Network: enabled?/0 -> configured?/0 (used only by the startup preflight)
- BootSpec.resolve: always attach the NIC + ip=/hyper.resolver= cmdline
- Daemon/Reaper: netns prepare/teardown/sweep run unconditionally
- Jailer.Checks.network_ready: {:error, :network_not_configured} when unset
- e2e network test: drop the self-skip (a booted node already has networking)
- install.md: 'VM Networking (required)', not optional
CI unmodified: the unit job runs --no-start (app never boots); the integration
job provisions [network], so the tree still starts there.
origin/main refactored the e2e test helper run() into the shared `support`
module (`use support::{is_root, run}`). The merge resolution kept HEAD's local
`fn run` too, so the file had a duplicate run() referencing Output/BIN that
live in support — a compile error the local `cargo nextest run` never caught
because the e2e tests only compile under --features insecure_test_seams (which
CI's --all-features enables). Delete the local duplicates; use support::run.
host-init creates the `hyper` nftables table and its masquerade/SNAT/DNAT rules at node start. A fresh GitHub-hosted runner has neither module loaded and the netfilter family does not reliably autoload on first `nft` use, so `nft add table ip hyper` failed (empty stderr, non-zero exit) and the node refused to start — failing every integration test. Load the base modules explicitly, as the script already does for dm_snapshot/dm_thin_pool/loop.
mknod_char asked for 0666 but mknodat masks its mode with the process umask (022 -> 0644), so dev/net/tun came out 0644 despite the code and doc both saying 0666. The privileged e2e test caught it (0644 vs 0666) once it finally ran as root. Set the exact mode with an explicit fchmodat, which ignores umask.
The privilege guard raises only euid to 0, leaving ruid at the caller. When the helper spawned nft/ip with that ruid!=euid mismatch, the kernel flagged the child AT_SECURE and modern nftables/iproute2 silently refuse to run — nft exits 111 with empty stdout+stderr, so host-init failed and the node would not start. Promote each spawned network command to full root (become_root_permanently) in a pre_exec, the same 'hand it a genuine root process' the jailer handoff needs. Only the integration path (BEAM as non-root -> setuid helper) hit this; the root-run e2e tests never had the uid mismatch. Also surface the child's real exit code + stdout in the error, since an empty-stderr 111 was undiagnosable.
…/ip) On merged-usr distros /usr/sbin/ip is a symlink to /usr/bin/ip, and SafeBin rejects symlinked tool paths — so network prepare failed with '/usr/sbin/ip is a symlink' and every VM create returned :no_capacity. Point the default at iproute2's real binary. nft/dmsetup/losetup/blockdev are real files at /usr/sbin, so only ip was affected.
prepare's NAT setup ran 'ip netns exec <ns> nft ...' with a bare 'nft'. The helper spawns every command with a cleared environment (no PATH), so 'ip netns exec' could not resolve nft: 'exec of "nft" failed: No such file or directory'. Thread the configured absolute nft path into nft_ns so the exec names the exact binary (consistent with the config-names-the-binary model). Verified end to end via the setuid path: host-init + prepare + teardown all succeed.
Daemon.clear_jail runs 'network teardown' speculatively before launching a fresh VM, to clear any stale prior incarnation — so on a first boot the netns and host veth do not exist yet. 'ip netns del' on a missing namespace exits non-zero, which failed prelaunch and returned :no_capacity for every fresh VM create. Mark both idempotent deletes allow_failure, matching the chroot removal beside them; a genuine leak is backstopped by the Reaper.
When a VM cold-boots after a failed attempt, jail staging re-runs while the prior attempt's dev/net/tun may still exist, so mknodat returned EEXIST and every retry failed 'staging jail'. Tolerate EEXIST like mkdir already does; the chown+chmod that follow re-assert the exact ownership and 0666 mode, so a surviving node ends up identical to a fresh one.
guest_mac/1 built [0x02, b1, b2, b3, b4] — only five octets. Firecracker's NIC
config rejects anything but a full XX:XX:XX:XX:XX:XX MAC (HTTP 400 'invalid MAC
address'), so with networking mandatory EVERY VM failed 'applying config' and
restart-looped, never booting the guest agent — which surfaced as the exec-over-
vsock :preface_timeout / {:error, :timeout} in every E2E test. Take a fifth hash
byte for a six-octet address. The MAC test asserted the leading byte but never
the octet count; add that assertion so a short MAC can't regress.
An unknown image id resolves to no layers, and build_chain/2 only matched
[base | deltas], so init/1 crashed with a FunctionClauseError + SASL report
instead of refusing cleanly. The placement still resolved to :no_capacity, so
no test failed, but every create_vm-refusal property run flooded CI logs with
~25 crash reports that buried the real diagnostics. Add a []-clause returning
{:error, :unknown_image} so init stops cleanly; the :no_capacity contract is
unchanged.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.