feat(minimald,minvmd): include SSH host key in the ready beacon - #582
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR extends READY signaling to carry an optional OpenSSH host key, updates host-side parsing to learn that key, and adds guest rootfs and egress setup plus new startup wiring for gvproxy and timeout/config handling. ChangesREADY beacon, boot, and egress wiring
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/minimald/src/guest.rs (1)
82-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider deduplicating the connect/retry loop shared with
emit_ready_marker.
emit_simple_ready_markeris an almost verbatim copy ofemit_ready_marker(Lines 50-75); only the written payload and the log message differ. The two loops will drift over time (e.g., a future change toMAX_ATTEMPTS/BACKOFFor the shutdown handling would need to be applied twice). A small private helper parameterized on the payload keeps both entry points in sync.♻️ Proposed consolidation
async fn emit_marker(pubkey: Option<&PublicKey>) -> std::io::Result<()> { const MAX_ATTEMPTS: u32 = 50; const BACKOFF: Duration = Duration::from_millis(100); let addr = VsockAddr::new(VMADDR_CID_HOST, BOOT_MARKER_PORT); let mut last_err = None; for attempt in 1..=MAX_ATTEMPTS { match VsockStream::connect(addr).await { Ok(mut stream) => { match pubkey { Some(pk) => write_ready_beacon(&mut stream, pk).await?, None => stream.write_all(b"READY\n").await?, } AsyncWriteExt::shutdown(&mut stream).await?; tracing::info!(attempt, simple = pubkey.is_none(), "emitted boot READY marker"); return Ok(()); } Err(e) => { tracing::debug!(attempt, error = %e, "vsock not ready, retrying"); last_err = Some(e); tokio::time::sleep(BACKOFF).await; } } } Err(last_err.unwrap_or_else(|| { std::io::Error::new(std::io::ErrorKind::TimedOut, "vsock never became available") })) } pub async fn emit_ready_marker(pubkey: &PublicKey) -> std::io::Result<()> { emit_marker(Some(pubkey)).await } pub async fn emit_simple_ready_marker() -> std::io::Result<()> { emit_marker(None).await }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/guest.rs` around lines 82 - 107, The retry/connect logic in emit_simple_ready_marker is duplicated from emit_ready_marker and should be consolidated to avoid drift. Refactor both functions to call a shared private helper (for example, a marker-emission helper in guest.rs) that owns the VsockStream::connect loop, MAX_ATTEMPTS/BACKOFF handling, shutdown, and retry logging, while letting emit_ready_marker and emit_simple_ready_marker differ only by the payload/write step and final log message.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/minimald/src/guest.rs`:
- Around line 82-107: The retry/connect logic in emit_simple_ready_marker is
duplicated from emit_ready_marker and should be consolidated to avoid drift.
Refactor both functions to call a shared private helper (for example, a
marker-emission helper in guest.rs) that owns the VsockStream::connect loop,
MAX_ATTEMPTS/BACKOFF handling, shutdown, and retry logging, while letting
emit_ready_marker and emit_simple_ready_marker differ only by the payload/write
step and final log message.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 85c85271-5f45-4310-9c70-75ea67ba1a19
📒 Files selected for processing (6)
crates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minvmd/Cargo.tomlcrates/minvmd/src/cmd/boot.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rs
|
Revise claim for head be055bf. |
|
Auto-revise 1 of 3. |
This comment has been minimized.
This comment has been minimized.
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
|
Commit pushed:
|
|
Commit pushed: Addressed three review comments from the latest
All tests pass (
|
This comment has been minimized.
This comment has been minimized.
twitchyliquid64
left a comment
There was a problem hiding this comment.
Think this just needs the conflict caused by the crate rename minimal2->minimal fixed and then g2g
|
/revise resolve the conflict |
|
Commit pushed:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/minimald/src/guest.rs (1)
551-555: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the beacon contains this key, not just any Ed25519 key.
This would still pass if
write_ready_beaconemitted a different Ed25519 public key. Compare the second line withpubkey.to_openssh()to lock the READY contract.Proposed test tightening
- assert!( - key_line.starts_with("ssh-ed25519 "), - "expected openssh pubkey, got: {key_line:?}" - ); + let expected = pubkey.to_openssh().unwrap(); + assert_eq!(key_line, expected, "unexpected openssh pubkey line");🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/minimald/src/guest.rs` around lines 551 - 555, The READY beacon test in the code around the `key_line` assertion is too loose because it only checks for an `ssh-ed25519` prefix. Tighten the check in the `write_ready_beacon`/`pubkey.to_openssh()` test path by comparing the second line directly against the expected OpenSSH string from `pubkey.to_openssh()`, so the test verifies the exact beacon key rather than any Ed25519 key.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/minimald/src/main.rs`:
- Around line 429-444: The startup sequence in main should not emit the READY
marker before SSH is actually available. Update the init flow around
guest::emit_ready_marker, guest::bring_up_root_egress, and the vsock SSH
listener setup so READY is sent only after the listener is bound, or move
bring_up_root_egress earlier with a bounded timeout and keep SSH startup from
being delayed by slow gvproxy/tap setup. If egress must remain best-effort, run
it asynchronously after the listener is ready rather than blocking the READY
path.
In `@crates/minvmd/src/cmd/mod.rs`:
- Around line 26-32: The documentation for OWN_IP_ENV no longer matches the
actual gvproxy startup path in run_foreground. Update the constant comment on
OWN_IP_ENV to reflect that the parent supervisor now attempts to spawn gvproxy
whenever the binary is present, and that own_ip_requested() only controls
whether gvproxy failures are fatal or downgraded to warnings; keep the
child-side OwnIp vs HostNet behavior description aligned with the current logic.
---
Nitpick comments:
In `@crates/minimald/src/guest.rs`:
- Around line 551-555: The READY beacon test in the code around the `key_line`
assertion is too loose because it only checks for an `ssh-ed25519` prefix.
Tighten the check in the `write_ready_beacon`/`pubkey.to_openssh()` test path by
comparing the second line directly against the expected OpenSSH string from
`pubkey.to_openssh()`, so the test verifies the exact beacon key rather than any
Ed25519 key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d4559b7f-a82c-4b61-9bbb-b5a12cdecdc3
📒 Files selected for processing (7)
crates/minimald/Cargo.tomlcrates/minimald/src/guest.rscrates/minimald/src/main.rscrates/minvmd/Cargo.tomlcrates/minvmd/src/cmd/boot.rscrates/minvmd/src/cmd/mod.rscrates/minvmd/src/cmd/run.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/minvmd/Cargo.toml
- crates/minvmd/src/cmd/boot.rs
Extend the minvmd ready-beacon protocol from READY\n to READY\n<openssh-pubkey>\n so the host can pre-populate known_hosts before the first SSH connection, eliminating the TOFU prompt. Guest side (minimald): - Add write_ready_beacon(writer, pubkey) — the testable core logic - Extend emit_ready_marker to accept &PublicKey and call it - Add emit_simple_ready_marker for the degraded READY-only path (no SSH server, no key to advertise) - Store host_private_key once in main.rs, reuse for both known_hosts write and the vsock beacon (R1.2) Host side (minvmd): - Add read_ready_beacon(reader, known_hosts_path) in cmd/mod.rs; reads the second line, parses it as a PublicKey, writes the known_hosts entry via learn_known_hosts_path (R2.1–R2.4) - Add default_vm_known_hosts_path() helper - Replace duplicated inline READY-reader in boot.rs and run.rs with the shared helper - Promote russh from [dev-dependencies] to [dependencies] (R2.5) Tests: - minimald: write_ready_beacon_formats_two_lines (R1.1) - minvmd: read_ready_beacon_writes_known_hosts_entry, read_ready_beacon_tolerates_missing_pubkey_line, read_ready_beacon_rejects_wrong_marker (R2.1–R2.4) References #467
…dy_marker Both public fns shared verbatim retry/connect logic. Extract a private emit_marker(Option<&PublicKey>) helper; the two public fns become one-liners. Use a structured `simple` field instead of separate log messages. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Add a TODO comment at both sites that hardcode the hostname "local-0" in `default_vm_known_hosts_path` and `read_ready_beacon`, noting that instance_num should be threaded through once multi-instance support is needed. This makes the intentional scope boundary explicit and prevents a future silent mismatch. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- Narrow write_ready_beacon visibility from pub to pub(crate); minimald is a binary crate with no external consumers (coding-standards) - Add 4096-byte length cap on the pubkey read_line in read_ready_beacon; bounds heap growth from a buggy/compromised guest before from_openssh can reject the input (security) - Remove ownerless TODO comments for hardcoded local-0 hostname; single- instance is the declared scope per spec Non-Goals and no tracking issue exists for multi-instance support (coding-standards)
9a200ec to
004cb8b
Compare
This comment has been minimized.
This comment has been minimized.
`read_ready_beacon_writes_known_hosts_entry` previously only asserted that "local-0" appeared in the known_hosts output. A regression that wrote the wrong key would still pass. The `openssh` variable (the exact key sent in the beacon) was already in scope — add a second assertion that the file also contains that key string, so the test verifies the full R2.2 round-trip.
|
Commit pushed:
|
This comment has been minimized.
This comment has been minimized.
Read::take() is applied before read_line so the 4096-byte ceiling is enforced at the I/O layer; the existing post-read length check is kept as defence-in-depth. Also caps the READY-marker read at 32 bytes. Adds a test for the oversized-pubkey case.
|
Commit pushed:
|
|
Revise claim for head 96f0a83. |
|
Auto-revise 2 of 3. |
sdd-validate findings — implementation boundaryBoundary resolved: Implementation (non-spec, non-architecture, non-spike diff) Gate 1 — Proof artifacts re-executed and passingTest proof — Unit 1:
|
| Gate | Severity | Summary |
|---|---|---|
| 1 — Test proof (Unit 1) | Info | Deferred to ci-success → test job |
| 1 — Test proofs (Unit 2) | Info | Deferred to ci-success → test job |
| 1 — File proof (Unit 2) | Blocker | No required gate covers runtime known_hosts check |
| 2 — Files in scope | Warning | cmd/mod.rs not in plan scope (anticipated by spec) |
| 3 — Credentials | Clean | None found |
One Blocker found. Applying needs-human. A human should boot a VM with this branch, confirm $XDG_STATE_HOME/minimal/providers/local-0/known_hosts is created and contains a local-0 entry matching the VM key, then clear needs-human to resume.
Fast-path note: no lifecycle move — sdd-execute is the declared writer of sdd:done on implementation PR merge (ADR 0012).
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
index.crates.io
To allow these domains, add them to the
network.allowedlist in your workflow frontmatter:
network:
allowed:
- defaults
- "index.crates.io"See Network Configuration for more information.
Generated by sdd-validate for issue #582 · ◷
- Check the pubkey length limit on the trimmed key so the trailing newline does not eat one character of the 4096 budget. - Warn when both XDG_STATE_HOME and HOME are unset and the known_hosts path falls back to /tmp. - Restore the TODO comments for the hardcoded local-0 hostname (added in 51e1df2, lost in a later revision) at both hardcoding sites. Refs: #582 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Revise claim for head 4300513. |
|
Auto-revise 3 of 3. |
[sdd-fastpath: tracking=467 tier=sonnet]
Extends the
minvmdready-beacon protocol from one line (READY\n) to two lines (READY\n<openssh-pubkey>\n), so the host pre-populatesknown_hostsbefore the first SSH connection and eliminates the TOFU prompt.Changes
Guest side (
minimald)write_ready_beacon(writer, pubkey)— testable core that formats the two-line payloademit_ready_markernow accepts&PublicKeyand callswrite_ready_beaconemit_simple_ready_markeradded for the degraded READY-only fallback path (rootfs mount failure — no SSH server, no key to advertise)main.rs: loadhost_private_keyonce, reuse for bothknown_hostswrite and the vsock beacon (R1.2 — no redundant disk read)Host side (
minvmd)read_ready_beacon(reader, known_hosts_path)incmd/mod.rs: reads the second line, parses it as aPublicKey, writes theknown_hostsentry vialearn_known_hosts_path; key-parse failures are logged as warnings and never abort boot (R2.3)default_vm_known_hosts_path()helper derives$XDG_STATE_HOME/minimal/providers/local-0/known_hostsboot.rsandrun.rs: duplicated inline READY-reader replaced with the shared helperrusshpromoted from[dev-dependencies]to[dependencies](R2.5)Proof artifacts
Test (
minimald) — R1.1: beacon writesREADY\n<pubkey>\nConstructs a generated Ed25519 key, calls
write_ready_beaconwith an in-memory duplex writer, asserts output equalsREADY\n<openssh-pubkey>\n. Fails against the old single-line implementation.Tests (
minvmd) — R2.1–R2.4: READY-marker read path writesknown_hostsread_ready_beacon_writes_known_hosts_entry: writes a two-line beacon to aCursor, callsread_ready_beacon, asserts the tempknown_hostsfile containslocal-0.read_ready_beacon_tolerates_missing_pubkey_line: single-line beacon →known_hostsnot written,Ok(())returned (backward compat with older guest).read_ready_beacon_rejects_wrong_marker: first line notREADY→Errreturned.All new tests fail against the previous single-line implementation.
Verification
Merging this pull request advances the tracking issue from
sdd:in-progresstosdd:done; a human does the final close.References #467
Summary by CodeRabbit
--gvproxy-binoption for proxy setup.