From 83c0e4b4bfcbedae18d3daf0244aefa9a54444e9 Mon Sep 17 00:00:00 2001 From: atlas Date: Sat, 29 Aug 2026 12:34:05 +0200 Subject: [PATCH] require network isolation, deleting the residual non-isolated branch Per mara on #3725: the on/off toggle is removed, and required env vars unset lead to a crash. HIVE_NETWORK_ISOLATION is gone from hive-network.nix -- it was the toggle. Validation happens once at daemon startup rather than per container. The variables are process-global, so a bad value breaks every container rather than one: failing at boot gives a single diagnostic naming the bad value, and cannot reach a state where some containers were configured before it was noticed. Option collapses to NetworkIsolation through the wire type, client and helper, which deletes the branch instead of leaving it unreachable. serde(default) is dropped on that field deliberately: a request omitting isolation is now rejected rather than defaulting to a container sharing the host's network namespace. What this replaces was a silent security downgrade. Of the four ways into the old fallback, two logged nothing at all -- a container came up without isolation and the journal agreed it was fine. Doc comments that still described the removed branch are updated (argus's note on #3723 scoped that to this issue). The hive-priv one is a minimal edit inside the block #3723 rewrites; de-splicing is that PR's job. --- hive-c0re/src/lifecycle/host_config.rs | 49 ++++++--------------- hive-c0re/src/lifecycle/mod.rs | 50 ++++++++++++++++++++++ hive-c0re/src/lifecycle/tests.rs | 59 ++++++++++++++++++++++++++ hive-c0re/src/main.rs | 9 ++++ hive-c0re/src/priv_client.rs | 2 +- hive-priv-sock/src/lib.rs | 23 ++++++---- hive-priv/src/main.rs | 54 +++++++++-------------- nix/host-modules/hive-network.nix | 1 - 8 files changed, 165 insertions(+), 82 deletions(-) diff --git a/hive-c0re/src/lifecycle/host_config.rs b/hive-c0re/src/lifecycle/host_config.rs index 3b572895..2064d501 100644 --- a/hive-c0re/src/lifecycle/host_config.rs +++ b/hive-c0re/src/lifecycle/host_config.rs @@ -10,8 +10,8 @@ use hive_priv_sock::{BindMount, CredentialMount}; use crate::coordinator::{AgentPaths, HiveEnv}; use super::{ - AGENT_PREFIX, CONTAINER_RUNTIME_MOUNT, CONTAINER_SHARED_MOUNT, bridge_gateway_ip, - container_claude_mount, container_name, validate, + AGENT_PREFIX, CONTAINER_RUNTIME_MOUNT, CONTAINER_SHARED_MOUNT, container_claude_mount, + container_name, validate, }; /// Re-apply the per-container host-side config: nspawn flags (bind @@ -303,40 +303,17 @@ async fn set_nspawn_flags( read_only: false, }); - // Network isolation: when HIVE_NETWORK_ISOLATION=1 is set (by the - // hive-network.nix module's `isolateContainers` option), flip the - // container to a private network namespace with a veth pair attached - // to the host bridge. Applies to all containers including the manager - // (all hive-c0re<->agent comms go through bind-mounted UDS, not TCP). - let isolation = { - let isolate = std::env::var("HIVE_NETWORK_ISOLATION").ok().as_deref() == Some("1"); - let bridge = std::env::var("HIVE_NETWORK_BRIDGE").unwrap_or_default(); - let subnet = std::env::var("HIVE_NETWORK_SUBNET").unwrap_or_default(); - if isolate && !bridge.is_empty() && !subnet.is_empty() { - let Some(gateway_ip) = bridge_gateway_ip(&subnet) else { - tracing::warn!( - %agent_name, %subnet, - "HIVE_NETWORK_SUBNET is set but the bridge gateway IP is unparseable; \ - skipping PRIVATE_NETWORK write to avoid an isolated container with no \ - default route or resolver" - ); - return crate::priv_client::write_nspawn_flags( - container, - &binds, - None, - &load_creds, - ) - .await; - }; - tracing::info!( - %agent_name, %gateway_ip, %bridge, - "network isolation: PRIVATE_NETWORK=1 (DHCP)" - ); - Some(hive_priv_sock::NetworkIsolation { bridge, gateway_ip }) - } else { - None - } - }; + // Every container runs in a private network namespace with a veth + // pair attached to the host bridge — including the manager (all + // hive-c0re<->agent comms go through bind-mounted UDS, not TCP). + // There is no non-isolated mode to fall back to, and the settings + // are process-global, so anything wrong here was already fatal at + // daemon startup; this call cannot newly fail. + let isolation = super::network_isolation_from_env()?; + tracing::info!( + %agent_name, gateway_ip = %isolation.gateway_ip, bridge = %isolation.bridge, + "network isolation: PRIVATE_NETWORK=1 (DHCP)" + ); // Delegate the actual conf-file rewrite to hive-priv (runs as root). crate::priv_client::write_nspawn_flags(container, &binds, isolation, &load_creds).await diff --git a/hive-c0re/src/lifecycle/mod.rs b/hive-c0re/src/lifecycle/mod.rs index 36cd6cc1..44b8816e 100644 --- a/hive-c0re/src/lifecycle/mod.rs +++ b/hive-c0re/src/lifecycle/mod.rs @@ -125,6 +125,56 @@ pub fn bridge_gateway_ip(subnet_cidr: &str) -> Option { Some(ip_str.to_owned()) } +/// Build the network-isolation settings every container is configured +/// with, from the variables `hive-network.nix` sets on the `hive-c0re` +/// unit. +/// +/// Isolation is the only supported mode: the on/off toggle is gone, so +/// there is no non-isolated branch to fall back to and a missing or +/// malformed value means the daemon is misconfigured — not that a +/// container should quietly come up sharing the host's netns. Silently +/// degrading here dropped a security boundary with nothing in the log +/// to say so. +/// +/// Split from [`network_isolation_from_env`] so the parsing is testable +/// without touching process environment. +pub fn network_isolation_from_vars( + bridge: Option<&str>, + subnet: Option<&str>, +) -> Result { + let bridge = bridge.filter(|s| !s.is_empty()).context( + "HIVE_NETWORK_BRIDGE is unset or empty — hive-network.nix sets it on the \ + hive-c0re unit, so this means the daemon is running outside its unit or \ + with a broken module evaluation", + )?; + let subnet = subnet.filter(|s| !s.is_empty()).context( + "HIVE_NETWORK_SUBNET is unset or empty — hive-network.nix sets it on the \ + hive-c0re unit, so this means the daemon is running outside its unit or \ + with a broken module evaluation", + )?; + let gateway_ip = bridge_gateway_ip(subnet).with_context(|| { + format!( + "HIVE_NETWORK_SUBNET={subnet} is not a valid / pair; \ + it comes from services.hyperhive.network.bridgeIp + bridgePrefixLength" + ) + })?; + Ok(hive_priv_sock::NetworkIsolation { + bridge: bridge.to_owned(), + gateway_ip, + }) +} + +/// [`network_isolation_from_vars`] over the real process environment. +/// +/// Called once at daemon startup so a bad value fails the unit loudly, +/// and again per container — the variables are process-global, so the +/// second call cannot start failing once the first has passed. +pub fn network_isolation_from_env() -> Result { + let bridge = std::env::var("HIVE_NETWORK_BRIDGE").ok(); + let subnet = std::env::var("HIVE_NETWORK_SUBNET").ok(); + network_isolation_from_vars(bridge.as_deref(), subnet.as_deref()) +} + /// Read the agent user's `(uid, gid)` from the container's nixos-managed /// `/etc/passwd`. Returns `None` when the container hasn't been built /// yet, the passwd file is unparseable, or the agent user is missing diff --git a/hive-c0re/src/lifecycle/tests.rs b/hive-c0re/src/lifecycle/tests.rs index fc699024..81e925eb 100644 --- a/hive-c0re/src/lifecycle/tests.rs +++ b/hive-c0re/src/lifecycle/tests.rs @@ -65,6 +65,65 @@ fn bridge_gateway_ip_rejects_bad_input() { assert!(bridge_gateway_ip("10.42.0/24").is_none()); // 3 octets } +/// The presence control for the two rejection tests below: with both +/// variables set and well-formed, the settings are built. Without this, +/// a `network_isolation_from_vars` that rejected *everything* would pass +/// every absence assertion and look like a working guard. +#[test] +fn network_isolation_accepts_a_well_formed_pair() { + let iso = network_isolation_from_vars(Some("hive0"), Some("10.42.0.1/24")) + .expect("well-formed bridge + subnet must be accepted"); + assert_eq!(iso.bridge, "hive0"); + // The gateway is the verbatim bridge address, prefix stripped. + assert_eq!(iso.gateway_ip, "10.42.0.1"); +} + +/// A missing or empty variable is fatal, not a fallback to the host +/// netns. Empty is tested alongside unset because `std::env::var` on a +/// variable set to `""` returns `Ok("")`, so treating only `None` as +/// missing would let an empty value through. +#[test] +fn network_isolation_rejects_missing_or_empty_vars() { + assert!(network_isolation_from_vars(None, Some("10.42.0.1/24")).is_err()); + assert!(network_isolation_from_vars(Some("hive0"), None).is_err()); + assert!(network_isolation_from_vars(None, None).is_err()); + assert!(network_isolation_from_vars(Some(""), Some("10.42.0.1/24")).is_err()); + assert!(network_isolation_from_vars(Some("hive0"), Some("")).is_err()); +} + +/// A malformed subnet is fatal too. Previously this logged a warning and +/// silently produced a container on the host netns — a dropped security +/// boundary with nothing in the journal saying so. +#[test] +fn network_isolation_rejects_a_malformed_subnet() { + assert!(network_isolation_from_vars(Some("hive0"), Some("notanip/24")).is_err()); + assert!(network_isolation_from_vars(Some("hive0"), Some("10.42.0.1")).is_err()); + assert!(network_isolation_from_vars(Some("hive0"), Some("10.42.0.1/33")).is_err()); + assert!(network_isolation_from_vars(Some("hive0"), Some("10.42.0.999/24")).is_err()); +} + +/// The error has to name the variable an operator must fix — these are +/// read at startup, so the message is the whole diagnostic. +#[test] +fn network_isolation_errors_name_the_offending_variable() { + let e = network_isolation_from_vars(None, Some("10.42.0.1/24")).unwrap_err(); + assert!( + format!("{e:#}").contains("HIVE_NETWORK_BRIDGE"), + "bridge error must name the variable, got: {e:#}" + ); + let e = network_isolation_from_vars(Some("hive0"), None).unwrap_err(); + assert!( + format!("{e:#}").contains("HIVE_NETWORK_SUBNET"), + "subnet error must name the variable, got: {e:#}" + ); + let e = network_isolation_from_vars(Some("hive0"), Some("nope/24")).unwrap_err(); + let msg = format!("{e:#}"); + assert!( + msg.contains("HIVE_NETWORK_SUBNET") && msg.contains("nope/24"), + "malformed-subnet error must name the variable and the bad value, got: {msg}" + ); +} + /// `setup_proposed` is idempotent: calling it on an existing repo is a /// no-op (the fresh guard skips all writes). #[tokio::test] diff --git a/hive-c0re/src/main.rs b/hive-c0re/src/main.rs index a2428fc0..1cfa654a 100644 --- a/hive-c0re/src/main.rs +++ b/hive-c0re/src/main.rs @@ -217,6 +217,15 @@ async fn main() -> Result<()> { if let Some(v) = build_slots { sc.build_slots = v; } + // Network isolation is required and its settings are + // process-global, so a bad value breaks every container, not + // one. Validate once here: the unit then fails visibly at + // start with a single diagnostic naming the bad value, + // instead of coming up "healthy" and failing each container + // configure separately — possibly after some already + // succeeded. + lifecycle::network_isolation_from_env() + .context("network isolation settings are required at startup")?; cmd_serve(sc.env, sc.model_prices, sc.build_slots, db, &cli.socket).await } } diff --git a/hive-c0re/src/priv_client.rs b/hive-c0re/src/priv_client.rs index 07a0f952..ec56567f 100644 --- a/hive-c0re/src/priv_client.rs +++ b/hive-c0re/src/priv_client.rs @@ -294,7 +294,7 @@ pub async fn read_container_journal( pub async fn write_nspawn_flags( container: &str, binds: &[BindMount], - isolation: Option, + isolation: NetworkIsolation, load_credentials: &[CredentialMount], ) -> Result<()> { ok(call(&PrivRequest::WriteNspawnFlags { diff --git a/hive-priv-sock/src/lib.rs b/hive-priv-sock/src/lib.rs index 6407c357..74364e30 100644 --- a/hive-priv-sock/src/lib.rs +++ b/hive-priv-sock/src/lib.rs @@ -291,9 +291,10 @@ pub struct CredentialMount { pub host_path: String, } -/// Network isolation parameters for `WriteNspawnFlags`. When `Some`, -/// hive-priv writes `PRIVATE_NETWORK=1` + veth bridge wiring instead -/// of the default `PRIVATE_NETWORK=0`. Containers receive their IP +/// Network isolation parameters for `WriteNspawnFlags`. hive-priv writes +/// `PRIVATE_NETWORK=1` + veth bridge wiring from these; every container +/// is isolated, so they are required rather than a mode selector. +/// Containers receive their IP /// dynamically via DHCP from the bridge dnsmasq pool (`networking.useDHCP` /// in `harness-base.nix`); no static address is pre-assigned here. #[derive(Debug, Clone, Serialize, Deserialize)] @@ -379,15 +380,19 @@ pub enum PrivRequest { /// Update `/etc/nixos-containers/.conf`: strip old network-isolation /// vars, write `PRIVATE_NETWORK` + bridge settings, and set `EXTRA_NSPAWN_FLAGS` /// from the provided bind-mount list. Written by `lifecycle::set_nspawn_flags`. - /// When `isolation` is `Some`, writes `PRIVATE_NETWORK=1` + veth wiring; - /// when `None`, writes `PRIVATE_NETWORK=0`. + /// Always writes `PRIVATE_NETWORK=1` + veth wiring: isolation is the only + /// mode, so there is no request shape that yields a container sharing the + /// host's network namespace. WriteNspawnFlags { container: String, binds: Vec, - /// `None` = host netns (`PRIVATE_NETWORK=0`). `Some` = private netns with - /// veth on the specified bridge (`PRIVATE_NETWORK=1`). - #[serde(default)] - isolation: Option, + /// Private netns with a veth on the given bridge + /// (`PRIVATE_NETWORK=1`). Required: isolation is the only + /// supported mode, so there is no value meaning "host netns". + /// Deliberately **not** `#[serde(default)]` — a request that + /// omits it is rejected rather than quietly configuring a + /// container that shares the host's network namespace. + isolation: NetworkIsolation, /// Host secrets forwarded into the container's credential store via /// nspawn `--load-credential=:`. Empty for agents /// with no credentials configured (the common case). `#[serde(default)]` diff --git a/hive-priv/src/main.rs b/hive-priv/src/main.rs index dbfb4ee6..8fc3305f 100644 --- a/hive-priv/src/main.rs +++ b/hive-priv/src/main.rs @@ -376,7 +376,7 @@ async fn exec( ref binds, ref isolation, ref load_credentials, - } => handle_write_nspawn_flags(container, binds, isolation.as_ref(), load_credentials), + } => handle_write_nspawn_flags(container, binds, isolation, load_credentials), PrivRequest::WriteResourceLimits { ref container, @@ -923,7 +923,7 @@ fn single_output_path(stdout: &str) -> Result<&str, usize> { fn handle_write_nspawn_flags( container: &str, binds: &[BindMount], - isolation: Option<&NetworkIsolation>, + isolation: &NetworkIsolation, load_credentials: &[CredentialMount], ) -> Result<(String, String)> { validate_container_system_name(container)?; @@ -2718,13 +2718,13 @@ fn git_overlay_flags(binds: &[BindMount]) -> Vec { /// Update `/etc/nixos-containers/.conf`: strip old network vars /// (`PRIVATE_NETWORK`, `HOST_ADDRESS*`, `LOCAL_ADDRESS*`, `HOST_BRIDGE`), /// write the current network-isolation settings, then append -/// `EXTRA_NSPAWN_FLAGS`. When `isolation` is `Some`, writes -/// `PRIVATE_NETWORK=1` + veth wiring; when `None`, writes -/// `PRIVATE_NETWORK=0`. +/// `EXTRA_NSPAWN_FLAGS`. Always writes `PRIVATE_NETWORK=1` + veth +/// wiring — isolation is the only mode, so there is no branch that +/// leaves a container on the host's network namespace. fn write_nspawn_flags( container: &str, binds: &[BindMount], - isolation: Option<&NetworkIsolation>, + isolation: &NetworkIsolation, load_credentials: &[CredentialMount], ) -> Result<()> { use std::fmt::Write as _; @@ -2747,7 +2747,8 @@ fn write_nspawn_flags( if !out.is_empty() { out.push('\n'); } - if let Some(iso) = isolation { + { + let iso = isolation; out.push_str("PRIVATE_NETWORK=1\n"); // HOST_ADDRESS = the bridge gateway IP. nixos-container's // container-side setup only installs a default route @@ -2766,13 +2767,6 @@ fn write_nspawn_flags( out.push_str("HOST_ADDRESS6=\n"); out.push_str("LOCAL_ADDRESS6=\n"); let _ = writeln!(out, "HOST_BRIDGE={}", iso.bridge); - } else { - out.push_str("PRIVATE_NETWORK=0\n"); - out.push_str("HOST_ADDRESS=\n"); - out.push_str("LOCAL_ADDRESS=\n"); - out.push_str("HOST_ADDRESS6=\n"); - out.push_str("LOCAL_ADDRESS6=\n"); - out.push_str("HOST_BRIDGE=\n"); } let mut flags: Vec = binds .iter() @@ -2817,29 +2811,19 @@ fn bridge_dns_marker_path(container: &str) -> String { /// Write (isolated) or remove (host-netns) the bridge-DNS marker the /// `hyperhive-isolated-dns` oneshot keys off. The marker file contains /// just the gateway IP. Best-effort on removal (absence is the goal). -fn write_bridge_dns_marker(container: &str, isolation: Option<&NetworkIsolation>) -> Result<()> { +fn write_bridge_dns_marker(container: &str, isolation: &NetworkIsolation) -> Result<()> { let path = bridge_dns_marker_path(container); - match isolation { - Some(iso) => { - // On a fresh install the container's `/etc` may not exist yet - // (rootfs not fully materialised before the first start), so - // `write` would fail with ENOENT. Create the parent dir first - // — it's the container's own `/etc`, which nixos-container - // populates on start; a pre-created dir + our marker persist. - if let Some(parent) = std::path::Path::new(&path).parent() { - std::fs::create_dir_all(parent).with_context(|| { - format!("create bridge-DNS marker dir {}", parent.display()) - })?; - } - std::fs::write(&path, format!("{}\n", iso.gateway_ip)) - .with_context(|| format!("write bridge-DNS marker {path}"))?; - } - None => match std::fs::remove_file(&path) { - Ok(()) => {} - Err(e) if e.kind() == std::io::ErrorKind::NotFound => {} - Err(e) => return Err(e).with_context(|| format!("remove bridge-DNS marker {path}")), - }, + // On a fresh install the container's `/etc` may not exist yet + // (rootfs not fully materialised before the first start), so + // `write` would fail with ENOENT. Create the parent dir first + // — it's the container's own `/etc`, which nixos-container + // populates on start; a pre-created dir + our marker persist. + if let Some(parent) = std::path::Path::new(&path).parent() { + std::fs::create_dir_all(parent) + .with_context(|| format!("create bridge-DNS marker dir {}", parent.display()))?; } + std::fs::write(&path, format!("{}\n", isolation.gateway_ip)) + .with_context(|| format!("write bridge-DNS marker {path}"))?; Ok(()) } diff --git a/nix/host-modules/hive-network.nix b/nix/host-modules/hive-network.nix index 957e5caf..c4eed090 100644 --- a/nix/host-modules/hive-network.nix +++ b/nix/host-modules/hive-network.nix @@ -264,7 +264,6 @@ in # container. HIVE_NETWORK_SUBNET is host-bridge IP/prefix, not canonical # network address — the Rust side normalises before subnet arithmetic. systemd.services.hive-c0re.environment = { - HIVE_NETWORK_ISOLATION = "1"; HIVE_NETWORK_BRIDGE = cfg.bridgeName; HIVE_NETWORK_SUBNET = "${cfg.bridgeIp}/${toString cfg.bridgePrefixLength}"; };