setup: keep guest hostname resolvable - #449
Conversation
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) <noreply@anthropic.com>
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) <noreply@anthropic.com>
| // 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") |
There was a problem hiding this comment.
Please confine the hosts-file operations to the mounted rootfs before writing or changing permissions. A guest can commit /etc/hosts as an absolute symlink to a host file; the new head → tee → chmod sequence follows it under host root. In a temporary reproduction, the target retained its secret contents while its mode changed from 0600 to 0644. This adds a disclosure capability beyond the existing hostname/network writes. Use descriptor-relative access and replacement, without checking a path and then reopening it.
| /// 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") |
There was a problem hiding this comment.
Please reject special files safely before reading. A guest can commit /etc/hosts as a FIFO, and head -c then blocks indefinitely waiting for a writer. The byte cap does not bound waiting, and Cmd::capture has no timeout, so create/restore hangs before the fallback runs. Reproduced with a temporary FIFO. Check the opened descriptor's file type using an approach that cannot block on the initial open.
|
Thanks for your contributions and for documenting the trade-offs and validation gaps clearly. I left two inline findings: a host-file disclosure through symlinks and a FIFO that hangs create/restore. All eight review lenses ran; no separate diff-noise findings. The 14 extracted helper tests passed and five targeted mutations were caught. Firecracker and Lima integration remain unverified; the green CI jobs do not boot a VM. |
patch_guest_networkwrites/etc/hostnameasclaude-<instance>but leaves/etc/hostsuntouched, so the guest's own hostname has no resolver entry. Onthe Firecracker rootfs that makes
sudoprintunable to resolve host claude-<instance>ahead of every guest command thatinvokes it.
This patches
/etc/hostsalongside the existing hostname write, on the sameloop mount, so it covers both
create_instanceandrestore_instance_rootfs.The two writes move into
patch_guest_identity; the line transform is a purehelper,
hosts_with_hostname.The
127.0.1.1line is replaced whole. An alias on that line named theprevious hostname (the template's
claude-vm), so preserving it would keep astale name resolvable. The read-modify-write is kept — rather than writing a
deterministic hosts file — because it preserves entries elsewhere in the
file that coop did not author. Duplicate
127.0.1.1lines collapse to one, sothe transform is idempotent.
The read is bounded and best-effort.
coop commitsnapshots aguest-mutated rootfs into an image template, so
/etc/hostscan come backmissing, oversized or non-UTF-8. An earlier draft propagated that with
?,which would have turned a cosmetic fix into a failed
coop up --image/coop restoreon a rootfs that used to boot fine.read_guest_hostsreads onebyte past a 64 KiB cap via
head -c(so an oversized file is detected ratherthan written back truncated) and degrades to a synthesized default.
chmod 644follows the write.teekeeps an existing file's mode butcreates a new one under the root session's umask, which comes from the host's
PAM/
login.defs; under the CIS baseline'sUMASK 027a freshly created/etc/hostswould land unreadable to the resolver it exists for — the trapvm.rs'sPID_TRAMPOLINEfixed for the PID file.The guest hostname is clamped to 63 bytes, one under Linux's
HOST_NAME_MAXof 64. Instance names allow 64 characters and the hostname isclaude-+ name, so 71 was reachable. An over-long name cannot reach therunning hostname intact, so
/etc/hostnameand the/etc/hostsalias wouldname different things — and the warning would stay. Deriving both from one
value keeps them equal. Names up to 56 characters are unaffected.
Migration
No image rebuild is needed: the patch is per-instance and the image's own
claude-vmentry is what gets overwritten. Butpatch_guest_networkruns onlyat create and restore —
start_existingdoes not re-patch — so an instancecreated before this lands keeps the stale entry until
coop restore <vm> --image <image>or a destroy and recreate.Testing
cargo fmt -- --check,cargo clippy --all-targets --all-features -D warnings,cargo test(1123 lib tests + doctests),prek run— all clean on macOS.duplicate collapsing, missing trailing newline, blank-input default, comment
and blank-line preservation, idempotency, the hostname clamp and its
exact-fit boundary, and the read bound at-cap / over-cap / on-error.
tests/integration.shtest_sudogains the assertion only that layer canmake: the guest resolves its own name (
getent hosts $(hostname)) andsudo -n trueprints no resolver warning. It is gated to the Firecrackerleg and skips on Lima, which owns its own guest hostname configuration.
guest-visible outcome remains reasoned from the code rather than observed.
CI's
checkjob runs only the host-side integration steps (coop update,coop uninstall) and never boots a VM, so it does not cover the newassertion either.
Known limits
src/setup.rsis whole-module excluded fromcargo-mutants(
.cargo/mutants.toml), so the unit tests are the only guard on the purehelpers here;
exclude_globshas no per-function re-include.at
/etc/hosts(or a symlinked/etc) is followed by the root-level read,write and
chmod, and a FIFO there blocks the read with no timeout. Theescape shape predates this PR — the
/etc/hostnameand network-config writesin the same function have it too, and
verify_chroot_binariesdocuments thesame resolution semantics — but this PR adds instances of it and the
chmodis a new primitive. Deliberately not fixed here; noted in
docs/trust-model.mdas unvalidated.Closeout review
Seven review lenses ran over the branch. Findings fixed in
46bc90f: a falseHOST_NAME_MAXderivation in a comment and in the CHANGELOG; an unverifiablesystemd claim; a dead char-boundary walk (deleted by taking
&InstanceName,whose construction already proves the input ASCII); a tautological clamp test
that compared its output against the constant that produced it; and the 64 KiB
read bound, which had no test at all. Both surviving mutants — clamp 63→70, and
the bound disabled — now fail. The integration block also stopped under-
reporting its skips, and its hostname assertion now checks the answer came from
127.0.1.1rather than from DNS.