From b1c2345190204c3fe29923b15016db41e3aee69d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:22:58 +0000 Subject: [PATCH 01/42] feat(net): [network] config table (uplink, clone_pool) on both sides 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. --- lib/hyper/cfg/network.ex | 29 ++++++++++++++ native/suidhelper/Cargo.toml | 4 ++ native/suidhelper/src/config.rs | 46 +++++++++++++++++++++++ native/suidhelper/tests/config_network.rs | 8 ++++ test/hyper/cfg/network_test.exs | 17 +++++++++ 5 files changed, 104 insertions(+) create mode 100644 lib/hyper/cfg/network.ex create mode 100644 native/suidhelper/tests/config_network.rs create mode 100644 test/hyper/cfg/network_test.exs diff --git a/lib/hyper/cfg/network.ex b/lib/hyper/cfg/network.ex new file mode 100644 index 00000000..8b03d4eb --- /dev/null +++ b/lib/hyper/cfg/network.ex @@ -0,0 +1,29 @@ +defmodule Hyper.Cfg.Network do + @moduledoc """ + VM egress networking settings from the `[network]` table — `config.toml`-only + because the setuid helper reads the same `uplink` and `clone_pool` to build + each VM's netns, veth, and NAT rules. Absent table ⇒ networking disabled: VMs + boot with no NIC (today's behaviour), the jailer omits `--netns`. + """ + + import Hyper.Cfg, only: [get_cfg: 1] + + @default_clone_pool "172.31.0.0/16" + + @doc "Whether VM egress networking is turned on (`[network] uplink` present)." + @spec enabled?() :: boolean() + def enabled?, do: not is_nil(get_cfg(toml: "network.uplink", default: nil)) + + @doc "Physical uplink interface guest egress is NAT'd out of. Raises if enabled but unset." + @spec uplink() :: String.t() + def uplink do + case get_cfg(toml: "network.uplink", default: nil) do + nil -> raise Hyper.Cfg.MissingError, "network.uplink is required when networking is enabled" + v when is_binary(v) -> v + end + end + + @doc "IPv4 CIDR the per-VM clone /30s are carved from. `[network] clone_pool`." + @spec clone_pool() :: String.t() + def clone_pool, do: get_cfg(toml: "network.clone_pool", default: @default_clone_pool) +end diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 610c63b0..ecc38c53 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -92,6 +92,10 @@ path = "tests/e2e/chroot_jail.rs" name = "config_uid_gid_range" path = "tests/config_uid_gid_range.rs" +[[test]] +name = "config_network" +path = "tests/config_network.rs" + [dependencies] clap = { version = "4", features = ["derive"] } hyper-suidhelper-meta = { path = "meta" } diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index 41b00a75..73443096 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -101,6 +101,8 @@ pub struct Config { tools: Tools, #[serde(default)] jails: Jails, + #[serde(default)] + network: Option, } /// The `[jails]` table: how the helper places and confines each VM jail. @@ -139,6 +141,8 @@ pub struct Tools { dmsetup: PathBuf, losetup: PathBuf, blockdev: PathBuf, + ip: PathBuf, + nft: PathBuf, firecracker: Option, jailer: Option, } @@ -149,6 +153,8 @@ impl Default for Tools { dmsetup: default_dmsetup(), losetup: default_losetup(), blockdev: default_blockdev(), + ip: default_ip(), + nft: default_nft(), firecracker: None, jailer: None, } @@ -174,6 +180,14 @@ fn default_blockdev() -> PathBuf { PathBuf::from("/usr/sbin/blockdev") } +fn default_ip() -> PathBuf { + PathBuf::from("/usr/sbin/ip") +} + +fn default_nft() -> PathBuf { + PathBuf::from("/usr/sbin/nft") +} + fn default_parent_cgroup() -> String { // Must match Elixir node's `@parent_cgroup`; operators need to keep them in sync. "hyper".into() @@ -185,10 +199,27 @@ impl Default for Config { work_dir: default_work_dir(), tools: Tools::default(), jails: Jails::default(), + network: None, } } } +/// The `[network]` table: VM egress networking. Absent ⇒ networking disabled. +/// `uplink` is the physical interface guest traffic is masqueraded out of; +/// `clone_pool` is the IPv4 CIDR per-VM /30 clone addresses are carved from +/// (default `172.31.0.0/16`). Both are read identically by the Elixir node +/// (`Hyper.Cfg.Network`) — keep the defaults in sync. +#[derive(Debug, Clone, Deserialize)] +pub struct Network { + pub uplink: String, + #[serde(default = "default_clone_pool")] + pub clone_pool: String, +} + +fn default_clone_pool() -> String { + "172.31.0.0/16".into() +} + impl Config { /// The process-wide config, loaded once (and forced unprivileged by /// [`Config::init`]). An absent file yields the built-in defaults; a @@ -232,6 +263,21 @@ impl Config { SafeBin::from_path(&self.tools.blockdev) } + /// The validated `ip` (iproute2) binary the network tool runs. + pub fn ip(&self) -> Result, safe_bin::Error> { + SafeBin::from_path(&self.tools.ip) + } + + /// The validated `nft` (nftables) binary the network tool runs. + pub fn nft(&self) -> Result, safe_bin::Error> { + SafeBin::from_path(&self.tools.nft) + } + + /// The `[network]` table, or `None` when VM networking is disabled. + pub fn network(&self) -> Option<&Network> { + self.network.as_ref() + } + /// The Firecracker VMM binary, validated as root-owned and correctly named. /// Errors [`BinError::Unconfigured`] when absent from config — an operator /// must set `[tools] firecracker` before any VM can be launched. diff --git a/native/suidhelper/tests/config_network.rs b/native/suidhelper/tests/config_network.rs new file mode 100644 index 00000000..2e0bc3f7 --- /dev/null +++ b/native/suidhelper/tests/config_network.rs @@ -0,0 +1,8 @@ +use hyper_suidhelper::config::Config; + +#[test] +fn absent_network_table_means_disabled() { + // Default config (no file) has no [network] table. + let cfg = Config::default(); + assert!(cfg.network().is_none()); +} diff --git a/test/hyper/cfg/network_test.exs b/test/hyper/cfg/network_test.exs new file mode 100644 index 00000000..d6c51b81 --- /dev/null +++ b/test/hyper/cfg/network_test.exs @@ -0,0 +1,17 @@ +defmodule Hyper.Cfg.NetworkTest do + use ExUnit.Case, async: true + alias Hyper.Cfg.Network + + describe "clone_pool/0" do + test "defaults when unset" do + assert Network.clone_pool() == "172.31.0.0/16" + end + end + + describe "enabled?/0" do + test "false when no uplink configured" do + # Base test config sets no [network] table. + refute Network.enabled?() + end + end +end From 1f2bf4e275d63a380e8f80a18b3d0295e225bd12 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:29:02 +0000 Subject: [PATCH 02/42] test(net): prove clone_pool default sync + uplink refusal/fallback contracts --- lib/hyper/cfg/network.ex | 1 + native/suidhelper/tests/config_network.rs | 11 ++++++++++- test/hyper/cfg/network_test.exs | 18 +++++++++++++++++- 3 files changed, 28 insertions(+), 2 deletions(-) diff --git a/lib/hyper/cfg/network.ex b/lib/hyper/cfg/network.ex index 8b03d4eb..2ec2b6bd 100644 --- a/lib/hyper/cfg/network.ex +++ b/lib/hyper/cfg/network.ex @@ -20,6 +20,7 @@ defmodule Hyper.Cfg.Network do case get_cfg(toml: "network.uplink", default: nil) do nil -> raise Hyper.Cfg.MissingError, "network.uplink is required when networking is enabled" v when is_binary(v) -> v + other -> raise ArgumentError, "network.uplink must be a string, got: #{inspect(other)}" end end diff --git a/native/suidhelper/tests/config_network.rs b/native/suidhelper/tests/config_network.rs index 2e0bc3f7..54988f8a 100644 --- a/native/suidhelper/tests/config_network.rs +++ b/native/suidhelper/tests/config_network.rs @@ -1,4 +1,4 @@ -use hyper_suidhelper::config::Config; +use hyper_suidhelper::config::{Config, Network}; #[test] fn absent_network_table_means_disabled() { @@ -6,3 +6,12 @@ fn absent_network_table_means_disabled() { let cfg = Config::default(); assert!(cfg.network().is_none()); } + +#[test] +fn present_table_with_only_uplink_defaults_clone_pool() { + // clone_pool is safety-critical: it must match Elixir's + // Hyper.Cfg.Network.clone_pool/0 default. This is the present-path test — + // it fails if this default ever drifts from the Elixir literal. + let network: Network = toml::from_str("uplink = \"eth0\"\n").expect("valid TOML"); + assert_eq!(network.clone_pool, "172.31.0.0/16"); +} diff --git a/test/hyper/cfg/network_test.exs b/test/hyper/cfg/network_test.exs index d6c51b81..d15de851 100644 --- a/test/hyper/cfg/network_test.exs +++ b/test/hyper/cfg/network_test.exs @@ -1,9 +1,12 @@ defmodule Hyper.Cfg.NetworkTest do - use ExUnit.Case, async: true + use ExUnit.Case, async: false alias Hyper.Cfg.Network + alias Hyper.Cfg.Toml describe "clone_pool/0" do test "defaults when unset" do + # Mirrors default_clone_pool() in native/suidhelper/src/config.rs — the + # two literals are safety-critical and must move together. assert Network.clone_pool() == "172.31.0.0/16" end end @@ -14,4 +17,17 @@ defmodule Hyper.Cfg.NetworkTest do refute Network.enabled?() end end + + describe "uplink/0" do + test "raises Hyper.Cfg.MissingError when network.uplink is unset" do + # Pins the refusal contract: a [network] table present without `uplink` + # must raise, not silently disable networking or crash uninformatively. + on_exit(fn -> Toml.reload() end) + Toml.put_cache(%{"network" => %{}}) + + assert_raise Hyper.Cfg.MissingError, ~r/network\.uplink/, fn -> + Network.uplink() + end + end + end end From 05c6803f4e59b55061325be2d871bbbb9a74119c Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:32:08 +0000 Subject: [PATCH 03/42] feat(suidhelper): pure per-VM network address math with refusal contracts --- native/suidhelper/Cargo.toml | 4 ++ native/suidhelper/src/tools/mod.rs | 1 + native/suidhelper/src/tools/network/addr.rs | 71 +++++++++++++++++++ native/suidhelper/src/tools/network/mod.rs | 3 + native/suidhelper/tests/tools/network_addr.rs | 55 ++++++++++++++ 5 files changed, 134 insertions(+) create mode 100644 native/suidhelper/src/tools/network/addr.rs create mode 100644 native/suidhelper/src/tools/network/mod.rs create mode 100644 native/suidhelper/tests/tools/network_addr.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index ecc38c53..4b50ae14 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -52,6 +52,10 @@ path = "tests/tools/dmsetup_parsers.rs" name = "tools_jailer" path = "tests/tools/jailer.rs" +[[test]] +name = "tools_network_addr" +path = "tests/tools/network_addr.rs" + [[test]] name = "util_confinement" path = "tests/util/confinement.rs" diff --git a/native/suidhelper/src/tools/mod.rs b/native/suidhelper/src/tools/mod.rs index 9f9fedae..ad85239d 100644 --- a/native/suidhelper/src/tools/mod.rs +++ b/native/suidhelper/src/tools/mod.rs @@ -9,6 +9,7 @@ pub mod chroot_jail; mod dmsetup; pub mod jailer; mod losetup; +pub mod network; pub use blockdev::{Blockdev, BlockdevArgs}; pub use chroot_jail::ChrootJailOp; diff --git a/native/suidhelper/src/tools/network/addr.rs b/native/suidhelper/src/tools/network/addr.rs new file mode 100644 index 00000000..905506a0 --- /dev/null +++ b/native/suidhelper/src/tools/network/addr.rs @@ -0,0 +1,71 @@ +//! Pure address & interface-name math for per-VM egress networking. +//! +//! Laws (see `tests/tools/network_addr.rs`): +//! - the derived clone `/30` always lands inside the configured pool, never in +//! the inner `172.30.0.0/30` constant space; +//! - `veth` names always satisfy `IFNAMSIZ` (<= 15); +//! - a `uid` below the accepted range, or a `slot` past the pool's capacity, is +//! *always* refused — never wrapped into another VM's address. +use crate::tools::jailer::VmId; +use std::net::Ipv4Addr; +use thiserror::Error; + +pub const INNER_TAP_IP: Ipv4Addr = Ipv4Addr::new(172, 30, 0, 1); +pub const INNER_GUEST_IP: Ipv4Addr = Ipv4Addr::new(172, 30, 0, 2); +pub const INNER_PREFIX: u8 = 30; +pub const TAP_NAME: &str = "tap0"; +pub const GUEST_IFACE: &str = "eth0"; + +#[derive(Debug, Error)] +pub enum AddrError { + #[error("uid {uid} is below the accepted range floor {floor}")] + UidBelowRange { uid: u32, floor: u32 }, + #[error("slot {slot} does not fit the clone pool")] + SlotOutOfPool { slot: u32 }, +} + +#[derive(Debug, Clone)] +pub struct Plan { + pub slot: u32, + pub netns: String, + pub veth_host: String, + pub veth_ns: String, + pub veth_host_ip: Ipv4Addr, + pub veth_ns_ip: Ipv4Addr, +} + +impl Plan { + pub fn derive( + uid: u32, + uid_range: (u32, u32), + clone_pool: Ipv4Addr, + vm_id: &VmId, + ) -> Result { + let floor = uid_range.0; + if uid < floor { + return Err(AddrError::UidBelowRange { uid, floor }); + } + let slot = uid - floor; + // Each VM consumes a /30 (4 addresses). The block's last address must + // stay within the 32-bit space above the pool base. + let base = u32::from(clone_pool); + let block = base + .checked_add( + slot.checked_mul(4) + .ok_or(AddrError::SlotOutOfPool { slot })?, + ) + .ok_or(AddrError::SlotOutOfPool { slot })?; + // Guard against overflowing the pool's own /16 (65_536 addresses). + if slot >= (1u32 << 16) / 4 { + return Err(AddrError::SlotOutOfPool { slot }); + } + Ok(Self { + slot, + netns: vm_id.to_string(), + veth_host: format!("hv{slot}"), + veth_ns: format!("hp{slot}"), + veth_host_ip: Ipv4Addr::from(block + 1), + veth_ns_ip: Ipv4Addr::from(block + 2), + }) + } +} diff --git a/native/suidhelper/src/tools/network/mod.rs b/native/suidhelper/src/tools/network/mod.rs new file mode 100644 index 00000000..2d9bc1ed --- /dev/null +++ b/native/suidhelper/src/tools/network/mod.rs @@ -0,0 +1,3 @@ +// SPDX-License-Identifier: AGPL-3.0-only +//! `network`: per-VM egress networking (netns + veth + TAP + NAT). +pub mod addr; diff --git a/native/suidhelper/tests/tools/network_addr.rs b/native/suidhelper/tests/tools/network_addr.rs new file mode 100644 index 00000000..9ecc086e --- /dev/null +++ b/native/suidhelper/tests/tools/network_addr.rs @@ -0,0 +1,55 @@ +use hyper_suidhelper::tools::jailer::VmId; +use hyper_suidhelper::tools::network::addr::{self, AddrError, Plan}; +use proptest::prelude::*; +use std::net::Ipv4Addr; +use std::str::FromStr; + +const POOL: Ipv4Addr = Ipv4Addr::new(172, 31, 0, 0); +const RANGE: (u32, u32) = (900_000, 999_999); + +fn vm_id() -> VmId { + VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap() +} + +#[test] +fn slot_zero_is_first_block() { + let p = Plan::derive(900_000, RANGE, POOL, &vm_id()).unwrap(); + assert_eq!(p.slot, 0); + assert_eq!(p.veth_host_ip, Ipv4Addr::new(172, 31, 0, 1)); + assert_eq!(p.veth_ns_ip, Ipv4Addr::new(172, 31, 0, 2)); + assert_eq!(p.veth_host, "hv0"); + assert_eq!(p.veth_ns, "hp0"); +} + +#[test] +fn uid_below_range_is_refused() { + assert!(matches!( + Plan::derive(42, RANGE, POOL, &vm_id()), + Err(AddrError::UidBelowRange { .. }) + )); +} + +proptest! { + // Inner and clone address spaces never overlap, and veth names always fit + // IFNAMSIZ, for every uid the helper would ever accept. + #[test] + fn derived_plan_is_well_formed(offset in 0u32..16_384) { + let uid = RANGE.0 + offset; + let p = Plan::derive(uid, RANGE, POOL, &vm_id()).unwrap(); + prop_assert!(p.veth_host.len() <= 15 && p.veth_ns.len() <= 15); + // clone addresses live in 172.31/16, never in the inner 172.30/30 + prop_assert_eq!(p.veth_host_ip.octets()[1], 31); + prop_assert_ne!(p.veth_host_ip, addr::INNER_GUEST_IP); + // host and ns ends are distinct and adjacent + prop_assert_eq!(u32::from(p.veth_ns_ip), u32::from(p.veth_host_ip) + 1); + } + + // A slot past the /16's capacity is always refused, never wrapped. + #[test] + fn slot_past_pool_is_refused(offset in 16_384u32..40_000) { + let uid = RANGE.0 + offset; + let result = Plan::derive(uid, RANGE, POOL, &vm_id()); + let is_slot_out_of_pool = matches!(result, Err(AddrError::SlotOutOfPool { .. })); + prop_assert!(is_slot_out_of_pool); + } +} From caccf44087d7befee9b0b9984367ca8235711d3e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:38:11 +0000 Subject: [PATCH 04/42] test(suidhelper): prove per-VM address injectivity + checked final address arithmetic --- native/suidhelper/src/tools/network/addr.rs | 10 ++++++-- native/suidhelper/tests/tools/network_addr.rs | 24 +++++++++++++++++++ 2 files changed, 32 insertions(+), 2 deletions(-) diff --git a/native/suidhelper/src/tools/network/addr.rs b/native/suidhelper/src/tools/network/addr.rs index 905506a0..2b2ef9b5 100644 --- a/native/suidhelper/src/tools/network/addr.rs +++ b/native/suidhelper/src/tools/network/addr.rs @@ -59,13 +59,19 @@ impl Plan { if slot >= (1u32 << 16) / 4 { return Err(AddrError::SlotOutOfPool { slot }); } + let veth_host_ip = block + .checked_add(1) + .ok_or(AddrError::SlotOutOfPool { slot })?; + let veth_ns_ip = block + .checked_add(2) + .ok_or(AddrError::SlotOutOfPool { slot })?; Ok(Self { slot, netns: vm_id.to_string(), veth_host: format!("hv{slot}"), veth_ns: format!("hp{slot}"), - veth_host_ip: Ipv4Addr::from(block + 1), - veth_ns_ip: Ipv4Addr::from(block + 2), + veth_host_ip: Ipv4Addr::from(veth_host_ip), + veth_ns_ip: Ipv4Addr::from(veth_ns_ip), }) } } diff --git a/native/suidhelper/tests/tools/network_addr.rs b/native/suidhelper/tests/tools/network_addr.rs index 9ecc086e..96ec7d93 100644 --- a/native/suidhelper/tests/tools/network_addr.rs +++ b/native/suidhelper/tests/tools/network_addr.rs @@ -52,4 +52,28 @@ proptest! { let is_slot_out_of_pool = matches!(result, Err(AddrError::SlotOutOfPool { .. })); prop_assert!(is_slot_out_of_pool); } + + // Two distinct uids always derive non-overlapping /30 blocks: the gap + // between their veth_host_ip addresses is exactly 4x the slot distance, + // never wrapping one VM's address into another's. + #[test] + fn distinct_slots_never_share_a_block( + offset_a in 0u32..16_384, + offset_b in 0u32..16_384, + ) { + let uid_a = RANGE.0 + offset_a; + let uid_b = RANGE.0 + offset_b; + let plan_a = Plan::derive(uid_a, RANGE, POOL, &vm_id()).unwrap(); + let plan_b = Plan::derive(uid_b, RANGE, POOL, &vm_id()).unwrap(); + + let ip_a = u32::from(plan_a.veth_host_ip); + let ip_b = u32::from(plan_b.veth_host_ip); + let slot_gap = i64::from(plan_b.slot) - i64::from(plan_a.slot); + let ip_gap = i64::from(ip_b) - i64::from(ip_a); + + prop_assert_eq!(ip_gap, slot_gap * 4); + if plan_a.slot != plan_b.slot { + prop_assert_ne!(ip_a, ip_b); + } + } } From 310bf595d852b3b600c965f57fc261c6bf6458a4 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:43:57 +0000 Subject: [PATCH 05/42] feat(suidhelper): network tool argv builders (prepare/teardown/host-init) Adds tools/network/args.rs producing pure ip/nft Command sequences (Which::Ip | Which::Nft, argv: Vec) 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. --- native/suidhelper/Cargo.toml | 4 + native/suidhelper/src/tools/mod.rs | 7 + native/suidhelper/src/tools/network/args.rs | 301 ++++++++++++++++++ native/suidhelper/src/tools/network/mod.rs | 54 ++++ native/suidhelper/tests/tools/network_args.rs | 56 ++++ 5 files changed, 422 insertions(+) create mode 100644 native/suidhelper/src/tools/network/args.rs create mode 100644 native/suidhelper/tests/tools/network_args.rs diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index 4b50ae14..d3e006e1 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -56,6 +56,10 @@ path = "tests/tools/jailer.rs" name = "tools_network_addr" path = "tests/tools/network_addr.rs" +[[test]] +name = "tools_network_args" +path = "tests/tools/network_args.rs" + [[test]] name = "util_confinement" path = "tests/util/confinement.rs" diff --git a/native/suidhelper/src/tools/mod.rs b/native/suidhelper/src/tools/mod.rs index ad85239d..4a33a748 100644 --- a/native/suidhelper/src/tools/mod.rs +++ b/native/suidhelper/src/tools/mod.rs @@ -15,6 +15,7 @@ pub use blockdev::{Blockdev, BlockdevArgs}; pub use chroot_jail::ChrootJailOp; pub use dmsetup::{DmTable, Dmsetup, DmsetupArgs, ThinMessage}; pub use losetup::{Losetup, LosetupArgs}; +pub use network::NetworkOp; use crate::config::Config; use crate::util::setuid_privileged::{self, Privileged}; @@ -93,6 +94,11 @@ pub enum Tool { #[command(subcommand)] op: ChrootJailOp, }, + /// Per-VM egress networking (netns + veth + TAP + NAT). + Network { + #[command(subcommand)] + op: NetworkOp, + }, } impl Tool { @@ -116,6 +122,7 @@ impl Tool { Blockdev::new(bin.into(), args).run() } Tool::ChrootJail { op } => op.run(), + Tool::Network { op } => op.run(), } } } diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs new file mode 100644 index 00000000..583ad4a8 --- /dev/null +++ b/native/suidhelper/src/tools/network/args.rs @@ -0,0 +1,301 @@ +//! Pure `ip`/`nft` argv builders for per-VM egress networking. +//! +//! Each step is data — a [`Command`] naming which privileged binary runs it and +//! its exact argv — so the sequences below can be asserted in +//! `tests/tools/network_args.rs` without root or a real network device. +//! Execution (actually invoking the binaries) lands in Task 4; this module only +//! builds the argv. +//! +//! Laws (tested): +//! - `prepare_commands` creates the netns first, and only requests the in-netns +//! default route after the host veth (`hv`) is brought up — the guest's +//! route target must already be reachable when the route is added. +//! - `teardown_commands` always deletes the netns, which reclaims the ns-side +//! veth peer, the TAP device, and any in-netns nftables state. +//! - `host_init_commands` masquerades the clone pool out the configured uplink +//! and drops guest traffic addressed to the cloud metadata IP +//! (`169.254.169.254`), so no VM can ever reach it. +use super::addr::{self, Plan}; + +/// Which privileged binary a [`Command`] invokes. Kept to exactly two — `ip` +/// and `nft` — so the setuid helper's whitelist never grows past what egress +/// networking needs. In-netns `nft` operations still run as `ip netns exec +/// nft ...`, so they are represented as [`Which::Ip`] commands. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum Which { + Ip, + Nft, +} + +/// One step of a prepare/teardown/host-init sequence: the binary to run and its +/// exact argv (argv[0] is the binary itself, named by [`Which`], not repeated +/// here). +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct Command { + pub bin: Which, + pub argv: Vec, +} + +macro_rules! argv { + ($($x:expr),* $(,)?) => { + vec![$($x.to_string()),*] + }; +} + +impl Command { + fn ip(argv: Vec) -> Self { + Self { + bin: Which::Ip, + argv, + } + } + + /// `ip -n ...` — runs an `ip` operation inside the VM's netns. + fn ip_ns(netns: &str, argv: Vec) -> Self { + let mut full = argv!["-n", netns]; + full.extend(argv); + Self::ip(full) + } + + fn nft(argv: Vec) -> Self { + Self { + bin: Which::Nft, + argv, + } + } + + /// `ip netns exec nft ...` — runs an `nft` operation inside the + /// VM's netns, kept as a [`Which::Ip`] command so only `ip` and `nft` are + /// ever the privileged binary named directly. + fn nft_ns(netns: &str, argv: Vec) -> Self { + let mut full = argv!["netns", "exec", netns, "nft"]; + full.extend(argv); + Self::ip(full) + } +} + +/// Bring up the VM's netns, host/ns veth pair, TAP device, and in-netns NAT so +/// the guest's inner address (`addr::INNER_GUEST_IP`) can reach the host's +/// uplink. The default route is requested last, after the host veth end +/// (`hv`) is up, so the route's target is already reachable. +pub fn prepare_commands(plan: &Plan) -> Vec { + let netns = plan.netns.as_str(); + let veth_host = plan.veth_host.as_str(); + let veth_ns = plan.veth_ns.as_str(); + let veth_host_ip = plan.veth_host_ip; + let veth_ns_ip = plan.veth_ns_ip; + + let mut commands = vec![ + Command::ip(argv!["netns", "add", netns]), + Command::ip(argv![ + "link", "add", veth_host, "type", "veth", "peer", "name", veth_ns + ]), + Command::ip(argv!["link", "set", veth_ns, "netns", netns]), + Command::ip(argv![ + "addr", + "add", + format!("{veth_host_ip}/{}", addr::INNER_PREFIX), + "dev", + veth_host + ]), + Command::ip(argv!["link", "set", veth_host, "up"]), + Command::ip_ns( + netns, + argv![ + "addr", + "add", + format!("{veth_ns_ip}/{}", addr::INNER_PREFIX), + "dev", + veth_ns + ], + ), + Command::ip_ns(netns, argv!["link", "set", veth_ns, "up"]), + Command::ip_ns(netns, argv!["tuntap", "add", addr::TAP_NAME, "mode", "tap"]), + Command::ip_ns( + netns, + argv![ + "addr", + "add", + format!("{}/{}", addr::INNER_TAP_IP, addr::INNER_PREFIX), + "dev", + addr::TAP_NAME + ], + ), + Command::ip_ns(netns, argv!["link", "set", addr::TAP_NAME, "up"]), + Command::ip_ns(netns, argv!["link", "set", "lo", "up"]), + Command::ip_ns(netns, argv!["route", "add", "default", "via", veth_host_ip]), + ]; + commands.extend(nft_prepare_commands(netns, veth_ns, veth_ns_ip)); + commands +} + +/// The in-netns NAT: SNAT the guest's inner source address to the netns's own +/// veth IP on the way out (so the host sees a source unique to this VM), and +/// DNAT return traffic addressed to that veth IP back to the guest. +fn nft_prepare_commands( + netns: &str, + veth_ns: &str, + veth_ns_ip: std::net::Ipv4Addr, +) -> Vec { + vec![ + Command::nft_ns(netns, argv!["add", "table", "ip", "nat"]), + Command::nft_ns( + netns, + argv![ + "add", + "chain", + "ip", + "nat", + "post", + "{", + "type", + "nat", + "hook", + "postrouting", + "priority", + "100", + ";", + "}" + ], + ), + Command::nft_ns( + netns, + argv![ + "add", + "rule", + "ip", + "nat", + "post", + "ip", + "saddr", + addr::INNER_GUEST_IP, + "oifname", + veth_ns, + "snat", + "to", + veth_ns_ip + ], + ), + Command::nft_ns( + netns, + argv![ + "add", + "chain", + "ip", + "nat", + "pre", + "{", + "type", + "nat", + "hook", + "prerouting", + "priority", + "-100", + ";", + "}" + ], + ), + Command::nft_ns( + netns, + argv![ + "add", + "rule", + "ip", + "nat", + "pre", + "ip", + "daddr", + veth_ns_ip, + "dnat", + "to", + addr::INNER_GUEST_IP + ], + ), + ] +} + +/// Tear down a VM's netns. Deleting the netns reclaims the ns-side veth peer, +/// the TAP device, and any in-netns nftables state with it; the host-side veth +/// end usually goes with it too, but `ip link del` is issued explicitly in +/// case it lingers (a no-op if already gone). +pub fn teardown_commands(plan: &Plan) -> Vec { + vec![ + Command::ip(argv!["netns", "del", plan.netns.as_str()]), + Command::ip(argv!["link", "del", plan.veth_host.as_str()]), + ] +} + +/// One-time host setup: a `hyper` nftables table that masquerades the clone +/// pool out `uplink`, and a forward-chain default-drop policy that only admits +/// the pool's own traffic — explicitly dropping anything addressed to the +/// cloud metadata IP (`169.254.169.254`), so no VM can ever reach it. +pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { + vec![ + Command::nft(argv!["add", "table", "ip", "hyper"]), + Command::nft(argv![ + "add", + "chain", + "ip", + "hyper", + "postrouting", + "{", + "type", + "nat", + "hook", + "postrouting", + "priority", + "100", + ";", + "}" + ]), + Command::nft(argv![ + "add", + "rule", + "ip", + "hyper", + "postrouting", + "ip", + "saddr", + clone_pool, + "oifname", + uplink, + "masquerade" + ]), + Command::nft(argv![ + "add", "chain", "ip", "hyper", "forward", "{", "type", "filter", "hook", "forward", + "priority", "0", ";", "policy", "drop", ";", "}" + ]), + Command::nft(argv![ + "add", "rule", "ip", "hyper", "forward", "ip", "saddr", clone_pool, "oifname", uplink, + "accept" + ]), + Command::nft(argv![ + "add", + "rule", + "ip", + "hyper", + "forward", + "ip", + "daddr", + clone_pool, + "ct", + "state", + "established,related", + "accept" + ]), + Command::nft(argv![ + "add", + "rule", + "ip", + "hyper", + "forward", + "ip", + "saddr", + clone_pool, + "ip", + "daddr", + "169.254.169.254", + "drop" + ]), + ] +} diff --git a/native/suidhelper/src/tools/network/mod.rs b/native/suidhelper/src/tools/network/mod.rs index 2d9bc1ed..3377db8b 100644 --- a/native/suidhelper/src/tools/network/mod.rs +++ b/native/suidhelper/src/tools/network/mod.rs @@ -1,3 +1,57 @@ // SPDX-License-Identifier: AGPL-3.0-only //! `network`: per-VM egress networking (netns + veth + TAP + NAT). +//! +//! This module currently wires the CLI skeleton (`NetworkOp` and its arg +//! structs) and the pure argv builders in [`args`]. The `run` bodies are typed +//! no-ops; the real privileged execution (invoking `ip`/`nft` with the argv +//! [`args`] builds) lands with the rest of the prepare/teardown/host-init logic. pub mod addr; +pub mod args; + +use crate::tools::jailer::VmId; +use clap::{Args, Subcommand}; + +#[derive(Args)] +pub struct PrepareArgs { + /// Microvm id; becomes the netns name. + #[arg(long)] + vm_id: VmId, + /// Unprivileged uid the VM runs as; derives its clone-pool `/30` slot. + #[arg(long)] + uid: u32, +} + +#[derive(Args)] +pub struct TeardownArgs { + /// Microvm id; names the netns to remove. + #[arg(long)] + vm_id: VmId, + /// Unprivileged uid the VM ran as; derives its clone-pool `/30` slot. + #[arg(long)] + uid: u32, +} + +#[derive(Args)] +pub struct HostInitArgs {} + +#[derive(Subcommand)] +pub enum NetworkOp { + /// Create a VM's netns, veth pair, TAP device, and in-netns NAT. + Prepare(PrepareArgs), + /// Remove a VM's netns and any lingering host-side veth end. + Teardown(TeardownArgs), + /// One-time host setup: the `hyper` nftables table and forward policy. + HostInit(HostInitArgs), +} + +impl NetworkOp { + /// Route to the selected op. Bodies are stubs for now (Task 4 fills the + /// privileged execution); each already returns its own serialized `Value`. + pub fn run(self) -> Result { + match self { + NetworkOp::Prepare(_) => Ok(serde_json::json!({"result": "noop"})), + NetworkOp::Teardown(_) => Ok(serde_json::json!({"result": "noop"})), + NetworkOp::HostInit(_) => Ok(serde_json::json!({"result": "noop"})), + } + } +} diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs new file mode 100644 index 00000000..c684bf4e --- /dev/null +++ b/native/suidhelper/tests/tools/network_args.rs @@ -0,0 +1,56 @@ +use hyper_suidhelper::tools::jailer::VmId; +use hyper_suidhelper::tools::network::addr::Plan; +use hyper_suidhelper::tools::network::args::{self, Which}; +use std::net::Ipv4Addr; +use std::str::FromStr; + +fn plan0() -> Plan { + Plan::derive( + 900_000, + (900_000, 999_999), + Ipv4Addr::new(172, 31, 0, 0), + &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), + ) + .unwrap() +} + +#[test] +fn prepare_creates_netns_first_and_default_route_last() { + let cmds = args::prepare_commands(&plan0()); + let first = &cmds[0]; + assert!(matches!(first.bin, Which::Ip)); + assert_eq!( + first.argv, + vec!["netns", "add", "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"] + ); + // The default route (guest reachability) must come after the veth is up. + let route_idx = cmds + .iter() + .position(|c| c.argv.contains(&"default".to_string())) + .unwrap(); + let veth_up_idx = cmds + .iter() + .position(|c| c.argv == vec!["link", "set", "hv0", "up"]) + .unwrap(); + assert!(route_idx > veth_up_idx); +} + +#[test] +fn teardown_deletes_netns() { + let cmds = args::teardown_commands(&plan0()); + assert!(cmds + .iter() + .any(|c| c.argv == vec!["netns", "del", "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"])); +} + +#[test] +fn host_init_masquerades_pool_out_uplink() { + let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); + assert!(cmds.iter().any( + |c| c.argv.contains(&"masquerade".to_string()) && c.argv.contains(&"eth0".to_string()) + )); + // metadata IP is dropped + assert!(cmds + .iter() + .any(|c| c.argv.contains(&"169.254.169.254".to_string()))); +} From a3ac3a09f931ed0c66666eca1d0c224a6d1c9529 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 01:52:59 +0000 Subject: [PATCH 06/42] fix(suidhelper): drop metadata IP before egress accept + golden argv 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 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. --- native/suidhelper/src/tools/network/args.rs | 30 ++- native/suidhelper/tests/tools/network_args.rs | 243 ++++++++++++++++++ 2 files changed, 260 insertions(+), 13 deletions(-) diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index 583ad4a8..7f6a1d31 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -265,10 +265,10 @@ pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { "add", "chain", "ip", "hyper", "forward", "{", "type", "filter", "hook", "forward", "priority", "0", ";", "policy", "drop", ";", "}" ]), - Command::nft(argv![ - "add", "rule", "ip", "hyper", "forward", "ip", "saddr", clone_pool, "oifname", uplink, - "accept" - ]), + // Must precede the broad egress accept below: `accept` is a + // terminating verdict in nftables, so a rule reachable before this + // one would let a guest packet to the metadata IP exit via the + // uplink before this drop is ever evaluated. Command::nft(argv![ "add", "rule", @@ -276,11 +276,15 @@ pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { "hyper", "forward", "ip", - "daddr", + "saddr", clone_pool, - "ct", - "state", - "established,related", + "ip", + "daddr", + "169.254.169.254", + "drop" + ]), + Command::nft(argv![ + "add", "rule", "ip", "hyper", "forward", "ip", "saddr", clone_pool, "oifname", uplink, "accept" ]), Command::nft(argv![ @@ -290,12 +294,12 @@ pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { "hyper", "forward", "ip", - "saddr", - clone_pool, - "ip", "daddr", - "169.254.169.254", - "drop" + clone_pool, + "ct", + "state", + "established,related", + "accept" ]), ] } diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index c684bf4e..a6b7779d 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -54,3 +54,246 @@ fn host_init_masquerades_pool_out_uplink() { .iter() .any(|c| c.argv.contains(&"169.254.169.254".to_string()))); } + +fn cmd(bin: Which, argv: &[&str]) -> args::Command { + args::Command { + bin, + argv: argv.iter().map(|s| s.to_string()).collect(), + } +} + +/// Full-sequence golden test: pins every bin/argv, in order, so a wrong flag, +/// a wrong address, or a reordered/reversed SNAT/DNAT target fails here even +/// though it would slip past the substring checks above. +#[test] +fn prepare_commands_golden_sequence() { + let netns = "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; + let cmds = args::prepare_commands(&plan0()); + let expected = vec![ + cmd(Which::Ip, &["netns", "add", netns]), + cmd( + Which::Ip, + &["link", "add", "hv0", "type", "veth", "peer", "name", "hp0"], + ), + cmd(Which::Ip, &["link", "set", "hp0", "netns", netns]), + cmd(Which::Ip, &["addr", "add", "172.31.0.1/30", "dev", "hv0"]), + cmd(Which::Ip, &["link", "set", "hv0", "up"]), + cmd( + Which::Ip, + &["-n", netns, "addr", "add", "172.31.0.2/30", "dev", "hp0"], + ), + cmd(Which::Ip, &["-n", netns, "link", "set", "hp0", "up"]), + cmd( + Which::Ip, + &["-n", netns, "tuntap", "add", "tap0", "mode", "tap"], + ), + cmd( + Which::Ip, + &["-n", netns, "addr", "add", "172.30.0.1/30", "dev", "tap0"], + ), + cmd(Which::Ip, &["-n", netns, "link", "set", "tap0", "up"]), + cmd(Which::Ip, &["-n", netns, "link", "set", "lo", "up"]), + cmd( + Which::Ip, + &["-n", netns, "route", "add", "default", "via", "172.31.0.1"], + ), + cmd( + Which::Ip, + &["netns", "exec", netns, "nft", "add", "table", "ip", "nat"], + ), + cmd( + Which::Ip, + &[ + "netns", + "exec", + netns, + "nft", + "add", + "chain", + "ip", + "nat", + "post", + "{", + "type", + "nat", + "hook", + "postrouting", + "priority", + "100", + ";", + "}", + ], + ), + cmd( + Which::Ip, + &[ + "netns", + "exec", + netns, + "nft", + "add", + "rule", + "ip", + "nat", + "post", + "ip", + "saddr", + "172.30.0.2", + "oifname", + "hp0", + "snat", + "to", + "172.31.0.2", + ], + ), + cmd( + Which::Ip, + &[ + "netns", + "exec", + netns, + "nft", + "add", + "chain", + "ip", + "nat", + "pre", + "{", + "type", + "nat", + "hook", + "prerouting", + "priority", + "-100", + ";", + "}", + ], + ), + cmd( + Which::Ip, + &[ + "netns", + "exec", + netns, + "nft", + "add", + "rule", + "ip", + "nat", + "pre", + "ip", + "daddr", + "172.31.0.2", + "dnat", + "to", + "172.30.0.2", + ], + ), + ]; + assert_eq!(cmds, expected); +} + +/// Full-sequence golden test for the host-init nftables program: pins the +/// metadata-IP drop rule immediately after the forward chain is created and +/// before the broad egress accept, since `accept` is a terminating verdict in +/// nftables and a drop reachable only after it would never fire. +#[test] +fn host_init_commands_golden_sequence() { + let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); + let expected = vec![ + cmd(Which::Nft, &["add", "table", "ip", "hyper"]), + cmd( + Which::Nft, + &[ + "add", + "chain", + "ip", + "hyper", + "postrouting", + "{", + "type", + "nat", + "hook", + "postrouting", + "priority", + "100", + ";", + "}", + ], + ), + cmd( + Which::Nft, + &[ + "add", + "rule", + "ip", + "hyper", + "postrouting", + "ip", + "saddr", + "172.31.0.0/16", + "oifname", + "eth0", + "masquerade", + ], + ), + cmd( + Which::Nft, + &[ + "add", "chain", "ip", "hyper", "forward", "{", "type", "filter", "hook", "forward", + "priority", "0", ";", "policy", "drop", ";", "}", + ], + ), + cmd( + Which::Nft, + &[ + "add", + "rule", + "ip", + "hyper", + "forward", + "ip", + "saddr", + "172.31.0.0/16", + "ip", + "daddr", + "169.254.169.254", + "drop", + ], + ), + cmd( + Which::Nft, + &[ + "add", + "rule", + "ip", + "hyper", + "forward", + "ip", + "saddr", + "172.31.0.0/16", + "oifname", + "eth0", + "accept", + ], + ), + cmd( + Which::Nft, + &[ + "add", + "rule", + "ip", + "hyper", + "forward", + "ip", + "daddr", + "172.31.0.0/16", + "ct", + "state", + "established,related", + "accept", + ], + ), + ]; + assert_eq!(cmds, expected); +} From 7723a377795f15fbcdab2312a9ff1a1204b1512d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:01:18 +0000 Subject: [PATCH 07/42] feat(suidhelper): execute network prepare/teardown/host-init via ip+nft 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. --- native/suidhelper/src/tools/network/exec.rs | 52 +++++++ .../suidhelper/src/tools/network/host_init.rs | 92 +++++++++++++ native/suidhelper/src/tools/network/mod.rs | 59 ++++---- .../suidhelper/src/tools/network/prepare.rs | 129 ++++++++++++++++++ .../suidhelper/src/tools/network/teardown.rs | 68 +++++++++ native/suidhelper/tests/tools/network_args.rs | 26 ++++ 6 files changed, 395 insertions(+), 31 deletions(-) create mode 100644 native/suidhelper/src/tools/network/exec.rs create mode 100644 native/suidhelper/src/tools/network/host_init.rs create mode 100644 native/suidhelper/src/tools/network/prepare.rs create mode 100644 native/suidhelper/src/tools/network/teardown.rs diff --git a/native/suidhelper/src/tools/network/exec.rs b/native/suidhelper/src/tools/network/exec.rs new file mode 100644 index 00000000..722bf45a --- /dev/null +++ b/native/suidhelper/src/tools/network/exec.rs @@ -0,0 +1,52 @@ +//! Shared execution of an [`args::Command`] sequence: resolve each command's +//! binary via `resolve`, spawn it with a cleared environment, and stop at the +//! first non-zero exit. This is the whole privileged window for every network +//! op — `prepare`, `teardown`, and `host_init` differ only in which commands +//! they hand to [`run_all`]. +use super::args::{Command, Which}; +use std::path::Path; +use std::process::Command as Proc; +use thiserror::Error as ThisError; + +#[derive(Debug, ThisError)] +pub enum Error { + #[error("running {bin:?} {argv:?}: {source}")] + Spawn { + bin: Which, + argv: Vec, + #[source] + source: std::io::Error, + }, + #[error("{bin:?} {argv:?} failed: {stderr}")] + Failed { + bin: Which, + argv: Vec, + stderr: String, + }, +} + +pub(super) fn run_all<'a>( + commands: &[Command], + resolve: impl Fn(Which) -> &'a Path, +) -> Result<(), Error> { + for command in commands { + let output = Proc::new(resolve(command.bin)) + .args(&command.argv) + .env_clear() + .output() + .map_err(|source| Error::Spawn { + bin: command.bin, + argv: command.argv.clone(), + source, + })?; + + if !output.status.success() { + return Err(Error::Failed { + bin: command.bin, + argv: command.argv.clone(), + stderr: String::from_utf8_lossy(&output.stderr).trim().to_string(), + }); + } + } + Ok(()) +} diff --git a/native/suidhelper/src/tools/network/host_init.rs b/native/suidhelper/src/tools/network/host_init.rs new file mode 100644 index 00000000..c97136e8 --- /dev/null +++ b/native/suidhelper/src/tools/network/host_init.rs @@ -0,0 +1,92 @@ +//! `network host-init`: one-time host setup of the `hyper` nftables table, +//! its masquerade rule, and the forward-chain default-drop policy. +//! +//! Idempotency: before building the program, list the `hyper` table (`nft +//! list table ip hyper`). If it already exists and its listing contains the +//! masquerade rule, host-init has already run and nothing more is added. This +//! is coarser than a per-rule list-then-add guard, but the whole program is +//! only ever created here as one atomic unit (never hand-edited), so "the +//! masquerade rule is present" is equivalent to "the whole program already +//! ran" — a single check covers every rule below it. +use super::args; +use super::exec; +use super::NetworkOut; +use crate::config::Config; +use crate::tools::IsTool; +use crate::util::safe_bin; +use clap::Args; +use std::path::PathBuf; +use std::process::Command; +use thiserror::Error as ThisError; + +#[derive(Args)] +pub struct HostInitArgs {} + +#[derive(Debug, ThisError)] +pub enum Error { + #[error("VM networking is not configured ([network] absent from config)")] + NetworkingDisabled, + #[error(transparent)] + Bin(#[from] safe_bin::Error), + #[error(transparent)] + Exec(#[from] exec::Error), + #[error("listing the existing hyper nft table: {0}")] + List(#[source] std::io::Error), +} + +pub fn run(args: HostInitArgs) -> Result { + HostInit::new(args) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))? + .run() +} + +struct HostInit { + uplink: String, + clone_pool: String, + nft: PathBuf, +} + +impl HostInit { + fn new(_args: HostInitArgs) -> Result { + let config = Config::get(); + let network = config.network().ok_or(Error::NetworkingDisabled)?; + Ok(Self { + uplink: network.uplink.clone(), + clone_pool: network.clone_pool.clone(), + nft: config.nft()?.into(), + }) + } + + /// `true` iff the `hyper` table already carries the masquerade rule, i.e. + /// host-init has already run. + fn already_done(&self) -> Result { + let output = Command::new(&self.nft) + .args(["list", "table", "ip", "hyper"]) + .env_clear() + .output() + .map_err(Error::List)?; + Ok(output.status.success() + && String::from_utf8_lossy(&output.stdout).contains("masquerade")) + } +} + +impl IsTool for HostInit { + type Args = HostInitArgs; + type Output = NetworkOut; + type RunT = Result<(), Error>; + + fn run_privileged(&self) -> Self::RunT { + if self.already_done()? { + return Ok(()); + } + let commands = args::host_init_commands(&self.uplink, &self.clone_pool); + // host_init_commands only ever issues `nft` commands (see args.rs). + exec::run_all(&commands, |_which| self.nft.as_path())?; + Ok(()) + } + + fn parse(&self, res: Self::RunT) -> Result> { + res?; + Ok(NetworkOut::HostReady) + } +} diff --git a/native/suidhelper/src/tools/network/mod.rs b/native/suidhelper/src/tools/network/mod.rs index 3377db8b..06189014 100644 --- a/native/suidhelper/src/tools/network/mod.rs +++ b/native/suidhelper/src/tools/network/mod.rs @@ -1,39 +1,36 @@ // SPDX-License-Identifier: AGPL-3.0-only //! `network`: per-VM egress networking (netns + veth + TAP + NAT). //! -//! This module currently wires the CLI skeleton (`NetworkOp` and its arg -//! structs) and the pure argv builders in [`args`]. The `run` bodies are typed -//! no-ops; the real privileged execution (invoking `ip`/`nft` with the argv -//! [`args`] builds) lands with the rest of the prepare/teardown/host-init logic. +//! Each op ([`prepare`], [`teardown`], [`host_init`]) derives its `ip`/`nft` +//! argv (built in [`args`]) from the validated uid and [`crate::config::Config::network`], +//! then runs it through [`exec::run_all`], which stops at the first non-zero +//! exit. Every refusal — `uid` outside the configured range, a malformed +//! `clone_pool` CIDR, or `[network]` absent from config — happens while +//! resolving the op (see `prepare::resolve`), strictly before `run_privileged` +//! is ever reached, so no privileged command runs on invalid input. pub mod addr; pub mod args; +mod exec; +pub mod host_init; +pub mod prepare; +pub mod teardown; -use crate::tools::jailer::VmId; -use clap::{Args, Subcommand}; +use clap::Subcommand; +use serde::Serialize; -#[derive(Args)] -pub struct PrepareArgs { - /// Microvm id; becomes the netns name. - #[arg(long)] - vm_id: VmId, - /// Unprivileged uid the VM runs as; derives its clone-pool `/30` slot. - #[arg(long)] - uid: u32, -} +pub use host_init::HostInitArgs; +pub use prepare::PrepareArgs; +pub use teardown::TeardownArgs; -#[derive(Args)] -pub struct TeardownArgs { - /// Microvm id; names the netns to remove. - #[arg(long)] - vm_id: VmId, - /// Unprivileged uid the VM ran as; derives its clone-pool `/30` slot. - #[arg(long)] - uid: u32, +/// The result shape shared by all three networking ops, tagged by which ran. +#[derive(Serialize)] +#[serde(tag = "result", rename_all = "snake_case")] +pub enum NetworkOut { + Prepared, + ToreDown, + HostReady, } -#[derive(Args)] -pub struct HostInitArgs {} - #[derive(Subcommand)] pub enum NetworkOp { /// Create a VM's netns, veth pair, TAP device, and in-netns NAT. @@ -45,13 +42,13 @@ pub enum NetworkOp { } impl NetworkOp { - /// Route to the selected op. Bodies are stubs for now (Task 4 fills the - /// privileged execution); each already returns its own serialized `Value`. + /// Route to the selected op; each runs in its own privileged scope and + /// returns its own serialized `Value`. pub fn run(self) -> Result { match self { - NetworkOp::Prepare(_) => Ok(serde_json::json!({"result": "noop"})), - NetworkOp::Teardown(_) => Ok(serde_json::json!({"result": "noop"})), - NetworkOp::HostInit(_) => Ok(serde_json::json!({"result": "noop"})), + NetworkOp::Prepare(args) => prepare::run(args), + NetworkOp::Teardown(args) => teardown::run(args), + NetworkOp::HostInit(args) => host_init::run(args), } } } diff --git a/native/suidhelper/src/tools/network/prepare.rs b/native/suidhelper/src/tools/network/prepare.rs new file mode 100644 index 00000000..7cd48766 --- /dev/null +++ b/native/suidhelper/src/tools/network/prepare.rs @@ -0,0 +1,129 @@ +//! `network prepare`: create a VM's netns, host/ns veth pair, TAP device, and +//! in-netns NAT so the guest's inner address can reach the host's uplink. +use super::addr::{AddrError, Plan}; +use super::args; +use super::exec; +use super::NetworkOut; +use crate::config::Config; +use crate::tools::jailer::{self, VmId}; +use crate::tools::IsTool; +use crate::util::safe_bin; +use clap::Args; +use std::net::Ipv4Addr; +use std::path::PathBuf; +use thiserror::Error as ThisError; + +#[derive(Args)] +pub struct PrepareArgs { + /// Microvm id; becomes the netns name. + #[arg(long)] + vm_id: VmId, + /// Unprivileged uid the VM runs as; derives its clone-pool `/30` slot. + #[arg(long)] + uid: u32, +} + +#[derive(Debug, ThisError)] +pub enum Error { + #[error("VM networking is not configured ([network] absent from config)")] + NetworkingDisabled, + #[error("clone_pool {0:?} is not a valid IPv4 CIDR")] + InvalidClonePool(String), + #[error(transparent)] + Uid(#[from] jailer::Error), + #[error(transparent)] + Addr(#[from] AddrError), + #[error(transparent)] + Bin(#[from] safe_bin::Error), + #[error(transparent)] + Exec(#[from] exec::Error), +} + +/// Derive the [`Plan`] for `uid`, refusing before any command is ever run. +/// `uid` must satisfy [`jailer::validate_id_number`] against `range` — the +/// same check the jailer itself applies to `--uid`, so a uid the jailer would +/// refuse is refused here too — and `clone_pool_str` must be a parseable IPv4 +/// CIDR. +pub fn plan_from( + uid: u32, + range: (u32, u32), + clone_pool_str: &str, + vm_id: &VmId, +) -> Result { + jailer::validate_id_number(uid, range)?; + let clone_pool = parse_cidr_base(clone_pool_str)?; + Ok(Plan::derive(uid, range, clone_pool, vm_id)?) +} + +/// Parse an IPv4 CIDR's base address, e.g. `"172.31.0.0/16"` -> `172.31.0.0`. +/// The prefix length is validated (`0..=32`) but otherwise unused: `Plan::derive` +/// only needs the pool's base address, not its width. +fn parse_cidr_base(s: &str) -> Result { + let reject = || Error::InvalidClonePool(s.to_string()); + let mut parts = s.splitn(2, '/'); + let addr: Ipv4Addr = parts + .next() + .ok_or_else(reject)? + .parse() + .map_err(|_| reject())?; + let prefix: u8 = parts + .next() + .ok_or_else(reject)? + .parse() + .map_err(|_| reject())?; + if prefix > 32 { + return Err(reject()); + } + Ok(addr) +} + +/// Resolve the [`Plan`] and the config's `ip`/`nft` binaries for a `prepare` or +/// `teardown` op. Shared by both since they derive the identical `Plan` from +/// the same `uid` + config; only the argv sequence they run differs. Every +/// refusal here (`[network]` absent, uid out of range, malformed CIDR, a +/// misconfigured binary) happens before any privileged command is spawned. +pub(super) fn resolve(uid: u32, vm_id: &VmId) -> Result<(Plan, PathBuf, PathBuf), Error> { + let config = Config::get(); + let network = config.network().ok_or(Error::NetworkingDisabled)?; + let plan = plan_from(uid, config.uid_gid_range(), &network.clone_pool, vm_id)?; + Ok((plan, config.ip()?.into(), config.nft()?.into())) +} + +pub fn run(args: PrepareArgs) -> Result { + Prepare::new(args) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))? + .run() +} + +struct Prepare { + plan: Plan, + ip: PathBuf, + nft: PathBuf, +} + +impl Prepare { + fn new(args: PrepareArgs) -> Result { + let (plan, ip, nft) = resolve(args.uid, &args.vm_id)?; + Ok(Self { plan, ip, nft }) + } +} + +impl IsTool for Prepare { + type Args = PrepareArgs; + type Output = NetworkOut; + type RunT = Result<(), Error>; + + fn run_privileged(&self) -> Self::RunT { + let commands = args::prepare_commands(&self.plan); + exec::run_all(&commands, |which| match which { + args::Which::Ip => self.ip.as_path(), + args::Which::Nft => self.nft.as_path(), + })?; + Ok(()) + } + + fn parse(&self, res: Self::RunT) -> Result> { + res?; + Ok(NetworkOut::Prepared) + } +} diff --git a/native/suidhelper/src/tools/network/teardown.rs b/native/suidhelper/src/tools/network/teardown.rs new file mode 100644 index 00000000..877500cb --- /dev/null +++ b/native/suidhelper/src/tools/network/teardown.rs @@ -0,0 +1,68 @@ +//! `network teardown`: remove a VM's netns, reclaiming its veth peer, TAP +//! device, and in-netns nftables state, plus any lingering host-side veth end. +use super::args; +use super::exec; +use super::prepare; +use super::NetworkOut; +use crate::tools::jailer::VmId; +use crate::tools::IsTool; +use clap::Args; +use std::path::PathBuf; +use thiserror::Error as ThisError; + +#[derive(Args)] +pub struct TeardownArgs { + /// Microvm id; names the netns to remove. + #[arg(long)] + vm_id: VmId, + /// Unprivileged uid the VM ran as; derives its clone-pool `/30` slot. + #[arg(long)] + uid: u32, +} + +#[derive(Debug, ThisError)] +pub enum Error { + #[error(transparent)] + Plan(#[from] prepare::Error), + #[error(transparent)] + Exec(#[from] exec::Error), +} + +pub fn run(args: TeardownArgs) -> Result { + Teardown::new(args) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))? + .run() +} + +struct Teardown { + plan: super::addr::Plan, + ip: PathBuf, + nft: PathBuf, +} + +impl Teardown { + fn new(args: TeardownArgs) -> Result { + let (plan, ip, nft) = prepare::resolve(args.uid, &args.vm_id)?; + Ok(Self { plan, ip, nft }) + } +} + +impl IsTool for Teardown { + type Args = TeardownArgs; + type Output = NetworkOut; + type RunT = Result<(), Error>; + + fn run_privileged(&self) -> Self::RunT { + let commands = args::teardown_commands(&self.plan); + exec::run_all(&commands, |which| match which { + args::Which::Ip => self.ip.as_path(), + args::Which::Nft => self.nft.as_path(), + })?; + Ok(()) + } + + fn parse(&self, res: Self::RunT) -> Result> { + res?; + Ok(NetworkOut::ToreDown) + } +} diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index a6b7779d..dcf97a1c 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -1,6 +1,7 @@ use hyper_suidhelper::tools::jailer::VmId; use hyper_suidhelper::tools::network::addr::Plan; use hyper_suidhelper::tools::network::args::{self, Which}; +use hyper_suidhelper::tools::network::prepare; use std::net::Ipv4Addr; use std::str::FromStr; @@ -297,3 +298,28 @@ fn host_init_commands_golden_sequence() { ]; assert_eq!(cmds, expected); } + +#[test] +fn prepare_refuses_uid_outside_range() { + // Derivation from an out-of-range uid must error before any command runs. + let err = prepare::plan_from( + 42, + (900_000, 999_999), + "172.31.0.0/16", + &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), + ); + assert!(err.is_err()); +} + +#[test] +fn prepare_refuses_malformed_clone_pool_cidr() { + // A malformed CIDR must error before any command runs, distinctly from a + // uid-range refusal. + let err = prepare::plan_from( + 900_000, + (900_000, 999_999), + "not-a-cidr", + &VmId::from_str("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa").unwrap(), + ); + assert!(err.is_err()); +} From b9b81c23a571fefaf854023e4b8a4c834bf1e108 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:11:18 +0000 Subject: [PATCH 08/42] fix(suidhelper): host-init reconciles to desired state + test networking-disabled refusal MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/suidhelper/src/tools/network/args.rs | 26 +++++++++- native/suidhelper/src/tools/network/exec.rs | 9 ++-- .../suidhelper/src/tools/network/host_init.rs | 41 +++++----------- native/suidhelper/src/tools/network/mod.rs | 17 +++++++ .../suidhelper/src/tools/network/prepare.rs | 6 +-- native/suidhelper/tests/tools/network_args.rs | 48 +++++++++++++++++-- 6 files changed, 107 insertions(+), 40 deletions(-) diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index 7f6a1d31..8b03bc4f 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -29,11 +29,15 @@ pub enum Which { /// One step of a prepare/teardown/host-init sequence: the binary to run and its /// exact argv (argv[0] is the binary itself, named by [`Which`], not repeated -/// here). +/// here). `allow_failure` marks a step whose non-zero exit must not stop the +/// sequence — used only by the leading `nft delete table ip hyper` in +/// [`host_init_commands`], since that command's whole purpose is to clear +/// state that may not exist yet. #[derive(Debug, Clone, PartialEq, Eq)] pub struct Command { pub bin: Which, pub argv: Vec, + pub allow_failure: bool, } macro_rules! argv { @@ -47,6 +51,7 @@ impl Command { Self { bin: Which::Ip, argv, + allow_failure: false, } } @@ -61,6 +66,16 @@ impl Command { Self { bin: Which::Nft, argv, + allow_failure: false, + } + } + + /// An `nft` step whose non-zero exit is tolerated — see the `allow_failure` + /// doc on [`Command`]. + fn nft_allow_failure(argv: Vec) -> Self { + Self { + allow_failure: true, + ..Self::nft(argv) } } @@ -229,8 +244,17 @@ pub fn teardown_commands(plan: &Plan) -> Vec { /// pool out `uplink`, and a forward-chain default-drop policy that only admits /// the pool's own traffic — explicitly dropping anything addressed to the /// cloud metadata IP (`169.254.169.254`), so no VM can ever reach it. +/// +/// Reconciles to desired state rather than diffing: the first command +/// unconditionally deletes the `hyper` table (tolerating "no such file" when +/// it doesn't exist yet), and every command after it rebuilds the table from +/// scratch. This makes host-init idempotent by construction — the table is +/// always either absent or complete, never partially applied — so a +/// crash/interruption mid-sequence on one run can never strand a later run +/// with, say, NAT but no forward-chain drop policy. pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { vec![ + Command::nft_allow_failure(argv!["delete", "table", "ip", "hyper"]), Command::nft(argv!["add", "table", "ip", "hyper"]), Command::nft(argv![ "add", diff --git a/native/suidhelper/src/tools/network/exec.rs b/native/suidhelper/src/tools/network/exec.rs index 722bf45a..988f3f10 100644 --- a/native/suidhelper/src/tools/network/exec.rs +++ b/native/suidhelper/src/tools/network/exec.rs @@ -1,8 +1,9 @@ //! Shared execution of an [`args::Command`] sequence: resolve each command's //! binary via `resolve`, spawn it with a cleared environment, and stop at the -//! first non-zero exit. This is the whole privileged window for every network -//! op — `prepare`, `teardown`, and `host_init` differ only in which commands -//! they hand to [`run_all`]. +//! first non-zero exit — unless the command is marked `allow_failure`, in +//! which case its exit status is ignored and the sequence continues. This is +//! the whole privileged window for every network op — `prepare`, `teardown`, +//! and `host_init` differ only in which commands they hand to [`run_all`]. use super::args::{Command, Which}; use std::path::Path; use std::process::Command as Proc; @@ -40,7 +41,7 @@ pub(super) fn run_all<'a>( source, })?; - if !output.status.success() { + if !output.status.success() && !command.allow_failure { return Err(Error::Failed { bin: command.bin, argv: command.argv.clone(), diff --git a/native/suidhelper/src/tools/network/host_init.rs b/native/suidhelper/src/tools/network/host_init.rs index c97136e8..21ddd42e 100644 --- a/native/suidhelper/src/tools/network/host_init.rs +++ b/native/suidhelper/src/tools/network/host_init.rs @@ -1,13 +1,16 @@ //! `network host-init`: one-time host setup of the `hyper` nftables table, //! its masquerade rule, and the forward-chain default-drop policy. //! -//! Idempotency: before building the program, list the `hyper` table (`nft -//! list table ip hyper`). If it already exists and its listing contains the -//! masquerade rule, host-init has already run and nothing more is added. This -//! is coarser than a per-rule list-then-add guard, but the whole program is -//! only ever created here as one atomic unit (never hand-edited), so "the -//! masquerade rule is present" is equivalent to "the whole program already -//! ran" — a single check covers every rule below it. +//! Idempotency: host-init reconciles to desired state rather than diffing. +//! [`args::host_init_commands`] always leads with a (failure-tolerant) delete +//! of the `hyper` table, then rebuilds it from scratch. A presence check +//! (e.g. "does `nft list table ip hyper` already mention masquerade?") looks +//! idempotent but isn't: masquerade is added partway through the sequence, so +//! a crash between that step and the later forward-chain drop policy would +//! leave the table permanently short a security rule — every subsequent +//! host-init would see masquerade present and skip, never adding the missing +//! policy. Delete-then-recreate has no such window: the table is always +//! either fully absent or fully complete. use super::args; use super::exec; use super::NetworkOut; @@ -16,7 +19,6 @@ use crate::tools::IsTool; use crate::util::safe_bin; use clap::Args; use std::path::PathBuf; -use std::process::Command; use thiserror::Error as ThisError; #[derive(Args)] @@ -24,14 +26,12 @@ pub struct HostInitArgs {} #[derive(Debug, ThisError)] pub enum Error { - #[error("VM networking is not configured ([network] absent from config)")] - NetworkingDisabled, + #[error(transparent)] + NetworkingDisabled(#[from] super::NetworkingDisabled), #[error(transparent)] Bin(#[from] safe_bin::Error), #[error(transparent)] Exec(#[from] exec::Error), - #[error("listing the existing hyper nft table: {0}")] - List(#[source] std::io::Error), } pub fn run(args: HostInitArgs) -> Result { @@ -49,25 +49,13 @@ struct HostInit { impl HostInit { fn new(_args: HostInitArgs) -> Result { let config = Config::get(); - let network = config.network().ok_or(Error::NetworkingDisabled)?; + let network = super::resolve_network(config.network())?; Ok(Self { uplink: network.uplink.clone(), clone_pool: network.clone_pool.clone(), nft: config.nft()?.into(), }) } - - /// `true` iff the `hyper` table already carries the masquerade rule, i.e. - /// host-init has already run. - fn already_done(&self) -> Result { - let output = Command::new(&self.nft) - .args(["list", "table", "ip", "hyper"]) - .env_clear() - .output() - .map_err(Error::List)?; - Ok(output.status.success() - && String::from_utf8_lossy(&output.stdout).contains("masquerade")) - } } impl IsTool for HostInit { @@ -76,9 +64,6 @@ impl IsTool for HostInit { type RunT = Result<(), Error>; fn run_privileged(&self) -> Self::RunT { - if self.already_done()? { - return Ok(()); - } let commands = args::host_init_commands(&self.uplink, &self.clone_pool); // host_init_commands only ever issues `nft` commands (see args.rs). exec::run_all(&commands, |_which| self.nft.as_path())?; diff --git a/native/suidhelper/src/tools/network/mod.rs b/native/suidhelper/src/tools/network/mod.rs index 06189014..5e6cfeaa 100644 --- a/native/suidhelper/src/tools/network/mod.rs +++ b/native/suidhelper/src/tools/network/mod.rs @@ -15,13 +15,30 @@ pub mod host_init; pub mod prepare; pub mod teardown; +use crate::config::Network; use clap::Subcommand; use serde::Serialize; +use thiserror::Error as ThisError; pub use host_init::HostInitArgs; pub use prepare::PrepareArgs; pub use teardown::TeardownArgs; +/// `[network]` is absent from config, i.e. VM networking is disabled. +#[derive(Debug, ThisError, PartialEq, Eq)] +#[error("VM networking is not configured ([network] absent from config)")] +pub struct NetworkingDisabled; + +/// Refuse before any privileged command is ever spawned when `[network]` is +/// absent from config. Pure — takes the already-loaded `Option<&Network>` +/// rather than reaching into [`crate::config::Config`] itself — so this exact +/// refusal path is exercisable from tests without root or a live config file, +/// and `prepare`/`teardown`/`host_init` all resolve through this one function +/// rather than duplicating the check. +pub fn resolve_network(network: Option<&Network>) -> Result<&Network, NetworkingDisabled> { + network.ok_or(NetworkingDisabled) +} + /// The result shape shared by all three networking ops, tagged by which ran. #[derive(Serialize)] #[serde(tag = "result", rename_all = "snake_case")] diff --git a/native/suidhelper/src/tools/network/prepare.rs b/native/suidhelper/src/tools/network/prepare.rs index 7cd48766..948a269b 100644 --- a/native/suidhelper/src/tools/network/prepare.rs +++ b/native/suidhelper/src/tools/network/prepare.rs @@ -25,8 +25,8 @@ pub struct PrepareArgs { #[derive(Debug, ThisError)] pub enum Error { - #[error("VM networking is not configured ([network] absent from config)")] - NetworkingDisabled, + #[error(transparent)] + NetworkingDisabled(#[from] super::NetworkingDisabled), #[error("clone_pool {0:?} is not a valid IPv4 CIDR")] InvalidClonePool(String), #[error(transparent)] @@ -84,7 +84,7 @@ fn parse_cidr_base(s: &str) -> Result { /// misconfigured binary) happens before any privileged command is spawned. pub(super) fn resolve(uid: u32, vm_id: &VmId) -> Result<(Plan, PathBuf, PathBuf), Error> { let config = Config::get(); - let network = config.network().ok_or(Error::NetworkingDisabled)?; + let network = super::resolve_network(config.network())?; let plan = plan_from(uid, config.uid_gid_range(), &network.clone_pool, vm_id)?; Ok((plan, config.ip()?.into(), config.nft()?.into())) } diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index dcf97a1c..13cc8597 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -1,7 +1,8 @@ +use hyper_suidhelper::config::Network; use hyper_suidhelper::tools::jailer::VmId; use hyper_suidhelper::tools::network::addr::Plan; use hyper_suidhelper::tools::network::args::{self, Which}; -use hyper_suidhelper::tools::network::prepare; +use hyper_suidhelper::tools::network::{prepare, resolve_network, NetworkingDisabled}; use std::net::Ipv4Addr; use std::str::FromStr; @@ -60,6 +61,14 @@ fn cmd(bin: Which, argv: &[&str]) -> args::Command { args::Command { bin, argv: argv.iter().map(|s| s.to_string()).collect(), + allow_failure: false, + } +} + +fn cmd_allow_failure(bin: Which, argv: &[&str]) -> args::Command { + args::Command { + allow_failure: true, + ..cmd(bin, argv) } } @@ -195,13 +204,17 @@ fn prepare_commands_golden_sequence() { } /// Full-sequence golden test for the host-init nftables program: pins the -/// metadata-IP drop rule immediately after the forward chain is created and -/// before the broad egress accept, since `accept` is a terminating verdict in -/// nftables and a drop reachable only after it would never fire. +/// leading, failure-tolerant `delete table` (the reconcile-to-desired-state +/// step — see `host_init.rs`'s module doc), the metadata-IP drop rule +/// immediately after the forward chain is created and before the broad +/// egress accept (since `accept` is a terminating verdict in nftables and a +/// drop reachable only after it would never fire), and that every command +/// after the delete is *not* failure-tolerant. #[test] fn host_init_commands_golden_sequence() { let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); let expected = vec![ + cmd_allow_failure(Which::Nft, &["delete", "table", "ip", "hyper"]), cmd(Which::Nft, &["add", "table", "ip", "hyper"]), cmd( Which::Nft, @@ -302,6 +315,11 @@ fn host_init_commands_golden_sequence() { #[test] fn prepare_refuses_uid_outside_range() { // Derivation from an out-of-range uid must error before any command runs. + // Proving `Err` here is sufficient, not just necessary, to prove no + // privileged command spawns: `plan_from` only ever returns a `Plan`, and + // every op derives its commands from that returned `Plan` in a separate + // step downstream, one that is structurally unreachable when derivation + // itself fails. let err = prepare::plan_from( 42, (900_000, 999_999), @@ -323,3 +341,25 @@ fn prepare_refuses_malformed_clone_pool_cidr() { ); assert!(err.is_err()); } + +#[test] +fn resolve_network_refuses_when_networking_disabled() { + // `[network]` absent from config is the security-critical refusal: every + // op (`prepare`, `teardown`, `host_init`) resolves through this same pure + // function before spawning anything privileged, so this exercises the + // real refusal path, not a copy of it — no root and no config file + // needed, since it takes the already-resolved `Option<&Network>` rather + // than reaching into `Config` itself. + assert!(matches!(resolve_network(None), Err(NetworkingDisabled))); +} + +#[test] +fn resolve_network_returns_the_configured_network_when_present() { + let network = Network { + uplink: "eth0".to_string(), + clone_pool: "172.31.0.0/16".to_string(), + }; + let resolved = resolve_network(Some(&network)).unwrap(); + assert_eq!(resolved.uplink, "eth0"); + assert_eq!(resolved.clone_pool, "172.31.0.0/16"); +} From 7ef7e18ca7554eb1a94d91fda1f3bf5dead1c87c Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:15:57 +0000 Subject: [PATCH 09/42] feat(net): Hyper.SuidHelper.Network wrapper for the network helper op --- lib/hyper/suid_helper/network.ex | 40 +++++++++++++++++++++++++ test/hyper/suid_helper/network_test.exs | 14 +++++++++ 2 files changed, 54 insertions(+) create mode 100644 lib/hyper/suid_helper/network.ex create mode 100644 test/hyper/suid_helper/network_test.exs diff --git a/lib/hyper/suid_helper/network.ex b/lib/hyper/suid_helper/network.ex new file mode 100644 index 00000000..df45de8c --- /dev/null +++ b/lib/hyper/suid_helper/network.ex @@ -0,0 +1,40 @@ +defmodule Hyper.SuidHelper.Network do + @moduledoc """ + Privileged per-VM egress networking via the setuid helper's `network` + subcommands. The helper derives every address, interface name, and netns path + from `vm_id` + the validated `uid` + the root-owned `[network]` config — the + node passes only those two untrusted values. + """ + use OpenTelemetryDecorator + alias Hyper.SuidHelper + + @type err :: SuidHelper.err() + + @doc false + @spec argv(:prepare | :teardown, Hyper.Vm.Id.t(), non_neg_integer()) :: [String.t()] + def argv(op, vm_id, uid), + do: ["network", Atom.to_string(op), "--vm-id", vm_id, "--uid", to_string(uid)] + + @doc "Create `vm_id`'s netns, veth pair, TAP, and NAT rules. Idempotent-safe on relaunch." + @spec prepare(Hyper.Vm.Id.t(), non_neg_integer()) :: :ok | {:error, err()} + @decorate with_span("Hyper.SuidHelper.Network.prepare", include: [:vm_id, :uid]) + def prepare(vm_id, uid), do: run(argv(:prepare, vm_id, uid)) + + @doc "Remove `vm_id`'s netns and host veth. Idempotent (a never-prepared VM is a no-op)." + @spec teardown(Hyper.Vm.Id.t(), non_neg_integer()) :: :ok | {:error, err()} + @decorate with_span("Hyper.SuidHelper.Network.teardown", include: [:vm_id, :uid]) + def teardown(vm_id, uid), do: run(argv(:teardown, vm_id, uid)) + + @doc "Ensure the host-wide `hyper` nft table (masquerade + forward policy) exists. Idempotent." + @spec host_init() :: :ok | {:error, err()} + @decorate with_span("Hyper.SuidHelper.Network.host_init", include: []) + def host_init, do: run(["network", "host-init"]) + + @spec run([String.t()]) :: :ok | {:error, err()} + defp run(args) do + case SuidHelper.exec(args) do + {:ok, _} -> :ok + {:error, _} = err -> err + end + end +end diff --git a/test/hyper/suid_helper/network_test.exs b/test/hyper/suid_helper/network_test.exs new file mode 100644 index 00000000..7e16bd4a --- /dev/null +++ b/test/hyper/suid_helper/network_test.exs @@ -0,0 +1,14 @@ +defmodule Hyper.SuidHelper.NetworkTest do + use ExUnit.Case, async: true + alias Hyper.SuidHelper.Network + + test "prepare/2 builds the network prepare argv" do + assert Network.argv(:prepare, "vabc", 900_100) == + ["network", "prepare", "--vm-id", "vabc", "--uid", "900100"] + end + + test "teardown/2 builds the network teardown argv" do + assert Network.argv(:teardown, "vabc", 900_100) == + ["network", "teardown", "--vm-id", "vabc", "--uid", "900100"] + end +end From 5c247ad36173655b961d1e265f3739954f20910d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:19:50 +0000 Subject: [PATCH 10/42] feat(net): inner-world MAC + ip= cmdline + NIC spec --- lib/hyper/node/fire_vmm/net.ex | 45 +++++++++++++++++++++++++++ test/hyper/node/fire_vmm/net_test.exs | 29 +++++++++++++++++ 2 files changed, 74 insertions(+) create mode 100644 lib/hyper/node/fire_vmm/net.ex create mode 100644 test/hyper/node/fire_vmm/net_test.exs diff --git a/lib/hyper/node/fire_vmm/net.ex b/lib/hyper/node/fire_vmm/net.ex new file mode 100644 index 00000000..456237f0 --- /dev/null +++ b/lib/hyper/node/fire_vmm/net.ex @@ -0,0 +1,45 @@ +defmodule Hyper.Node.FireVMM.Net do + @moduledoc """ + The per-VM *inner-world* networking contract. Every VM's netns is identical — + `tap0` at 172.30.0.1/30, guest at 172.30.0.2/30 — so a snapshot clone restores + a correct network with zero renumbering; host-side uniqueness is the helper's + job (see `Hyper.SuidHelper.Network`). This module owns the three values that + must agree between the guest kernel cmdline, the Firecracker NIC config, and + the helper: the guest MAC, the `ip=` autoconfig string, and the NIC spec. + """ + alias Hyper.Firecracker.Api.NetworkInterface + + @guest_ip "172.30.0.2" + @tap_ip "172.30.0.1" + @netmask "255.255.255.252" + @iface "eth0" + @tap "tap0" + + @doc "Kernel `ip=` autoconfig fragment appended to every networked VM's cmdline." + @spec ip_cmdline() :: String.t() + def ip_cmdline, do: "ip=#{@guest_ip}::#{@tap_ip}:#{@netmask}::#{@iface}:off" + + @doc """ + Stable locally-administered unicast MAC for `vm_id`. Derived from a SHA-256 of + the id so it is deterministic on any node (aids debugging); duplicate MACs are + harmless since no two guests ever share an L2 segment (each is alone in its netns). + """ + @spec guest_mac(Hyper.Vm.Id.t()) :: String.t() + def guest_mac(vm_id) do + <<_::8, b1, b2, b3, b4, _::binary>> = :crypto.hash(:sha256, vm_id) + + [0x02, b1, b2, b3, b4] + |> Enum.map_join(":", &(&1 |> Integer.to_string(16) |> String.pad_leading(2, "0"))) + |> String.downcase() + end + + @doc "The Firecracker NIC spec for `vm_id` (guest `eth0` bound to in-netns `tap0`)." + @spec interface(Hyper.Vm.Id.t()) :: NetworkInterface.t() + def interface(vm_id) do + %NetworkInterface{iface_id: @iface, host_dev_name: @tap, guest_mac: guest_mac(vm_id)} + end + + @doc false + @spec guest_iface() :: String.t() + def guest_iface, do: @iface +end diff --git a/test/hyper/node/fire_vmm/net_test.exs b/test/hyper/node/fire_vmm/net_test.exs new file mode 100644 index 00000000..a48e97de --- /dev/null +++ b/test/hyper/node/fire_vmm/net_test.exs @@ -0,0 +1,29 @@ +defmodule Hyper.Node.FireVMM.NetTest do + use ExUnit.Case, async: true + alias Hyper.Node.FireVMM.Net + + test "guest_mac is a stable locally-administered unicast MAC" do + mac = Net.guest_mac("vabcdef") + assert mac == Net.guest_mac("vabcdef"), "must be deterministic in vm_id" + [first | _] = String.split(mac, ":") + <> = Base.decode16!(first, case: :mixed) + # locally administered (bit 1 set), unicast (bit 0 clear) + assert Bitwise.band(byte, 0x02) == 0x02 + assert Bitwise.band(byte, 0x01) == 0x00 + end + + test "distinct vm_ids get distinct MACs" do + refute Net.guest_mac("vaaa") == Net.guest_mac("vbbb") + end + + test "ip_cmdline pins the inner-world contract" do + assert Net.ip_cmdline() == "ip=172.30.0.2::172.30.0.1:255.255.255.252::eth0:off" + end + + test "interface targets tap0/eth0 with the derived MAC" do + nic = Net.interface("vabcdef") + assert nic.iface_id == "eth0" + assert nic.host_dev_name == "tap0" + assert nic.guest_mac == Net.guest_mac("vabcdef") + end +end From 001d7c458f4639ba2b95e39461fb5a43cd86231b Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:24:07 +0000 Subject: [PATCH 11/42] feat(net): populate NIC + ip= cmdline in BootSpec when networking enabled 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. --- lib/hyper/node/fire_vmm/boot_spec.ex | 17 +++++++++++++++-- lib/hyper/node/fire_vmm/state.ex | 3 ++- lib/hyper/vm.ex | 6 ++++-- test/hyper/node/fire_vmm/boot_spec_test.exs | 13 +++++++++++++ 4 files changed, 34 insertions(+), 5 deletions(-) diff --git a/lib/hyper/node/fire_vmm/boot_spec.ex b/lib/hyper/node/fire_vmm/boot_spec.ex index 700780f0..66805431 100644 --- a/lib/hyper/node/fire_vmm/boot_spec.ex +++ b/lib/hyper/node/fire_vmm/boot_spec.ex @@ -32,11 +32,23 @@ defmodule Hyper.Node.FireVMM.BootSpec do @spec resolve(Hyper.Vm.source(), Instance.t()) :: Cold.t() def resolve(source, type) when is_map(source) do + base_args = Map.get(source, :boot_args, @default_boot_args) + + {nics, boot_args} = + if Hyper.Cfg.Network.enabled?() do + vm_id = Map.fetch!(source, :vm_id) + + {[Hyper.Node.FireVMM.Net.interface(vm_id)], + base_args <> " " <> Hyper.Node.FireVMM.Net.ip_cmdline()} + else + {[], base_args} + end + %Cold{ machine_config: Instance.Spec.machine_config(Instance.spec(type)), boot_source: %BootSource{ kernel_image_path: Map.fetch!(source, :kernel_image_path), - boot_args: Map.get(source, :boot_args, @default_boot_args) + boot_args: boot_args }, drives: [ %Drive{ @@ -45,7 +57,8 @@ defmodule Hyper.Node.FireVMM.BootSpec do is_read_only: false, path_on_host: Map.fetch!(source, :root_drive_path) } - ] + ], + network_interfaces: nics } end end diff --git a/lib/hyper/node/fire_vmm/state.ex b/lib/hyper/node/fire_vmm/state.ex index 9d16d04d..274db93b 100644 --- a/lib/hyper/node/fire_vmm/state.ex +++ b/lib/hyper/node/fire_vmm/state.ex @@ -82,7 +82,8 @@ defmodule Hyper.Node.FireVMM.State do ) do case Hyper.Cluster.Routing.register_self({id, :state}) do :ok -> - spec = BootSpec.resolve(boot_source(kernel, Mutable.blk_path(mutable), boot_args), type) + source = Map.put(boot_source(kernel, Mutable.blk_path(mutable), boot_args), :vm_id, id) + spec = BootSpec.resolve(source, type) deadline = System.monotonic_time(:millisecond) + Time.as_ms(@ready_timeout) data = %State{opts: opts, spec: spec, boot_deadline: deadline} diff --git a/lib/hyper/vm.ex b/lib/hyper/vm.ex index 3001e41c..819dac89 100644 --- a/lib/hyper/vm.ex +++ b/lib/hyper/vm.ex @@ -14,12 +14,14 @@ defmodule Hyper.Vm do @typedoc """ What a VM boots from: explicit, already-jail-visible artifact paths for a cold boot (kernel + root drive). `boot_args` defaults to a standard serial-console - cmdline when omitted. + cmdline when omitted. `vm_id` is required only when networking is enabled + (`BootSpec.resolve/2` derives the guest NIC from it). """ @type source :: %{ required(:kernel_image_path) => Path.t(), required(:root_drive_path) => Path.t(), - optional(:boot_args) => String.t() + optional(:boot_args) => String.t(), + optional(:vm_id) => Hyper.Vm.Id.t() } @doc """ diff --git a/test/hyper/node/fire_vmm/boot_spec_test.exs b/test/hyper/node/fire_vmm/boot_spec_test.exs index b79185f8..463dd7e9 100644 --- a/test/hyper/node/fire_vmm/boot_spec_test.exs +++ b/test/hyper/node/fire_vmm/boot_spec_test.exs @@ -3,6 +3,12 @@ defmodule Hyper.Node.FireVMM.BootSpecTest do alias Hyper.Node.FireVMM.BootSpec + # The enabled path (NIC populated + `ip=` appended) needs a `[network]` config + # override, which the unit config harness doesn't provide; it's asserted in + # the e2e/integration suite instead. This module only pins the disabled path + # (the base test config has no `[network]` table), which must hold for every + # existing VM. + test "default cmdline boots our agent as init with no serial console" do source = %{kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} cold = BootSpec.resolve(source, :micro) @@ -11,4 +17,11 @@ defmodule Hyper.Node.FireVMM.BootSpecTest do # re-adds console=ttyS0 through the per-VM boot_args override. refute cold.boot_source.boot_args =~ "console=" end + + test "no NIC and no ip= when networking disabled" do + source = %{vm_id: "vabc", kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} + cold = BootSpec.resolve(source, :micro) + assert cold.network_interfaces == [] + refute cold.boot_source.boot_args =~ "ip=" + end end From a4295d23dd8ecf959bcc9ad38a0726575adf02c0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:29:33 +0000 Subject: [PATCH 12/42] feat(suidhelper): jailer --netns derived from vm id when networking enabled 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 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. --- native/suidhelper/src/tools/jailer.rs | 22 +++++++- native/suidhelper/tests/e2e/jailer.rs | 67 +++++++++++++++++++++++++ native/suidhelper/tests/tools/jailer.rs | 52 ++++++++++++++++++- 3 files changed, 139 insertions(+), 2 deletions(-) diff --git a/native/suidhelper/src/tools/jailer.rs b/native/suidhelper/src/tools/jailer.rs index bef1dc7b..f029da52 100644 --- a/native/suidhelper/src/tools/jailer.rs +++ b/native/suidhelper/src/tools/jailer.rs @@ -241,8 +241,14 @@ fn cstr_path(p: &Path) -> Result { /// Build the exact argv handed to the jailer. argv[0] is the jailer path. The /// caller never names the jailer, the `--exec-file`, the chroot base, the cgroup /// version, or the parent cgroup — those are derived from trusted config here. +/// +/// `netns` is `Some(path)` when VM networking is enabled (see [`Config::network`]); +/// the path itself is computed by the caller from the validated `id`, never taken +/// from caller input. Kept as a plain parameter (rather than reading config here) +/// so this function stays pure and directly testable without the process-wide +/// config singleton. #[allow(clippy::too_many_arguments)] -fn build_argv( +pub fn build_argv( jailer: &Path, id: &VmId, firecracker: &Path, @@ -252,6 +258,7 @@ fn build_argv( parent_cgroup: &str, cgroups: &[CgroupSetting], api_sock: &JailSock, + netns: Option<&Path>, ) -> Result, Error> { let mut argv = vec![ cstr_path(jailer)?, @@ -276,6 +283,11 @@ fn build_argv( argv.push(cstr_str(&cg.to_string())?); } + if let Some(netns) = netns { + argv.push(cstr_str("--netns")?); + argv.push(cstr_path(netns)?); + } + argv.push(cstr_str("--")?); argv.push(cstr_str("--api-sock")?); argv.push(cstr_str(&api_sock.to_string())?); @@ -329,6 +341,13 @@ pub fn run(args: JailerArgs) -> Result { return Err(Error::TooManyCgroups(args.cgroup.len())); } + // The netns path is derived entirely from trusted config (whether networking + // is enabled) and the already-validated `--id` — the caller cannot name it. + // This must match the `ip netns add ` path the network tool creates. + let netns: Option = config + .network() + .map(|_| PathBuf::from(format!("/var/run/netns/{}", args.id))); + let argv = build_argv( &jailer, &args.id, @@ -339,6 +358,7 @@ pub fn run(args: JailerArgs) -> Result { parent_cgroup, &args.cgroup, &args.api_sock, + netns.as_deref(), )?; let jailer_cstr = cstr_path(&jailer)?; diff --git a/native/suidhelper/tests/e2e/jailer.rs b/native/suidhelper/tests/e2e/jailer.rs index ba45468c..55e164bc 100644 --- a/native/suidhelper/tests/e2e/jailer.rs +++ b/native/suidhelper/tests/e2e/jailer.rs @@ -67,6 +67,20 @@ fn write_root_config(dir: &Path, jailer: &Path, firecracker: &Path) -> PathBuf { p } +/// Same as [`write_root_config`] but with a `[network]` table present, so +/// `Config::network()` is `Some` and `run()` derives a `--netns` flag. +fn write_root_config_with_network(dir: &Path, jailer: &Path, firecracker: &Path) -> PathBuf { + let p = dir.join("config.toml"); + let body = format!( + "work_dir = \"/srv/hyper\"\n[tools]\njailer = \"{}\"\nfirecracker = \"{}\"\n[network]\nuplink = \"eth0\"\n", + jailer.display(), + firecracker.display(), + ); + fs::write(&p, body).unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + p +} + fn run(config: &Path, args: &[&str]) -> std::process::Output { Command::new(HELPER) .args(args) @@ -172,6 +186,59 @@ fn execs_jailer_with_canonical_argv_and_empty_env_as_root() { ); } +#[test] +fn execs_jailer_with_netns_flag_when_networking_enabled_as_root() { + if !is_root() { + eprintln!("SKIP jailer netns: needs root to become_root_permanently + own the fakes"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let argv_rec = tmp.path().join("argv.json"); + let env_rec = tmp.path().join("environ.bin"); + let jailer = install_recorder(tmp.path(), &argv_rec, &env_rec); + let firecracker = install_firecracker(tmp.path()); + let cfg = write_root_config_with_network(tmp.path(), &jailer, &firecracker); + + let out = run( + &cfg, + &[ + "jailer", + "--id", + "vnet1", + "--uid", + "900001", + "--gid", + "900002", + "--api-sock", + "/api.sock", + ], + ); + + assert_eq!( + out.status.code(), + Some(RECORDER_EXIT), + "exit status did not propagate; stderr: {}", + String::from_utf8_lossy(&out.stderr), + ); + + let argv: Vec = + serde_json::from_str(&fs::read_to_string(&argv_rec).expect("recorded argv")).unwrap(); + let i = argv + .iter() + .position(|a| a == "--netns") + .expect("--netns present when [network] is configured"); + assert_eq!( + argv[i + 1], + "/var/run/netns/vnet1", + "netns path must be derived from --id, not caller-supplied" + ); + let sep = argv.iter().position(|a| a == "--").unwrap(); + assert!( + i < sep, + "--netns must precede the `--` firecracker separator" + ); +} + #[test] fn refuses_uid_zero_without_exec_as_root() { if !is_root() { diff --git a/native/suidhelper/tests/tools/jailer.rs b/native/suidhelper/tests/tools/jailer.rs index 9ab75447..7cea1769 100644 --- a/native/suidhelper/tests/tools/jailer.rs +++ b/native/suidhelper/tests/tools/jailer.rs @@ -4,8 +4,11 @@ //! a compromised BEAM from naming uid 0, a privileged path, a traversal, or a //! flag through the jailer subcommand. -use hyper_suidhelper::tools::jailer::{validate_id_number, CgroupSetting, JailSock, VmId}; +use hyper_suidhelper::tools::jailer::{ + build_argv, validate_id_number, CgroupSetting, JailSock, VmId, +}; use proptest::prelude::*; +use std::path::Path; use std::str::FromStr; proptest! { @@ -160,3 +163,50 @@ fn jailsock_rejects_known_bad_shapes() { ); } } + +/// `build_argv` is a pure function of its parameters — no process-wide config is +/// read here, so the `netns` presence/absence is exercised directly rather than +/// through the `[network]` config table (see `src/tools/jailer.rs::run`, which is +/// the thin wrapper that derives the `Option` from `Config::network()`). +fn sample_argv(netns: Option<&Path>) -> Vec { + let id = VmId::from_str("vabc").unwrap(); + let api_sock = JailSock::from_str("/api.sock").unwrap(); + let argv = build_argv( + Path::new("/usr/bin/jailer"), + &id, + Path::new("/usr/bin/firecracker"), + 900_001, + 900_002, + Path::new("/srv/hyper/jails"), + "hyper", + &[], + &api_sock, + netns, + ) + .unwrap(); + argv.into_iter() + .map(|c| c.to_str().unwrap().to_string()) + .collect() +} + +#[test] +fn netns_flag_derived_from_id_when_networking_enabled() { + let netns = std::path::PathBuf::from("/var/run/netns/vabc"); + let joined = sample_argv(Some(&netns)); + let i = joined + .iter() + .position(|a| a == "--netns") + .expect("--netns present"); + assert_eq!(joined[i + 1], "/var/run/netns/vabc"); + let sep = joined.iter().position(|a| a == "--").unwrap(); + assert!( + i < sep, + "--netns must precede the `--` firecracker separator" + ); +} + +#[test] +fn netns_flag_absent_when_networking_disabled() { + let joined = sample_argv(None); + assert!(!joined.contains(&"--netns".to_string())); +} From 975b389016180ec40f87de4dbf81d242e4221ec9 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:36:36 +0000 Subject: [PATCH 13/42] feat(suidhelper): mknod /dev/net/tun into the jail when networking enabled --- native/suidhelper/src/util/chroot_jail.rs | 28 +++++++- native/suidhelper/src/util/safe_dir.rs | 44 +++++++++++- native/suidhelper/tests/e2e/chroot_jail.rs | 82 +++++++++++++++++++++- 3 files changed, 151 insertions(+), 3 deletions(-) diff --git a/native/suidhelper/src/util/chroot_jail.rs b/native/suidhelper/src/util/chroot_jail.rs index 6a618187..22c6523a 100644 --- a/native/suidhelper/src/util/chroot_jail.rs +++ b/native/suidhelper/src/util/chroot_jail.rs @@ -8,7 +8,7 @@ use crate::util::safe_file::{self, Any, IsBlockDevice, SafeFile}; use crate::util::safe_path::{self, IsAbsolute, SafePath, StrictComponents}; use nix::errno::Errno; use nix::fcntl::{open as nix_open, OFlag}; -use nix::sys::stat::Mode; +use nix::sys::stat::{makedev, Mode}; use std::io; use std::os::unix::io::FromRawFd; use std::path::{Path, PathBuf}; @@ -19,6 +19,10 @@ use thiserror::Error as ThisError; const KERNEL_NAME: &str = "vmlinux"; const ROOTFS_NAME: &str = "rootfs"; +/// `/dev/net/tun`'s well-known major:minor, per Linux's `Documentation/admin-guide/devices.txt`. +const TUN_MAJOR: u64 = 10; +const TUN_MINOR: u64 = 200; + #[derive(Debug, ThisError)] pub enum Error { #[error(transparent)] @@ -122,6 +126,9 @@ impl ChrootJail { let chroot = open_chroot_under(jail_base, &self.chroot)?; stage_kernel_under(&chroot, &self.kernel.0, hyper_base, self.uid, self.gid)?; make_rootfs(&chroot, &self.rootfs.0, self.uid, self.gid)?; + if Config::get().network().is_some() { + make_tun_node(&chroot, self.uid, self.gid)?; + } Ok(()) } } @@ -213,3 +220,22 @@ fn make_rootfs(chroot: &SafeDir, device: &BlockDev, uid: u32, gid: u32) -> Resul chroot.mknod_block(Path::new(ROOTFS_NAME), rdev, uid, gid)?; Ok(()) } + +/// Create `dev/net/tun` (char `10:200`, mode `0666`) inside the jail, owned +/// `uid:gid`, so the chrooted firecracker can `open("/dev/net/tun")` and +/// `TUNSETIFF` onto the tap device waiting in its netns. Only called when VM +/// networking is enabled; a jail built without it has no `dev` tree at all +/// today (see [`ChrootJail::build_under`]). +fn make_tun_node(chroot: &SafeDir, uid: u32, gid: u32) -> Result<(), Error> { + let dev = Path::new("dev"); + chroot.mkdir(dev, 0o755)?; + let dev_dir = chroot.openat_dir(dev)?; + + let net = Path::new("net"); + dev_dir.mkdir(net, 0o755)?; + let net_dir = dev_dir.openat_dir(net)?; + + let rdev = makedev(TUN_MAJOR, TUN_MINOR); + net_dir.mknod_char(Path::new("tun"), rdev, uid, gid)?; + Ok(()) +} diff --git a/native/suidhelper/src/util/safe_dir.rs b/native/suidhelper/src/util/safe_dir.rs index 61808d8a..52f2c576 100644 --- a/native/suidhelper/src/util/safe_dir.rs +++ b/native/suidhelper/src/util/safe_dir.rs @@ -15,9 +15,12 @@ use super::safe_file::{Any, SafeFile}; use super::safe_path::SafePath; use nix::dir::{Dir, Type}; +use nix::errno::Errno; use nix::fcntl::{openat, AtFlags, OFlag}; use nix::libc::dev_t; -use nix::sys::stat::{fchmod, fchmodat, fstatat, mknodat, FchmodatFlags, FileStat, Mode, SFlag}; +use nix::sys::stat::{ + fchmod, fchmodat, fstatat, mkdirat, mknodat, FchmodatFlags, FileStat, Mode, SFlag, +}; use nix::unistd::{dup, fchown, fchownat, linkat, unlinkat, write, Gid, Uid, UnlinkatFlags}; use std::ffi::OsStr; use std::os::unix::ffi::OsStrExt; @@ -35,6 +38,8 @@ pub enum Error { Unlink { name: PathBuf, source: nix::Error }, #[error("write {name:?}: {source}")] Write { name: PathBuf, source: nix::Error }, + #[error("mkdirat {name:?}: {source}")] + Mkdir { name: PathBuf, source: nix::Error }, #[error("mknodat {name:?}: {source}")] Mknod { name: PathBuf, source: nix::Error }, #[error("fchownat {name:?}: {source}")] @@ -57,6 +62,7 @@ impl Error { Error::Open { source, .. } | Error::Unlink { source, .. } | Error::Write { source, .. } + | Error::Mkdir { source, .. } | Error::Mknod { source, .. } | Error::Chown { source, .. } | Error::Chmod { source, .. } @@ -192,6 +198,42 @@ impl SafeDir { self.chown(name, uid, gid) } + /// Create a character device node `name` in this directory (mode `0666`, + /// matching `/dev/net/tun`'s conventional permissions) with the given + /// `rdev`, then chown it to `uid:gid`. + pub fn mknod_char(&self, name: &Path, rdev: dev_t, uid: u32, gid: u32) -> Result<(), Error> { + mknodat( + Some(self.0.as_raw_fd()), + name, + SFlag::S_IFCHR, + Mode::from_bits_truncate(0o666), + rdev, + ) + .map_err(|source| Error::Mknod { + name: name.to_path_buf(), + source, + })?; + self.chown(name, uid, gid) + } + + /// Create directory `name` in this directory with `mode`, tolerating + /// `EEXIST` (`mkdir -p` semantics): the jailer may have already created + /// intermediate directories (e.g. `dev`, to bind-mount `/dev/kvm`) before + /// this helper runs. + pub fn mkdir(&self, name: &Path, mode: u32) -> Result<(), Error> { + match mkdirat( + Some(self.0.as_raw_fd()), + name, + Mode::from_bits_truncate(mode), + ) { + Ok(()) | Err(Errno::EEXIST) => Ok(()), + Err(source) => Err(Error::Mkdir { + name: name.to_path_buf(), + source, + }), + } + } + /// Hard-link the file at host path `src` into this directory as `name`. pub fn link_from(&self, src: &Path, name: &Path) -> Result<(), Error> { linkat( diff --git a/native/suidhelper/tests/e2e/chroot_jail.rs b/native/suidhelper/tests/e2e/chroot_jail.rs index 0d0e290e..11983570 100644 --- a/native/suidhelper/tests/e2e/chroot_jail.rs +++ b/native/suidhelper/tests/e2e/chroot_jail.rs @@ -3,10 +3,15 @@ //! never touch /etc/hyper or /srv. Root-gated: the helper acquires privileges for //! mknod/chown, so these self-skip without root. The cgroup half is exercised //! only with a MISSING leaf (idempotent, mutates nothing on the real host). +//! +//! The `dev/net/tun` node `prepare` creates when `[network]` is configured is +//! likewise privileged `mknod` — no unit-level substitute exists; it is proven +//! only here, root-gated, with the insecure config seam supplying a `[network]` +//! table. #![cfg(feature = "insecure_test_seams")] use std::fs; -use std::os::unix::fs::{MetadataExt, PermissionsExt}; +use std::os::unix::fs::{FileTypeExt, MetadataExt, PermissionsExt}; use std::path::{Path, PathBuf}; use std::process::{Command, Output}; @@ -25,6 +30,22 @@ fn write_config(dir: &Path, work_dir: &Path) -> PathBuf { p } +/// Same as [`write_config`], plus a `[network]` table — the gate that makes +/// `chroot-jail prepare` also mknod `dev/net/tun` into the jail. +fn write_config_with_network(dir: &Path, work_dir: &Path) -> PathBuf { + let p = dir.join("config.toml"); + fs::write( + &p, + format!( + "work_dir = \"{}\"\n[network]\nuplink = \"eth0\"\n", + work_dir.display() + ), + ) + .unwrap(); + fs::set_permissions(&p, fs::Permissions::from_mode(0o644)).unwrap(); + p +} + fn run(config: &Path, args: &[&str]) -> Output { Command::new(BIN) .args(args) @@ -115,6 +136,65 @@ fn prepare_succeeds_and_builds_jail_as_root() { ); } +// `chroot-jail prepare` against a config with `[network]` set also mknods +// `dev/net/tun` into the jail: char device 10:200, mode 0666, owned uid:gid. +#[test] +fn prepare_creates_tun_node_when_networking_enabled_as_root() { + if !is_root() { + eprintln!("SKIP prepare_creates_tun_node_when_networking_enabled: needs root"); + return; + } + let tmp = tempfile::tempdir().unwrap(); + let work = tmp.path().join("srv"); + let chroot = work.join("jails").join("exec").join("id"); + fs::create_dir_all(&chroot).unwrap(); + let kernel = work.join("vmlinux-src"); + fs::write(&kernel, b"kernel image").unwrap(); + let cfg = write_config_with_network(tmp.path(), &work); + + let Some(dev) = setup_loop(tmp.path()) else { + eprintln!("SKIP prepare_creates_tun_node: losetup unavailable"); + return; + }; + + let uid = 1234; + let gid = 5678; + let out = run( + &cfg, + &[ + "chroot-jail", + "prepare", + "--chroot", + chroot.to_str().unwrap(), + "--kernel", + kernel.to_str().unwrap(), + "--device", + dev.to_str().unwrap(), + "--uid", + &uid.to_string(), + "--gid", + &gid.to_string(), + ], + ); + teardown_loop(&dev); + + assert_eq!( + out.status.code(), + Some(0), + "stderr: {}", + String::from_utf8_lossy(&out.stderr) + ); + + let tun = chroot.join("dev").join("net").join("tun"); + let meta = fs::metadata(&tun).expect("dev/net/tun must exist"); + assert!(meta.file_type().is_char_device(), "must be a char device"); + assert_eq!(nix::sys::stat::major(meta.rdev()), 10); + assert_eq!(nix::sys::stat::minor(meta.rdev()), 200); + assert_eq!(meta.uid(), uid); + assert_eq!(meta.gid(), gid); + assert_eq!(meta.permissions().mode() & 0o777, 0o666); +} + // `chroot-jail prepare` with a system device (not a loop / hyper-* dm) is // rejected by BlockDev parsing at clap parse time — exit 2, nothing built. This // case needs no root (clap rejects before the privilege boundary) but also no From e4e3ff4dea13cb50bb8a8fc72ef02dcd02e43d52 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 02:52:43 +0000 Subject: [PATCH 14/42] feat(net): prepare netns before jailer launch, tear down on jail clear MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/hyper/node/fire_vmm/daemon.ex | 40 +++++-- test/hyper/node/fire_vmm/daemon_test.exs | 139 +++++++++++++++++++++++ 2 files changed, 170 insertions(+), 9 deletions(-) create mode 100644 test/hyper/node/fire_vmm/daemon_test.exs diff --git a/lib/hyper/node/fire_vmm/daemon.ex b/lib/hyper/node/fire_vmm/daemon.ex index 882a6cca..d0d9dd99 100644 --- a/lib/hyper/node/fire_vmm/daemon.ex +++ b/lib/hyper/node/fire_vmm/daemon.ex @@ -57,11 +57,13 @@ defmodule Hyper.Node.FireVMM.Daemon do # link kill) and so `terminate/2` runs on supervisor shutdown. Process.flag(:trap_exit, true) - with :ok <- reset_stale_jail(id), + with :ok <- prelaunch(opts), {:ok, muontrap} <- launch(opts) do {:ok, %__MODULE__{opts: opts, muontrap: muontrap}} else - {:error, reason} -> {:stop, reason} + {:error, reason} -> + Logger.error("vm #{id}: init failed: #{inspect(reason)}") + {:stop, reason} end end @@ -79,8 +81,8 @@ defmodule Hyper.Node.FireVMM.Daemon do # here is logged, and the `Reaper` will retry, but it must not crash teardown. @impl true @decorate with_span("Hyper.Node.FireVMM.Daemon.terminate", include: [:id]) - def terminate(_reason, %__MODULE__{opts: %Opts{vm_id: id}}) do - case clear_jail(id) do + def terminate(_reason, %__MODULE__{opts: %Opts{vm_id: id} = opts}) do + case clear_jail(opts) do :ok -> :ok @@ -89,12 +91,32 @@ defmodule Hyper.Node.FireVMM.Daemon do end end - @spec reset_stale_jail(Hyper.Vm.Id.t()) :: :ok | {:error, term()} - defp reset_stale_jail(id), do: clear_jail(id) + # The testable seam: reset any stale jail from a prior incarnation, then — + # gated on networking being enabled — prepare the netns the jailer's + # `--netns` will enter. Must run before `launch/1`: the jailer refuses to + # start if the netns it is told to enter does not already exist. + @spec prelaunch(Opts.t()) :: :ok | {:error, term()} + defp prelaunch(%Opts{vm_id: id, uid: uid} = opts) do + with :ok <- reset_stale_jail(opts) do + if Hyper.Cfg.Network.enabled?(), do: Hyper.SuidHelper.Network.prepare(id, uid), else: :ok + end + end - @spec clear_jail(Hyper.Vm.Id.t()) :: :ok | {:error, term()} - defp clear_jail(id) do - SuidHelper.ChrootJail.remove(Jailer.chroot_dir(id), Jailer.cgroup_dir(id)) + @spec reset_stale_jail(Opts.t()) :: :ok | {:error, term()} + defp reset_stale_jail(opts), do: clear_jail(opts) + + # Best-effort: both the chroot/cgroup removal and the network teardown are + # attempted regardless of the other's outcome (a `Reaper` backstops either + # failure); the first error is surfaced to the caller. + @spec clear_jail(Opts.t()) :: :ok | {:error, term()} + defp clear_jail(%Opts{vm_id: id, uid: uid}) do + net = + if Hyper.Cfg.Network.enabled?(), + do: Hyper.SuidHelper.Network.teardown(id, uid), + else: :ok + + jail = SuidHelper.ChrootJail.remove(Jailer.chroot_dir(id), Jailer.cgroup_dir(id)) + with :ok <- net, do: jail end @spec launch(Opts.t()) :: {:ok, pid()} | {:error, term()} diff --git a/test/hyper/node/fire_vmm/daemon_test.exs b/test/hyper/node/fire_vmm/daemon_test.exs new file mode 100644 index 00000000..108750b5 --- /dev/null +++ b/test/hyper/node/fire_vmm/daemon_test.exs @@ -0,0 +1,139 @@ +defmodule Hyper.Node.FireVMM.DaemonTest do + @moduledoc """ + `Daemon` shells every privileged step (netns prepare/teardown, chroot + removal, jailer launch) through the setuid helper binary named by + `Hyper.Cfg.Tools.suidhelper/0`. These tests point that at a small fake + script instead of the real setuid helper — the same seam `JailerTest` uses + for `tools.firecracker`/`tools.jailer` — so they can observe real call + order and gating without root, a real netns, or a real firecracker process. + + Invariants under test: + + * the jailer enters the netns via `--netns` (Task 8), so it must already + exist: `network prepare` must run before the `jailer` launch. + * jail clearing (`chroot-jail remove`) and network teardown both fire + wherever the jail is cleared (stale-reset on relaunch, and terminate), + gated on `Hyper.Cfg.Network.enabled?/0`. Disabled is behavior-identical + to before this feature: no network call at all. + """ + + use ExUnit.Case, async: false + + alias Hyper.Node.FireVMM + alias Hyper.Node.FireVMM.Daemon + + @vm_id "vdaemontest01" + + setup do + log = Path.join(System.tmp_dir!(), "daemon_test_#{System.unique_integer([:positive])}.log") + fake = write_fake_suidhelper!(log) + + on_exit(fn -> + Hyper.Cfg.Toml.reload() + File.rm(fake) + File.rm(log) + end) + + {:ok, log: log, fake: fake} + end + + defp opts do + %FireVMM.Opts{ + vm_id: @vm_id, + uid: 900_500, + gid: 900_500, + type: :micro, + arch: :x86_64, + mutable: nil, + kernel: "/nonexistent/vmlinux", + boot_args: nil + } + end + + # A stand-in for hyper-suidhelper: appends "" as one line per + # invocation (so call order is observable across the test) and prints an + # empty JSON object so `SuidHelper.exec/1`'s `Jason.decode!/1` succeeds. + # Exits immediately, so a `jailer` invocation looks like an instant crash + # rather than a live firecracker — fine here, only the call log matters. + defp write_fake_suidhelper!(log) do + path = + Path.join(System.tmp_dir!(), "fake_suidhelper_#{System.unique_integer([:positive])}.sh") + + File.write!(path, """ + #!/usr/bin/env bash + echo "$*" >> #{log} + echo '{}' + """) + + File.chmod!(path, 0o755) + path + end + + defp calls(log) do + log + |> File.read!() + |> String.split("\n", trim: true) + |> Enum.map(&hd(String.split(&1, " "))) + end + + # `Jailer.chroot_dir/1` derives the in-jail exec name from `tools.firecracker` + # (see `JailerTest`), so every scenario stubs it alongside `tools.suidhelper` + # to keep it off the real filesystem. + defp put_toml(fake, network \\ nil) do + tools = %{"suidhelper" => fake, "firecracker" => "/usr/local/bin/firecracker"} + base = %{"tools" => tools} + Hyper.Cfg.Toml.put_cache(if network, do: Map.put(base, "network", network), else: base) + end + + describe "networking disabled (default)" do + test "start_link never calls network, and jail clearing still runs", %{log: log, fake: fake} do + refute Hyper.Cfg.Network.enabled?() + put_toml(fake) + + Process.flag(:trap_exit, true) + {:ok, pid} = Daemon.start_link(opts()) + assert_receive {:EXIT, ^pid, _reason}, 2_000 + + ops = calls(log) + refute "network" in ops + assert "chroot-jail" in ops + assert "jailer" in ops + # the netns-prepare (or its absence) always precedes the launch attempt + assert Enum.find_index(ops, &(&1 == "chroot-jail")) < + Enum.find_index(ops, &(&1 == "jailer")) + end + end + + describe "networking enabled" do + test "start_link prepares the netns before launching the jailer", %{log: log, fake: fake} do + put_toml(fake, %{"uplink" => "eth0"}) + assert Hyper.Cfg.Network.enabled?() + + Process.flag(:trap_exit, true) + {:ok, pid} = Daemon.start_link(opts()) + assert_receive {:EXIT, ^pid, _reason}, 2_000 + + ops = calls(log) + prepare_at = Enum.find_index(ops, &(&1 == "network")) + jailer_at = Enum.find_index(ops, &(&1 == "jailer")) + + assert prepare_at, "expected a `network` (prepare) call before the jailer launch" + assert jailer_at, "expected the jailer to have been launched" + assert prepare_at < jailer_at, "netns must be prepared before the jailer enters it" + end + + test "terminate/2 tears down both the network and the chroot jail", %{log: log, fake: fake} do + put_toml(fake, %{"uplink" => "eth0"}) + + assert :ok = + Daemon.terminate(:shutdown, %Daemon{ + opts: opts(), + muontrap: nil + }) + + ops = calls(log) + assert "network" in ops + assert "chroot-jail" in ops + end + end +end From 2ee99f5ee9fd68b0e7f093c71bce8c9f1c09871d Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:00:02 +0000 Subject: [PATCH 15/42] feat(net): host-init nft table at node start + uplink/ip_forward preflight 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. --- lib/hyper/node.ex | 15 ++++++- lib/hyper/node/fire_vmm/jailer.ex | 41 ++++++++++++++++- native/suidhelper/Cargo.toml | 4 ++ .../suidhelper/src/tools/network/host_init.rs | 23 ++++++++++ native/suidhelper/tests/tools/host_init.rs | 44 +++++++++++++++++++ test/hyper/node/fire_vmm/jailer_test.exs | 16 +++++++ 6 files changed, 140 insertions(+), 3 deletions(-) create mode 100644 native/suidhelper/tests/tools/host_init.rs diff --git a/lib/hyper/node.ex b/lib/hyper/node.ex index 66f1b0f7..9ccabe55 100644 --- a/lib/hyper/node.ex +++ b/lib/hyper/node.ex @@ -190,11 +190,24 @@ defmodule Hyper.Node do :ok <- Hyper.Node.Layer.Repo.test_system(), :ok <- Hyper.SuidHelper.test_system(), {:ok, base} <- Hyper.SuidHelper.sys_test(), - :ok <- check_helper_base(base) do + :ok <- check_helper_base(base), + :ok <- maybe_host_init() do Hyper.Node.FireVMM.test_system() end end + # Idempotent: reconciles the host-wide `hyper` nftables table on every start. + # No-op when `[network]` is absent from config. Runs after + # `Hyper.SuidHelper.test_system/0` so the helper is confirmed present first. + @spec maybe_host_init :: :ok | {:error, term()} + defp maybe_host_init do + if Hyper.Cfg.Network.enabled?() do + Hyper.SuidHelper.Network.host_init() + else + :ok + end + end + @spec check_firecracker_bins :: :ok | {:error, {:firecracker_bin_missing | :jailer_bin_missing, Path.t()}} diff --git a/lib/hyper/node/fire_vmm/jailer.ex b/lib/hyper/node/fire_vmm/jailer.ex index cc86040b..732dce48 100644 --- a/lib/hyper/node/fire_vmm/jailer.ex +++ b/lib/hyper/node/fire_vmm/jailer.ex @@ -38,7 +38,9 @@ defmodule Hyper.Node.FireVMM.Jailer do first failure. """ - alias Hyper.Cfg.{Dirs, Jails} + alias Hyper.Cfg.{Dirs, Jails, Network} + + @ip_forward_path "/proc/sys/net/ipv4/ip_forward" @doc "Run every pre-requisite check, halting at the first failure." @spec run() :: :ok | {:error, term()} @@ -56,10 +58,45 @@ defmodule Hyper.Node.FireVMM.Jailer do &kvm_present/0, &cgroup_v2_available/0, &parent_cgroup_present/0, - &chroot_writable/0 + &chroot_writable/0, + &network_ready/0 ] end + @doc """ + Host preflight for VM egress networking: a no-op when `[network]` is + absent from config (`Network.enabled?/0` false). When enabled, confirms + the uplink interface exists and the kernel is forwarding IPv4 — both + prerequisites `Hyper.SuidHelper.Network.host_init/0` and per-VM + `prepare/2` depend on but cannot themselves provision. + """ + @spec network_ready() :: :ok | {:error, term()} + def network_ready do + if Network.enabled?() do + with :ok <- uplink_present(Network.uplink()) do + ip_forward_enabled() + end + else + :ok + end + end + + defp uplink_present(uplink) do + if File.dir?("/sys/class/net/#{uplink}"), + do: :ok, + else: {:error, {:missing_uplink, uplink}} + end + + defp ip_forward_enabled do + case File.read(@ip_forward_path) do + {:ok, contents} -> + if String.trim(contents) == "1", do: :ok, else: {:error, :ip_forward_disabled} + + {:error, reason} -> + {:error, {:ip_forward_unreadable, reason}} + end + end + defp kvm_present do if File.exists?("/dev/kvm"), do: :ok, else: {:error, :kvm_unavailable} end diff --git a/native/suidhelper/Cargo.toml b/native/suidhelper/Cargo.toml index d3e006e1..c73f3353 100644 --- a/native/suidhelper/Cargo.toml +++ b/native/suidhelper/Cargo.toml @@ -60,6 +60,10 @@ path = "tests/tools/network_addr.rs" name = "tools_network_args" path = "tests/tools/network_args.rs" +[[test]] +name = "tools_host_init" +path = "tests/tools/host_init.rs" + [[test]] name = "util_confinement" path = "tests/util/confinement.rs" diff --git a/native/suidhelper/src/tools/network/host_init.rs b/native/suidhelper/src/tools/network/host_init.rs index 21ddd42e..4bdeec88 100644 --- a/native/suidhelper/src/tools/network/host_init.rs +++ b/native/suidhelper/src/tools/network/host_init.rs @@ -24,6 +24,9 @@ use thiserror::Error as ThisError; #[derive(Args)] pub struct HostInitArgs {} +/// `/proc` file the kernel exposes IPv4 forwarding through: `1` when enabled. +const IP_FORWARD_PATH: &str = "/proc/sys/net/ipv4/ip_forward"; + #[derive(Debug, ThisError)] pub enum Error { #[error(transparent)] @@ -32,6 +35,21 @@ pub enum Error { Bin(#[from] safe_bin::Error), #[error(transparent)] Exec(#[from] exec::Error), + #[error("failed to read {IP_FORWARD_PATH}: {0}")] + IpForwardRead(#[source] std::io::Error), + #[error( + "net.ipv4.ip_forward is disabled ({IP_FORWARD_PATH} != 1); provision the host with \ + `sysctl -w net.ipv4.ip_forward=1` (see docs/cookbook/install.md) before VM networking \ + can be enabled" + )] + IpForwardDisabled, +} + +/// Whether `contents` (the raw text of `/proc/sys/net/ipv4/ip_forward`) says +/// forwarding is on. Pure so the parsing rule is testable without root or a +/// real `/proc`; the privileged read itself lives in `run_privileged`. +pub fn ip_forward_enabled(contents: &str) -> bool { + contents.trim() == "1" } pub fn run(args: HostInitArgs) -> Result { @@ -64,6 +82,11 @@ impl IsTool for HostInit { type RunT = Result<(), Error>; fn run_privileged(&self) -> Self::RunT { + let ip_forward = std::fs::read_to_string(IP_FORWARD_PATH).map_err(Error::IpForwardRead)?; + if !ip_forward_enabled(&ip_forward) { + return Err(Error::IpForwardDisabled); + } + let commands = args::host_init_commands(&self.uplink, &self.clone_pool); // host_init_commands only ever issues `nft` commands (see args.rs). exec::run_all(&commands, |_which| self.nft.as_path())?; diff --git a/native/suidhelper/tests/tools/host_init.rs b/native/suidhelper/tests/tools/host_init.rs new file mode 100644 index 00000000..9e195adf --- /dev/null +++ b/native/suidhelper/tests/tools/host_init.rs @@ -0,0 +1,44 @@ +//! `ip_forward_enabled` is the pure predicate `host_init` uses to decide +//! whether `/proc/sys/net/ipv4/ip_forward` says forwarding is on. Law under +//! test: it is `true` exactly for `"1"` modulo surrounding whitespace, and +//! `false` for everything else (kernel default `"0"`, empty, garbage). +use hyper_suidhelper::tools::network::host_init::ip_forward_enabled; +use proptest::prelude::*; + +#[test] +fn enabled_for_bare_one() { + assert!(ip_forward_enabled("1")); +} + +#[test] +fn enabled_for_one_with_trailing_newline() { + assert!(ip_forward_enabled("1\n")); +} + +#[test] +fn disabled_for_zero() { + assert!(!ip_forward_enabled("0")); + assert!(!ip_forward_enabled("0\n")); +} + +#[test] +fn disabled_for_empty_contents() { + assert!(!ip_forward_enabled("")); +} + +proptest! { + #[test] + fn whitespace_padding_around_one_is_still_enabled( + leading in "[ \t\n]{0,4}", + trailing in "[ \t\n]{0,4}", + ) { + let contents = format!("{leading}1{trailing}"); + prop_assert!(ip_forward_enabled(&contents)); + } + + #[test] + fn anything_but_one_after_trim_is_disabled(digits in "[0-9]{2,4}") { + // Multi-digit strings never equal the literal "1" after trimming. + prop_assert!(!ip_forward_enabled(&digits)); + } +} diff --git a/test/hyper/node/fire_vmm/jailer_test.exs b/test/hyper/node/fire_vmm/jailer_test.exs index 1f36dfd8..cd4328b3 100644 --- a/test/hyper/node/fire_vmm/jailer_test.exs +++ b/test/hyper/node/fire_vmm/jailer_test.exs @@ -85,4 +85,20 @@ defmodule Hyper.Node.FireVMM.JailerTest do assert Jailer.host_vsock(@vm_id) == "/srv/hyper/jails/firecracker/#{@vm_id}/root/vsock.sock" end + + describe "Checks.network_ready/0" do + test "ok when networking is disabled (no [network] table)" do + Hyper.Cfg.Toml.put_cache(%{}) + on_exit(fn -> Hyper.Cfg.Toml.reload() end) + + assert Jailer.Checks.network_ready() == :ok + end + + test "fails when uplink iface is absent" do + Hyper.Cfg.Toml.put_cache(%{"network" => %{"uplink" => "hyper-nonexistent-nic"}}) + on_exit(fn -> Hyper.Cfg.Toml.reload() end) + + assert {:error, {:missing_uplink, "hyper-nonexistent-nic"}} = Jailer.Checks.network_ready() + end + end end From 2d428c325b58ad563a98b4f58b955f7e1c2fca50 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:09:16 +0000 Subject: [PATCH 16/42] feat(net): reaper sweeps orphan VM netns (backstops crashed-node leaks) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 ` directly, tolerating an ENOENT-shaped failure as success so it doesn't error out when it races an already-completed normal teardown. --- lib/hyper/node/reaper.ex | 40 +++++- lib/hyper/node/reaper/plan.ex | 14 +- lib/hyper/suid_helper/network.ex | 14 ++ native/suidhelper/src/tools/network/args.rs | 11 ++ native/suidhelper/src/tools/network/mod.rs | 5 + .../src/tools/network/teardown_orphan.rs | 124 ++++++++++++++++++ native/suidhelper/tests/tools/network_args.rs | 15 +++ test/hyper/node/reaper/liveness_test.exs | 12 +- .../node/reaper/plan_properties_test.exs | 16 ++- test/hyper/node/reaper/plan_test.exs | 20 ++- test/hyper/suid_helper/network_test.exs | 5 + 11 files changed, 259 insertions(+), 17 deletions(-) create mode 100644 native/suidhelper/src/tools/network/teardown_orphan.rs diff --git a/lib/hyper/node/reaper.ex b/lib/hyper/node/reaper.ex index c6effa84..f34b2ecf 100644 --- a/lib/hyper/node/reaper.ex +++ b/lib/hyper/node/reaper.ex @@ -9,12 +9,16 @@ defmodule Hyper.Node.Reaper do Liveness is the whole point. The reaper consults three independent sources of truth for "this vm is alive" — the local VM supervisor's children, the cluster routing table, and the node's live mutable layers (`Img.Mutable.active_vm_ids/0`) - — (`Plan.orphans/3` removes their union from the candidate set) and only ever - touches `hyper-rw-*` dm names and per-VM cgroup leaves - never `hyper-thinpool`, + — (`Plan.orphans/4` removes their union from the candidate set) and only ever + touches `hyper-rw-*` dm names, per-VM cgroup leaves, and (when networking is + enabled) per-VM netns names under `/var/run/netns` - never `hyper-thinpool`, `hyper-img-*`, or a live VM's resources. A candidate must also survive two consecutive ticks (`Plan.confirm/2`) before it is reaped, so a VM caught mid-boot (resources present, not yet registered) is - given a grace tick rather than destroyed. + given a grace tick rather than destroyed — this is what protects a netns that + `Hyper.Node.FireVMM.Daemon.prelaunch/1` just created for a VM that has not yet + appeared in `gather_live/0`: it takes two orphan sightings to reap, and a + normally-booting VM never produces a second one. The decision logic lives in the pure `Hyper.Node.Reaper.Plan`; this module is a thin I/O adapter that gathers the inputs, calls the plan, and executes the @@ -84,8 +88,9 @@ defmodule Hyper.Node.Reaper do live = gather_live() leaves = list_cgroup_leaves() rw = Plan.rw_ids(list_rw_dm()) + netns = list_netns() - current = Plan.orphans(live, leaves, rw) + current = Plan.orphans(live, leaves, rw, netns) {to_reap, next} = Plan.confirm(current, state.last_orphans) Enum.each(to_reap, &reap_one/1) @@ -161,6 +166,28 @@ defmodule Hyper.Node.Reaper do end end + # `/var/run/netns` only exists (and only matters) when VM egress networking is + # turned on; skip the listing entirely when it is off rather than logging + # spurious enoent warnings on every tick. + @spec list_netns() :: [String.t()] + defp list_netns do + if Hyper.Cfg.Network.enabled?() do + case File.ls("/var/run/netns") do + {:ok, names} -> + names + + {:error, :enoent} -> + [] + + {:error, reason} -> + Logger.warning("reaper: could not list netns: #{inspect(reason)}") + [] + end + else + [] + end + end + @spec reap_one(String.t()) :: :ok @decorate with_span("Hyper.Node.Reaper.reap_one", include: [:id]) defp reap_one(id) do @@ -173,6 +200,11 @@ defmodule Hyper.Node.Reaper do ) log_result("dm volume", id, Dmsetup.remove(Img.Mutable.dm_name(id))) + + if Hyper.Cfg.Network.enabled?() do + log_result("netns", id, Hyper.SuidHelper.Network.teardown_orphan(id)) + end + :ok end diff --git a/lib/hyper/node/reaper/plan.ex b/lib/hyper/node/reaper/plan.ex index 17b6dde9..24ccf955 100644 --- a/lib/hyper/node/reaper/plan.ex +++ b/lib/hyper/node/reaper/plan.ex @@ -16,12 +16,20 @@ defmodule Hyper.Node.Reaper.Plan do do: String.replace_prefix(name, @rw_prefix, "") end - @doc "Candidate orphans this tick: (cgroup leaves ∪ rw vm_ids) minus the live set." - @spec orphans(MapSet.t(String.t()), [String.t()], [String.t()]) :: MapSet.t(String.t()) - def orphans(live, cgroup_leaves, rw_ids) do + @doc """ + Candidate orphans this tick: (cgroup leaves ∪ rw vm_ids ∪ netns names) minus + the live set. `netns_names` are the leaf names of `/var/run/netns/*` — a VM's + netns is named exactly its vm_id (see `Hyper.SuidHelper.Network`), so it folds + into the same union/difference as the other two sources and is subject to the + same two-strike `confirm/2` grace before `Hyper.Node.Reaper` ever reaps it. + """ + @spec orphans(MapSet.t(String.t()), [String.t()], [String.t()], [String.t()]) :: + MapSet.t(String.t()) + def orphans(live, cgroup_leaves, rw_ids, netns_names) do cgroup_leaves |> MapSet.new() |> MapSet.union(MapSet.new(rw_ids)) + |> MapSet.union(MapSet.new(netns_names)) |> MapSet.difference(live) end diff --git a/lib/hyper/suid_helper/network.ex b/lib/hyper/suid_helper/network.ex index df45de8c..e4eea2f7 100644 --- a/lib/hyper/suid_helper/network.ex +++ b/lib/hyper/suid_helper/network.ex @@ -15,6 +15,10 @@ defmodule Hyper.SuidHelper.Network do def argv(op, vm_id, uid), do: ["network", Atom.to_string(op), "--vm-id", vm_id, "--uid", to_string(uid)] + @doc false + @spec argv(:teardown_orphan, Hyper.Vm.Id.t()) :: [String.t()] + def argv(:teardown_orphan, vm_id), do: ["network", "teardown-orphan", "--vm-id", vm_id] + @doc "Create `vm_id`'s netns, veth pair, TAP, and NAT rules. Idempotent-safe on relaunch." @spec prepare(Hyper.Vm.Id.t(), non_neg_integer()) :: :ok | {:error, err()} @decorate with_span("Hyper.SuidHelper.Network.prepare", include: [:vm_id, :uid]) @@ -25,6 +29,16 @@ defmodule Hyper.SuidHelper.Network do @decorate with_span("Hyper.SuidHelper.Network.teardown", include: [:vm_id, :uid]) def teardown(vm_id, uid), do: run(argv(:teardown, vm_id, uid)) + @doc """ + Remove `vm_id`'s netns by id alone, with no `uid` — for the `Reaper`'s + crashed-node backstop, where the owning node never persisted a uid to derive + the clone-pool slot from. Idempotent: a netns already gone (raced with a + normal `teardown/2`, or never fully created) is not an error. + """ + @spec teardown_orphan(Hyper.Vm.Id.t()) :: :ok | {:error, err()} + @decorate with_span("Hyper.SuidHelper.Network.teardown_orphan", include: [:vm_id]) + def teardown_orphan(vm_id), do: run(argv(:teardown_orphan, vm_id)) + @doc "Ensure the host-wide `hyper` nft table (masquerade + forward policy) exists. Idempotent." @spec host_init() :: :ok | {:error, err()} @decorate with_span("Hyper.SuidHelper.Network.host_init", include: []) diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index 8b03bc4f..c3f03ac7 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -240,6 +240,17 @@ pub fn teardown_commands(plan: &Plan) -> Vec { ] } +/// Tear down an *orphan* VM's netns by vm_id alone — no `uid`, so no [`Plan`] +/// and no host-veth `link del` (that command needs the derived `hv` +/// name, which needs the uid). Deleting the netns is sufficient on its own: +/// it reclaims the ns-side veth peer, the TAP device, and any in-netns +/// nftables state, and the kernel removes the host-side veth end along with +/// its peer's netns. See `teardown_orphan`'s module doc for why the caller +/// (`Hyper.Node.Reaper`) has no uid to give here. +pub fn teardown_orphan_commands(netns: &str) -> Vec { + vec![Command::ip(argv!["netns", "del", netns])] +} + /// One-time host setup: a `hyper` nftables table that masquerades the clone /// pool out `uplink`, and a forward-chain default-drop policy that only admits /// the pool's own traffic — explicitly dropping anything addressed to the diff --git a/native/suidhelper/src/tools/network/mod.rs b/native/suidhelper/src/tools/network/mod.rs index 5e6cfeaa..ca2a9007 100644 --- a/native/suidhelper/src/tools/network/mod.rs +++ b/native/suidhelper/src/tools/network/mod.rs @@ -14,6 +14,7 @@ mod exec; pub mod host_init; pub mod prepare; pub mod teardown; +pub mod teardown_orphan; use crate::config::Network; use clap::Subcommand; @@ -23,6 +24,7 @@ use thiserror::Error as ThisError; pub use host_init::HostInitArgs; pub use prepare::PrepareArgs; pub use teardown::TeardownArgs; +pub use teardown_orphan::TeardownOrphanArgs; /// `[network]` is absent from config, i.e. VM networking is disabled. #[derive(Debug, ThisError, PartialEq, Eq)] @@ -54,6 +56,8 @@ pub enum NetworkOp { Prepare(PrepareArgs), /// Remove a VM's netns and any lingering host-side veth end. Teardown(TeardownArgs), + /// Idempotently remove an orphan VM's netns by vm_id alone (no uid). + TeardownOrphan(TeardownOrphanArgs), /// One-time host setup: the `hyper` nftables table and forward policy. HostInit(HostInitArgs), } @@ -65,6 +69,7 @@ impl NetworkOp { match self { NetworkOp::Prepare(args) => prepare::run(args), NetworkOp::Teardown(args) => teardown::run(args), + NetworkOp::TeardownOrphan(args) => teardown_orphan::run(args), NetworkOp::HostInit(args) => host_init::run(args), } } diff --git a/native/suidhelper/src/tools/network/teardown_orphan.rs b/native/suidhelper/src/tools/network/teardown_orphan.rs new file mode 100644 index 00000000..72f64da6 --- /dev/null +++ b/native/suidhelper/src/tools/network/teardown_orphan.rs @@ -0,0 +1,124 @@ +//! `network teardown-orphan`: reclaim a crashed node's leaked VM netns by +//! vm_id alone. Unlike [`super::teardown`], this takes no `uid` — a crashed +//! node never persisted one for `Hyper.Node.Reaper` to pass back in — so it +//! cannot derive a full [`super::addr::Plan`] and instead runs the one +//! command that needs no derived address or interface name: `ip netns del +//! `, which reclaims the ns-side veth peer, TAP device, and in-netns +//! nftables state with it (see `args::teardown_orphan_commands`). +//! +//! Idempotent by design, not by accident: the reaper races a normal +//! `teardown/2` that may already have deleted the netns (or one that never +//! finished being created), so `ip netns del` failing with "No such file or +//! directory" is treated as success rather than propagated. Any other +//! failure (wrong permissions, `ip` itself unusable) still errors. The real +//! ENOENT path only exists once a genuine `/var/run/netns` entry is deleted +//! out from under a concurrent call, which needs root and a real netns to +//! exercise — covered by the root e2e / reaper e2e suites, not this crate's +//! unit tests. +use super::args::{self, Command}; +use super::NetworkOut; +use crate::config::Config; +use crate::tools::jailer::VmId; +use crate::tools::IsTool; +use crate::util::safe_bin; +use clap::Args; +use std::path::{Path, PathBuf}; +use std::process::Command as Proc; +use thiserror::Error as ThisError; + +#[derive(Args)] +pub struct TeardownOrphanArgs { + /// Microvm id; names the netns to remove. No `uid` — see the module doc. + #[arg(long)] + vm_id: VmId, +} + +#[derive(Debug, ThisError)] +pub enum Error { + #[error(transparent)] + NetworkingDisabled(#[from] super::NetworkingDisabled), + #[error(transparent)] + Bin(#[from] safe_bin::Error), + #[error("running ip {argv:?}: {source}")] + Spawn { + argv: Vec, + #[source] + source: std::io::Error, + }, + #[error("ip {argv:?} failed: {stderr}")] + Failed { argv: Vec, stderr: String }, +} + +pub fn run(args: TeardownOrphanArgs) -> Result { + TeardownOrphan::new(args) + .map_err(|e| crate::tools::Error::Tool(Box::new(e)))? + .run() +} + +struct TeardownOrphan { + netns: String, + ip: PathBuf, +} + +impl TeardownOrphan { + fn new(args: TeardownOrphanArgs) -> Result { + let config = Config::get(); + super::resolve_network(config.network())?; + Ok(Self { + netns: args.vm_id.to_string(), + ip: config.ip()?.into(), + }) + } +} + +impl IsTool for TeardownOrphan { + type Args = TeardownOrphanArgs; + type Output = NetworkOut; + type RunT = Result<(), Error>; + + fn run_privileged(&self) -> Self::RunT { + run_idempotent(&args::teardown_orphan_commands(&self.netns), &self.ip) + } + + fn parse(&self, res: Self::RunT) -> Result> { + res?; + Ok(NetworkOut::ToreDown) + } +} + +/// Run each command against `ip`, tolerating an ENOENT-shaped failure (the +/// namespace is already gone) as success. Kept separate from +/// `super::exec::run_all`, whose contract for `prepare`/`teardown` is that +/// any non-`allow_failure` non-zero exit is a real error — this op's whole +/// point is that a missing netns is *not* an error. +fn run_idempotent(commands: &[Command], ip: &Path) -> Result<(), Error> { + for command in commands { + let output = Proc::new(ip) + .args(&command.argv) + .env_clear() + .output() + .map_err(|source| Error::Spawn { + argv: command.argv.clone(), + source, + })?; + + if output.status.success() { + continue; + } + + let stderr = String::from_utf8_lossy(&output.stderr).trim().to_string(); + if is_enoent(&stderr) { + continue; + } + + return Err(Error::Failed { + argv: command.argv.clone(), + stderr, + }); + } + Ok(()) +} + +fn is_enoent(stderr: &str) -> bool { + stderr.to_lowercase().contains("no such file or directory") +} diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index 13cc8597..acea5a5d 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -45,6 +45,21 @@ fn teardown_deletes_netns() { .any(|c| c.argv == vec!["netns", "del", "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"])); } +#[test] +fn teardown_orphan_is_exactly_ip_netns_del_by_id_alone() { + // No uid, no veth cleanup: an orphan's teardown has only the vm_id to work + // from (see `teardown_orphan`'s module doc), and `ip netns del` alone is + // sufficient to reclaim the veth peer, TAP, and in-netns nftables state. + let cmds = args::teardown_orphan_commands("vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"); + assert_eq!( + cmds, + vec![cmd( + Which::Ip, + &["netns", "del", "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"] + )] + ); +} + #[test] fn host_init_masquerades_pool_out_uplink() { let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); diff --git a/test/hyper/node/reaper/liveness_test.exs b/test/hyper/node/reaper/liveness_test.exs index e9068dc6..13c250b9 100644 --- a/test/hyper/node/reaper/liveness_test.exs +++ b/test/hyper/node/reaper/liveness_test.exs @@ -1,5 +1,5 @@ defmodule Hyper.Node.Reaper.LivenessTest do - # Guards the Img.Mutable.active_vm_ids/0 + Plan.orphans/3 contract: a live mutable + # Guards the Img.Mutable.active_vm_ids/0 + Plan.orphans/4 contract: a live mutable # layer protects its vm_id from reaping. The gather_live/0 union of this source into # the full live set is verified by manual live-node testing (Task 4), not CI. use ExUnit.Case, async: false @@ -38,6 +38,14 @@ defmodule Hyper.Node.Reaper.LivenessTest do # The reaper would see hyper-rw-vm-live in `dmsetup ls` (rw candidate) and no # cgroup leaf, yet the live mutable owner must protect it from reaping. - assert Plan.orphans(live, [], ["vm-live"]) == MapSet.new([]) + assert Plan.orphans(live, [], ["vm-live"], []) == MapSet.new([]) + end + + test "a vm with a live mutable layer is never an orphan, even with a leftover netns" do + register_live("vm-live-netns") + + live = MapSet.new(Mutable.active_vm_ids()) + + assert Plan.orphans(live, [], [], ["vm-live-netns"]) == MapSet.new([]) end end diff --git a/test/hyper/node/reaper/plan_properties_test.exs b/test/hyper/node/reaper/plan_properties_test.exs index 6720a608..35514b2b 100644 --- a/test/hyper/node/reaper/plan_properties_test.exs +++ b/test/hyper/node/reaper/plan_properties_test.exs @@ -29,13 +29,25 @@ defmodule Hyper.Node.Reaper.PlanPropertiesTest do check all( live <- id_set(), leaves <- id_list(), - rw <- id_list() + rw <- id_list(), + netns <- id_list() ) do - orphans = Plan.orphans(live, leaves, rw) + orphans = Plan.orphans(live, leaves, rw, netns) assert MapSet.disjoint?(orphans, live) end end + property "every orphan netns name not in live is a candidate, and none from live are" do + check all( + live <- id_set(), + netns <- id_list() + ) do + orphans = Plan.orphans(live, [], [], netns) + expected = MapSet.difference(MapSet.new(netns), live) + assert orphans == expected + end + end + property "only twice-seen orphans are reaped; current is carried forward" do check all( current <- id_set(), diff --git a/test/hyper/node/reaper/plan_test.exs b/test/hyper/node/reaper/plan_test.exs index 0021db98..b1e57ac9 100644 --- a/test/hyper/node/reaper/plan_test.exs +++ b/test/hyper/node/reaper/plan_test.exs @@ -5,27 +5,35 @@ defmodule Hyper.Node.Reaper.PlanTest do defp set(ids), do: MapSet.new(ids) - describe "orphans/3" do + describe "orphans/4" do test "a cgroup-leaf-only orphan is a candidate" do - assert Plan.orphans(set([]), ["dead"], []) == set(["dead"]) + assert Plan.orphans(set([]), ["dead"], [], []) == set(["dead"]) end test "a dm-only orphan is a candidate" do - assert Plan.orphans(set([]), [], ["dead"]) == set(["dead"]) + assert Plan.orphans(set([]), [], ["dead"], []) == set(["dead"]) end test "an id seen in both sources is a single candidate" do - assert Plan.orphans(set([]), ["dead"], ["dead"]) == set(["dead"]) + assert Plan.orphans(set([]), ["dead"], ["dead"], []) == set(["dead"]) end test "an id present in live is never a candidate, even if it also has resources" do - assert Plan.orphans(set(["alive"]), ["alive"], ["alive"]) == set([]) + assert Plan.orphans(set(["alive"]), ["alive"], ["alive"], ["alive"]) == set([]) end test "only the non-live ids survive as candidates" do - assert Plan.orphans(set(["alive"]), ["alive", "dead"], ["alive", "gone"]) == + assert Plan.orphans(set(["alive"]), ["alive", "dead"], ["alive", "gone"], []) == set(["dead", "gone"]) end + + test "a netns-only orphan is a candidate" do + assert Plan.orphans(set([]), [], [], ["dead-netns"]) == set(["dead-netns"]) + end + + test "a netns name whose vm_id is live is never a candidate" do + assert Plan.orphans(set(["alive"]), [], [], ["alive"]) == set([]) + end end describe "confirm/2 two-strike grace" do diff --git a/test/hyper/suid_helper/network_test.exs b/test/hyper/suid_helper/network_test.exs index 7e16bd4a..0b406814 100644 --- a/test/hyper/suid_helper/network_test.exs +++ b/test/hyper/suid_helper/network_test.exs @@ -11,4 +11,9 @@ defmodule Hyper.SuidHelper.NetworkTest do assert Network.argv(:teardown, "vabc", 900_100) == ["network", "teardown", "--vm-id", "vabc", "--uid", "900100"] end + + test "teardown_orphan/1 builds the network teardown-orphan argv, with no uid" do + assert Network.argv(:teardown_orphan, "vabc") == + ["network", "teardown-orphan", "--vm-id", "vabc"] + end end From b970758e4c01b78d29a9cea0e7c203293ee279d2 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:16:51 +0000 Subject: [PATCH 17/42] feat(net): guest DNS via hyper.resolver= cmdline written by PID-1 agent 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= 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. --- lib/hyper/cfg/network.ex | 10 +++++ lib/hyper/node/fire_vmm/boot_spec.ex | 5 ++- lib/hyper/node/fire_vmm/net.ex | 9 +++++ native/guest-agent/Cargo.toml | 4 ++ native/guest-agent/src/init.rs | 28 ++++++++++++++ native/guest-agent/tests/init.rs | 41 +++++++++++++++++++++ test/hyper/cfg/network_test.exs | 6 +++ test/hyper/node/fire_vmm/boot_spec_test.exs | 14 ++++--- test/hyper/node/fire_vmm/net_test.exs | 5 +++ 9 files changed, 115 insertions(+), 7 deletions(-) create mode 100644 native/guest-agent/tests/init.rs diff --git a/lib/hyper/cfg/network.ex b/lib/hyper/cfg/network.ex index 2ec2b6bd..8c8ee04e 100644 --- a/lib/hyper/cfg/network.ex +++ b/lib/hyper/cfg/network.ex @@ -9,6 +9,7 @@ defmodule Hyper.Cfg.Network do import Hyper.Cfg, only: [get_cfg: 1] @default_clone_pool "172.31.0.0/16" + @default_resolver "1.1.1.1" @doc "Whether VM egress networking is turned on (`[network] uplink` present)." @spec enabled?() :: boolean() @@ -27,4 +28,13 @@ defmodule Hyper.Cfg.Network do @doc "IPv4 CIDR the per-VM clone /30s are carved from. `[network] clone_pool`." @spec clone_pool() :: String.t() def clone_pool, do: get_cfg(toml: "network.clone_pool", default: @default_clone_pool) + + @doc """ + DNS resolver IP handed to the guest via the `hyper.resolver=` kernel cmdline + param, since the kernel's `ip=` autoconfig sets the guest's address/route but + never DNS. The PID-1 guest agent reads this back out of `/proc/cmdline` and + writes it to `/etc/resolv.conf`. `[network] resolver`. + """ + @spec resolver() :: String.t() + def resolver, do: get_cfg(toml: "network.resolver", default: @default_resolver) end diff --git a/lib/hyper/node/fire_vmm/boot_spec.ex b/lib/hyper/node/fire_vmm/boot_spec.ex index 66805431..53b4cb09 100644 --- a/lib/hyper/node/fire_vmm/boot_spec.ex +++ b/lib/hyper/node/fire_vmm/boot_spec.ex @@ -39,7 +39,10 @@ defmodule Hyper.Node.FireVMM.BootSpec do vm_id = Map.fetch!(source, :vm_id) {[Hyper.Node.FireVMM.Net.interface(vm_id)], - base_args <> " " <> Hyper.Node.FireVMM.Net.ip_cmdline()} + base_args <> + " " <> + Hyper.Node.FireVMM.Net.ip_cmdline() <> + " " <> Hyper.Node.FireVMM.Net.resolver_cmdline(Hyper.Cfg.Network.resolver())} else {[], base_args} end diff --git a/lib/hyper/node/fire_vmm/net.ex b/lib/hyper/node/fire_vmm/net.ex index 456237f0..a9dbad92 100644 --- a/lib/hyper/node/fire_vmm/net.ex +++ b/lib/hyper/node/fire_vmm/net.ex @@ -19,6 +19,15 @@ defmodule Hyper.Node.FireVMM.Net do @spec ip_cmdline() :: String.t() def ip_cmdline, do: "ip=#{@guest_ip}::#{@tap_ip}:#{@netmask}::#{@iface}:off" + @doc """ + Kernel `hyper.resolver=` cmdline fragment carrying the DNS resolver IP the + PID-1 guest agent writes to `/etc/resolv.conf` — kernel `ip=` autoconfig sets + the guest's address/route but never DNS, so this closes that gap. Pure: + callers (`BootSpec.resolve/2`) read the configured resolver themselves. + """ + @spec resolver_cmdline(String.t()) :: String.t() + def resolver_cmdline(resolver), do: "hyper.resolver=#{resolver}" + @doc """ Stable locally-administered unicast MAC for `vm_id`. Derived from a SHA-256 of the id so it is deterministic on any node (aids debugging); duplicate MACs are diff --git a/native/guest-agent/Cargo.toml b/native/guest-agent/Cargo.toml index 353b6907..5392bb3b 100644 --- a/native/guest-agent/Cargo.toml +++ b/native/guest-agent/Cargo.toml @@ -31,6 +31,10 @@ tokio-stream = "0.1" name = "exec" path = "tests/exec.rs" +[[test]] +name = "init" +path = "tests/init.rs" + [profile.release] opt-level = "z" lto = true diff --git a/native/guest-agent/src/init.rs b/native/guest-agent/src/init.rs index ae3b524b..7de75a1f 100644 --- a/native/guest-agent/src/init.rs +++ b/native/guest-agent/src/init.rs @@ -1,7 +1,10 @@ use std::io; +use std::net::Ipv4Addr; use nix::mount::{mount, MsFlags}; +const RESOLVER_PARAM: &str = "hyper.resolver="; + // As PID 1 nothing else mounts these; exec'd programs need /proc and /dev. pub fn setup() -> io::Result<()> { let none: Option<&str> = None; @@ -16,5 +19,30 @@ pub fn setup() -> io::Result<()> { // must not abort the agent; a missing /proc only degrades exec'd tools. let _ = mount(Some(src), target, Some(fstype), MsFlags::empty(), none); } + // Best-effort, run after /proc is mounted: the kernel's `ip=` autoconfig + // sets the guest's address/route but never DNS, so BootSpec appends + // `hyper.resolver=` to the cmdline for us to relay into resolv.conf. A + // missing param or unwritable /etc only degrades DNS, never boot. + let _ = write_resolv_conf(); Ok(()) } + +/// Extracts the value of the `hyper.resolver=` token from a `/proc/cmdline`-style +/// space-separated string. `None` if the param is absent, has an empty value, +/// or the value isn't a valid IPv4 address. Where the same param repeats, the +/// first occurrence wins (matches kernel cmdline parsing conventions). +pub fn resolver_from_cmdline(cmdline: &str) -> Option { + cmdline + .split_whitespace() + .find_map(|token| token.strip_prefix(RESOLVER_PARAM)) + .filter(|value| !value.is_empty()) + .and_then(|value| value.parse().ok()) +} + +fn write_resolv_conf() -> io::Result<()> { + let cmdline = std::fs::read_to_string("/proc/cmdline")?; + let Some(resolver) = resolver_from_cmdline(&cmdline) else { + return Ok(()); + }; + std::fs::write("/etc/resolv.conf", format!("nameserver {resolver}\n")) +} diff --git a/native/guest-agent/tests/init.rs b/native/guest-agent/tests/init.rs new file mode 100644 index 00000000..6c1be0b0 --- /dev/null +++ b/native/guest-agent/tests/init.rs @@ -0,0 +1,41 @@ +use hyper_guest_agent::init::resolver_from_cmdline; +use std::net::Ipv4Addr; + +#[test] +fn present_among_other_params() { + let cmdline = "reboot=k panic=1 hyper.resolver=1.1.1.1 ip=172.30.0.2::172.30.0.1:255.255.255.252::eth0:off"; + assert_eq!( + resolver_from_cmdline(cmdline), + Some(Ipv4Addr::new(1, 1, 1, 1)) + ); +} + +#[test] +fn absent_is_none() { + let cmdline = "reboot=k panic=1 ip=172.30.0.2::172.30.0.1:255.255.255.252::eth0:off"; + assert_eq!(resolver_from_cmdline(cmdline), None); +} + +#[test] +fn empty_cmdline_is_none() { + assert_eq!(resolver_from_cmdline(""), None); +} + +#[test] +fn empty_value_is_none() { + assert_eq!(resolver_from_cmdline("hyper.resolver= ip=foo"), None); +} + +#[test] +fn non_ipv4_value_is_none() { + assert_eq!(resolver_from_cmdline("hyper.resolver=not-an-ip"), None); +} + +#[test] +fn first_of_multiple_tokens_wins() { + let cmdline = "hyper.resolver=1.1.1.1 hyper.resolver=8.8.8.8"; + assert_eq!( + resolver_from_cmdline(cmdline), + Some(Ipv4Addr::new(1, 1, 1, 1)) + ); +} diff --git a/test/hyper/cfg/network_test.exs b/test/hyper/cfg/network_test.exs index d15de851..a0429c3b 100644 --- a/test/hyper/cfg/network_test.exs +++ b/test/hyper/cfg/network_test.exs @@ -11,6 +11,12 @@ defmodule Hyper.Cfg.NetworkTest do end end + describe "resolver/0" do + test "defaults when unset" do + assert Network.resolver() == "1.1.1.1" + end + end + describe "enabled?/0" do test "false when no uplink configured" do # Base test config sets no [network] table. diff --git a/test/hyper/node/fire_vmm/boot_spec_test.exs b/test/hyper/node/fire_vmm/boot_spec_test.exs index 463dd7e9..ffa875c0 100644 --- a/test/hyper/node/fire_vmm/boot_spec_test.exs +++ b/test/hyper/node/fire_vmm/boot_spec_test.exs @@ -3,11 +3,12 @@ defmodule Hyper.Node.FireVMM.BootSpecTest do alias Hyper.Node.FireVMM.BootSpec - # The enabled path (NIC populated + `ip=` appended) needs a `[network]` config - # override, which the unit config harness doesn't provide; it's asserted in - # the e2e/integration suite instead. This module only pins the disabled path - # (the base test config has no `[network]` table), which must hold for every - # existing VM. + # The enabled path (NIC populated + `ip=`/`hyper.resolver=` appended) needs a + # `[network]` config override, which the unit config harness doesn't provide; + # it's asserted in the e2e/integration suite instead. The pure formatting of + # the resolver fragment is pinned in net_test.exs. This module only pins the + # disabled path (the base test config has no `[network]` table), which must + # hold for every existing VM. test "default cmdline boots our agent as init with no serial console" do source = %{kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} @@ -18,10 +19,11 @@ defmodule Hyper.Node.FireVMM.BootSpecTest do refute cold.boot_source.boot_args =~ "console=" end - test "no NIC and no ip= when networking disabled" do + test "no NIC and no ip=/hyper.resolver= when networking disabled" do source = %{vm_id: "vabc", kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} cold = BootSpec.resolve(source, :micro) assert cold.network_interfaces == [] refute cold.boot_source.boot_args =~ "ip=" + refute cold.boot_source.boot_args =~ "hyper.resolver=" end end diff --git a/test/hyper/node/fire_vmm/net_test.exs b/test/hyper/node/fire_vmm/net_test.exs index a48e97de..e2917565 100644 --- a/test/hyper/node/fire_vmm/net_test.exs +++ b/test/hyper/node/fire_vmm/net_test.exs @@ -20,6 +20,11 @@ defmodule Hyper.Node.FireVMM.NetTest do assert Net.ip_cmdline() == "ip=172.30.0.2::172.30.0.1:255.255.255.252::eth0:off" end + test "resolver_cmdline formats the hyper.resolver= fragment for the given resolver" do + assert Net.resolver_cmdline("1.1.1.1") == "hyper.resolver=1.1.1.1" + assert Net.resolver_cmdline("10.0.0.53") == "hyper.resolver=10.0.0.53" + end + test "interface targets tap0/eth0 with the derived MAC" do nic = Net.interface("vabcdef") assert nic.iface_id == "eth0" From 3ae89d41a62ccbd3ae8f403e7b00d4ad20a306d2 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:20:08 +0000 Subject: [PATCH 18/42] test(net): pin guest resolver parser rejects look-alike cmdline tokens MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/guest-agent/tests/init.rs | 24 ++++++++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/native/guest-agent/tests/init.rs b/native/guest-agent/tests/init.rs index 6c1be0b0..98014376 100644 --- a/native/guest-agent/tests/init.rs +++ b/native/guest-agent/tests/init.rs @@ -39,3 +39,27 @@ fn first_of_multiple_tokens_wins() { Some(Ipv4Addr::new(1, 1, 1, 1)) ); } + +#[test] +fn first_of_multiple_tokens_wins_among_other_params() { + let cmdline = "a hyper.resolver=1.1.1.1 b hyper.resolver=8.8.8.8 c"; + assert_eq!( + resolver_from_cmdline(cmdline), + Some(Ipv4Addr::new(1, 1, 1, 1)) + ); +} + +// Regression: split_whitespace() + strip_prefix() requires the param to be its +// own whitespace-delimited token, not merely a substring anywhere on the +// cmdline. A future refactor to `cmdline.contains("hyper.resolver=")` would +// match this look-alike token and could point the guest at an attacker +// resolver embedded in an unrelated kernel param — that must never happen. +#[test] +fn prefixed_look_alike_token_is_not_matched() { + assert_eq!(resolver_from_cmdline("xhyper.resolver=9.9.9.9"), None); +} + +#[test] +fn suffixed_look_alike_key_is_not_matched() { + assert_eq!(resolver_from_cmdline("hyper.resolverX=1.1.1.1"), None); +} From 03f03e8bb0bc06dc445874e617f00030df532a8b Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:23:01 +0000 Subject: [PATCH 19/42] docs: host bootstrap for VM egress networking (ip_forward, nft, [network]) --- .github/scripts/provision-kvm-host.sh | 23 +++++++++++-- docs/cookbook/install.md | 47 +++++++++++++++++++++++++++ 2 files changed, 68 insertions(+), 2 deletions(-) diff --git a/.github/scripts/provision-kvm-host.sh b/.github/scripts/provision-kvm-host.sh index 74180026..91eadd0e 100755 --- a/.github/scripts/provision-kvm-host.sh +++ b/.github/scripts/provision-kvm-host.sh @@ -17,6 +17,9 @@ # the node) # - DEBIAN_FRONTEND=noninteractive (install.md is written for an interactive # operator session; CI has no tty to prompt on) +# - [network] is always written here (install.md presents it as an optional +# section) since the runner's default route interface is detected fresh +# each run and integration coverage wants egress networking exercised set -euo pipefail export DEBIAN_FRONTEND=noninteractive @@ -30,7 +33,7 @@ sudo udevadm trigger --name-match=kvm sudo apt-get update sudo apt-get install -y \ - coreutils e2fsprogs libc-bin lvm2 skopeo util-linux \ + coreutils e2fsprogs iproute2 libc-bin lvm2 nftables skopeo util-linux \ "linux-modules-extra-$(uname -r)" # -a is load-bearing: without it modprobe reads the 2nd+ names as module @@ -40,8 +43,19 @@ targets="$(sudo dmsetup targets)" echo "dmsetup targets: ${targets}" grep -q thin-pool <<<"${targets}" || { echo "ERROR: thin-pool dm target missing" >&2; exit 1; } +# host-init (run by the node at start) asserts ip_forward=1 rather than +# setting it, so provisioning must turn it on and persist it across reboots. +sudo sysctl -w net.ipv4.ip_forward=1 +echo 'net.ipv4.ip_forward=1' \ + | sudo tee /etc/sysctl.d/99-hyper-ip-forward.conf >/dev/null + +# The default-route interface is the uplink guests NAT egress through. +uplink="$(ip route show default | awk '{print $5; exit}')" +[ -n "${uplink}" ] || { echo "ERROR: could not detect default-route uplink interface" >&2; exit 1; } +echo "detected uplink: ${uplink}" + sudo mkdir -p /etc/hyper -sudo tee /etc/hyper/config.toml >/dev/null <<'EOF' +sudo tee /etc/hyper/config.toml >/dev/null < #### Persistent Config {: .warning} +> +> Loading this via `sysctl -w` is ephemeral and resets on reboot. Persist it: +> +> ```sh +> echo 'net.ipv4.ip_forward=1' \ +> | sudo tee /etc/sysctl.d/99-hyper-ip-forward.conf +> ``` + +Then set `uplink` to the physical NIC guests should NAT out through — the +default-route interface is a reasonable choice on a single-uplink host: + +```sh +ip route show default | awk '{print $5; exit}' +``` + +```toml +[network] +# **required**. The physical uplink interface guests NAT egress through. +uplink = "eth0" + +# optional, defaults shown +clone_pool = "172.31.0.0/16" +resolver = "1.1.1.1" +``` + ### Cgroups Hyper uses cgroups to impose limits on each VM. Each VM has its own cgroup, From 0f4e06b2cb61c6a1be88d6014708edb22fcadf66 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 03:27:43 +0000 Subject: [PATCH 20/42] =?UTF-8?q?test(net):=20live=20E2E=20=E2=80=94=20net?= =?UTF-8?q?worked=20guest=20reaches=20the=20internet?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- test/e2e/network_test.exs | 54 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 54 insertions(+) create mode 100644 test/e2e/network_test.exs diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs new file mode 100644 index 00000000..2aadd1bf --- /dev/null +++ b/test/e2e/network_test.exs @@ -0,0 +1,54 @@ +defmodule Hyper.E2e.NetworkTest do + @moduledoc """ + Live end-to-end contract of VM egress networking on a provisioned host: a + booted guest's `eth0` carries the uniform inner-world address + (`172.30.0.2`), and the netns+veth+tap+NAT path it rides on actually + reaches the internet — DNS resolves and an HTTP fetch completes. + + Self-skips (does not fail) when `[network]` is absent from `config.toml` + (`Hyper.Cfg.Network.enabled?/0` false): a host that never provisioned + networking has no uplink/clone_pool to test against, and this suite must + not break integration runs on such a host. CI's `integration` job + provisions `[network]` (see `.github/scripts/provision-kvm-host.sh`), so + the real assertions run there. + + Runs only under `--only integration` / `--include integration` on a host + provisioned per docs/cookbook/install.md. + """ + use ExUnit.Case, async: false + + import Hyper.E2e + + @moduletag :integration + @moduletag timeout: :timer.minutes(10) + + # public.ecr.aws mirrors library images without Docker Hub's per-IP pull + # limits, which shared GHA egress IPs routinely exhaust. + @image System.get_env("HYPER_E2E_IMAGE", "public.ecr.aws/docker/library/alpine:3.19") + + test "a booted guest can reach the internet over its NIC" do + if Hyper.Cfg.Network.enabled?() do + assert {:ok, img_id} = Hyper.Img.OciLoader.load(@image) + + # :micro, not the :base default — :base asks for 32 GiB of disk budget, + # which the default node budget (4 GiB) refuses with :no_capacity on a + # small CI runner. + assert {:ok, vm} = Hyper.create_vm(%Hyper.Vm.Spec{img_id: img_id, type: :micro}) + on_exit(fn -> Hyper.Node.stop_image_vm(vm) end) + + assert {:ok, %{stdout: ip_out, exit_code: 0}} = + await_exec(vm, ["ip", "-4", "addr", "show", "eth0"]) + + assert ip_out =~ "172.30.0.2" + + assert {:ok, %{exit_code: 0}} = + await_exec( + vm, + ["sh", "-c", "wget -qO- http://example.com >/dev/null"], + :timer.seconds(90) + ) + else + IO.puts("SKIP: [network] not enabled on this host — see Hyper.Cfg.Network.enabled?/0") + end + end +end From 7ea8621036a4f71700288c90d48277f23acced3e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 06:10:36 +0000 Subject: [PATCH 21/42] =?UTF-8?q?fix(net):=20drop=20guest=E2=86=92host=20t?= =?UTF-8?q?raffic=20(INPUT=20isolation)=20+=20partial=20[network]=20table?= =?UTF-8?q?=20disables=20not=20bricks?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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; Config::network() returns None when it's unset, matching the Elixir side's enabled?/0 exactly. --- native/suidhelper/src/config.rs | 36 +++++++++++--- native/suidhelper/src/tools/network/args.rs | 37 +++++++++++--- .../suidhelper/src/tools/network/host_init.rs | 8 +++- native/suidhelper/tests/config_network.rs | 13 +++++ native/suidhelper/tests/tools/network_args.rs | 48 +++++++++++++++++-- 5 files changed, 124 insertions(+), 18 deletions(-) diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index 73443096..93a1bc58 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -204,14 +204,21 @@ impl Default for Config { } } -/// The `[network]` table: VM egress networking. Absent ⇒ networking disabled. -/// `uplink` is the physical interface guest traffic is masqueraded out of; -/// `clone_pool` is the IPv4 CIDR per-VM /30 clone addresses are carved from -/// (default `172.31.0.0/16`). Both are read identically by the Elixir node +/// The `[network]` table: VM egress networking. Absent, or present without +/// `uplink`, ⇒ networking disabled — see [`Config::network`]. `uplink` is the +/// physical interface guest traffic is masqueraded out of; `clone_pool` is the +/// IPv4 CIDR per-VM /30 clone addresses are carved from (default +/// `172.31.0.0/16`). Both are read identically by the Elixir node /// (`Hyper.Cfg.Network`) — keep the defaults in sync. +/// +/// `uplink` is `Option` rather than required so that a `[network]` table +/// present with only `clone_pool` set (`uplink` omitted) parses as +/// networking-disabled rather than failing to deserialize the whole +/// [`Config`] — see [`Config::network`]'s doc for why that asymmetry would be +/// dangerous. #[derive(Debug, Clone, Deserialize)] pub struct Network { - pub uplink: String, + pub uplink: Option, #[serde(default = "default_clone_pool")] pub clone_pool: String, } @@ -273,9 +280,24 @@ impl Config { SafeBin::from_path(&self.tools.nft) } - /// The `[network]` table, or `None` when VM networking is disabled. + /// The `[network]` table, or `None` when VM networking is disabled — either + /// because `[network]` is absent entirely, or because it is present but + /// `uplink` is not set. + /// + /// The latter case matters on its own: `Config` is a process-wide + /// [`LazyLock`] that exits the whole helper on any parse failure (every + /// subcommand — dmsetup, losetup, chroot-jail, jailer, not just + /// networking — calls [`Config::get`] first), so if `uplink` were a + /// required field, an operator who sets `clone_pool` before `uplink` (or + /// simply omits `uplink` to disable networking, mirroring the Elixir + /// node's `Hyper.Cfg.Network.enabled?/0`, which treats absent `uplink` as + /// disabled rather than an error) would brick every tool the helper + /// offers, not just networking. Filtering here — rather than making + /// `uplink` required — keeps this side symmetric with Elixir's. pub fn network(&self) -> Option<&Network> { - self.network.as_ref() + self.network + .as_ref() + .filter(|network| network.uplink.is_some()) } /// The Firecracker VMM binary, validated as root-owned and correctly named. diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index c3f03ac7..ee55d8f0 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -12,9 +12,11 @@ //! route target must already be reachable when the route is added. //! - `teardown_commands` always deletes the netns, which reclaims the ns-side //! veth peer, the TAP device, and any in-netns nftables state. -//! - `host_init_commands` masquerades the clone pool out the configured uplink -//! and drops guest traffic addressed to the cloud metadata IP -//! (`169.254.169.254`), so no VM can ever reach it. +//! - `host_init_commands` masquerades the clone pool out the configured uplink, +//! drops guest traffic addressed to the cloud metadata IP +//! (`169.254.169.254`), and drops all clone-pool-sourced traffic addressed to +//! the host itself on the INPUT hook (distinct from FORWARD — see that +//! function's doc), so no VM can ever reach it or a host-local service. use super::addr::{self, Plan}; /// Which privileged binary a [`Command`] invokes. Kept to exactly two — `ip` @@ -252,9 +254,24 @@ pub fn teardown_orphan_commands(netns: &str) -> Vec { } /// One-time host setup: a `hyper` nftables table that masquerades the clone -/// pool out `uplink`, and a forward-chain default-drop policy that only admits +/// pool out `uplink`, a forward-chain default-drop policy that only admits /// the pool's own traffic — explicitly dropping anything addressed to the -/// cloud metadata IP (`169.254.169.254`), so no VM can ever reach it. +/// cloud metadata IP (`169.254.169.254`), so no VM can ever reach it — and an +/// input-chain rule that drops all clone-pool-sourced traffic addressed to +/// the host itself. +/// +/// The input-chain drop closes a distinct hook from the forward-chain policy: +/// a guest packet addressed to a *host-owned* IP (the guest's own gateway +/// address, or any other address the host answers for) is locally delivered +/// and hits the kernel's INPUT hook, never FORWARD — so without this rule a +/// guest could reach host-local services (epmd, distribution, Postgres, the +/// gRPC listener) regardless of the forward-chain policy above. The input +/// chain's own policy stays `accept` so unrelated host traffic (SSH, other +/// subnets) is untouched; only clone-pool-sourced packets are dropped. This +/// does not affect guest egress (which is FORWARDed, never delivered to +/// INPUT) or ARP (an L2 protocol the `ip` family's INPUT hook never sees), and +/// the guest's DNS queries are forwarded to an external resolver, not +/// terminated on the host, so they are unaffected too. /// /// Reconciles to desired state rather than diffing: the first command /// unconditionally deletes the `hyper` table (tolerating "no such file" when @@ -262,7 +279,8 @@ pub fn teardown_orphan_commands(netns: &str) -> Vec { /// scratch. This makes host-init idempotent by construction — the table is /// always either absent or complete, never partially applied — so a /// crash/interruption mid-sequence on one run can never strand a later run -/// with, say, NAT but no forward-chain drop policy. +/// with, say, NAT but no forward-chain drop policy (or no input-chain +/// isolation). pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { vec![ Command::nft_allow_failure(argv!["delete", "table", "ip", "hyper"]), @@ -336,5 +354,12 @@ pub fn host_init_commands(uplink: &str, clone_pool: &str) -> Vec { "established,related", "accept" ]), + Command::nft(argv![ + "add", "chain", "ip", "hyper", "input", "{", "type", "filter", "hook", "input", + "priority", "0", ";", "policy", "accept", ";", "}" + ]), + Command::nft(argv![ + "add", "rule", "ip", "hyper", "input", "ip", "saddr", clone_pool, "drop" + ]), ] } diff --git a/native/suidhelper/src/tools/network/host_init.rs b/native/suidhelper/src/tools/network/host_init.rs index 4bdeec88..85cc79dc 100644 --- a/native/suidhelper/src/tools/network/host_init.rs +++ b/native/suidhelper/src/tools/network/host_init.rs @@ -69,7 +69,13 @@ impl HostInit { let config = Config::get(); let network = super::resolve_network(config.network())?; Ok(Self { - uplink: network.uplink.clone(), + // `Config::network()` only ever returns `Some` when `uplink` is + // set (see its doc), so `resolve_network`'s `Ok` carries that same + // guarantee. + uplink: network + .uplink + .clone() + .expect("Config::network() guarantees uplink is set"), clone_pool: network.clone_pool.clone(), nft: config.nft()?.into(), }) diff --git a/native/suidhelper/tests/config_network.rs b/native/suidhelper/tests/config_network.rs index 54988f8a..9e2cf7e5 100644 --- a/native/suidhelper/tests/config_network.rs +++ b/native/suidhelper/tests/config_network.rs @@ -15,3 +15,16 @@ fn present_table_with_only_uplink_defaults_clone_pool() { let network: Network = toml::from_str("uplink = \"eth0\"\n").expect("valid TOML"); assert_eq!(network.clone_pool, "172.31.0.0/16"); } + +#[test] +fn partial_network_table_without_uplink_disables_not_bricks() { + // A `[network]` table with `clone_pool` set but `uplink` omitted must + // parse the WHOLE config successfully (not fail the process-wide, + // exit-on-error `Config` load that every subcommand — dmsetup, losetup, + // chroot-jail, jailer, not just networking — depends on) and must resolve + // to networking-disabled, mirroring Elixir's Hyper.Cfg.Network.enabled?/0 + // (uplink absent ⇒ disabled, not an error). + let toml_str = "work_dir = \"/srv/hyper\"\n[network]\nclone_pool = \"10.0.0.0/8\"\n"; + let config: Config = toml::from_str(toml_str).expect("a partial [network] table must parse"); + assert!(config.network().is_none()); +} diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index acea5a5d..8c577c6a 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -72,6 +72,23 @@ fn host_init_masquerades_pool_out_uplink() { .any(|c| c.argv.contains(&"169.254.169.254".to_string()))); } +#[test] +fn host_init_drops_guest_traffic_to_host() { + // The INPUT hook is a distinct chain from FORWARD: a guest packet + // addressed to a host-owned IP (its own gateway, or any other host IP) is + // locally delivered, never forwarded, so without an explicit input-chain + // drop a guest could reach host-local services (epmd, distribution, + // Postgres, gRPC) regardless of the forward-chain policy. Pinned as its + // own assertion so a future edit to the forward chain can't silently drop + // this isolation. + let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); + assert!(cmds.iter().any(|c| c.bin == Which::Nft + && c.argv.contains(&"input".to_string()) + && c.argv.contains(&"saddr".to_string()) + && c.argv.contains(&"172.31.0.0/16".to_string()) + && c.argv.contains(&"drop".to_string()))); +} + fn cmd(bin: Which, argv: &[&str]) -> args::Command { args::Command { bin, @@ -223,8 +240,10 @@ fn prepare_commands_golden_sequence() { /// step — see `host_init.rs`'s module doc), the metadata-IP drop rule /// immediately after the forward chain is created and before the broad /// egress accept (since `accept` is a terminating verdict in nftables and a -/// drop reachable only after it would never fire), and that every command -/// after the delete is *not* failure-tolerant. +/// drop reachable only after it would never fire), the trailing input-chain +/// isolation (drops all clone-pool-sourced traffic addressed to the host +/// itself, on the INPUT hook FORWARD never sees — see `host_init_commands`'s +/// doc), and that every command after the delete is *not* failure-tolerant. #[test] fn host_init_commands_golden_sequence() { let cmds = args::host_init_commands("eth0", "172.31.0.0/16"); @@ -323,6 +342,27 @@ fn host_init_commands_golden_sequence() { "accept", ], ), + cmd( + Which::Nft, + &[ + "add", "chain", "ip", "hyper", "input", "{", "type", "filter", "hook", "input", + "priority", "0", ";", "policy", "accept", ";", "}", + ], + ), + cmd( + Which::Nft, + &[ + "add", + "rule", + "ip", + "hyper", + "input", + "ip", + "saddr", + "172.31.0.0/16", + "drop", + ], + ), ]; assert_eq!(cmds, expected); } @@ -371,10 +411,10 @@ fn resolve_network_refuses_when_networking_disabled() { #[test] fn resolve_network_returns_the_configured_network_when_present() { let network = Network { - uplink: "eth0".to_string(), + uplink: Some("eth0".to_string()), clone_pool: "172.31.0.0/16".to_string(), }; let resolved = resolve_network(Some(&network)).unwrap(); - assert_eq!(resolved.uplink, "eth0"); + assert_eq!(resolved.uplink.as_deref(), Some("eth0")); assert_eq!(resolved.clone_pool, "172.31.0.0/16"); } From 709b458041ede4c0fc27242bc1acdf49cd187971 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:10:19 +0000 Subject: [PATCH 22/42] refactor(suidhelper): inline trivial tool-path defaults into Tools::default MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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). --- native/suidhelper/src/config.rs | 36 ++++++--------------------------- 1 file changed, 6 insertions(+), 30 deletions(-) diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index ae5e4aa7..c8bdcfdb 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -152,12 +152,12 @@ pub struct Tools { impl Default for Tools { fn default() -> Self { Self { - dmsetup: default_dmsetup(), - losetup: default_losetup(), - blockdev: default_blockdev(), - ip: default_ip(), - nft: default_nft(), - thin_dump: default_thin_dump(), + dmsetup: "/usr/sbin/dmsetup".into(), + losetup: "/usr/sbin/losetup".into(), + blockdev: "/usr/sbin/blockdev".into(), + ip: "/usr/sbin/ip".into(), + nft: "/usr/sbin/nft".into(), + thin_dump: "/usr/sbin/thin_dump".into(), firecracker: None, jailer: None, } @@ -171,30 +171,6 @@ fn default_work_dir() -> PathBuf { PathBuf::from("/srv/hyper") } -fn default_dmsetup() -> PathBuf { - PathBuf::from("/usr/sbin/dmsetup") -} - -fn default_losetup() -> PathBuf { - PathBuf::from("/usr/sbin/losetup") -} - -fn default_blockdev() -> PathBuf { - PathBuf::from("/usr/sbin/blockdev") -} - -fn default_ip() -> PathBuf { - PathBuf::from("/usr/sbin/ip") -} - -fn default_nft() -> PathBuf { - PathBuf::from("/usr/sbin/nft") -} - -fn default_thin_dump() -> PathBuf { - PathBuf::from("/usr/sbin/thin_dump") -} - fn default_parent_cgroup() -> String { // Must match Elixir node's `@parent_cgroup`; operators need to keep them in sync. "hyper".into() From ac764b921b5d74a36b83ba31122aa6f4b42a5c5c Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:15:16 +0000 Subject: [PATCH 23/42] test(net): assert explicit HTTP 200 from guest egress fetch 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. --- test/e2e/network_test.exs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 2aadd1bf..ef7fac16 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -41,12 +41,22 @@ defmodule Hyper.E2e.NetworkTest do assert ip_out =~ "172.30.0.2" - assert {:ok, %{exit_code: 0}} = + # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free + # exit. `wget -S` writes the server's response headers to stderr; we merge + # them into stdout (`2>&1`) and assert the status line is `... 200 ...`. + # (Alpine ships busybox wget, not curl — installing curl would need egress + # to the apk mirror first, adding a flaky dependency to prove the same + # thing.) `-O /dev/null` discards the body; the fetch still exercises the + # full netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. + assert {:ok, %{stdout: http_out, exit_code: 0}} = await_exec( vm, - ["sh", "-c", "wget -qO- http://example.com >/dev/null"], + ["sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], :timer.seconds(90) ) + + assert http_out =~ ~r"HTTP/[\d.]+ 200\b", + "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}" else IO.puts("SKIP: [network] not enabled on this host — see Hyper.Cfg.Network.enabled?/0") end From 3187337a68f859f292aada89c7d42cbe6e888a84 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:26:15 +0000 Subject: [PATCH 24/42] feat(net): make VM networking mandatory, not opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- docs/cookbook/install.md | 16 +++-- lib/hyper/cfg/network.ex | 31 ++++++--- lib/hyper/node.ex | 17 ++--- lib/hyper/node/fire_vmm/boot_spec.ex | 24 +++---- lib/hyper/node/fire_vmm/daemon.ex | 14 ++-- lib/hyper/node/fire_vmm/jailer.ex | 15 +++-- lib/hyper/node/reaper.ex | 34 ++++------ test/e2e/network_test.exs | 73 ++++++++++----------- test/hyper/cfg/network_test.exs | 7 +- test/hyper/node/fire_vmm/boot_spec_test.exs | 26 ++++---- test/hyper/node/fire_vmm/daemon_test.exs | 40 +++-------- test/hyper/node/fire_vmm/jailer_test.exs | 6 +- 12 files changed, 140 insertions(+), 163 deletions(-) diff --git a/docs/cookbook/install.md b/docs/cookbook/install.md index cbd5fb81..106340dd 100644 --- a/docs/cookbook/install.md +++ b/docs/cookbook/install.md @@ -191,13 +191,15 @@ cgroup = "hyper" For more details on configuring and tuning Hyper, we suggest you see the [configuration guide](config.md). -### VM Networking (optional) - -Adding a `[network]` table to `/etc/hyper/config.toml` turns on real network -egress for VMs: each guest gets NAT'd out through a physical interface on the -host. The `hyper` nft table itself is created (and owned) by `host-init`, -which runs automatically at node start — you never create nft rules by hand. -You do need to prepare the host first: +### VM Networking (required) + +VM networking is **mandatory**: a node refuses to start without a `[network]` +table in `/etc/hyper/config.toml`, and every VM gets NAT'd egress out through a +physical interface on the host. There is no opt-out — a node that fails this +preflight will not boot (`{:error, :network_not_configured}`, a missing uplink, +or IPv4 forwarding off). The `hyper` nft table itself is created (and owned) by +`host-init`, which runs automatically at node start — you never create nft rules +by hand. You must prepare the host first: Install `iproute2` and `nftables`: diff --git a/lib/hyper/cfg/network.ex b/lib/hyper/cfg/network.ex index 8c8ee04e..a7228973 100644 --- a/lib/hyper/cfg/network.ex +++ b/lib/hyper/cfg/network.ex @@ -2,8 +2,12 @@ defmodule Hyper.Cfg.Network do @moduledoc """ VM egress networking settings from the `[network]` table — `config.toml`-only because the setuid helper reads the same `uplink` and `clone_pool` to build - each VM's netns, veth, and NAT rules. Absent table ⇒ networking disabled: VMs - boot with no NIC (today's behaviour), the jailer omits `--netns`. + each VM's netns, veth, and NAT rules. + + Networking is **mandatory**: a node refuses to start unless `[network]` is + configured (see `Hyper.Node.FireVMM.Jailer.Checks.network_ready/0`). Every VM + gets a NIC; there is no opt-out. `configured?/0` exists only so the startup + preflight can fail fast with a clear message rather than crash later. """ import Hyper.Cfg, only: [get_cfg: 1] @@ -11,17 +15,26 @@ defmodule Hyper.Cfg.Network do @default_clone_pool "172.31.0.0/16" @default_resolver "1.1.1.1" - @doc "Whether VM egress networking is turned on (`[network] uplink` present)." - @spec enabled?() :: boolean() - def enabled?, do: not is_nil(get_cfg(toml: "network.uplink", default: nil)) + @doc """ + Whether the required `[network] uplink` is present. Used only by the startup + preflight to fail fast; once a node is running, networking is guaranteed + configured, so runtime code calls `uplink/0` directly. + """ + @spec configured?() :: boolean() + def configured?, do: not is_nil(get_cfg(toml: "network.uplink", default: nil)) - @doc "Physical uplink interface guest egress is NAT'd out of. Raises if enabled but unset." + @doc "Physical uplink interface guest egress is NAT'd out of. Raises if unset (a running node has already passed the preflight)." @spec uplink() :: String.t() def uplink do case get_cfg(toml: "network.uplink", default: nil) do - nil -> raise Hyper.Cfg.MissingError, "network.uplink is required when networking is enabled" - v when is_binary(v) -> v - other -> raise ArgumentError, "network.uplink must be a string, got: #{inspect(other)}" + nil -> + raise Hyper.Cfg.MissingError, "network.uplink is required — VM networking is mandatory" + + v when is_binary(v) -> + v + + other -> + raise ArgumentError, "network.uplink must be a string, got: #{inspect(other)}" end end diff --git a/lib/hyper/node.ex b/lib/hyper/node.ex index 383e89f6..01c7d13f 100644 --- a/lib/hyper/node.ex +++ b/lib/hyper/node.ex @@ -311,20 +311,21 @@ defmodule Hyper.Node do :ok <- Hyper.SuidHelper.test_system(), {:ok, base} <- Hyper.SuidHelper.sys_test(), :ok <- check_helper_base(base), - :ok <- maybe_host_init() do + :ok <- init_network() do Hyper.Node.FireVMM.test_system() end end - # Idempotent: reconciles the host-wide `hyper` nftables table on every start. - # No-op when `[network]` is absent from config. Runs after - # `Hyper.SuidHelper.test_system/0` so the helper is confirmed present first. - @spec maybe_host_init :: :ok | {:error, term()} - defp maybe_host_init do - if Hyper.Cfg.Network.enabled?() do + # VM networking is mandatory: refuse to start unless `[network]` is + # configured, then idempotently reconcile the host-wide `hyper` nftables + # table. Runs after `Hyper.SuidHelper.test_system/0` so the helper is + # confirmed present first. + @spec init_network :: :ok | {:error, term()} + defp init_network do + if Hyper.Cfg.Network.configured?() do Hyper.SuidHelper.Network.host_init() else - :ok + {:error, :network_not_configured} end end diff --git a/lib/hyper/node/fire_vmm/boot_spec.ex b/lib/hyper/node/fire_vmm/boot_spec.ex index 53b4cb09..dc663854 100644 --- a/lib/hyper/node/fire_vmm/boot_spec.ex +++ b/lib/hyper/node/fire_vmm/boot_spec.ex @@ -32,20 +32,16 @@ defmodule Hyper.Node.FireVMM.BootSpec do @spec resolve(Hyper.Vm.source(), Instance.t()) :: Cold.t() def resolve(source, type) when is_map(source) do - base_args = Map.get(source, :boot_args, @default_boot_args) + # Networking is mandatory: every VM gets a NIC on the uniform inner-world + # address plus the kernel `ip=`/`hyper.resolver=` cmdline. A node that + # reached here has already passed the startup network preflight. + vm_id = Map.fetch!(source, :vm_id) - {nics, boot_args} = - if Hyper.Cfg.Network.enabled?() do - vm_id = Map.fetch!(source, :vm_id) - - {[Hyper.Node.FireVMM.Net.interface(vm_id)], - base_args <> - " " <> - Hyper.Node.FireVMM.Net.ip_cmdline() <> - " " <> Hyper.Node.FireVMM.Net.resolver_cmdline(Hyper.Cfg.Network.resolver())} - else - {[], base_args} - end + boot_args = + Map.get(source, :boot_args, @default_boot_args) <> + " " <> + Hyper.Node.FireVMM.Net.ip_cmdline() <> + " " <> Hyper.Node.FireVMM.Net.resolver_cmdline(Hyper.Cfg.Network.resolver()) %Cold{ machine_config: Instance.Spec.machine_config(Instance.spec(type)), @@ -61,7 +57,7 @@ defmodule Hyper.Node.FireVMM.BootSpec do path_on_host: Map.fetch!(source, :root_drive_path) } ], - network_interfaces: nics + network_interfaces: [Hyper.Node.FireVMM.Net.interface(vm_id)] } end end diff --git a/lib/hyper/node/fire_vmm/daemon.ex b/lib/hyper/node/fire_vmm/daemon.ex index d0d9dd99..18a564df 100644 --- a/lib/hyper/node/fire_vmm/daemon.ex +++ b/lib/hyper/node/fire_vmm/daemon.ex @@ -92,13 +92,13 @@ defmodule Hyper.Node.FireVMM.Daemon do end # The testable seam: reset any stale jail from a prior incarnation, then — - # gated on networking being enabled — prepare the netns the jailer's - # `--netns` will enter. Must run before `launch/1`: the jailer refuses to - # start if the netns it is told to enter does not already exist. + # Prepare the netns the jailer's `--netns` will enter. Must run before + # `launch/1`: the jailer refuses to start if the netns it is told to enter + # does not already exist. Networking is mandatory, so this is unconditional. @spec prelaunch(Opts.t()) :: :ok | {:error, term()} defp prelaunch(%Opts{vm_id: id, uid: uid} = opts) do with :ok <- reset_stale_jail(opts) do - if Hyper.Cfg.Network.enabled?(), do: Hyper.SuidHelper.Network.prepare(id, uid), else: :ok + Hyper.SuidHelper.Network.prepare(id, uid) end end @@ -110,11 +110,7 @@ defmodule Hyper.Node.FireVMM.Daemon do # failure); the first error is surfaced to the caller. @spec clear_jail(Opts.t()) :: :ok | {:error, term()} defp clear_jail(%Opts{vm_id: id, uid: uid}) do - net = - if Hyper.Cfg.Network.enabled?(), - do: Hyper.SuidHelper.Network.teardown(id, uid), - else: :ok - + net = Hyper.SuidHelper.Network.teardown(id, uid) jail = SuidHelper.ChrootJail.remove(Jailer.chroot_dir(id), Jailer.cgroup_dir(id)) with :ok <- net, do: jail end diff --git a/lib/hyper/node/fire_vmm/jailer.ex b/lib/hyper/node/fire_vmm/jailer.ex index 732dce48..eb15b986 100644 --- a/lib/hyper/node/fire_vmm/jailer.ex +++ b/lib/hyper/node/fire_vmm/jailer.ex @@ -64,20 +64,21 @@ defmodule Hyper.Node.FireVMM.Jailer do end @doc """ - Host preflight for VM egress networking: a no-op when `[network]` is - absent from config (`Network.enabled?/0` false). When enabled, confirms - the uplink interface exists and the kernel is forwarding IPv4 — both - prerequisites `Hyper.SuidHelper.Network.host_init/0` and per-VM - `prepare/2` depend on but cannot themselves provision. + Host preflight for VM egress networking, which is mandatory: refuses to + start unless `[network]` is configured, the uplink interface exists, and + the kernel is forwarding IPv4 — all prerequisites + `Hyper.SuidHelper.Network.host_init/0` and per-VM `prepare/2` depend on but + cannot themselves provision. This is the gate that makes networking + non-optional: a node missing any of these will not boot. """ @spec network_ready() :: :ok | {:error, term()} def network_ready do - if Network.enabled?() do + if Network.configured?() do with :ok <- uplink_present(Network.uplink()) do ip_forward_enabled() end else - :ok + {:error, :network_not_configured} end end diff --git a/lib/hyper/node/reaper.ex b/lib/hyper/node/reaper.ex index f34b2ecf..5541db41 100644 --- a/lib/hyper/node/reaper.ex +++ b/lib/hyper/node/reaper.ex @@ -166,25 +166,20 @@ defmodule Hyper.Node.Reaper do end end - # `/var/run/netns` only exists (and only matters) when VM egress networking is - # turned on; skip the listing entirely when it is off rather than logging - # spurious enoent warnings on every tick. + # Networking is mandatory, so `/var/run/netns` is expected present; an :enoent + # (nothing booted yet) is still normal and yields no orphans. @spec list_netns() :: [String.t()] defp list_netns do - if Hyper.Cfg.Network.enabled?() do - case File.ls("/var/run/netns") do - {:ok, names} -> - names - - {:error, :enoent} -> - [] - - {:error, reason} -> - Logger.warning("reaper: could not list netns: #{inspect(reason)}") - [] - end - else - [] + case File.ls("/var/run/netns") do + {:ok, names} -> + names + + {:error, :enoent} -> + [] + + {:error, reason} -> + Logger.warning("reaper: could not list netns: #{inspect(reason)}") + [] end end @@ -200,10 +195,7 @@ defmodule Hyper.Node.Reaper do ) log_result("dm volume", id, Dmsetup.remove(Img.Mutable.dm_name(id))) - - if Hyper.Cfg.Network.enabled?() do - log_result("netns", id, Hyper.SuidHelper.Network.teardown_orphan(id)) - end + log_result("netns", id, Hyper.SuidHelper.Network.teardown_orphan(id)) :ok end diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index ef7fac16..19e919b6 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -5,12 +5,11 @@ defmodule Hyper.E2e.NetworkTest do (`172.30.0.2`), and the netns+veth+tap+NAT path it rides on actually reaches the internet — DNS resolves and an HTTP fetch completes. - Self-skips (does not fail) when `[network]` is absent from `config.toml` - (`Hyper.Cfg.Network.enabled?/0` false): a host that never provisioned - networking has no uplink/clone_pool to test against, and this suite must - not break integration runs on such a host. CI's `integration` job - provisions `[network]` (see `.github/scripts/provision-kvm-host.sh`), so - the real assertions run there. + VM networking is mandatory (`Hyper.Node.FireVMM.Jailer.Checks.network_ready/0` + refuses to start a node without `[network]`), so there is nothing to skip: a + host that reached the point of running this suite already booted the node, + which means networking is provisioned. CI's `integration` job provisions it + (see `.github/scripts/provision-kvm-host.sh`). Runs only under `--only integration` / `--include integration` on a host provisioned per docs/cookbook/install.md. @@ -27,38 +26,34 @@ defmodule Hyper.E2e.NetworkTest do @image System.get_env("HYPER_E2E_IMAGE", "public.ecr.aws/docker/library/alpine:3.19") test "a booted guest can reach the internet over its NIC" do - if Hyper.Cfg.Network.enabled?() do - assert {:ok, img_id} = Hyper.Img.OciLoader.load(@image) - - # :micro, not the :base default — :base asks for 32 GiB of disk budget, - # which the default node budget (4 GiB) refuses with :no_capacity on a - # small CI runner. - assert {:ok, vm} = Hyper.create_vm(%Hyper.Vm.Spec{img_id: img_id, type: :micro}) - on_exit(fn -> Hyper.Node.stop_image_vm(vm) end) - - assert {:ok, %{stdout: ip_out, exit_code: 0}} = - await_exec(vm, ["ip", "-4", "addr", "show", "eth0"]) - - assert ip_out =~ "172.30.0.2" - - # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free - # exit. `wget -S` writes the server's response headers to stderr; we merge - # them into stdout (`2>&1`) and assert the status line is `... 200 ...`. - # (Alpine ships busybox wget, not curl — installing curl would need egress - # to the apk mirror first, adding a flaky dependency to prove the same - # thing.) `-O /dev/null` discards the body; the fetch still exercises the - # full netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. - assert {:ok, %{stdout: http_out, exit_code: 0}} = - await_exec( - vm, - ["sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], - :timer.seconds(90) - ) - - assert http_out =~ ~r"HTTP/[\d.]+ 200\b", - "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}" - else - IO.puts("SKIP: [network] not enabled on this host — see Hyper.Cfg.Network.enabled?/0") - end + assert {:ok, img_id} = Hyper.Img.OciLoader.load(@image) + + # :micro, not the :base default — :base asks for 32 GiB of disk budget, + # which the default node budget (4 GiB) refuses with :no_capacity on a + # small CI runner. + assert {:ok, vm} = Hyper.create_vm(%Hyper.Vm.Spec{img_id: img_id, type: :micro}) + on_exit(fn -> Hyper.Node.stop_image_vm(vm) end) + + assert {:ok, %{stdout: ip_out, exit_code: 0}} = + await_exec(vm, ["ip", "-4", "addr", "show", "eth0"]) + + assert ip_out =~ "172.30.0.2" + + # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free + # exit. `wget -S` writes the server's response headers to stderr; we merge + # them into stdout (`2>&1`) and assert the status line is `... 200 ...`. + # (Alpine ships busybox wget, not curl — installing curl would need egress + # to the apk mirror first, adding a flaky dependency to prove the same + # thing.) `-O /dev/null` discards the body; the fetch still exercises the + # full netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. + assert {:ok, %{stdout: http_out, exit_code: 0}} = + await_exec( + vm, + ["sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], + :timer.seconds(90) + ) + + assert http_out =~ ~r"HTTP/[\d.]+ 200\b", + "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}" end end diff --git a/test/hyper/cfg/network_test.exs b/test/hyper/cfg/network_test.exs index a0429c3b..b852d1c3 100644 --- a/test/hyper/cfg/network_test.exs +++ b/test/hyper/cfg/network_test.exs @@ -17,10 +17,11 @@ defmodule Hyper.Cfg.NetworkTest do end end - describe "enabled?/0" do + describe "configured?/0" do test "false when no uplink configured" do - # Base test config sets no [network] table. - refute Network.enabled?() + # Base test config sets no [network] table. This is the predicate the + # startup preflight uses to refuse booting a node without networking. + refute Network.configured?() end end diff --git a/test/hyper/node/fire_vmm/boot_spec_test.exs b/test/hyper/node/fire_vmm/boot_spec_test.exs index ffa875c0..b7aa8ea4 100644 --- a/test/hyper/node/fire_vmm/boot_spec_test.exs +++ b/test/hyper/node/fire_vmm/boot_spec_test.exs @@ -3,27 +3,25 @@ defmodule Hyper.Node.FireVMM.BootSpecTest do alias Hyper.Node.FireVMM.BootSpec - # The enabled path (NIC populated + `ip=`/`hyper.resolver=` appended) needs a - # `[network]` config override, which the unit config harness doesn't provide; - # it's asserted in the e2e/integration suite instead. The pure formatting of - # the resolver fragment is pinned in net_test.exs. This module only pins the - # disabled path (the base test config has no `[network]` table), which must - # hold for every existing VM. + # Networking is mandatory: every VM gets a NIC plus the kernel + # `ip=`/`hyper.resolver=` cmdline, derived entirely from the vm_id and the + # uniform inner world (no `[network]` config needed — the base test config + # has none). The pure formatting of those fragments is pinned in net_test.exs; + # this module pins that resolve/2 always emits them. + @source %{vm_id: "vabc", kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} test "default cmdline boots our agent as init with no serial console" do - source = %{kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} - cold = BootSpec.resolve(source, :micro) + cold = BootSpec.resolve(@source, :micro) assert cold.boot_source.boot_args =~ "init=/hyper-init" # No console by default: serial printk blocks the boot vCPU. A debug boot # re-adds console=ttyS0 through the per-VM boot_args override. refute cold.boot_source.boot_args =~ "console=" end - test "no NIC and no ip=/hyper.resolver= when networking disabled" do - source = %{vm_id: "vabc", kernel_image_path: "/vmlinux", root_drive_path: "/rootfs"} - cold = BootSpec.resolve(source, :micro) - assert cold.network_interfaces == [] - refute cold.boot_source.boot_args =~ "ip=" - refute cold.boot_source.boot_args =~ "hyper.resolver=" + test "every VM gets a NIC and the ip=/hyper.resolver= cmdline" do + cold = BootSpec.resolve(@source, :micro) + assert length(cold.network_interfaces) == 1 + assert cold.boot_source.boot_args =~ "ip=" + assert cold.boot_source.boot_args =~ "hyper.resolver=" end end diff --git a/test/hyper/node/fire_vmm/daemon_test.exs b/test/hyper/node/fire_vmm/daemon_test.exs index 108750b5..e564d7c9 100644 --- a/test/hyper/node/fire_vmm/daemon_test.exs +++ b/test/hyper/node/fire_vmm/daemon_test.exs @@ -7,14 +7,13 @@ defmodule Hyper.Node.FireVMM.DaemonTest do for `tools.firecracker`/`tools.jailer` — so they can observe real call order and gating without root, a real netns, or a real firecracker process. - Invariants under test: + Invariants under test (networking is mandatory — these fire unconditionally, + even with no `[network]` table in config): * the jailer enters the netns via `--netns` (Task 8), so it must already exist: `network prepare` must run before the `jailer` launch. * jail clearing (`chroot-jail remove`) and network teardown both fire - wherever the jail is cleared (stale-reset on relaunch, and terminate), - gated on `Hyper.Cfg.Network.enabled?/0`. Disabled is behavior-identical - to before this feature: no network call at all. + wherever the jail is cleared (stale-reset on relaunch, and terminate). """ use ExUnit.Case, async: false @@ -79,35 +78,16 @@ defmodule Hyper.Node.FireVMM.DaemonTest do # `Jailer.chroot_dir/1` derives the in-jail exec name from `tools.firecracker` # (see `JailerTest`), so every scenario stubs it alongside `tools.suidhelper` # to keep it off the real filesystem. - defp put_toml(fake, network \\ nil) do + defp put_toml(fake) do tools = %{"suidhelper" => fake, "firecracker" => "/usr/local/bin/firecracker"} - base = %{"tools" => tools} - Hyper.Cfg.Toml.put_cache(if network, do: Map.put(base, "network", network), else: base) + Hyper.Cfg.Toml.put_cache(%{"tools" => tools}) end - describe "networking disabled (default)" do - test "start_link never calls network, and jail clearing still runs", %{log: log, fake: fake} do - refute Hyper.Cfg.Network.enabled?() - put_toml(fake) - - Process.flag(:trap_exit, true) - {:ok, pid} = Daemon.start_link(opts()) - assert_receive {:EXIT, ^pid, _reason}, 2_000 - - ops = calls(log) - refute "network" in ops - assert "chroot-jail" in ops - assert "jailer" in ops - # the netns-prepare (or its absence) always precedes the launch attempt - assert Enum.find_index(ops, &(&1 == "chroot-jail")) < - Enum.find_index(ops, &(&1 == "jailer")) - end - end - - describe "networking enabled" do + describe "netns lifecycle (mandatory)" do test "start_link prepares the netns before launching the jailer", %{log: log, fake: fake} do - put_toml(fake, %{"uplink" => "eth0"}) - assert Hyper.Cfg.Network.enabled?() + # No [network] table on purpose: the netns prepare must fire regardless, + # since networking is mandatory rather than config-gated. + put_toml(fake) Process.flag(:trap_exit, true) {:ok, pid} = Daemon.start_link(opts()) @@ -123,7 +103,7 @@ defmodule Hyper.Node.FireVMM.DaemonTest do end test "terminate/2 tears down both the network and the chroot jail", %{log: log, fake: fake} do - put_toml(fake, %{"uplink" => "eth0"}) + put_toml(fake) assert :ok = Daemon.terminate(:shutdown, %Daemon{ diff --git a/test/hyper/node/fire_vmm/jailer_test.exs b/test/hyper/node/fire_vmm/jailer_test.exs index cd4328b3..3b1d9f14 100644 --- a/test/hyper/node/fire_vmm/jailer_test.exs +++ b/test/hyper/node/fire_vmm/jailer_test.exs @@ -87,11 +87,13 @@ defmodule Hyper.Node.FireVMM.JailerTest do end describe "Checks.network_ready/0" do - test "ok when networking is disabled (no [network] table)" do + test "refuses to start when no [network] table is configured" do + # Networking is mandatory: absence is a hard startup failure, not a + # silent opt-out. Hyper.Cfg.Toml.put_cache(%{}) on_exit(fn -> Hyper.Cfg.Toml.reload() end) - assert Jailer.Checks.network_ready() == :ok + assert Jailer.Checks.network_ready() == {:error, :network_not_configured} end test "fails when uplink iface is absent" do From 29ca6cd67b51eed85e345de25a856264c16f7294 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:36:01 +0000 Subject: [PATCH 25/42] fix(net): drop duplicate run() helpers left by the origin/main merge MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/suidhelper/tests/e2e/chroot_jail.rs | 10 ---------- native/suidhelper/tests/e2e/jailer.rs | 10 ---------- 2 files changed, 20 deletions(-) diff --git a/native/suidhelper/tests/e2e/chroot_jail.rs b/native/suidhelper/tests/e2e/chroot_jail.rs index cd1f0408..fcea9aed 100644 --- a/native/suidhelper/tests/e2e/chroot_jail.rs +++ b/native/suidhelper/tests/e2e/chroot_jail.rs @@ -43,16 +43,6 @@ fn write_config_with_network(dir: &Path, work_dir: &Path) -> PathBuf { p } -fn run(config: &Path, args: &[&str]) -> Output { - Command::new(BIN) - .args(args) - .env_clear() - .env("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1") - .env("HYPER_SETUIDHELPER_CONFIG_PATH", config) - .output() - .expect("spawn helper") -} - fn setup_loop(tmp: &Path) -> Option { let backing = tmp.join("backing.img"); let f = fs::File::create(&backing).ok()?; diff --git a/native/suidhelper/tests/e2e/jailer.rs b/native/suidhelper/tests/e2e/jailer.rs index f51bcd7e..8ed0dc4c 100644 --- a/native/suidhelper/tests/e2e/jailer.rs +++ b/native/suidhelper/tests/e2e/jailer.rs @@ -78,16 +78,6 @@ fn write_root_config_with_network(dir: &Path, jailer: &Path, firecracker: &Path) p } -fn run(config: &Path, args: &[&str]) -> std::process::Output { - Command::new(HELPER) - .args(args) - .env_clear() - .env("HYPER_SETUIDHELPER_IS_INSECURE_MODE", "1") - .env("HYPER_SETUIDHELPER_CONFIG_PATH", config) - .output() - .expect("spawn helper") -} - #[test] fn execs_jailer_with_canonical_argv_and_empty_env_as_root() { if !is_root() { From fdad25a223c7a1b1d9524f3604ae3782aee48fdb Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:36:01 +0000 Subject: [PATCH 26/42] ci(net): modprobe nf_tables + nf_nat on the KVM runner MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .github/scripts/provision-kvm-host.sh | 10 +++++++++- 1 file changed, 9 insertions(+), 1 deletion(-) diff --git a/.github/scripts/provision-kvm-host.sh b/.github/scripts/provision-kvm-host.sh index ab4ae824..4275024b 100755 --- a/.github/scripts/provision-kvm-host.sh +++ b/.github/scripts/provision-kvm-host.sh @@ -38,7 +38,15 @@ sudo apt-get install -y \ # -a is load-bearing: without it modprobe reads the 2nd+ names as module # PARAMETERS of the first, and the load fails. -sudo modprobe -av dm_snapshot dm_thin_pool loop +# +# nf_tables + nf_nat back host-init's `hyper` nftables table and its +# masquerade/SNAT/DNAT rules. A fresh GitHub-hosted runner does not have them +# loaded, and the netfilter family does not always autoload on first `nft` use, +# so `nft add table ip hyper` fails (empty stderr, non-zero exit) and the node +# refuses to start. Load the base explicitly, as we do for the dm modules; the +# nat expression sub-modules (nft_chain_nat, nft_masq) autoload once the base is +# present and a rule is issued. +sudo modprobe -av dm_snapshot dm_thin_pool loop nf_tables nf_nat targets="$(sudo dmsetup targets)" echo "dmsetup targets: ${targets}" grep -q thin-pool <<<"${targets}" || { echo "ERROR: thin-pool dm target missing" >&2; exit 1; } From 7343e31f6d36cbb5e687b335ef378667facd97f5 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:52:28 +0000 Subject: [PATCH 27/42] fix(net): force tun node to 0666 (mknod umask masked it to 0644) 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. --- native/suidhelper/src/util/safe_dir.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/native/suidhelper/src/util/safe_dir.rs b/native/suidhelper/src/util/safe_dir.rs index 52f2c576..36a880c3 100644 --- a/native/suidhelper/src/util/safe_dir.rs +++ b/native/suidhelper/src/util/safe_dir.rs @@ -201,6 +201,10 @@ impl SafeDir { /// Create a character device node `name` in this directory (mode `0666`, /// matching `/dev/net/tun`'s conventional permissions) with the given /// `rdev`, then chown it to `uid:gid`. + /// + /// `mknodat` masks its mode argument with the process umask (typically 022, + /// which would yield `0644`), so the exact `0666` is set with an explicit + /// `chmod` afterwards — `fchmodat` ignores the umask. pub fn mknod_char(&self, name: &Path, rdev: dev_t, uid: u32, gid: u32) -> Result<(), Error> { mknodat( Some(self.0.as_raw_fd()), @@ -213,7 +217,8 @@ impl SafeDir { name: name.to_path_buf(), source, })?; - self.chown(name, uid, gid) + self.chown(name, uid, gid)?; + self.chmod(name, 0o666) } /// Create directory `name` in this directory with `mode`, tolerating From 8665d9ab4cd8bb50cf450d1995f91627a006d8f2 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 20:52:28 +0000 Subject: [PATCH 28/42] fix(net): run ip/nft as genuine root so they don't trip AT_SECURE MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/suidhelper/src/tools/network/exec.rs | 46 ++++++++++++++++----- 1 file changed, 36 insertions(+), 10 deletions(-) diff --git a/native/suidhelper/src/tools/network/exec.rs b/native/suidhelper/src/tools/network/exec.rs index 988f3f10..2eb199aa 100644 --- a/native/suidhelper/src/tools/network/exec.rs +++ b/native/suidhelper/src/tools/network/exec.rs @@ -5,6 +5,9 @@ //! the whole privileged window for every network op — `prepare`, `teardown`, //! and `host_init` differ only in which commands they hand to [`run_all`]. use super::args::{Command, Which}; +use crate::util::setuid_privileged::become_root_permanently; +use std::io; +use std::os::unix::process::CommandExt; use std::path::Path; use std::process::Command as Proc; use thiserror::Error as ThisError; @@ -18,11 +21,13 @@ pub enum Error { #[source] source: std::io::Error, }, - #[error("{bin:?} {argv:?} failed: {stderr}")] + #[error("{bin:?} {argv:?} exited {code}: stderr={stderr:?} stdout={stdout:?}")] Failed { bin: Which, argv: Vec, + code: String, stderr: String, + stdout: String, }, } @@ -31,21 +36,42 @@ pub(super) fn run_all<'a>( resolve: impl Fn(Which) -> &'a Path, ) -> Result<(), Error> { for command in commands { - let output = Proc::new(resolve(command.bin)) - .args(&command.argv) - .env_clear() - .output() - .map_err(|source| Error::Spawn { - bin: command.bin, - argv: command.argv.clone(), - source, - })?; + let mut proc = Proc::new(resolve(command.bin)); + proc.args(&command.argv).env_clear(); + + // `ip` and `nft` refuse to run under `AT_SECURE` — the setuid-like state + // the kernel flags when a child is exec'd with `ruid != euid`. Our + // privilege guard (`Privileged::acquire`) raises only euid to 0, leaving + // ruid at the caller, so without this the child inherits that mismatch + // and nft exits 111 with no output at all. Promote the child to genuine + // root (all of real/effective/saved uid == 0) just before exec — the + // same reason the jailer handoff uses `become_root_permanently`. This + // runs in the forked child, so it never affects the parent's own + // privilege window. + // + // SAFETY: `become_root_permanently` calls only async-signal-safe + // setresuid/setresgid/setgroups syscalls and allocates nothing, which is + // the contract `pre_exec` requires of the post-fork, pre-exec child. + unsafe { + proc.pre_exec(|| become_root_permanently().map_err(io::Error::other)); + } + + let output = proc.output().map_err(|source| Error::Spawn { + bin: command.bin, + argv: command.argv.clone(), + source, + })?; if !output.status.success() && !command.allow_failure { return Err(Error::Failed { bin: command.bin, argv: command.argv.clone(), + code: output + .status + .code() + .map_or_else(|| "signal".to_string(), |c| c.to_string()), stderr: String::from_utf8_lossy(&output.stderr).trim().to_string(), + stdout: String::from_utf8_lossy(&output.stdout).trim().to_string(), }); } } From dc3a768062db3c3b453435ce5d854a4e0901cc5e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:02:59 +0000 Subject: [PATCH 29/42] fix(net): default ip path to /usr/bin/ip (not the symlinked /usr/sbin/ip) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/suidhelper/src/config.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index c8bdcfdb..ad32af3b 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -155,7 +155,10 @@ impl Default for Tools { dmsetup: "/usr/sbin/dmsetup".into(), losetup: "/usr/sbin/losetup".into(), blockdev: "/usr/sbin/blockdev".into(), - ip: "/usr/sbin/ip".into(), + // /usr/bin/ip, not /usr/sbin/ip: on merged-usr distros the latter + // is a symlink to the former, and SafeBin rejects symlinked tool + // paths. iproute2's real binary lives at /usr/bin/ip. + ip: "/usr/bin/ip".into(), nft: "/usr/sbin/nft".into(), thin_dump: "/usr/sbin/thin_dump".into(), firecracker: None, From f7a6d9a41cbfcb77ce1ab4b9baa8f08201bdd2b6 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:02:59 +0000 Subject: [PATCH 30/42] fix(net): run in-netns nft by absolute path under ip netns exec prepare's NAT setup ran 'ip netns exec 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. --- native/suidhelper/src/tools/network/args.rs | 22 +++++++++++++------ .../suidhelper/src/tools/network/prepare.rs | 2 +- native/suidhelper/tests/tools/network_args.rs | 17 ++++++++------ 3 files changed, 26 insertions(+), 15 deletions(-) diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index ee55d8f0..2d703935 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -81,11 +81,14 @@ impl Command { } } - /// `ip netns exec nft ...` — runs an `nft` operation inside the + /// `ip netns exec ...` — runs an `nft` operation inside the /// VM's netns, kept as a [`Which::Ip`] command so only `ip` and `nft` are - /// ever the privileged binary named directly. - fn nft_ns(netns: &str, argv: Vec) -> Self { - let mut full = argv!["netns", "exec", netns, "nft"]; + /// ever the privileged binary named directly. `nft` is the *absolute* path + /// to the nft binary, not the bare name: the helper spawns every command + /// with a cleared environment (no `PATH`), so `ip netns exec` could not + /// resolve a bare `nft`, and the config names the exact binary anyway. + fn nft_ns(netns: &str, nft: &str, argv: Vec) -> Self { + let mut full = argv!["netns", "exec", netns, nft]; full.extend(argv); Self::ip(full) } @@ -95,7 +98,7 @@ impl Command { /// the guest's inner address (`addr::INNER_GUEST_IP`) can reach the host's /// uplink. The default route is requested last, after the host veth end /// (`hv`) is up, so the route's target is already reachable. -pub fn prepare_commands(plan: &Plan) -> Vec { +pub fn prepare_commands(plan: &Plan, nft: &str) -> Vec { let netns = plan.netns.as_str(); let veth_host = plan.veth_host.as_str(); let veth_ns = plan.veth_ns.as_str(); @@ -142,7 +145,7 @@ pub fn prepare_commands(plan: &Plan) -> Vec { Command::ip_ns(netns, argv!["link", "set", "lo", "up"]), Command::ip_ns(netns, argv!["route", "add", "default", "via", veth_host_ip]), ]; - commands.extend(nft_prepare_commands(netns, veth_ns, veth_ns_ip)); + commands.extend(nft_prepare_commands(netns, veth_ns, veth_ns_ip, nft)); commands } @@ -153,11 +156,13 @@ fn nft_prepare_commands( netns: &str, veth_ns: &str, veth_ns_ip: std::net::Ipv4Addr, + nft: &str, ) -> Vec { vec![ - Command::nft_ns(netns, argv!["add", "table", "ip", "nat"]), + Command::nft_ns(netns, nft, argv!["add", "table", "ip", "nat"]), Command::nft_ns( netns, + nft, argv![ "add", "chain", @@ -177,6 +182,7 @@ fn nft_prepare_commands( ), Command::nft_ns( netns, + nft, argv![ "add", "rule", @@ -195,6 +201,7 @@ fn nft_prepare_commands( ), Command::nft_ns( netns, + nft, argv![ "add", "chain", @@ -214,6 +221,7 @@ fn nft_prepare_commands( ), Command::nft_ns( netns, + nft, argv![ "add", "rule", diff --git a/native/suidhelper/src/tools/network/prepare.rs b/native/suidhelper/src/tools/network/prepare.rs index 948a269b..27ca6dd4 100644 --- a/native/suidhelper/src/tools/network/prepare.rs +++ b/native/suidhelper/src/tools/network/prepare.rs @@ -114,7 +114,7 @@ impl IsTool for Prepare { type RunT = Result<(), Error>; fn run_privileged(&self) -> Self::RunT { - let commands = args::prepare_commands(&self.plan); + let commands = args::prepare_commands(&self.plan, &self.nft.to_string_lossy()); exec::run_all(&commands, |which| match which { args::Which::Ip => self.ip.as_path(), args::Which::Nft => self.nft.as_path(), diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index 8c577c6a..51f87ba5 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -18,7 +18,7 @@ fn plan0() -> Plan { #[test] fn prepare_creates_netns_first_and_default_route_last() { - let cmds = args::prepare_commands(&plan0()); + let cmds = args::prepare_commands(&plan0(), "/usr/sbin/nft"); let first = &cmds[0]; assert!(matches!(first.bin, Which::Ip)); assert_eq!( @@ -110,7 +110,10 @@ fn cmd_allow_failure(bin: Which, argv: &[&str]) -> args::Command { #[test] fn prepare_commands_golden_sequence() { let netns = "vaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa"; - let cmds = args::prepare_commands(&plan0()); + // In-netns nft runs via `ip netns exec ` — the absolute + // path, since the helper clears PATH before spawning (see nft_ns). + let nft = "/usr/sbin/nft"; + let cmds = args::prepare_commands(&plan0(), nft); let expected = vec![ cmd(Which::Ip, &["netns", "add", netns]), cmd( @@ -141,7 +144,7 @@ fn prepare_commands_golden_sequence() { ), cmd( Which::Ip, - &["netns", "exec", netns, "nft", "add", "table", "ip", "nat"], + &["netns", "exec", netns, nft, "add", "table", "ip", "nat"], ), cmd( Which::Ip, @@ -149,7 +152,7 @@ fn prepare_commands_golden_sequence() { "netns", "exec", netns, - "nft", + nft, "add", "chain", "ip", @@ -172,7 +175,7 @@ fn prepare_commands_golden_sequence() { "netns", "exec", netns, - "nft", + nft, "add", "rule", "ip", @@ -194,7 +197,7 @@ fn prepare_commands_golden_sequence() { "netns", "exec", netns, - "nft", + nft, "add", "chain", "ip", @@ -217,7 +220,7 @@ fn prepare_commands_golden_sequence() { "netns", "exec", netns, - "nft", + nft, "add", "rule", "ip", From 3de0600d8f4de5b4d60e3d4c935ffd68e13703e2 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:09:23 +0000 Subject: [PATCH 31/42] fix(net): make network teardown idempotent (tolerate missing netns/veth) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- native/suidhelper/src/tools/network/args.rs | 21 +++++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index 2d703935..a33d49c3 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -81,6 +81,16 @@ impl Command { } } + /// An `ip` step whose non-zero exit is tolerated — for idempotent deletes + /// (`ip netns del`, `ip link del`) issued speculatively where the target may + /// not exist. See the `allow_failure` doc on [`Command`]. + fn ip_allow_failure(argv: Vec) -> Self { + Self { + allow_failure: true, + ..Self::ip(argv) + } + } + /// `ip netns exec ...` — runs an `nft` operation inside the /// VM's netns, kept as a [`Which::Ip`] command so only `ip` and `nft` are /// ever the privileged binary named directly. `nft` is the *absolute* path @@ -243,10 +253,17 @@ fn nft_prepare_commands( /// the TAP device, and any in-netns nftables state with it; the host-side veth /// end usually goes with it too, but `ip link del` is issued explicitly in /// case it lingers (a no-op if already gone). +/// +/// Both deletes tolerate a missing target: `Daemon` runs teardown speculatively +/// in `clear_jail` *before* launching a fresh VM (to clear any stale prior +/// incarnation), so on a first boot the netns and veth do not exist yet — +/// `ip netns del` on a missing namespace exits non-zero, which must not fail +/// the launch. Like the chroot removal it sits beside, teardown is idempotent +/// cleanup; a genuine leak is backstopped by `Hyper.Node.Reaper`. pub fn teardown_commands(plan: &Plan) -> Vec { vec![ - Command::ip(argv!["netns", "del", plan.netns.as_str()]), - Command::ip(argv!["link", "del", plan.veth_host.as_str()]), + Command::ip_allow_failure(argv!["netns", "del", plan.netns.as_str()]), + Command::ip_allow_failure(argv!["link", "del", plan.veth_host.as_str()]), ] } From 18bf73dd89fc31173b1092f42dd2453dc9cfb40b Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:22:20 +0000 Subject: [PATCH 32/42] fix(net): make jail tun-node creation idempotent (tolerate EEXIST) 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. --- native/suidhelper/src/util/safe_dir.rs | 20 ++++++++++++++------ 1 file changed, 14 insertions(+), 6 deletions(-) diff --git a/native/suidhelper/src/util/safe_dir.rs b/native/suidhelper/src/util/safe_dir.rs index 36a880c3..ccf886d9 100644 --- a/native/suidhelper/src/util/safe_dir.rs +++ b/native/suidhelper/src/util/safe_dir.rs @@ -206,17 +206,25 @@ impl SafeDir { /// which would yield `0644`), so the exact `0666` is set with an explicit /// `chmod` afterwards — `fchmodat` ignores the umask. pub fn mknod_char(&self, name: &Path, rdev: dev_t, uid: u32, gid: u32) -> Result<(), Error> { - mknodat( + // Tolerate EEXIST: jail staging is re-run when a VM cold-boots after a + // failed attempt, so the node may survive from the prior attempt. The + // chown+chmod below re-assert the exact ownership and mode regardless, + // so a pre-existing node ends up identical to a freshly-created one. + match mknodat( Some(self.0.as_raw_fd()), name, SFlag::S_IFCHR, Mode::from_bits_truncate(0o666), rdev, - ) - .map_err(|source| Error::Mknod { - name: name.to_path_buf(), - source, - })?; + ) { + Ok(()) | Err(Errno::EEXIST) => {} + Err(source) => { + return Err(Error::Mknod { + name: name.to_path_buf(), + source, + }) + } + } self.chown(name, uid, gid)?; self.chmod(name, 0o666) } From 830bd20df4ea62cd98bd79e240fb91e02900e690 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:47:06 +0000 Subject: [PATCH 33/42] fix(net): emit a full six-octet guest MAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- lib/hyper/node/fire_vmm/net.ex | 7 +++++-- test/hyper/node/fire_vmm/net_test.exs | 10 ++++++++-- 2 files changed, 13 insertions(+), 4 deletions(-) diff --git a/lib/hyper/node/fire_vmm/net.ex b/lib/hyper/node/fire_vmm/net.ex index a9dbad92..f4f1942e 100644 --- a/lib/hyper/node/fire_vmm/net.ex +++ b/lib/hyper/node/fire_vmm/net.ex @@ -35,9 +35,12 @@ defmodule Hyper.Node.FireVMM.Net do """ @spec guest_mac(Hyper.Vm.Id.t()) :: String.t() def guest_mac(vm_id) do - <<_::8, b1, b2, b3, b4, _::binary>> = :crypto.hash(:sha256, vm_id) + # Six octets: the fixed locally-administered-unicast leading byte plus five + # from the id hash. Firecracker rejects anything but a full XX:XX:XX:XX:XX:XX + # MAC (a five-octet address 400s the NIC config and the VM never boots). + <<_::8, b1, b2, b3, b4, b5, _::binary>> = :crypto.hash(:sha256, vm_id) - [0x02, b1, b2, b3, b4] + [0x02, b1, b2, b3, b4, b5] |> Enum.map_join(":", &(&1 |> Integer.to_string(16) |> String.pad_leading(2, "0"))) |> String.downcase() end diff --git a/test/hyper/node/fire_vmm/net_test.exs b/test/hyper/node/fire_vmm/net_test.exs index e2917565..e38f086a 100644 --- a/test/hyper/node/fire_vmm/net_test.exs +++ b/test/hyper/node/fire_vmm/net_test.exs @@ -5,8 +5,14 @@ defmodule Hyper.Node.FireVMM.NetTest do test "guest_mac is a stable locally-administered unicast MAC" do mac = Net.guest_mac("vabcdef") assert mac == Net.guest_mac("vabcdef"), "must be deterministic in vm_id" - [first | _] = String.split(mac, ":") - <> = Base.decode16!(first, case: :mixed) + + octets = String.split(mac, ":") + # Firecracker rejects anything but a full six-octet MAC — a short address + # 400s the NIC config and the VM never boots. + assert length(octets) == 6, "MAC must have six octets, got #{mac}" + assert Enum.all?(octets, &(byte_size(&1) == 2 and match?({_, ""}, Integer.parse(&1, 16)))) + + <> = Base.decode16!(hd(octets), case: :mixed) # locally administered (bit 1 set), unicast (bit 0 clear) assert Bitwise.band(byte, 0x02) == 0x02 assert Bitwise.band(byte, 0x01) == 0x00 From 96aa80cf87c7cd0781b76989b61be78d63a3e478 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:47:06 +0000 Subject: [PATCH 34/42] fix(img): make build_chain total for an unknown (empty-chain) image 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. --- lib/hyper/node/img/server.ex | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/lib/hyper/node/img/server.ex b/lib/hyper/node/img/server.ex index 8fd7f0b8..bcdd5f83 100644 --- a/lib/hyper/node/img/server.ex +++ b/lib/hyper/node/img/server.ex @@ -186,6 +186,12 @@ defmodule Hyper.Node.Img.Server do # on top. Rolls back any devices it created if a later one fails. @spec build_chain(Hyper.Img.id(), [Path.t()]) :: {:ok, %{blk_path: Path.t(), dm_names: [String.t()]}} | {:error, term()} + # An unknown image id resolves to no layers; return a clean domain error so + # `init/1` stops with `{:stop, :unknown_image}` (which the scheduler maps to + # `:no_capacity`) rather than crashing with a FunctionClauseError + SASL + # report. This is the placement-refusal path for a never-loaded image. + defp build_chain(_img_id, []), do: {:error, :unknown_image} + defp build_chain(img_id, [base | deltas]) do with {:ok, sectors} <- SuidHelper.Blockdev.device_sectors(base) do deltas From a1ea08964d5dc78622f0a54ee91d53f11651945e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 21:54:50 +0000 Subject: [PATCH 35/42] test(e2e): exec guest net checks via absolute /bin/sh MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Hyper.exec runs argv directly with no PATH, so the bare `ip`/`sh` the network E2E used returned exit 127 (command not found) — the guest never resolved them. Route both checks through an absolute `/bin/sh -c`, letting busybox's own default PATH find the `ip`/`wget` applets, matching the documented contract. --- test/e2e/network_test.exs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 19e919b6..80ebb647 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -34,8 +34,11 @@ defmodule Hyper.E2e.NetworkTest do assert {:ok, vm} = Hyper.create_vm(%Hyper.Vm.Spec{img_id: img_id, type: :micro}) on_exit(fn -> Hyper.Node.stop_image_vm(vm) end) + # Hyper.exec runs argv directly with no PATH (see its @doc): a bare command + # name returns exit 127. Route through an absolute `/bin/sh -c` so busybox's + # own default PATH resolves the `ip`/`wget` applets inside the guest. assert {:ok, %{stdout: ip_out, exit_code: 0}} = - await_exec(vm, ["ip", "-4", "addr", "show", "eth0"]) + await_exec(vm, ["/bin/sh", "-c", "ip addr show eth0"]) assert ip_out =~ "172.30.0.2" @@ -49,7 +52,7 @@ defmodule Hyper.E2e.NetworkTest do assert {:ok, %{stdout: http_out, exit_code: 0}} = await_exec( vm, - ["sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], + ["/bin/sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], :timer.seconds(90) ) From 1c149e220a0d3b57d72eb2a3aa10a254610584e0 Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:06:54 +0000 Subject: [PATCH 36/42] fix(net): enable ip_forward inside each VM's netns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A per-VM netns routes between two /30s — the guest's tap (172.30.0.1/30) and the veth uplink — so it only carries the guest's egress if it forwards between its interfaces. But a fresh netns defaults net.ipv4.ip_forward to 0 independent of the host, and nothing turned it on, so every forwarded egress packet was silently dropped: the guest configured eth0 fine (`ip addr` worked) but no traffic left — DNS was just the first packet to try (`wget: bad address`). Enable it in `prepare` via `ip netns exec sysctl -w net.ipv4.ip_forward=1`, threading a configurable absolute sysctl path the same way nft is (the helper clears PATH, so bare names can't resolve). host_init already requires the host's own ip_forward; this closes the per-netns gap the host check can't see. --- native/suidhelper/src/config.rs | 9 +++++++ native/suidhelper/src/tools/network/args.rs | 17 ++++++++++++- .../suidhelper/src/tools/network/prepare.rs | 25 +++++++++++++++---- .../suidhelper/src/tools/network/teardown.rs | 4 ++- native/suidhelper/tests/tools/network_args.rs | 16 ++++++++++-- 5 files changed, 62 insertions(+), 9 deletions(-) diff --git a/native/suidhelper/src/config.rs b/native/suidhelper/src/config.rs index ad32af3b..09e43c5c 100644 --- a/native/suidhelper/src/config.rs +++ b/native/suidhelper/src/config.rs @@ -144,6 +144,7 @@ pub struct Tools { blockdev: PathBuf, ip: PathBuf, nft: PathBuf, + sysctl: PathBuf, thin_dump: PathBuf, firecracker: Option, jailer: Option, @@ -160,6 +161,7 @@ impl Default for Tools { // paths. iproute2's real binary lives at /usr/bin/ip. ip: "/usr/bin/ip".into(), nft: "/usr/sbin/nft".into(), + sysctl: "/usr/sbin/sysctl".into(), thin_dump: "/usr/sbin/thin_dump".into(), firecracker: None, jailer: None, @@ -266,6 +268,13 @@ impl Config { SafeBin::from_path(&self.tools.nft) } + /// The `sysctl` binary, used to enable IPv4 forwarding inside a VM's netns + /// (a fresh netns defaults `net.ipv4.ip_forward` to 0, independent of the + /// host, so the netns would not route the guest's egress without it). + pub fn sysctl(&self) -> Result, safe_bin::Error> { + SafeBin::from_path(&self.tools.sysctl) + } + /// The `[network]` table, or `None` when VM networking is disabled — either /// because `[network]` is absent entirely, or because it is present but /// `uplink` is not set. diff --git a/native/suidhelper/src/tools/network/args.rs b/native/suidhelper/src/tools/network/args.rs index a33d49c3..4f25a1ba 100644 --- a/native/suidhelper/src/tools/network/args.rs +++ b/native/suidhelper/src/tools/network/args.rs @@ -102,13 +102,23 @@ impl Command { full.extend(argv); Self::ip(full) } + + /// `ip netns exec ...` — runs a `sysctl` write inside the + /// VM's netns. Like [`nft_ns`], it stays a [`Which::Ip`] command (only `ip` + /// and `nft` are ever the directly-named privileged binary) and takes the + /// *absolute* sysctl path, since the helper clears `PATH` before spawning. + fn sysctl_ns(netns: &str, sysctl: &str, argv: Vec) -> Self { + let mut full = argv!["netns", "exec", netns, sysctl]; + full.extend(argv); + Self::ip(full) + } } /// Bring up the VM's netns, host/ns veth pair, TAP device, and in-netns NAT so /// the guest's inner address (`addr::INNER_GUEST_IP`) can reach the host's /// uplink. The default route is requested last, after the host veth end /// (`hv`) is up, so the route's target is already reachable. -pub fn prepare_commands(plan: &Plan, nft: &str) -> Vec { +pub fn prepare_commands(plan: &Plan, nft: &str, sysctl: &str) -> Vec { let netns = plan.netns.as_str(); let veth_host = plan.veth_host.as_str(); let veth_ns = plan.veth_ns.as_str(); @@ -154,6 +164,11 @@ pub fn prepare_commands(plan: &Plan, nft: &str) -> Vec { Command::ip_ns(netns, argv!["link", "set", addr::TAP_NAME, "up"]), Command::ip_ns(netns, argv!["link", "set", "lo", "up"]), Command::ip_ns(netns, argv!["route", "add", "default", "via", veth_host_ip]), + // A fresh netns defaults net.ipv4.ip_forward to 0 (independent of the + // host's setting), so without this the netns silently drops every + // packet it would route between the guest's tap and the veth uplink — + // i.e. all guest egress. Enable it once the netns exists. + Command::sysctl_ns(netns, sysctl, argv!["-w", "net.ipv4.ip_forward=1"]), ]; commands.extend(nft_prepare_commands(netns, veth_ns, veth_ns_ip, nft)); commands diff --git a/native/suidhelper/src/tools/network/prepare.rs b/native/suidhelper/src/tools/network/prepare.rs index 27ca6dd4..e64c2e85 100644 --- a/native/suidhelper/src/tools/network/prepare.rs +++ b/native/suidhelper/src/tools/network/prepare.rs @@ -82,11 +82,16 @@ fn parse_cidr_base(s: &str) -> Result { /// the same `uid` + config; only the argv sequence they run differs. Every /// refusal here (`[network]` absent, uid out of range, malformed CIDR, a /// misconfigured binary) happens before any privileged command is spawned. -pub(super) fn resolve(uid: u32, vm_id: &VmId) -> Result<(Plan, PathBuf, PathBuf), Error> { +pub(super) fn resolve(uid: u32, vm_id: &VmId) -> Result<(Plan, PathBuf, PathBuf, PathBuf), Error> { let config = Config::get(); let network = super::resolve_network(config.network())?; let plan = plan_from(uid, config.uid_gid_range(), &network.clone_pool, vm_id)?; - Ok((plan, config.ip()?.into(), config.nft()?.into())) + Ok(( + plan, + config.ip()?.into(), + config.nft()?.into(), + config.sysctl()?.into(), + )) } pub fn run(args: PrepareArgs) -> Result { @@ -99,12 +104,18 @@ struct Prepare { plan: Plan, ip: PathBuf, nft: PathBuf, + sysctl: PathBuf, } impl Prepare { fn new(args: PrepareArgs) -> Result { - let (plan, ip, nft) = resolve(args.uid, &args.vm_id)?; - Ok(Self { plan, ip, nft }) + let (plan, ip, nft, sysctl) = resolve(args.uid, &args.vm_id)?; + Ok(Self { + plan, + ip, + nft, + sysctl, + }) } } @@ -114,7 +125,11 @@ impl IsTool for Prepare { type RunT = Result<(), Error>; fn run_privileged(&self) -> Self::RunT { - let commands = args::prepare_commands(&self.plan, &self.nft.to_string_lossy()); + let commands = args::prepare_commands( + &self.plan, + &self.nft.to_string_lossy(), + &self.sysctl.to_string_lossy(), + ); exec::run_all(&commands, |which| match which { args::Which::Ip => self.ip.as_path(), args::Which::Nft => self.nft.as_path(), diff --git a/native/suidhelper/src/tools/network/teardown.rs b/native/suidhelper/src/tools/network/teardown.rs index 877500cb..146b8401 100644 --- a/native/suidhelper/src/tools/network/teardown.rs +++ b/native/suidhelper/src/tools/network/teardown.rs @@ -42,7 +42,9 @@ struct Teardown { impl Teardown { fn new(args: TeardownArgs) -> Result { - let (plan, ip, nft) = prepare::resolve(args.uid, &args.vm_id)?; + // Teardown only deletes the netns/veth (no in-netns sysctl), so the + // resolved sysctl path is unused here. + let (plan, ip, nft, _sysctl) = prepare::resolve(args.uid, &args.vm_id)?; Ok(Self { plan, ip, nft }) } } diff --git a/native/suidhelper/tests/tools/network_args.rs b/native/suidhelper/tests/tools/network_args.rs index 51f87ba5..98e6226f 100644 --- a/native/suidhelper/tests/tools/network_args.rs +++ b/native/suidhelper/tests/tools/network_args.rs @@ -18,7 +18,7 @@ fn plan0() -> Plan { #[test] fn prepare_creates_netns_first_and_default_route_last() { - let cmds = args::prepare_commands(&plan0(), "/usr/sbin/nft"); + let cmds = args::prepare_commands(&plan0(), "/usr/sbin/nft", "/usr/sbin/sysctl"); let first = &cmds[0]; assert!(matches!(first.bin, Which::Ip)); assert_eq!( @@ -113,7 +113,8 @@ fn prepare_commands_golden_sequence() { // In-netns nft runs via `ip netns exec ` — the absolute // path, since the helper clears PATH before spawning (see nft_ns). let nft = "/usr/sbin/nft"; - let cmds = args::prepare_commands(&plan0(), nft); + let sysctl = "/usr/sbin/sysctl"; + let cmds = args::prepare_commands(&plan0(), nft, sysctl); let expected = vec![ cmd(Which::Ip, &["netns", "add", netns]), cmd( @@ -142,6 +143,17 @@ fn prepare_commands_golden_sequence() { Which::Ip, &["-n", netns, "route", "add", "default", "via", "172.31.0.1"], ), + cmd( + Which::Ip, + &[ + "netns", + "exec", + netns, + sysctl, + "-w", + "net.ipv4.ip_forward=1", + ], + ), cmd( Which::Ip, &["netns", "exec", netns, nft, "add", "table", "ip", "nat"], From 1d673739f2676860f6048fab0df416d566a8d2cb Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:15:04 +0000 Subject: [PATCH 37/42] test(e2e): temporary guest network diagnostics on egress failure Capture resolv.conf, route, raw-IP egress, and DNS from inside the guest so a failing egress run pinpoints the broken layer. Reverted once egress is green. --- test/e2e/network_test.exs | 19 ++++++++++++++++++- 1 file changed, 18 insertions(+), 1 deletion(-) diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 80ebb647..272cdb73 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -42,6 +42,23 @@ defmodule Hyper.E2e.NetworkTest do assert ip_out =~ "172.30.0.2" + # TEMP DIAGNOSTIC: capture the guest's runtime network state so a failing + # egress run tells us which layer is broken (resolv.conf write? raw L3 + # egress? DNS specifically?). Removed once egress is green. + {:ok, %{stdout: diag}} = + await_exec( + vm, + [ + "/bin/sh", + "-c", + "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> + "echo '--route--'; ip route 2>&1; " <> + "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> + "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -6" + ], + :timer.seconds(90) + ) + # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free # exit. `wget -S` writes the server's response headers to stderr; we merge # them into stdout (`2>&1`) and assert the status line is `... 200 ...`. @@ -57,6 +74,6 @@ defmodule Hyper.E2e.NetworkTest do ) assert http_out =~ ~r"HTTP/[\d.]+ 200\b", - "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}" + "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}\n\nguest net diagnostics:\n#{diag}" end end From 9daeb6031f0f3c07306ee8f49265768ee8ab96eb Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:22:13 +0000 Subject: [PATCH 38/42] test(e2e): log guest net diagnostics via Logger ExUnit drops the custom message on a pattern-match assert, so the prior diag never surfaced. Emit it via Logger.warning (flows to CI console) and tolerate a non-ok exec. Temporary. --- test/e2e/network_test.exs | 35 +++++++++++++++++++++-------------- 1 file changed, 21 insertions(+), 14 deletions(-) diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 272cdb73..6f8f4c77 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -44,20 +44,27 @@ defmodule Hyper.E2e.NetworkTest do # TEMP DIAGNOSTIC: capture the guest's runtime network state so a failing # egress run tells us which layer is broken (resolv.conf write? raw L3 - # egress? DNS specifically?). Removed once egress is green. - {:ok, %{stdout: diag}} = - await_exec( - vm, - [ - "/bin/sh", - "-c", - "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> - "echo '--route--'; ip route 2>&1; " <> - "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> - "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -6" - ], - :timer.seconds(90) - ) + # egress? DNS specifically?). Logged via Logger (ExUnit drops the custom + # message on a pattern-match assert). Removed once egress is green. + diag = + case await_exec( + vm, + [ + "/bin/sh", + "-c", + "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> + "echo '--route--'; ip route 2>&1; " <> + "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> + "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -8" + ], + :timer.seconds(90) + ) do + {:ok, r} -> r.stdout + other -> inspect(other) + end + + require Logger + Logger.warning("=== GUEST NET DIAG ===\n#{diag}=== END GUEST NET DIAG ===") # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free # exit. `wget -S` writes the server's response headers to stderr; we merge From dd46b5dadd069dd8aecd5bea626fba18790f400e Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:31:04 +0000 Subject: [PATCH 39/42] test(e2e): single long-deadline egress exec with full diagnostics Collapse the three separate guest execs (each a fresh agent-readiness gamble, one flaking to :timeout) into one 120s exec that runs ip/resolv/route/egress-by- ip/dns/http and logs the lot. Fewer flake surfaces; full guest net state on any failure. Temporary diagnostics. --- test/e2e/network_test.exs | 66 ++++++++++++++------------------------- 1 file changed, 23 insertions(+), 43 deletions(-) diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 6f8f4c77..188dd3ab 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -34,53 +34,33 @@ defmodule Hyper.E2e.NetworkTest do assert {:ok, vm} = Hyper.create_vm(%Hyper.Vm.Spec{img_id: img_id, type: :micro}) on_exit(fn -> Hyper.Node.stop_image_vm(vm) end) - # Hyper.exec runs argv directly with no PATH (see its @doc): a bare command - # name returns exit 127. Route through an absolute `/bin/sh -c` so busybox's - # own default PATH resolves the `ip`/`wget` applets inside the guest. - assert {:ok, %{stdout: ip_out, exit_code: 0}} = - await_exec(vm, ["/bin/sh", "-c", "ip addr show eth0"]) + # One exec gambles on guest-agent readiness once (not per-check), with a + # generous deadline for a cold boot on a shared runner. Hyper.exec runs argv + # directly with no PATH (see its @doc) — a bare name returns exit 127 — so + # route through an absolute `/bin/sh -c` and let busybox's own default PATH + # resolve the `ip`/`wget`/`nslookup` applets inside the guest. + # + # `wget -S` writes response headers to stderr; `2>&1` merges them into stdout + # so we can assert the `... 200 ...` status line. Alpine ships busybox wget + # (not curl); `-O /dev/null` discards the body but still drives the full + # netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. + script = + "echo '--ip--'; ip addr show eth0 2>&1; " <> + "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> + "echo '--route--'; ip route 2>&1; " <> + "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> + "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -8; " <> + "echo '--http--'; wget -S -O /dev/null http://example.com 2>&1" - assert ip_out =~ "172.30.0.2" - - # TEMP DIAGNOSTIC: capture the guest's runtime network state so a failing - # egress run tells us which layer is broken (resolv.conf write? raw L3 - # egress? DNS specifically?). Logged via Logger (ExUnit drops the custom - # message on a pattern-match assert). Removed once egress is green. - diag = - case await_exec( - vm, - [ - "/bin/sh", - "-c", - "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> - "echo '--route--'; ip route 2>&1; " <> - "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> - "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -8" - ], - :timer.seconds(90) - ) do - {:ok, r} -> r.stdout - other -> inspect(other) - end + assert {:ok, %{stdout: out, exit_code: _}} = + await_exec(vm, ["/bin/sh", "-c", script], :timer.seconds(120)) require Logger - Logger.warning("=== GUEST NET DIAG ===\n#{diag}=== END GUEST NET DIAG ===") + Logger.warning("=== GUEST NET DIAG ===\n#{out}\n=== END GUEST NET DIAG ===") - # Prove real egress AND an explicit HTTP 200 — not just a non-zero-free - # exit. `wget -S` writes the server's response headers to stderr; we merge - # them into stdout (`2>&1`) and assert the status line is `... 200 ...`. - # (Alpine ships busybox wget, not curl — installing curl would need egress - # to the apk mirror first, adding a flaky dependency to prove the same - # thing.) `-O /dev/null` discards the body; the fetch still exercises the - # full netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. - assert {:ok, %{stdout: http_out, exit_code: 0}} = - await_exec( - vm, - ["/bin/sh", "-c", "wget -S -O /dev/null http://example.com 2>&1"], - :timer.seconds(90) - ) + assert out =~ "172.30.0.2", "guest eth0 lacks the inner-world address:\n#{out}" - assert http_out =~ ~r"HTTP/[\d.]+ 200\b", - "expected an HTTP 200 status line from the guest's egress fetch, got:\n#{http_out}\n\nguest net diagnostics:\n#{diag}" + assert out =~ ~r"HTTP/[\d.]+ 200\b", + "expected an HTTP 200 status line from the guest's egress fetch:\n#{out}" end end From 7174abac05a3a655e75153c6e28fae5a71530a1b Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:39:50 +0000 Subject: [PATCH 40/42] ci(integration): admit guest egress through Docker's FORWARD drop MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GitHub runners ship Docker, which sets the iptables filter FORWARD policy to DROP. Hyper's per-VM guest egress is forwarded (netns veth -> uplink), so every outbound packet was silently dropped there — the guest configured eth0/route/ resolv.conf correctly but nothing ever returned (all egress timed out, DNS included). Admit the clone pool via DOCKER-USER both directions; the hyper nft forward chain still enforces isolation. --- .github/scripts/provision-kvm-host.sh | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/.github/scripts/provision-kvm-host.sh b/.github/scripts/provision-kvm-host.sh index 4275024b..86eb231d 100755 --- a/.github/scripts/provision-kvm-host.sh +++ b/.github/scripts/provision-kvm-host.sh @@ -64,6 +64,22 @@ sudo sysctl -w net.ipv4.ip_forward=1 echo 'net.ipv4.ip_forward=1' \ | sudo tee /etc/sysctl.d/99-hyper-ip-forward.conf >/dev/null +# GitHub-hosted runners ship Docker, which sets the iptables `filter` FORWARD +# policy to DROP and only admits its own bridge traffic. Hyper's guest egress is +# *forwarded* (per-VM netns veth -> uplink), so Docker's drop silently eats +# every packet the guest sends — the guest configures its NIC fine but nothing +# ever returns. Admit the clone pool (172.31.0.0/16, the helper's default) both +# directions via DOCKER-USER, which Docker evaluates before its own drops; the +# `hyper` nftables forward chain still enforces guest isolation. Fall back to an +# accepting FORWARD policy on a host without Docker's chain. +clone_pool="172.31.0.0/16" +if sudo iptables -L DOCKER-USER >/dev/null 2>&1; then + sudo iptables -I DOCKER-USER -s "${clone_pool}" -j ACCEPT + sudo iptables -I DOCKER-USER -d "${clone_pool}" -j ACCEPT +else + sudo iptables -P FORWARD ACCEPT +fi + # The default-route interface is the uplink guests NAT egress through. uplink="$(ip route show default | awk '{print $5; exit}')" [ -n "${uplink}" ] || { echo "ERROR: could not detect default-route uplink interface" >&2; exit 1; } From 60e603987dc1698a2243a5d7b98f795232f9beae Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Wed, 8 Jul 2026 22:46:41 +0000 Subject: [PATCH 41/42] test(e2e): clean up network egress test; docs: FORWARD firewall note Egress is confirmed working end-to-end (guest resolves DNS + gets HTTP 200). Revert the network test from diagnostic mode to a clean single-exec (one agent- readiness gamble, 120s deadline): assert eth0 carries 172.30.0.2 and a real wget returns an explicit HTTP 200. Mirror the provision script's Docker-FORWARD workaround into install.md so operators on a Docker/firewalled host aren't silently bitten. --- docs/cookbook/install.md | 17 +++++++++++++++++ test/e2e/network_test.exs | 21 ++++++++------------- 2 files changed, 25 insertions(+), 13 deletions(-) diff --git a/docs/cookbook/install.md b/docs/cookbook/install.md index 106340dd..23fa9fc7 100644 --- a/docs/cookbook/install.md +++ b/docs/cookbook/install.md @@ -223,6 +223,23 @@ sudo sysctl -w net.ipv4.ip_forward=1 > | sudo tee /etc/sysctl.d/99-hyper-ip-forward.conf > ``` +> #### Conflicting FORWARD firewall {: .warning} +> +> Guest egress is *forwarded* (per-VM netns veth → uplink). If the host already +> runs a firewall that defaults the `filter` FORWARD chain to DROP — Docker does +> this, and so do some hardened base images — those drops silently eat every +> guest packet: the guest configures its NIC correctly but nothing ever returns. +> Admit the clone pool through the firewall's user hook (Docker evaluates +> `DOCKER-USER` before its own drops); the `hyper` nft forward chain still +> enforces guest isolation: +> +> ```sh +> sudo iptables -I DOCKER-USER -s 172.31.0.0/16 -j ACCEPT +> sudo iptables -I DOCKER-USER -d 172.31.0.0/16 -j ACCEPT +> ``` +> +> On a host without Docker, ensure nothing else sets the FORWARD policy to DROP. + Then set `uplink` to the physical NIC guests should NAT out through — the default-route interface is a reasonable choice on a single-uplink host: diff --git a/test/e2e/network_test.exs b/test/e2e/network_test.exs index 188dd3ab..4a7e4eae 100644 --- a/test/e2e/network_test.exs +++ b/test/e2e/network_test.exs @@ -38,26 +38,21 @@ defmodule Hyper.E2e.NetworkTest do # generous deadline for a cold boot on a shared runner. Hyper.exec runs argv # directly with no PATH (see its @doc) — a bare name returns exit 127 — so # route through an absolute `/bin/sh -c` and let busybox's own default PATH - # resolve the `ip`/`wget`/`nslookup` applets inside the guest. + # resolve the `ip`/`wget` applets inside the guest. # - # `wget -S` writes response headers to stderr; `2>&1` merges them into stdout - # so we can assert the `... 200 ...` status line. Alpine ships busybox wget - # (not curl); `-O /dev/null` discards the body but still drives the full + # `ip addr show eth0` proves the uniform inner-world address is configured; + # the `wget` then proves real egress with an explicit HTTP 200 (not just a + # zero exit): `-S` writes response headers to stderr, merged via `2>&1` so we + # can match the `... 200 ...` status line. Alpine ships busybox wget, not + # curl; `-O /dev/null` discards the body but still drives the full # netns → SNAT → veth → MASQUERADE → uplink path and DNS resolution. script = - "echo '--ip--'; ip addr show eth0 2>&1; " <> - "echo '--resolv--'; cat /etc/resolv.conf 2>&1; " <> - "echo '--route--'; ip route 2>&1; " <> - "echo '--egress-by-ip--'; wget -T 10 -S -O /dev/null http://1.1.1.1/ 2>&1 | head -4; " <> - "echo '--dns--'; nslookup example.com 1.1.1.1 2>&1 | head -8; " <> + "echo '--ip--'; ip addr show eth0; " <> "echo '--http--'; wget -S -O /dev/null http://example.com 2>&1" - assert {:ok, %{stdout: out, exit_code: _}} = + assert {:ok, %{stdout: out, exit_code: 0}} = await_exec(vm, ["/bin/sh", "-c", script], :timer.seconds(120)) - require Logger - Logger.warning("=== GUEST NET DIAG ===\n#{out}\n=== END GUEST NET DIAG ===") - assert out =~ "172.30.0.2", "guest eth0 lacks the inner-world address:\n#{out}" assert out =~ ~r"HTTP/[\d.]+ 200\b", From 295509b3414fdf275692d5f3c2dfe6441b2f8d0f Mon Sep 17 00:00:00 2001 From: Marko Vejnovic Date: Thu, 9 Jul 2026 03:08:56 +0000 Subject: [PATCH 42/42] refactor(fire_vmm): tighten networking types + drop dead code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review follow-ups on the mandatory-networking work: - Make Vm.source's :vm_id required, not optional. Networking is mandatory and BootSpec.resolve/2 Map.fetch!'es it, so optional was a lie a future caller could typecheck past and crash on. Thread vm_id through State.boot_source/4 so the map is complete by construction rather than patched in via Map.put. - Remove Net.guest_iface/0 (dead — no callers anywhere). - Inline Daemon.reset_stale_jail/1 (a one-line delegate) into prelaunch/1 and fix its mangled comment. - Drop a bare resolver/0 default-value test (restated a literal, no contract). --- lib/hyper/node/fire_vmm/daemon.ex | 13 +++++-------- lib/hyper/node/fire_vmm/net.ex | 4 ---- lib/hyper/node/fire_vmm/state.ex | 18 ++++++++++-------- lib/hyper/vm.ex | 8 ++++---- test/hyper/cfg/network_test.exs | 6 ------ 5 files changed, 19 insertions(+), 30 deletions(-) diff --git a/lib/hyper/node/fire_vmm/daemon.ex b/lib/hyper/node/fire_vmm/daemon.ex index 18a564df..56713f40 100644 --- a/lib/hyper/node/fire_vmm/daemon.ex +++ b/lib/hyper/node/fire_vmm/daemon.ex @@ -91,20 +91,17 @@ defmodule Hyper.Node.FireVMM.Daemon do end end - # The testable seam: reset any stale jail from a prior incarnation, then — - # Prepare the netns the jailer's `--netns` will enter. Must run before - # `launch/1`: the jailer refuses to start if the netns it is told to enter - # does not already exist. Networking is mandatory, so this is unconditional. + # Clear any stale jail + netns left by a prior incarnation, then prepare the + # fresh netns the jailer's `--netns` will enter. Must run before `launch/1`: + # the jailer refuses to start if the netns it is told to enter does not yet + # exist. Networking is mandatory, so this is unconditional. @spec prelaunch(Opts.t()) :: :ok | {:error, term()} defp prelaunch(%Opts{vm_id: id, uid: uid} = opts) do - with :ok <- reset_stale_jail(opts) do + with :ok <- clear_jail(opts) do Hyper.SuidHelper.Network.prepare(id, uid) end end - @spec reset_stale_jail(Opts.t()) :: :ok | {:error, term()} - defp reset_stale_jail(opts), do: clear_jail(opts) - # Best-effort: both the chroot/cgroup removal and the network teardown are # attempted regardless of the other's outcome (a `Reaper` backstops either # failure); the first error is surfaced to the caller. diff --git a/lib/hyper/node/fire_vmm/net.ex b/lib/hyper/node/fire_vmm/net.ex index f4f1942e..a323f0ab 100644 --- a/lib/hyper/node/fire_vmm/net.ex +++ b/lib/hyper/node/fire_vmm/net.ex @@ -50,8 +50,4 @@ defmodule Hyper.Node.FireVMM.Net do def interface(vm_id) do %NetworkInterface{iface_id: @iface, host_dev_name: @tap, guest_mac: guest_mac(vm_id)} end - - @doc false - @spec guest_iface() :: String.t() - def guest_iface, do: @iface end diff --git a/lib/hyper/node/fire_vmm/state.ex b/lib/hyper/node/fire_vmm/state.ex index 2a1405b9..4e1b7bbe 100644 --- a/lib/hyper/node/fire_vmm/state.ex +++ b/lib/hyper/node/fire_vmm/state.ex @@ -92,7 +92,7 @@ defmodule Hyper.Node.FireVMM.State do ) do case Hyper.Cluster.Routing.register_self({id, :state}) do :ok -> - source = Map.put(boot_source(kernel, Mutable.blk_path(mutable), boot_args), :vm_id, id) + source = boot_source(id, kernel, Mutable.blk_path(mutable), boot_args) spec = BootSpec.resolve(source, type) deadline = System.monotonic_time(:millisecond) + Time.as_ms(@ready_timeout) data = %State{opts: opts, spec: spec, boot_deadline: deadline} @@ -105,13 +105,15 @@ defmodule Hyper.Node.FireVMM.State do end # Assemble the `Hyper.Vm.source()` BootSpec expects from the resolved kernel + - # device. `boot_args` is omitted when nil so BootSpec applies its default. - @spec boot_source(Path.t(), Path.t(), String.t() | nil) :: Hyper.Vm.source() - defp boot_source(kernel, dev, nil), - do: %{kernel_image_path: kernel, root_drive_path: dev} - - defp boot_source(kernel, dev, boot_args), - do: %{kernel_image_path: kernel, root_drive_path: dev, boot_args: boot_args} + # device. `boot_args` is omitted when nil so BootSpec applies its default; + # `vm_id` is always included (networking is mandatory — BootSpec derives the + # guest NIC from it). + @spec boot_source(Hyper.Vm.Id.t(), Path.t(), Path.t(), String.t() | nil) :: Hyper.Vm.source() + defp boot_source(vm_id, kernel, dev, nil), + do: %{vm_id: vm_id, kernel_image_path: kernel, root_drive_path: dev} + + defp boot_source(vm_id, kernel, dev, boot_args), + do: %{vm_id: vm_id, kernel_image_path: kernel, root_drive_path: dev, boot_args: boot_args} @impl :gen_statem # Answered in every lifecycle state: the opts are static data, and a fork must diff --git a/lib/hyper/vm.ex b/lib/hyper/vm.ex index 39fcd416..219a9c3a 100644 --- a/lib/hyper/vm.ex +++ b/lib/hyper/vm.ex @@ -8,14 +8,14 @@ defmodule Hyper.Vm do @typedoc """ What a VM boots from: explicit, already-jail-visible artifact paths for a cold boot (kernel + root drive). `boot_args` defaults to a standard serial-console - cmdline when omitted. `vm_id` is required only when networking is enabled - (`BootSpec.resolve/2` derives the guest NIC from it). + cmdline when omitted. `vm_id` is required — networking is mandatory and + `BootSpec.resolve/2` derives the guest NIC (`ip=` cmdline + MAC) from it. """ @type source :: %{ required(:kernel_image_path) => Path.t(), required(:root_drive_path) => Path.t(), - optional(:boot_args) => String.t(), - optional(:vm_id) => Hyper.Vm.Id.t() + required(:vm_id) => Hyper.Vm.Id.t(), + optional(:boot_args) => String.t() } @doc """ diff --git a/test/hyper/cfg/network_test.exs b/test/hyper/cfg/network_test.exs index b852d1c3..50fddcb9 100644 --- a/test/hyper/cfg/network_test.exs +++ b/test/hyper/cfg/network_test.exs @@ -11,12 +11,6 @@ defmodule Hyper.Cfg.NetworkTest do end end - describe "resolver/0" do - test "defaults when unset" do - assert Network.resolver() == "1.1.1.1" - end - end - describe "configured?/0" do test "false when no uplink configured" do # Base test config sets no [network] table. This is the predicate the