From 3363188a3895282b7a7edd146c97024fd4067c4f Mon Sep 17 00:00:00 2001 From: Alexis Date: Fri, 4 Sep 2026 13:30:09 +0200 Subject: [PATCH 1/3] setup: keep guest hostname resolvable --- src/setup.rs | 58 ++++++++++++++++++++++++++++++++++++++++++++++++++-- 1 file changed, 56 insertions(+), 2 deletions(-) diff --git a/src/setup.rs b/src/setup.rs index e35106d..d6ce899 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -358,8 +358,10 @@ fn patch_guest_network(inst: &Instance) -> Result<()> { .stdin_write(network_config.as_bytes()) .context("Failed to write guest network config")?; - // Also patch hostname to include instance name - let hostname = format!("claude-{}\n", inst.name); + // Also patch hostname to include instance name and keep /etc/hosts in sync, + // otherwise sudo emits a resolver warning for every guest command. + let guest_hostname = format!("claude-{}", inst.name); + let hostname = format!("{guest_hostname}\n"); let hostfile = format!("{mount_str}/etc/hostname"); Cmd::new("tee") .arg(&hostfile) @@ -367,10 +369,45 @@ fn patch_guest_network(inst: &Instance) -> Result<()> { .stdin_write(hostname.as_bytes()) .context("Failed to write hostname")?; + let hostsfile = format!("{mount_str}/etc/hosts"); + let hosts = Cmd::new("cat") + .arg(&hostsfile) + .sudo() + .capture() + .context("Failed to read guest hosts file")?; + let hosts = hosts_with_hostname(&hosts, &guest_hostname); + Cmd::new("tee") + .arg(&hostsfile) + .sudo() + .stdin_write(hosts.as_bytes()) + .context("Failed to write guest hosts file")?; + // _guard dropped here → unmount + rmdir Ok(()) } +fn hosts_with_hostname(contents: &str, hostname: &str) -> String { + let mut found = false; + let mut output = String::new(); + for line in contents.lines() { + if line.split_whitespace().next() == Some("127.0.1.1") { + output.push_str("127.0.1.1 "); + output.push_str(hostname); + output.push('\n'); + found = true; + } else { + output.push_str(line); + output.push('\n'); + } + } + if !found { + output.push_str("127.0.1.1 "); + output.push_str(hostname); + output.push('\n'); + } + output +} + // ── Template management ─────────────────────────────────────── fn build_or_check_template(cfg: &CoopConfig, opts: &SetupOptions) -> Result<()> { @@ -1959,4 +1996,21 @@ mod tests { .to_string(); assert!(err.contains("network is on fire"), "{err}"); } + + #[test] + fn hosts_with_hostname_replaces_stale_guest_alias() { + let existing = "127.0.0.1 localhost\n127.0.1.1 claude-vm\n::1 localhost\n"; + assert_eq!( + hosts_with_hostname(existing, "claude-auditor-1"), + "127.0.0.1 localhost\n127.0.1.1 claude-auditor-1\n::1 localhost\n" + ); + } + + #[test] + fn hosts_with_hostname_adds_missing_guest_alias() { + assert_eq!( + hosts_with_hostname("127.0.0.1 localhost\n", "claude-auditor-1"), + "127.0.0.1 localhost\n127.0.1.1 claude-auditor-1\n" + ); + } } From 226a924db95445898e16898d49b6a916e10469c5 Mon Sep 17 00:00:00 2001 From: Alexis Date: Fri, 4 Sep 2026 13:51:37 +0200 Subject: [PATCH 2/3] setup: address review of the guest hosts patch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Follow-up on 3363188, which kept /etc/hosts in sync with the renamed guest hostname. Review raised nine items; this closes them. A missing, oversized or non-UTF-8 /etc/hosts no longer aborts the lifecycle. `coop commit` snapshots a guest-mutated rootfs into an image template, so those bytes are guest-authored: an agent deleting the file or writing a Latin-1 byte into it made every later `coop up --image` / `coop restore` fail on a `cat` the previous version propagated with `?`. The read is now best-effort and bounded one byte past 64 KiB via `head -c`, so an oversized file is detected rather than written back truncated, and an unusable one degrades to a synthesized default instead of allocating whatever the guest chose. `chmod 644` follows the write because `tee` creates a new file under the root session's umask, and under the CIS baseline's UMASK 027 a freshly created /etc/hosts would land unreadable to the resolver it exists for — the trap PID_TRAMPOLINE fixed. The guest hostname is clamped to 63 bytes. Instance names allow 64 characters and the hostname is `claude-` + name, so 71 was reachable; systemd rejects an over-long /etc/hostname rather than truncating it, leaving the running hostname out of sync with the alias written beside it and the warning in place — the opposite of the point. Whole-line replacement of the 127.0.1.1 entry is kept and now documented: an alias on that line named the previous hostname, so preserving it preserves a stale name. The read-modify-write stays because it protects entries elsewhere in the file that coop did not author. Duplicate matches now collapse instead of being rewritten one-for-one. Also: hostname and hosts writes move into patch_guest_identity (the caller's doc comment claimed only the network config), the repeated "127.0.1.1" literal becomes one const, and six more helper tests pin the trailing-newline, blank, duplicate, comment-preservation, idempotency and clamp behaviors. The integration suite gains the assertion only it can make — that the guest resolves its own name and `sudo` prints no resolver warning — on the Firecracker leg. It skips on Lima, which owns its own guest hostname configuration; coop does not patch it there and no live Lima guest was checked. CHANGELOG records the migration: no image rebuild, but patch_guest_network runs only at create and restore, so an existing VM keeps the stale entry until `coop restore` or a destroy and recreate. docs/backends.md and the trust model's taint-source list pick up the loop-mounted rootfs read. Verified locally on macOS: cargo fmt, clippy -D warnings, and the full lib suite pass. The integration suite has NOT been run on either backend, so the new assertion and the guest-visible outcome remain unobserved. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 12 +++ docs/backends.md | 2 +- docs/trust-model.md | 7 ++ src/setup.rs | 204 ++++++++++++++++++++++++++++++++++++++----- tests/integration.sh | 33 +++++++ 5 files changed, 235 insertions(+), 23 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a66e8..0201f7d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -83,6 +83,18 @@ ### Fixes +- **The guest hostname resolves, so `sudo` stops warning** — Instance creation + renamed the Firecracker guest to `claude-` in `/etc/hostname` but left + the image's `127.0.1.1 claude-vm` entry in `/etc/hosts`, so every `sudo` in + the guest printed `sudo: unable to resolve host claude-` before running. + Both files are now written together at create and restore, and the guest + hostname is clamped to the kernel's 63-byte limit so long instance names + still get a resolvable name. No image rebuild is needed — the patch is + per-instance, and the image's own entry is what gets overwritten — but + `patch_guest_network` runs only on create and restore, so an existing VM + keeps the stale entry until `coop restore --image ` or a destroy + and recreate. + - **Install Codex's complete runtime package** (#442) — Recent Codex releases use a companion `codex-code-mode-host` executable, but coop installed only the raw `codex` binary, causing Code Mode to fail closed at startup. Image diff --git a/docs/backends.md b/docs/backends.md index 0c6c347..7bd7b97 100644 --- a/docs/backends.md +++ b/docs/backends.md @@ -77,7 +77,7 @@ All three steps are idempotent. If the artifact already exists and is up to date Creating an instance (`coop up`) follows this sequence: 1. Copies the template rootfs to the instance directory using `cp --reflink=auto` for copy-on-write on supported filesystems. -2. Mounts the copy and patches the guest network config with the instance's unique IP address and hostname. +2. Mounts the copy and patches the guest network config with the instance's unique IP address, plus `/etc/hostname` and the matching `/etc/hosts` alias so the guest can resolve its own name. 3. Optionally resizes the rootfs if a larger disk was requested (truncate + e2fsck + resize2fs). 4. Writes a Firecracker JSON config specifying the kernel, rootfs drive, vCPU/memory allocation, network interface, and vsock device. 5. Creates and attaches a TAP device to the bridge (see TAP networking below). diff --git a/docs/trust-model.md b/docs/trust-model.md index 2fd27f9..46729d9 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -49,6 +49,13 @@ user launched it. `tar_pipe_pull` / `rsync_pull` bring guest-authored file contents, filenames, and symlinks onto the host filesystem. This is the **widest guest→host channel** and the primary place a path-traversal or symlink escape could land. +- **Rootfs files read while loop-mounted during setup.** `setup.rs` + `patch_guest_network` reads the guest's `/etc/hosts` before rewriting it, and + `coop commit` turns a guest-mutated rootfs into an image template — so those + bytes are guest-authored on every later create/restore from that image. Such + a read must be **bounded and best-effort**: it degrades to a default + (`read_guest_hosts`) rather than aborting the VM lifecycle, since the same + rootfs booted fine before coop touched the file. - **Guest command output read by the host.** e.g. `check_guest_dirty` reads `git status --porcelain` from the guest. Today this only gates control flow / is printed to the user — it is never fed into `sh -c` on the host. Keep it diff --git a/src/setup.rs b/src/setup.rs index d6ce899..5e1adbb 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -334,8 +334,9 @@ impl Drop for MountGuard { } } -/// Mount the instance rootfs and rewrite the systemd-networkd config -/// with the instance's unique guest IP. +/// Mount the instance rootfs and rewrite its network identity: the +/// systemd-networkd config with the instance's unique guest IP, plus +/// `/etc/hostname` and the matching `/etc/hosts` alias. fn patch_guest_network(inst: &Instance) -> Result<()> { let rootfs_str = inst.rootfs_path().display().to_string(); let mount_dir = inst.dir.join("rootfs-mount"); @@ -358,42 +359,136 @@ fn patch_guest_network(inst: &Instance) -> Result<()> { .stdin_write(network_config.as_bytes()) .context("Failed to write guest network config")?; - // Also patch hostname to include instance name and keep /etc/hosts in sync, - // otherwise sudo emits a resolver warning for every guest command. - let guest_hostname = format!("claude-{}", inst.name); - let hostname = format!("{guest_hostname}\n"); + patch_guest_identity(&mount_str, &guest_hostname(inst.name.as_str()))?; + + // _guard dropped here → unmount + rmdir + Ok(()) +} + +/// Debian/Ubuntu convention: the machine's own name lives on a `127.0.1.1` +/// line, separate from `127.0.0.1 localhost`. That is the line the guest's +/// resolver hits when looking up its own hostname. +const GUEST_HOSTS_ALIAS_IP: &str = "127.0.1.1"; + +/// Cap on the guest `/etc/hosts` read. The file is guest-authored — `coop +/// commit` snapshots a mutated rootfs into an image template — so the guest +/// must not get to choose how much the host allocates. A real hosts file is a +/// few hundred bytes. +const MAX_GUEST_HOSTS_BYTES: usize = 64 * 1024; + +/// Linux's `HOST_NAME_MAX` is 64 bytes including the terminator, so 63 usable. +const MAX_GUEST_HOSTNAME_LEN: usize = 63; + +/// The guest hostname for an instance: `claude-`, clamped to +/// `HOST_NAME_MAX`. +/// +/// Instance names allow 64 characters (`config.rs`'s `MAX_INSTANCE_NAME_LEN`), +/// so the prefixed form reaches 71 — past the kernel's limit. systemd rejects +/// an over-long `/etc/hostname` outright instead of truncating it, which would +/// leave the running hostname different from the `/etc/hosts` alias written +/// beside it and bring the resolver warning back. +fn guest_hostname(name: &str) -> String { + let full = format!("claude-{name}"); + // Instance names are validated ASCII, so this is also a char truncation; + // the boundary walk keeps a hypothetical non-ASCII name from panicking. + let mut end = MAX_GUEST_HOSTNAME_LEN.min(full.len()); + while !full.is_char_boundary(end) { + end -= 1; + } + full[..end].to_string() +} + +/// Write `/etc/hostname` and the matching `/etc/hosts` alias inside a mounted +/// guest rootfs. +/// +/// `sudo` resolves the machine's own hostname on every invocation, so a +/// hostname with no hosts entry makes it print `unable to resolve host` ahead +/// of each `sudo` command in the guest. The template ships `claude-vm` +/// (`scripts/guest/guest-config.sh`) while each instance gets its own name, so +/// the two files have to move together. +fn patch_guest_identity(mount_str: &str, hostname: &str) -> Result<()> { let hostfile = format!("{mount_str}/etc/hostname"); Cmd::new("tee") .arg(&hostfile) .sudo() - .stdin_write(hostname.as_bytes()) + .stdin_write(format!("{hostname}\n").as_bytes()) .context("Failed to write hostname")?; let hostsfile = format!("{mount_str}/etc/hosts"); - let hosts = Cmd::new("cat") - .arg(&hostsfile) - .sudo() - .capture() - .context("Failed to read guest hosts file")?; - let hosts = hosts_with_hostname(&hosts, &guest_hostname); + let hosts = hosts_with_hostname(&read_guest_hosts(&hostsfile), hostname); Cmd::new("tee") .arg(&hostsfile) .sudo() .stdin_write(hosts.as_bytes()) .context("Failed to write guest hosts file")?; - // _guard dropped here → unmount + rmdir - Ok(()) + // `tee` keeps an existing file's mode but creates a new one under the root + // session's umask, which comes from the host's PAM/`login.defs` rather than + // from coop: under the CIS baseline's `UMASK 027` a freshly created + // /etc/hosts would land unreadable to the guest's own resolver. Same trap + // the PID file hit (`vm.rs`'s `PID_TRAMPOLINE`). + Cmd::new("chmod") + .arg("644") + .arg(&hostsfile) + .sudo() + .run() + .context("Failed to set guest hosts file mode") +} + +/// Read a guest `/etc/hosts` — bounded, and best-effort by design. +/// +/// A committed image can carry a missing, oversized or non-UTF-8 `/etc/hosts`, +/// because the guest is untrusted and `coop commit` preserves whatever it left +/// behind. Such a rootfs booted fine before coop patched this file, so a +/// cosmetic fix must not turn it into a failed `coop up`: an unusable file +/// degrades to empty, and [`hosts_with_hostname`] synthesizes a default from +/// there. `head -c` bounds the read one byte past the cap, so an oversized +/// file is detected rather than silently written back truncated. +fn read_guest_hosts(path: &str) -> String { + let read = Cmd::new("head") + .arg("-c") + .arg((MAX_GUEST_HOSTS_BYTES + 1).to_string()) + .arg(path) + .sudo() + .capture(); + match read { + Ok(contents) if contents.len() > MAX_GUEST_HOSTS_BYTES => { + tracing::warn!( + "Guest {path} is larger than {MAX_GUEST_HOSTS_BYTES} bytes — replacing it" + ); + String::new() + } + Ok(contents) => contents, + Err(e) => { + // The error names the command and path, never the file's contents. + tracing::warn!("Guest {path} is unreadable ({e}) — writing a fresh one"); + String::new() + } + } } +/// Return `contents` with the guest's own-hostname entry set to `hostname`. +/// +/// The first `127.0.1.1` line is replaced **whole**: any alias on it named the +/// previous hostname (the template's `claude-vm`), so carrying those forward +/// would keep a stale name resolvable. Later duplicates are dropped, every +/// other line — comments, blanks, entries coop did not author — is passed +/// through verbatim, and the entry is appended when absent. Blank input yields +/// a minimal default file. Idempotent. fn hosts_with_hostname(contents: &str, hostname: &str) -> String { + let entry = format!("{GUEST_HOSTS_ALIAS_IP} {hostname}\n"); + if contents.trim().is_empty() { + return format!("127.0.0.1 localhost\n{entry}"); + } + let mut found = false; let mut output = String::new(); for line in contents.lines() { - if line.split_whitespace().next() == Some("127.0.1.1") { - output.push_str("127.0.1.1 "); - output.push_str(hostname); - output.push('\n'); + if line.split_whitespace().next() == Some(GUEST_HOSTS_ALIAS_IP) { + if found { + continue; + } + output.push_str(&entry); found = true; } else { output.push_str(line); @@ -401,9 +496,7 @@ fn hosts_with_hostname(contents: &str, hostname: &str) -> String { } } if !found { - output.push_str("127.0.1.1 "); - output.push_str(hostname); - output.push('\n'); + output.push_str(&entry); } output } @@ -2013,4 +2106,71 @@ mod tests { "127.0.0.1 localhost\n127.0.1.1 claude-auditor-1\n" ); } + + /// The documented contract: the matched line is replaced whole, so aliases + /// naming the *previous* hostname do not survive the rename. + #[test] + fn hosts_with_hostname_replaces_the_whole_matched_line() { + assert_eq!( + hosts_with_hostname("127.0.1.1 claude-vm claude-vm.local # old\n", "claude-a"), + "127.0.1.1 claude-a\n" + ); + } + + #[test] + fn hosts_with_hostname_collapses_duplicate_aliases() { + let existing = "127.0.1.1 claude-vm\n127.0.0.1 localhost\n127.0.1.1 claude-vm\n"; + assert_eq!( + hosts_with_hostname(existing, "claude-a"), + "127.0.1.1 claude-a\n127.0.0.1 localhost\n" + ); + } + + #[test] + fn hosts_with_hostname_normalizes_a_missing_trailing_newline() { + assert_eq!( + hosts_with_hostname("127.0.0.1 localhost", "claude-a"), + "127.0.0.1 localhost\n127.0.1.1 claude-a\n" + ); + } + + /// The degraded path from [`read_guest_hosts`]: a missing, oversized or + /// non-UTF-8 guest file reads as empty, so the helper has to produce a + /// usable hosts file rather than one holding only the guest alias. + #[test] + fn hosts_with_hostname_synthesizes_a_default_from_blank_input() { + let expected = "127.0.0.1 localhost\n127.0.1.1 claude-a\n"; + assert_eq!(hosts_with_hostname("", "claude-a"), expected); + assert_eq!(hosts_with_hostname(" \n\n", "claude-a"), expected); + } + + #[test] + fn hosts_with_hostname_preserves_comments_and_blank_lines() { + let existing = "# managed by post-install\n\n10.0.0.5 registry.internal\n127.0.1.1 old\n"; + assert_eq!( + hosts_with_hostname(existing, "claude-a"), + "# managed by post-install\n\n10.0.0.5 registry.internal\n127.0.1.1 claude-a\n" + ); + } + + #[test] + fn hosts_with_hostname_is_idempotent() { + let once = hosts_with_hostname("127.0.0.1 localhost\n127.0.1.1 claude-vm\n", "claude-a"); + assert_eq!(hosts_with_hostname(&once, "claude-a"), once); + } + + #[test] + fn guest_hostname_prefixes_the_instance_name() { + assert_eq!(guest_hostname("auditor-1"), "claude-auditor-1"); + } + + /// A 64-char instance name would yield 71 bytes, which systemd rejects + /// rather than truncates — leaving the running hostname out of sync with + /// the `/etc/hosts` entry written next to it. + #[test] + fn guest_hostname_clamps_to_host_name_max() { + let hostname = guest_hostname(&"n".repeat(64)); + assert_eq!(hostname.len(), MAX_GUEST_HOSTNAME_LEN); + assert!(hostname.starts_with("claude-n"), "{hostname}"); + } } diff --git a/tests/integration.sh b/tests/integration.sh index c5b6b81..7b1bc38 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1883,6 +1883,39 @@ test_sudo() { else fail "sudo can write to /root" "stderr: $(guest_stderr)" fi + + # `sudo` resolves the machine's own hostname on every invocation, so + # /etc/hostname and /etc/hosts have to agree — setup writes both. Only + # unit-testable up to the string transform; this is the layer that can + # observe the two files agreeing at boot. + # + # Firecracker only: coop renames the guest there. Lima owns its own guest + # hostname configuration and coop does not patch it, so whether a Lima + # guest resolves its own name is Lima's business, not this assertion's. + if [[ "$(uname -s)" == "Darwin" ]]; then + skip "guest hostname resolves" "Lima owns guest hostname configuration" + return + fi + + local guest_host + guest_host=$(guest_exec hostname) || guest_host="" + if [[ -n "$guest_host" ]] && guest_exec getent hosts "$guest_host" >/dev/null; then + pass "guest hostname resolves ($guest_host)" + else + fail "guest hostname resolves" "hostname='$guest_host'; stderr: $(guest_stderr)" + fi + + if guest_exec sudo -n true; then + local sudo_err + sudo_err=$(guest_stderr) + if echo "$sudo_err" | grep -qi "unable to resolve host"; then + fail "sudo emits no resolver warning" "stderr: $sudo_err" + else + pass "sudo emits no resolver warning" + fi + else + fail "sudo emits no resolver warning" "sudo -n failed; stderr: $(guest_stderr)" + fi } test_network() { From 46bc90f381961642e37834e484f574da806961ac Mon Sep 17 00:00:00 2001 From: Alexis Date: Fri, 4 Sep 2026 14:18:10 +0200 Subject: [PATCH 3/3] setup: correct the hostname clamp rationale and cover the read bound MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Closeout review of the two prior commits. Three of the seven lenses landed on the same false claim, and mutation testing found two behaviors no test could observe. HOST_NAME_MAX is 64 bytes not counting the terminator, so the comment's "64 including the terminator, so 63 usable" derivation was wrong, as was the CHANGELOG's "63-byte limit". The clamp value stays at 63 — verified working, and safe under either reading of the limit — but it no longer claims a derivation it doesn't have. The neighbouring claim that systemd rejects an over-long /etc/hostname outright is also removed: systemd truncates it via hostname_cleanup before validating, and the comment now states only the consequence that holds either way, that the running hostname would stop matching the /etc/hosts alias written beside it. guest_hostname takes &InstanceName instead of &str. The newtype's construction already guarantees [a-zA-Z0-9_-]{1,64}, which is what made the char-boundary walk dead code — every byte index in an ASCII string is a boundary. Taking the proof by type deletes the loop and the comment that apologised for it. Two mutants survived the previous commit's tests: raising MAX_GUEST_HOSTNAME_LEN from 63 to 70 kept the suite green, because the clamp test compared the output length against the same constant that produced it; and disabling the 64 KiB read bound kept it green too, because the bound had no test at all. The clamp now asserts literals and pins the 56-character exact-fit boundary, and the size decision moves into bound_guest_hosts, a pure function over the read's Result, with tests for at-cap, over-cap and error. Both mutants now fail. The integration block gates with if/else rather than a mid-function return, which recorded one skip while skipping two assertions and would have silently killed anything appended to test_sudo on macOS. Its hostname check now asserts the answer came from 127.0.1.1 — getent also consults DNS, which could satisfy the bare name with no hosts entry at all. Also: log the hostname alongside the IP (the clamp was otherwise invisible), drop two comments the diff made stale, and note in the trust model that these are host paths on a loop mount, not a chroot — the traversal rule applies to them and they are not currently validated. Not fixed here, tracked separately: a guest-planted symlink or FIFO at /etc/hosts is followed by the root-level read, write and chmod, because the rootfs is loop-mounted rather than chrooted. The shape predates this branch for /etc/hostname and the network config. Verified: cargo fmt, clippy -D warnings, cargo test (1123 lib tests), prek, and both mutants re-run by hand. shellcheck on tests/integration.sh reports the same 9 pre-existing findings as before the change. The integration suite still has not run on either backend. Co-Authored-By: Claude Opus 5 (1M context) --- CHANGELOG.md | 4 +- docs/trust-model.md | 17 +++-- src/setup.rs | 158 ++++++++++++++++++++++++++----------------- tests/integration.sh | 54 +++++++-------- 4 files changed, 135 insertions(+), 98 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0201f7d..b2fb9ac 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -88,8 +88,8 @@ the image's `127.0.1.1 claude-vm` entry in `/etc/hosts`, so every `sudo` in the guest printed `sudo: unable to resolve host claude-` before running. Both files are now written together at create and restore, and the guest - hostname is clamped to the kernel's 63-byte limit so long instance names - still get a resolvable name. No image rebuild is needed — the patch is + hostname is clamped to fit the kernel's 64-byte hostname limit so long + instance names still get a resolvable name. No image rebuild is needed — the patch is per-instance, and the image's own entry is what gets overwritten — but `patch_guest_network` runs only on create and restore, so an existing VM keeps the stale entry until `coop restore --image ` or a destroy diff --git a/docs/trust-model.md b/docs/trust-model.md index 46729d9..bac01e7 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -49,13 +49,16 @@ user launched it. `tar_pipe_pull` / `rsync_pull` bring guest-authored file contents, filenames, and symlinks onto the host filesystem. This is the **widest guest→host channel** and the primary place a path-traversal or symlink escape could land. -- **Rootfs files read while loop-mounted during setup.** `setup.rs` - `patch_guest_network` reads the guest's `/etc/hosts` before rewriting it, and - `coop commit` turns a guest-mutated rootfs into an image template — so those - bytes are guest-authored on every later create/restore from that image. Such - a read must be **bounded and best-effort**: it degrades to a default - (`read_guest_hosts`) rather than aborting the VM lifecycle, since the same - rootfs booted fine before coop touched the file. +- **Rootfs files touched while loop-mounted during setup.** `setup.rs` + `patch_guest_network` reads and rewrites the guest's `/etc/hosts`, and `coop + commit` turns a guest-mutated rootfs into an image template — so the guest + authors both the contents and the directory entry at that path on every later + create/restore. Contents are read bounded and best-effort + (`bound_guest_hosts` degrades to a default rather than aborting the + lifecycle). The paths are **host** paths: `MountGuard::simple` is a loop + mount, not a chroot, so the traversal rule below applies to every + `{mount}/…` string there — compare `verify_chroot_binaries`, which runs + `test -x` *inside* a chroot for that reason. Not currently validated. - **Guest command output read by the host.** e.g. `check_guest_dirty` reads `git status --porcelain` from the guest. Today this only gates control flow / is printed to the user — it is never fed into `sh -c` on the host. Keep it diff --git a/src/setup.rs b/src/setup.rs index 5e1adbb..df322b0 100644 --- a/src/setup.rs +++ b/src/setup.rs @@ -9,7 +9,7 @@ use anyhow::{Context, Result, bail}; use serde::{Deserialize, Serialize}; use crate::cmd::{Cmd, command_exists}; -use crate::config::{CoopConfig, ImageName, Instance}; +use crate::config::{CoopConfig, ImageName, Instance, InstanceName}; use crate::devcontainer_oci::{InstalledFeature, ResolvedFeature, installed_features}; use crate::guest::{ BASE_PACKAGES, DOCKER_PACKAGES, GH_PACKAGES, GuestUser, ProfileDef, SCRIPT_CLAUDE_CODE, @@ -163,7 +163,6 @@ pub fn create_instance( } } - // Patch the guest's network config with the correct IP for this instance patch_guest_network(inst)?; Ok(()) @@ -254,8 +253,8 @@ pub fn commit_instance_rootfs(cfg: &CoopConfig, inst: &Instance, image: &ImageNa /// /// Mirrors [`create_instance`]'s copy + network-patch, but sources the /// rootfs from an arbitrary image rather than the instance's origin -/// image. The network config is re-patched for this instance's IP, -/// overwriting whatever address the template baked in at commit time. +/// image. The network config and guest identity are re-patched for this +/// instance, overwriting whatever the template baked in at commit time. pub fn restore_instance_rootfs(cfg: &CoopConfig, inst: &Instance, image: &ImageName) -> Result<()> { let template = cfg.template_path_for(image); if !template.exists() { @@ -342,7 +341,11 @@ fn patch_guest_network(inst: &Instance) -> Result<()> { let mount_dir = inst.dir.join("rootfs-mount"); let mount_str = mount_dir.display().to_string(); - tracing::info!("Patching guest network: IP={}", inst.guest_ip()); + let hostname = guest_hostname(&inst.name); + tracing::info!( + "Patching guest network: IP={}, hostname={hostname}", + inst.guest_ip() + ); let _guard = MountGuard::simple(&rootfs_str, &mount_str)?; @@ -359,15 +362,14 @@ fn patch_guest_network(inst: &Instance) -> Result<()> { .stdin_write(network_config.as_bytes()) .context("Failed to write guest network config")?; - patch_guest_identity(&mount_str, &guest_hostname(inst.name.as_str()))?; + patch_guest_identity(&mount_str, &hostname)?; // _guard dropped here → unmount + rmdir Ok(()) } /// Debian/Ubuntu convention: the machine's own name lives on a `127.0.1.1` -/// line, separate from `127.0.0.1 localhost`. That is the line the guest's -/// resolver hits when looking up its own hostname. +/// line, separate from `127.0.0.1 localhost`. const GUEST_HOSTS_ALIAS_IP: &str = "127.0.1.1"; /// Cap on the guest `/etc/hosts` read. The file is guest-authored — `coop @@ -376,26 +378,26 @@ const GUEST_HOSTS_ALIAS_IP: &str = "127.0.1.1"; /// few hundred bytes. const MAX_GUEST_HOSTS_BYTES: usize = 64 * 1024; -/// Linux's `HOST_NAME_MAX` is 64 bytes including the terminator, so 63 usable. +/// One byte under Linux's `HOST_NAME_MAX` of 64, which POSIX counts without +/// the terminator. A 57-character instance name is already pathological, so +/// the spare byte is free — and the clamp holds even where the limit is read +/// as including the terminator. const MAX_GUEST_HOSTNAME_LEN: usize = 63; -/// The guest hostname for an instance: `claude-`, clamped to +/// The guest hostname for an instance: `claude-`, clamped to fit /// `HOST_NAME_MAX`. /// -/// Instance names allow 64 characters (`config.rs`'s `MAX_INSTANCE_NAME_LEN`), -/// so the prefixed form reaches 71 — past the kernel's limit. systemd rejects -/// an over-long `/etc/hostname` outright instead of truncating it, which would -/// leave the running hostname different from the `/etc/hosts` alias written -/// beside it and bring the resolver warning back. -fn guest_hostname(name: &str) -> String { - let full = format!("claude-{name}"); - // Instance names are validated ASCII, so this is also a char truncation; - // the boundary walk keeps a hypothetical non-ASCII name from panicking. - let mut end = MAX_GUEST_HOSTNAME_LEN.min(full.len()); - while !full.is_char_boundary(end) { - end -= 1; - } - full[..end].to_string() +/// Instance names allow 64 characters, so the prefixed form reaches 71 — past +/// the kernel's limit. An over-long name cannot reach +/// the running hostname intact, so `/etc/hostname` and the `/etc/hosts` alias +/// would then name different things. Deriving both from this one value is what +/// keeps them equal. +fn guest_hostname(name: &InstanceName) -> String { + // `truncate` is a byte index, and `InstanceName` is validated ASCII, so + // every index is a char boundary. + let mut hostname = format!("claude-{name}"); + hostname.truncate(MAX_GUEST_HOSTNAME_LEN); + hostname } /// Write `/etc/hostname` and the matching `/etc/hosts` alias inside a mounted @@ -422,11 +424,10 @@ fn patch_guest_identity(mount_str: &str, hostname: &str) -> Result<()> { .stdin_write(hosts.as_bytes()) .context("Failed to write guest hosts file")?; - // `tee` keeps an existing file's mode but creates a new one under the root - // session's umask, which comes from the host's PAM/`login.defs` rather than - // from coop: under the CIS baseline's `UMASK 027` a freshly created - // /etc/hosts would land unreadable to the guest's own resolver. Same trap - // the PID file hit (`vm.rs`'s `PID_TRAMPOLINE`). + // `tee` preserves an existing file's mode but creates a new one under + // root's umask, which comes from the host's PAM/`login.defs`, not coop — a + // hardened umask hides /etc/hosts from the guest's resolver. Reachable only + // if the guest deleted the file before `coop commit`. Cf. `PID_TRAMPOLINE`. Cmd::new("chmod") .arg("644") .arg(&hostsfile) @@ -435,15 +436,12 @@ fn patch_guest_identity(mount_str: &str, hostname: &str) -> Result<()> { .context("Failed to set guest hosts file mode") } -/// Read a guest `/etc/hosts` — bounded, and best-effort by design. +/// Read a guest `/etc/hosts`, bounded one byte past the cap so that an +/// oversized file is detected rather than written back truncated. /// -/// A committed image can carry a missing, oversized or non-UTF-8 `/etc/hosts`, -/// because the guest is untrusted and `coop commit` preserves whatever it left -/// behind. Such a rootfs booted fine before coop patched this file, so a -/// cosmetic fix must not turn it into a failed `coop up`: an unusable file -/// degrades to empty, and [`hosts_with_hostname`] synthesizes a default from -/// there. `head -c` bounds the read one byte past the cap, so an oversized -/// file is detected rather than silently written back truncated. +/// The file is guest-authored (see `docs/trust-model.md`), so the read is +/// best-effort: a cosmetic fix must not fail `coop up` on a rootfs that used +/// to boot. [`bound_guest_hosts`] holds that decision. fn read_guest_hosts(path: &str) -> String { let read = Cmd::new("head") .arg("-c") @@ -451,17 +449,22 @@ fn read_guest_hosts(path: &str) -> String { .arg(path) .sudo() .capture(); + bound_guest_hosts(read) +} + +/// Decide what a bounded read of the guest hosts file yields: the contents, or +/// empty for "no usable file" — which [`hosts_with_hostname`] turns into a +/// default. +fn bound_guest_hosts(read: Result) -> String { match read { Ok(contents) if contents.len() > MAX_GUEST_HOSTS_BYTES => { - tracing::warn!( - "Guest {path} is larger than {MAX_GUEST_HOSTS_BYTES} bytes — replacing it" - ); + tracing::warn!("Guest hosts file exceeds {MAX_GUEST_HOSTS_BYTES} bytes — replacing it"); String::new() } Ok(contents) => contents, Err(e) => { - // The error names the command and path, never the file's contents. - tracing::warn!("Guest {path} is unreadable ({e}) — writing a fresh one"); + // `capture`'s error text is the command line, not the file's bytes. + tracing::warn!("Guest hosts file is unreadable ({e}) — writing a fresh one"); String::new() } } @@ -469,12 +472,9 @@ fn read_guest_hosts(path: &str) -> String { /// Return `contents` with the guest's own-hostname entry set to `hostname`. /// -/// The first `127.0.1.1` line is replaced **whole**: any alias on it named the -/// previous hostname (the template's `claude-vm`), so carrying those forward -/// would keep a stale name resolvable. Later duplicates are dropped, every -/// other line — comments, blanks, entries coop did not author — is passed -/// through verbatim, and the entry is appended when absent. Blank input yields -/// a minimal default file. Idempotent. +/// The matched line is replaced whole rather than edited: its aliases name the +/// previous hostname, and carrying them forward keeps a stale name resolvable. +/// Blank input yields a minimal default file. Idempotent. fn hosts_with_hostname(contents: &str, hostname: &str) -> String { let entry = format!("{GUEST_HOSTS_ALIAS_IP} {hostname}\n"); if contents.trim().is_empty() { @@ -2107,8 +2107,6 @@ mod tests { ); } - /// The documented contract: the matched line is replaced whole, so aliases - /// naming the *previous* hostname do not survive the rename. #[test] fn hosts_with_hostname_replaces_the_whole_matched_line() { assert_eq!( @@ -2134,9 +2132,8 @@ mod tests { ); } - /// The degraded path from [`read_guest_hosts`]: a missing, oversized or - /// non-UTF-8 guest file reads as empty, so the helper has to produce a - /// usable hosts file rather than one holding only the guest alias. + /// Blank input is what [`bound_guest_hosts`] degrades to, so the result has + /// to be a usable hosts file — not one holding only the guest alias. #[test] fn hosts_with_hostname_synthesizes_a_default_from_blank_input() { let expected = "127.0.0.1 localhost\n127.0.1.1 claude-a\n"; @@ -2159,18 +2156,57 @@ mod tests { assert_eq!(hosts_with_hostname(&once, "claude-a"), once); } + fn instance_name(name: &str) -> InstanceName { + InstanceName::new(name).unwrap() + } + #[test] fn guest_hostname_prefixes_the_instance_name() { - assert_eq!(guest_hostname("auditor-1"), "claude-auditor-1"); + assert_eq!( + guest_hostname(&instance_name("auditor-1")), + "claude-auditor-1" + ); + } + + /// Asserted against literals, not `MAX_GUEST_HOSTNAME_LEN` — comparing the + /// output length to the constant that produced it passes for any bound. + #[test] + fn guest_hostname_clamps_the_longest_instance_name() { + let longest = instance_name(&"n".repeat(64)); + assert_eq!( + guest_hostname(&longest), + format!("claude-{}", "n".repeat(56)) + ); + } + + /// The exact-fit boundary: 56 characters is the longest name that survives + /// the clamp untouched, so this pins which names the fix still reaches. + #[test] + fn guest_hostname_leaves_an_exactly_fitting_name_intact() { + let name = "n".repeat(56); + assert_eq!( + guest_hostname(&instance_name(&name)), + format!("claude-{name}") + ); + assert_eq!(guest_hostname(&instance_name(&name)).len(), 63); + } + + #[test] + fn bound_guest_hosts_passes_through_a_file_at_the_cap() { + let at_cap = "a".repeat(MAX_GUEST_HOSTS_BYTES); + assert_eq!(bound_guest_hosts(Ok(at_cap.clone())), at_cap); + } + + /// `head -c` reads one byte past the cap, so an oversized file arrives as + /// `MAX + 1` bytes and must be discarded rather than written back truncated. + #[test] + fn bound_guest_hosts_discards_a_file_over_the_cap() { + let over_cap = "a".repeat(MAX_GUEST_HOSTS_BYTES + 1); + assert_eq!(bound_guest_hosts(Ok(over_cap)), ""); } - /// A 64-char instance name would yield 71 bytes, which systemd rejects - /// rather than truncates — leaving the running hostname out of sync with - /// the `/etc/hosts` entry written next to it. #[test] - fn guest_hostname_clamps_to_host_name_max() { - let hostname = guest_hostname(&"n".repeat(64)); - assert_eq!(hostname.len(), MAX_GUEST_HOSTNAME_LEN); - assert!(hostname.starts_with("claude-n"), "{hostname}"); + fn bound_guest_hosts_degrades_on_a_read_error() { + assert_eq!(bound_guest_hosts(Err(anyhow::anyhow!("no such file"))), ""); } } diff --git a/tests/integration.sh b/tests/integration.sh index 7b1bc38..8f6d2b0 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1884,37 +1884,35 @@ test_sudo() { fail "sudo can write to /root" "stderr: $(guest_stderr)" fi - # `sudo` resolves the machine's own hostname on every invocation, so - # /etc/hostname and /etc/hosts have to agree — setup writes both. Only - # unit-testable up to the string transform; this is the layer that can - # observe the two files agreeing at boot. - # - # Firecracker only: coop renames the guest there. Lima owns its own guest - # hostname configuration and coop does not patch it, so whether a Lima - # guest resolves its own name is Lima's business, not this assertion's. - if [[ "$(uname -s)" == "Darwin" ]]; then - skip "guest hostname resolves" "Lima owns guest hostname configuration" - return - fi - - local guest_host - guest_host=$(guest_exec hostname) || guest_host="" - if [[ -n "$guest_host" ]] && guest_exec getent hosts "$guest_host" >/dev/null; then - pass "guest hostname resolves ($guest_host)" - else - fail "guest hostname resolves" "hostname='$guest_host'; stderr: $(guest_stderr)" - fi - - if guest_exec sudo -n true; then - local sudo_err - sudo_err=$(guest_stderr) - if echo "$sudo_err" | grep -qi "unable to resolve host"; then - fail "sudo emits no resolver warning" "stderr: $sudo_err" + # Firecracker only: coop renames the guest there. Lima configures its own + # guest hostname, so a Lima guest's self-resolution is Lima's business. + if [[ "$(uname -s)" != "Darwin" ]]; then + local guest_host + guest_host=$(guest_exec hostname) || guest_host="" + # Assert the answer comes from the hosts entry coop wrote: getent also + # consults DNS, which could resolve the bare name without one. + if [[ -n "$guest_host" ]] && + guest_exec getent hosts "$guest_host" | grep -q '^127\.0\.1\.1'; then + pass "guest hostname resolves via /etc/hosts ($guest_host)" + else + fail "guest hostname resolves via /etc/hosts" \ + "hostname='$guest_host'; stderr: $(guest_stderr)" + fi + + if guest_exec sudo -n true; then + local sudo_err + sudo_err=$(guest_stderr) + if echo "$sudo_err" | grep -qi "unable to resolve host"; then + fail "sudo emits no resolver warning" "stderr: $sudo_err" + else + pass "sudo emits no resolver warning" + fi else - pass "sudo emits no resolver warning" + fail "sudo emits no resolver warning" "sudo -n failed; stderr: $(guest_stderr)" fi else - fail "sudo emits no resolver warning" "sudo -n failed; stderr: $(guest_stderr)" + skip "guest hostname resolves via /etc/hosts" "Lima owns guest hostname config" + skip "sudo emits no resolver warning" "Lima owns guest hostname config" fi }