diff --git a/docs/agent-lifecycle/persistence.md b/docs/agent-lifecycle/persistence.md index c3daa839..85d6c563 100644 --- a/docs/agent-lifecycle/persistence.md +++ b/docs/agent-lifecycle/persistence.md @@ -406,8 +406,14 @@ repairs: the operator removes the offending entry, and the next start goes through. The drop-in names the `hive-priv` binary that wrote it, so a drop-in never points at a build without the sub-command. A drop-in written by an older -hive-priv has no `ExecStartPre=` and the container starts as before. - +points at a build without the sub-command. That build's store path has a GC +root, `/nix/var/nix/gcroots/hyperhive-check-rootfs/h-`, written before the +drop-in and replaced on each write, so a garbage collection can't remove the +binary a restart would run. The directory sits beside `gcroots/per-container`, +which is the only gcroots path bound into a container, so only root writes it. +Destroying the container removes the drop-in and then its root; the build stays +until the next collection. A drop-in written by an older hive-priv has no +`ExecStartPre=` and no root, and the container starts as before. The journal still ships. The agent's collector (`nix/agent-modules/otel.nix`) reads the guest's own `/var/log/journal` and pushes it over OTLP, so no journal link to the host takes part; nspawn skips `--link-journal=try-guest` diff --git a/docs/trust-boundary/security.md b/docs/trust-boundary/security.md index d8323985..3d7412e0 100644 --- a/docs/trust-boundary/security.md +++ b/docs/trust-boundary/security.md @@ -263,8 +263,8 @@ known operations; there is no arbitrary command pass-through: | `ReloadGatewayNginx` | `systemctl reload/start/reset-failed nginx` (host unit; the unit name is hard-coded, not a parameter) | | `WriteNspawnFlags` | write `/etc/nixos-containers/.conf` (bind-mount list, network isolation vars, and `--ephemeral` after the root's salvage). hive-priv first copies a stopped container's root to the agent's `work/`, then renames it aside (it deletes nothing), following no link the container could have planted | | `EnsureAgentSocketDir` | `mkdirat` `/run/hive-agent/` `0751 root:root` under an `O_NOFOLLOW` parent fd; leaves an existing directory alone, refuses anything else | -| `WriteResourceLimits` | write `CPUQuota=`/`MemoryMax=`/`CPUWeight=`/`IOWeight=` systemd drop-in for agent container, with an `ExecStartPre=` that runs `hive-priv check-rootfs ` (no-follow check of the root paths the start script touches; see persistence.md) | -| `RemoveServiceDropin` | remove `container@.service.d/` drop-in on destroy | +| `WriteResourceLimits` | write `CPUQuota=`/`MemoryMax=`/`CPUWeight=`/`IOWeight=` systemd drop-in for agent container, with an `ExecStartPre=` that runs `hive-priv check-rootfs ` (no-follow check of the root paths the start script touches; see persistence.md), and a GC root on the build it names | +| `RemoveServiceDropin` | remove `container@.service.d/` drop-in on destroy, and the GC root of the hive-priv build it named | | `DaemonReload` | `systemctl daemon-reload` | | `ControlInfraContainer` | `systemctl nginx.service` for the gateway — the `InfraContainer` enum is the allowlist, and serde rejects unknown names at the wire boundary (`hive-c0re`, the forge and the matrix have no variant, so no request can name them) | | `SyncAgentTmpfiles` | unlink `/etc/tmpfiles.d/hyperhive-agents.conf` and return `Ok` | diff --git a/hive-priv-sock/src/lib.rs b/hive-priv-sock/src/lib.rs index 68f57e31..623fe470 100644 --- a/hive-priv-sock/src/lib.rs +++ b/hive-priv-sock/src/lib.rs @@ -372,7 +372,8 @@ pub enum PrivRequest { /// `CPUWeight=` / `IOWeight=` (cgroup v2 relative shares, contention-only), /// and an `ExecStartPre=` that runs `hive-priv check-rootfs ` /// to refuse a start over a root with links where the start script - /// expects directories. + /// expects directories. The build that line names is rooted under + /// `/nix/var/nix/gcroots/hyperhive-check-rootfs/`. /// Written by `lifecycle::set_resource_limits`. WriteResourceLimits { container: String, @@ -391,7 +392,8 @@ pub enum PrivRequest { io_weight: Option, }, - /// Remove `/run/systemd/system/container@.service.d/` if present. + /// Remove `/run/systemd/system/container@.service.d/` if present, + /// and the GC root of the hive-priv build its `ExecStartPre=` named. /// Called by `lifecycle::destroy` to clean up the resource-limits drop-in. RemoveServiceDropin { container: String }, diff --git a/hive-priv/src/container_config.rs b/hive-priv/src/container_config.rs index be2ed2ab..8d88a26a 100644 --- a/hive-priv/src/container_config.rs +++ b/hive-priv/src/container_config.rs @@ -74,13 +74,14 @@ pub(crate) fn validate_container_system_name(name: &str) -> Result<&str> { } /// `RemoveServiceDropin` — remove the container service's drop-in dir -/// if present (idempotent). +/// if present (idempotent), then the GC root of the build it named. pub(crate) fn remove_service_dropin(container: &str) -> Result<(String, String)> { validate_container_system_name(container)?; let dir = format!("/run/systemd/system/container@{container}.service.d"); if Path::new(&dir).exists() { std::fs::remove_dir_all(&dir).with_context(|| format!("remove {dir}"))?; } + crate::prestart::unpin_binary(container)?; Ok((String::new(), String::new())) } @@ -91,7 +92,9 @@ pub(crate) fn remove_service_dropin(container: &str) -> Result<(String, String)> /// The drop-in also carries the `ExecStartPre=` that runs /// `hive-priv check-rootfs ` ([`crate::prestart`]) ahead of /// nixpkgs' start script. It names this process's own binary, so the drop-in -/// and the binary that implements the check are always the same build. +/// and the binary that implements the check are always the same build. The +/// build is rooted against garbage collection for as long as the drop-in +/// names it ([`crate::prestart::pin_binary`]). /// /// Two different kinds of setting land in the same file. `MemoryMax=` / /// `CPUQuota=` are hard caps that throttle even on an idle host; @@ -118,7 +121,11 @@ pub(crate) fn write_resource_limits( let dir = format!("/run/systemd/system/container@{container}.service.d"); std::fs::create_dir_all(&dir).with_context(|| format!("create {dir}"))?; let path = format!("{dir}/hyperhive-limits.conf"); - let rootfs_check = rootfs_check_command(container)?; + let exe = hive_priv_exe()?; + // Rooted before the drop-in names it, so the drop-in never names a build a + // collection can take. + crate::prestart::pin_binary(container, Path::new(&exe))?; + let rootfs_check = rootfs_check_command(&exe, container); let content = limits_dropin_body( &runtime_dir, &rootfs_check, @@ -203,14 +210,13 @@ fn limits_dropin_body( ) } -/// The `ExecStartPre=` command of the pre-start root check for `container`: -/// this binary's absolute path, then the sub-command and the name. +/// This binary's absolute path, for the drop-in's `ExecStartPre=`. /// /// The path is the running binary's, not a fixed one, so a drop-in never names /// a build without the sub-command. systemd expands `%` and `$` in command /// lines and splits on spaces, so a path with any of them is refused rather /// than escaped; store paths never have them. -fn rootfs_check_command(container: &str) -> Result { +fn hive_priv_exe() -> Result { let exe = std::env::current_exe().context("resolve the hive-priv binary path")?; let exe = exe .to_str() @@ -225,7 +231,13 @@ fn rootfs_check_command(container: &str) -> Result { exe.display() ) })?; - Ok(format!("{exe} {} {container}", crate::prestart::SUBCOMMAND)) + Ok(exe.to_owned()) +} + +/// The `ExecStartPre=` command of the pre-start root check for `container`: +/// the binary `exe`, then the sub-command and the name. +fn rootfs_check_command(exe: &str, container: &str) -> String { + format!("{exe} {} {container}", crate::prestart::SUBCOMMAND) } /// `DaemonReload` — `systemctl daemon-reload` on the host. @@ -586,9 +598,9 @@ pub(crate) fn write_container_etc_file_in(rootfs: &Path, name: &str, contents: & mod tests { use super::{ BindMount, HiveFacts, NetworkIsolation, git_overlay_flags, handle_write_nspawn_flags, - limits_dropin_body, nspawn_flags, publish_file, remove_service_dropin, render_hive_facts, - rootfs_check_command, validate_container_system_name, write_bridge_dns_marker_in, - write_resource_limits, + hive_priv_exe, limits_dropin_body, nspawn_flags, publish_file, remove_service_dropin, + render_hive_facts, rootfs_check_command, validate_container_system_name, + write_bridge_dns_marker_in, write_resource_limits, }; use crate::shared::test_support::{entries, scratch}; @@ -748,7 +760,7 @@ mod tests { #[test] fn the_check_command_names_this_binary_the_subcommand_and_the_container() { let exe = std::env::current_exe().unwrap(); - let cmd = rootfs_check_command("h-iris").unwrap(); + let cmd = rootfs_check_command(&hive_priv_exe().unwrap(), "h-iris"); assert_eq!(cmd, format!("{} check-rootfs h-iris", exe.display())); assert!(cmd.starts_with('/'), "{cmd}"); } diff --git a/hive-priv/src/prestart.rs b/hive-priv/src/prestart.rs index f4428830..5faccc75 100644 --- a/hive-priv/src/prestart.rs +++ b/hive-priv/src/prestart.rs @@ -13,14 +13,21 @@ //! never followed. Nothing is deleted or repaired, so whatever is there stays //! for the operator. //! +//! The drop-in names the running hive-priv binary, which nothing else roots +//! once the system has moved on to a newer build. [`pin_binary`] keeps a GC +//! root on it for as long as the drop-in exists, and [`unpin_binary`] drops +//! the root with the drop-in. +//! //! The entry point is [`run`]; [`check`] holds the rules. use std::fs::{FileType, Metadata}; +use std::os::unix::fs::{DirBuilderExt as _, symlink}; use std::path::{Path, PathBuf}; use anyhow::{Context as _, Result, bail}; use crate::container_config::{container_rootfs, validate_container_system_name}; +use crate::shared::partial_name; /// The sub-command the drop-in's `ExecStartPre=` names. pub(crate) const SUBCOMMAND: &str = "check-rootfs"; @@ -57,6 +64,74 @@ const REGULAR_ONLY: [&str; 2] = ["etc/machine-id", "etc/resolv.conf"]; /// `/etc/static` once it has booted. const OS_RELEASE: &str = "etc/os-release"; +/// Where hive-priv roots the build named by each container's `ExecStartPre=`: +/// `` is a symlink to that build's store path. The directory is +/// under the host's `/nix/var/nix/gcroots`, not `per-container`, which is the +/// only part of it a container is given (`--bind=.../gcroots/per-container/` +/// in the nixpkgs start script), so a container can't reach it. +const GCROOTS: &str = "/nix/var/nix/gcroots/hyperhive-check-rootfs"; + +/// Root the store path of `exe` for `container`, replacing the root a previous +/// write left, so the build a drop-in names outlives a garbage collection for +/// as long as the drop-in does. One link per container, so they don't pile up. +pub(crate) fn pin_binary(container: &str, exe: &Path) -> Result<()> { + pin_binary_in(Path::new(GCROOTS), Path::new(STORE), container, exe) +} + +/// Drop the root [`pin_binary`] left for `container`, once its drop-in is gone. +/// A name that holds anything but a link is left alone and reported. +pub(crate) fn unpin_binary(container: &str) -> Result<()> { + unpin_binary_in(Path::new(GCROOTS), container) +} + +/// [`pin_binary`] with the roots directory and the store as parameters. +/// +/// A binary outside `store` can't be collected, so it gets no root. The link +/// is made under a temporary name and renamed over ``: `rename` +/// replaces whatever link is there without following it. +fn pin_binary_in(roots: &Path, store: &Path, container: &str, exe: &Path) -> Result<()> { + let Some(entry) = exe + .strip_prefix(store) + .ok() + .and_then(|rest| rest.components().next()) + else { + return Ok(()); + }; + let target = store.join(entry); + std::fs::DirBuilder::new() + .recursive(true) + .mode(0o755) + .create(roots) + .with_context(|| format!("create {}", roots.display()))?; + if !lstat(roots)?.is_some_and(|m| m.file_type().is_dir()) { + bail!("{} is not a real directory", roots.display()); + } + let tmp = roots.join(partial_name(container)); + symlink(&target, &tmp).with_context(|| format!("create {}", tmp.display()))?; + let root = roots.join(container); + std::fs::rename(&tmp, &root).map_err(|e| { + // Best effort: a leftover temp is a root too, but a harmless one. + let _ = std::fs::remove_file(&tmp); + anyhow::Error::new(e).context(format!("replace {}", root.display())) + }) +} + +/// [`unpin_binary`] with the roots directory as a parameter. +fn unpin_binary_in(roots: &Path, container: &str) -> Result<()> { + let root = roots.join(container); + match lstat(&root)? { + None => Ok(()), + Some(meta) if meta.file_type().is_symlink() => { + std::fs::remove_file(&root).with_context(|| format!("remove {}", root.display())) + } + Some(meta) => bail!( + "{} is {}, not a link: left alone", + root.display(), + kind(meta.file_type()) + ), + } +} + /// Handle `hive-priv ...` when the first argument names one. /// `None` means it is not a sub-command and the daemon starts. /// @@ -210,8 +285,8 @@ mod tests { use std::os::unix::fs::symlink; use std::path::PathBuf; - use super::{check, run}; - use crate::shared::test_support::scratch; + use super::{check, pin_binary_in, run, unpin_binary_in}; + use crate::shared::test_support::{entries, scratch}; /// A scratch container: root, the profile link's path and a store with one /// toplevel in it. Nothing is linked yet. @@ -439,6 +514,100 @@ mod tests { assert!(target.is_dir()); } + /// A scratch store holding one build with a binary in it, a roots dir and + /// the binary's path. + fn pin_fixture() -> (PathBuf, PathBuf, PathBuf, PathBuf) { + let base = scratch(); + let store = base.join("store"); + let build = store.join("abc-hive-priv"); + std::fs::create_dir_all(build.join("bin")).unwrap(); + let exe = build.join("bin/hive-priv"); + std::fs::write(&exe, "").unwrap(); + (base.join("roots"), store, build, exe) + } + + #[test] + fn the_root_points_at_the_store_path_the_drop_in_names() { + let (roots, store, build, exe) = pin_fixture(); + pin_binary_in(&roots, &store, "h-iris", &exe).unwrap(); + let root = roots.join("h-iris"); + assert!(std::fs::symlink_metadata(&root).unwrap().is_symlink()); + // The build, not the binary inside it. + assert_eq!(std::fs::read_link(&root).unwrap(), build); + assert_eq!(std::fs::read_dir(&roots).unwrap().count(), 1); + } + + #[test] + fn a_rewrite_replaces_the_root_and_leaves_no_temp() { + let (roots, store, build, exe) = pin_fixture(); + let newer = store.join("def-hive-priv"); + std::fs::create_dir_all(newer.join("bin")).unwrap(); + pin_binary_in(&roots, &store, "h-iris", &exe).unwrap(); + pin_binary_in(&roots, &store, "h-iris", &newer.join("bin/hive-priv")).unwrap(); + pin_binary_in(&roots, &store, "h-bob", &exe).unwrap(); + assert_eq!(std::fs::read_link(roots.join("h-iris")).unwrap(), newer); + assert_eq!(std::fs::read_link(roots.join("h-bob")).unwrap(), build); + assert_eq!(entries(&roots), ["h-bob", "h-iris"]); + } + + #[test] + fn a_link_planted_at_the_root_name_is_replaced_not_followed() { + let (roots, store, build, exe) = pin_fixture(); + let victim = victim(); + std::fs::write(victim.join("keep"), "data").unwrap(); + std::fs::create_dir_all(&roots).unwrap(); + symlink(&victim, roots.join("h-iris")).unwrap(); + pin_binary_in(&roots, &store, "h-iris", &exe).unwrap(); + assert_eq!(std::fs::read_link(roots.join("h-iris")).unwrap(), build); + assert_eq!( + std::fs::read_to_string(victim.join("keep")).unwrap(), + "data" + ); + assert_eq!(std::fs::read_dir(&victim).unwrap().count(), 1); + } + + #[test] + fn a_roots_dir_that_is_a_link_is_refused() { + let (roots, store, _, exe) = pin_fixture(); + let victim = victim(); + symlink(&victim, &roots).unwrap(); + let msg = format!( + "{:#}", + pin_binary_in(&roots, &store, "h-iris", &exe).unwrap_err() + ); + assert!(msg.contains("is not a real directory"), "{msg}"); + assert_eq!(std::fs::read_dir(&victim).unwrap().count(), 0); + } + + #[test] + fn a_binary_outside_the_store_gets_no_root() { + let (roots, store, _, _) = pin_fixture(); + let elsewhere = scratch().join("hive-priv"); + pin_binary_in(&roots, &store, "h-iris", &elsewhere).unwrap(); + assert!(!roots.exists()); + } + + #[test] + fn unpinning_removes_the_container_root_and_nothing_else() { + let (roots, store, build, exe) = pin_fixture(); + pin_binary_in(&roots, &store, "h-iris", &exe).unwrap(); + pin_binary_in(&roots, &store, "h-bob", &exe).unwrap(); + unpin_binary_in(&roots, "h-iris").unwrap(); + assert_eq!(entries(&roots), ["h-bob"]); + // Already gone: fine. The build it named is untouched. + unpin_binary_in(&roots, "h-iris").unwrap(); + assert!(build.join("bin/hive-priv").is_file()); + } + + #[test] + fn unpinning_leaves_a_name_that_is_not_a_link_alone() { + let (roots, _, _, _) = pin_fixture(); + std::fs::create_dir_all(roots.join("h-iris")).unwrap(); + std::fs::write(roots.join("h-iris/keep"), "").unwrap(); + let msg = format!("{:#}", unpin_binary_in(&roots, "h-iris").unwrap_err()); + assert!(msg.contains("left alone"), "{msg}"); + assert!(roots.join("h-iris/keep").exists()); + } #[test] fn only_the_check_rootfs_sub_command_is_taken() { assert_eq!(run(&[]), None); diff --git a/hive-priv/src/shared.rs b/hive-priv/src/shared.rs index 0c06f35e..a8ae7332 100644 --- a/hive-priv/src/shared.rs +++ b/hive-priv/src/shared.rs @@ -75,7 +75,7 @@ pub(crate) fn ensure_plain_filename(who: &str, filename: &str) -> Result<()> { /// The leading dot and the `.partial` suffix are load-bearing: a reader /// globbing on a published name's prefix, or systemd reading `*.conf` out of /// `tmpfiles.d` and drop-in dirs, cannot pick up a half-written temp. -fn partial_name(filename: &str) -> String { +pub(crate) fn partial_name(filename: &str) -> String { use std::sync::atomic::{AtomicU64, Ordering}; static SEQ: AtomicU64 = AtomicU64::new(0); let n = SEQ.fetch_add(1, Ordering::Relaxed); diff --git a/nix/vm-tests/agent-ephemeral-rootfs.nix b/nix/vm-tests/agent-ephemeral-rootfs.nix index eba9fe94..c07451ad 100644 --- a/nix/vm-tests/agent-ephemeral-rootfs.nix +++ b/nix/vm-tests/agent-ephemeral-rootfs.nix @@ -14,8 +14,9 @@ # - a package installed into the user's nix profile is still there, and on # PATH, after a restart; # - a link planted at `root` in the template refuses the next start through -# the limits drop-in's `ExecStartPre=` check, leaves the link's target -# untouched, and the start goes through once the link is gone. +# the limits drop-in's `ExecStartPre=` check (whose build has a GC root), +# leaves the link's target untouched, and the start goes through once the +# link is gone. # # The agent's journal forwarder itself (an OTLP push to the hive's collector) is # not booted here; the module-eval suite covers its wiring. @@ -227,6 +228,9 @@ pkgs.testers.runNixOSTest { assert json.loads(out.strip().splitlines()[-1])["ok"] is True, out dropin = host.succeed(f"cat /run/systemd/system/container@{MACHINE}.service.d/hyperhive-limits.conf") assert "ExecStartPre=/nix/store/" in dropin and f"check-rootfs {MACHINE}\n" in dropin, dropin + # The build the drop-in names is rooted against garbage collection. + build = "/".join(dropin.split("ExecStartPre=")[1].split()[0].split("/")[:4]) + assert host.succeed(f"readlink /nix/var/nix/gcroots/hyperhive-check-rootfs/{MACHINE}").strip() == build, build # `root` is a link to a directory outside the root, which the start # script would `chmod 0700` as host root.