diff --git a/CHANGELOG.md b/CHANGELOG.md index 63a66e8..b2fb9ac 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 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 + 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..bac01e7 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -49,6 +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 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 e35106d..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() { @@ -334,14 +333,19 @@ 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"); 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)?; @@ -358,17 +362,143 @@ 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); + 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`. +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; + +/// 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 fit +/// `HOST_NAME_MAX`. +/// +/// 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 +/// 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")?; - // _guard dropped here → unmount + rmdir - Ok(()) + let hostsfile = format!("{mount_str}/etc/hosts"); + 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")?; + + // `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) + .sudo() + .run() + .context("Failed to set guest hosts file mode") +} + +/// Read a guest `/etc/hosts`, bounded one byte past the cap so that an +/// oversized file is detected rather than 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") + .arg((MAX_GUEST_HOSTS_BYTES + 1).to_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 hosts file exceeds {MAX_GUEST_HOSTS_BYTES} bytes — replacing it"); + String::new() + } + Ok(contents) => contents, + Err(e) => { + // `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() + } + } +} + +/// Return `contents` with the guest's own-hostname entry set to `hostname`. +/// +/// 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() { + 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(GUEST_HOSTS_ALIAS_IP) { + if found { + continue; + } + output.push_str(&entry); + found = true; + } else { + output.push_str(line); + output.push('\n'); + } + } + if !found { + output.push_str(&entry); + } + output } // ── Template management ─────────────────────────────────────── @@ -1959,4 +2089,124 @@ 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" + ); + } + + #[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" + ); + } + + /// 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"; + 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); + } + + fn instance_name(name: &str) -> InstanceName { + InstanceName::new(name).unwrap() + } + + #[test] + fn guest_hostname_prefixes_the_instance_name() { + 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)), ""); + } + + #[test] + 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 c5b6b81..8f6d2b0 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1883,6 +1883,37 @@ test_sudo() { else fail "sudo can write to /root" "stderr: $(guest_stderr)" fi + + # 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 + fail "sudo emits no resolver warning" "sudo -n failed; stderr: $(guest_stderr)" + fi + else + skip "guest hostname resolves via /etc/hosts" "Lima owns guest hostname config" + skip "sudo emits no resolver warning" "Lima owns guest hostname config" + fi } test_network() {