diff --git a/CHANGELOG.md b/CHANGELOG.md index 6e838cc..0563687 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -117,6 +117,18 @@ and a liveness probe, so a guest whose sshd stops answering fails after ~90s — the bound interactive sessions already had. +- **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. + - **Fail closed on an unmanaged `CODEX_HOME` in ChatGPT auth mode** (#441) — The guest wrapper now refuses an explicitly set `CODEX_HOME` when coop's managed `~/.codex/config.toml` selects keyring storage. This prevents `codex 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 948d7f1..427da4d 100644 --- a/docs/trust-model.md +++ b/docs/trust-model.md @@ -49,6 +49,18 @@ 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). Hosts-file operations use pinned directory descriptors, reject + symlinked `/etc`, and read only regular files checked through an `O_PATH` + descriptor. Replacement is atomic; permissions are set on the new file's + descriptor. Other paths remain **host** paths: `MountGuard::simple` is a + loop mount, not a chroot, so the traversal rule below still applies to the + hostname and network-config writes. Those paths are 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/lib.rs b/src/lib.rs index 0253779..7b704c0 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -97,6 +97,11 @@ pub(crate) struct Cli { #[derive(Subcommand)] enum Commands { + /// Internal privileged helper for a mounted Firecracker rootfs. + #[cfg(target_os = "linux")] + #[command(name = "__patch-guest-hosts", hide = true)] + PatchGuestHosts { mount: PathBuf, hostname: String }, + /// Ensure an environment for a project directory exists and is running. /// /// Re-runnable: if an instance already exists for DIR it is reused @@ -926,6 +931,15 @@ pub fn run() -> Result<()> { let cli = Cli::parse(); init_tracing(cli.verbose); + #[cfg(target_os = "linux")] + if let Commands::PatchGuestHosts { + ref mount, + ref hostname, + } = cli.command + { + return setup::patch_guest_hosts(mount, hostname); + } + if let Commands::Completions { shell } = cli.command { completions::emit_static(shell); return Ok(()); @@ -997,6 +1011,8 @@ pub fn run() -> Result<()> { let raw_args: Vec = std::env::args().collect(); match cli.command { + #[cfg(target_os = "linux")] + Commands::PatchGuestHosts { .. } => unreachable!("handled before config loading"), Commands::Up { dir, name, diff --git a/src/setup.rs b/src/setup.rs index e35106d..55d1b79 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,19 +362,171 @@ 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 + Cmd::new(std::env::current_exe()?) + .arg("__patch-guest-hosts") + .arg(mount_str) + .arg(hostname) + .sudo() + .run() + .context("Failed to patch guest hosts file") +} + +/// Privileged hosts-file update. Keep directory descriptors alive throughout: +/// `/proc/self/fd` anchors each operation to the opened directory, even if a +/// directory entry is replaced. Never follow guest-authored symlinks. +#[cfg(target_os = "linux")] +pub(crate) fn patch_guest_hosts(mount: &Path, hostname: &str) -> Result<()> { + use std::io::Read as _; + use std::os::fd::AsRawFd as _; + use std::os::unix::fs::{OpenOptionsExt as _, PermissionsExt as _}; + + let open_directory = |path: &Path| { + fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_PATH | libc::O_DIRECTORY | libc::O_NOFOLLOW) + .open(path) + }; + let root = open_directory(mount).context("Failed to open mounted rootfs")?; + let etc = open_directory(&PathBuf::from(format!( + "/proc/self/fd/{}/etc", + root.as_raw_fd() + ))) + .context("Guest /etc must be a real directory")?; + let etc_path = PathBuf::from(format!("/proc/self/fd/{}", etc.as_raw_fd())); + let hosts_path = etc_path.join("hosts"); + + let read = (|| -> Result { + // O_PATH obtains a descriptor without opening a FIFO or device for IO. + let entry = fs::OpenOptions::new() + .read(true) + .custom_flags(libc::O_PATH | libc::O_NOFOLLOW) + .open(&hosts_path)?; + if !entry.metadata()?.is_file() { + bail!("Guest hosts entry is not a regular file"); + } + // Reopen the checked descriptor, never the guest directory entry. + let file = fs::File::open(format!("/proc/self/fd/{}", entry.as_raw_fd()))?; + let mut contents = String::new(); + file.take((MAX_GUEST_HOSTS_BYTES + 1) as u64) + .read_to_string(&mut contents)?; + Ok(contents) + })(); + let hosts = hosts_with_hostname(&bound_guest_hosts(read), hostname); + let mut replacement = tempfile::NamedTempFile::new_in(&etc_path)?; + replacement.write_all(hosts.as_bytes())?; + replacement + .as_file() + .set_permissions(fs::Permissions::from_mode(0o644))?; + // Drop a failed replacement while the directory descriptor is still open. + replacement + .persist(&hosts_path) + .map_err(|error| error.error)?; Ok(()) } +/// 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) => { + // IO errors contain no guest file contents. + 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 ─────────────────────────────────────── fn build_or_check_template(cfg: &CoopConfig, opts: &SetupOptions) -> Result<()> { @@ -1959,4 +2115,223 @@ 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); + } + + #[cfg(target_os = "linux")] + #[test] + fn patch_guest_hosts_replaces_regular_file_without_modifying_hardlinks() { + use std::os::unix::fs::PermissionsExt as _; + + let root = tempfile::tempdir().unwrap(); + let etc = root.path().join("etc"); + fs::create_dir(&etc).unwrap(); + let original = "127.0.0.1 localhost\n127.0.1.1 old\n10.0.0.5 registry\n"; + fs::write(etc.join("hosts"), original).unwrap(); + fs::set_permissions(etc.join("hosts"), fs::Permissions::from_mode(0o600)).unwrap(); + fs::hard_link(etc.join("hosts"), root.path().join("original")).unwrap(); + + patch_guest_hosts(root.path(), "claude-a").unwrap(); + assert_eq!( + fs::read_to_string(etc.join("hosts")).unwrap(), + "127.0.0.1 localhost\n127.0.1.1 claude-a\n10.0.0.5 registry\n" + ); + assert_eq!( + fs::metadata(etc.join("hosts")) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o644 + ); + assert_eq!( + fs::read_to_string(root.path().join("original")).unwrap(), + original + ); + assert_eq!( + fs::metadata(root.path().join("original")) + .unwrap() + .permissions() + .mode() + & 0o777, + 0o600 + ); + assert_eq!(fs::read_dir(&etc).unwrap().count(), 1); + } + + #[cfg(target_os = "linux")] + #[test] + fn patch_guest_hosts_replaces_unusable_entries_with_defaults() { + use std::os::unix::fs::symlink; + + for kind in ["missing", "symlink", "fifo", "oversized", "non-utf8"] { + let root = tempfile::tempdir().unwrap(); + let etc = root.path().join("etc"); + fs::create_dir(&etc).unwrap(); + let hosts = etc.join("hosts"); + let other = root.path().join("other"); + fs::write(&other, "10.0.0.1 other\n").unwrap(); + match kind { + "symlink" => symlink(&other, &hosts).unwrap(), + "fifo" => assert!( + Command::new("mkfifo") + .arg(&hosts) + .status() + .unwrap() + .success() + ), + "oversized" => fs::write(&hosts, vec![b'a'; MAX_GUEST_HOSTS_BYTES + 1]).unwrap(), + "non-utf8" => fs::write(&hosts, [0xff]).unwrap(), + _ => {} + } + patch_guest_hosts(root.path(), "claude-a").unwrap(); + assert!(fs::symlink_metadata(&hosts).unwrap().is_file(), "{kind}"); + assert_eq!( + fs::read_to_string(&hosts).unwrap(), + "127.0.0.1 localhost\n127.0.1.1 claude-a\n", + "{kind}" + ); + assert_eq!( + fs::read_to_string(&other).unwrap(), + "10.0.0.1 other\n", + "{kind}" + ); + assert_eq!(fs::read_dir(&etc).unwrap().count(), 1, "{kind}"); + } + } + + #[cfg(target_os = "linux")] + #[test] + fn patch_guest_hosts_rejects_symlinked_etc_and_directory_hosts() { + use std::os::unix::fs::symlink; + + let root = tempfile::tempdir().unwrap(); + let other = tempfile::tempdir().unwrap(); + symlink(other.path(), root.path().join("etc")).unwrap(); + assert!(patch_guest_hosts(root.path(), "claude-a").is_err()); + assert_eq!(fs::read_dir(other.path()).unwrap().count(), 0); + + fs::remove_file(root.path().join("etc")).unwrap(); + fs::create_dir_all(root.path().join("etc/hosts")).unwrap(); + assert!(patch_guest_hosts(root.path(), "claude-a").is_err()); + assert_eq!(fs::read_dir(root.path().join("etc")).unwrap().count(), 1); + } + + 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); + } + + /// The read takes 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 a8a8eed..0d1d41e 100755 --- a/tests/integration.sh +++ b/tests/integration.sh @@ -1935,6 +1935,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() {