fix(minvmd): bridge-socket permissions, boot bench hardening, exec example - #382
Conversation
libkrun creates the listening UDS with default permissions (0755), so the verify-only check warned on every boot. Tighten to owner-only from the parent once the socket exists, then verify. The containing dir is already 0700, so this is defense in depth. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- Redirect boot stdin from /dev/null: GNU timeout runs the command in a background process group, and libkrun's console tcsetattr() with a TTY on stdin raises SIGTTOU, stopping the group until the timeout fires. - Require MINVMD_INITRAMFS and validate all three artifact paths. - SIGKILL leaked __krun-vmm processes in teardown (a VM inside krun_start_enter ignores SIGTERM and holds the bridge socket). - Trap INT/TERM so Ctrl-C tears down instead of leaving a detached VM. - Fail the warmup loudly with the boot log tail; print per-run timings. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
russh client over the host-side bridge UDS: authenticates with auth_none, creates a session via the minimald-v1-CreateSession subsystem, and execs the given command scoped to that session, propagating stdout and the exit status. All dependencies are already dev-dependencies from the session e2e test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughAdds socket permission enforcement (0600) and test, integrates enforcement into macOS boot READY path, introduces a russh-based exec example that creates sessions and runs commands via the bridge UDS, and improves the boot benchmark script with temp-file output capture, stricter timeouts, and stronger teardown/traps. ChangesBridge Socket Hardening and Demo Infrastructure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
Comment |
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/bench-minvmd-boot.sh (1)
19-20:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate
Nas a positive integer before entering the loop.Invalid
Nvalues currently fail indirectly and produce non-specific output. Early validation gives a clear failure mode.Suggested patch
N="${1:-10}" BIN="${2:-./target/debug/minvmd}" +[[ "$N" =~ ^[1-9][0-9]*$ ]] || { echo "N must be a positive integer" >&2; exit 1; }Also applies to: 64-64
🤖 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 `@scripts/bench-minvmd-boot.sh` around lines 19 - 20, Validate the N parameter (set via N="${1:-10}") before entering the loop: check that N is a positive integer (e.g., match against a regex like '^[1-9][0-9]*$' or use a safe arithmetic test) and if it fails print a clear error referencing N and exit non-zero; leave BIN handling as-is and only proceed to the loop when N is valid.
🧹 Nitpick comments (1)
crates/minvmd/src/sock.rs (1)
197-205: ⚡ Quick winUse a real Unix socket in the hardening test.
Line 201 currently creates a regular file, so this test doesn’t fully exercise the production case (socket inode permissions).
♻️ Proposed test-fidelity fix
fn enforce_socket_permissions_tightens_to_0600() { use std::os::unix::fs::PermissionsExt as _; let tmp = tempfile::tempdir().unwrap(); let path = tmp.path().join("test.sock"); - std::fs::File::create(&path).unwrap(); + let _listener = std::os::unix::net::UnixListener::bind(&path).unwrap(); std::fs::set_permissions(&path, std::fs::Permissions::from_mode(0o755)).unwrap(); enforce_socket_permissions(&path).unwrap(); verify_socket_permissions(&path).unwrap(); }🤖 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/minvmd/src/sock.rs` around lines 197 - 205, The test enforce_socket_permissions_tightens_to_0600 currently creates a regular file instead of a Unix domain socket, so replace the File::create(&path) call with creation of a real Unix socket (e.g. bind a std::os::unix::net::UnixListener to path) before setting permissions and calling enforce_socket_permissions and verify_socket_permissions; ensure the listener is created at that path (and dropped/closed if needed) so the test exercises socket inode permissions rather than a regular file.
🤖 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/minvmd/examples/exec.rs`:
- Around line 93-103: The code currently treats a missing guest ExitStatus as
host success by calling exit.unwrap_or(0) before std::process::exit; change the
fallback to a non‑zero code and consistently map the ExitStatus to an i32 (for
example replace std::process::exit(exit.unwrap_or(0) as i32) with
std::process::exit(exit.map(|c| c as i32).unwrap_or(1))) so that absent/aborted
exec flows return failure; apply the same fix to the other identical block
referenced (lines ~209-221) in the exec example.
In `@scripts/bench-minvmd-boot.sh`:
- Around line 29-30: Add a preflight check for the timeout utility similar to
the existing perl check: use "command -v timeout >/dev/null || { echo \"timeout
required for run-time limiting\" >&2; exit 1; }" placed near the existing
"command -v perl" line (and add equivalent checks where timeout is assumed at
the other spots referenced). Update the script symbols around the perl check and
the warmup/measurement blocks that call timeout so the script exits early with a
clear error if timeout is missing.
- Around line 56-57: The script currently uses substring matching (grep -q
vm-up) against the output file ($OUT) which can falsely succeed; change the
checks that look for readiness to perform exact-line matching (use grep -Fxq or
grep -x for the exact "vm-up" line) wherever the script checks for vm-up (the
boot verification blocks that use BOOT_TIMEOUT, BIN, and OUT, including the
second check later in the file). Ensure the readiness test matches the whole
line "vm-up" only and fail the boot path if no exact match is found.
---
Outside diff comments:
In `@scripts/bench-minvmd-boot.sh`:
- Around line 19-20: Validate the N parameter (set via N="${1:-10}") before
entering the loop: check that N is a positive integer (e.g., match against a
regex like '^[1-9][0-9]*$' or use a safe arithmetic test) and if it fails print
a clear error referencing N and exit non-zero; leave BIN handling as-is and only
proceed to the loop when N is valid.
---
Nitpick comments:
In `@crates/minvmd/src/sock.rs`:
- Around line 197-205: The test enforce_socket_permissions_tightens_to_0600
currently creates a regular file instead of a Unix domain socket, so replace the
File::create(&path) call with creation of a real Unix socket (e.g. bind a
std::os::unix::net::UnixListener to path) before setting permissions and calling
enforce_socket_permissions and verify_socket_permissions; ensure the listener is
created at that path (and dropped/closed if needed) so the test exercises socket
inode permissions rather than a regular file.
🪄 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: 3ba27f3e-4e4a-4d5f-90b1-dec91817ea44
📒 Files selected for processing (4)
crates/minvmd/examples/exec.rscrates/minvmd/src/cmd/boot.rscrates/minvmd/src/sock.rsscripts/bench-minvmd-boot.sh
- exec example: map missing guest exit status to host failure (1), not 0 - bench script: preflight-check GNU timeout availability - bench script: exact-line match the vm-up READY marker
Follow-ups to #374. Three commits, independent.
Bridge socket permissions
libkrun creates the listening UDS with default permissions (0755); the existing verify-only check warned on every boot.
enforce_socket_permissionschmods it to 0600 from the parent once the socket exists, then verifies. Containing dir is already 0700 — defense in depth. Unit test included.Boot bench hardening (
scripts/bench-minvmd-boot.sh)/dev/null: GNU timeout runs the command in a background process group, and libkrun's consoletcsetattr()with a TTY on stdin raises SIGTTOU, stopping the group until the timeout fires — boots looked hung for 20s and failed silently.MINVMD_INITRAMFSrequired; all three artifact paths validated up front.__krun-vmm(a VM inkrun_start_enterignores SIGTERM and holds the bridge socket, failing every subsequent boot).exec example
cargo run -p minvmd --example exec -- <command>— russh client over the bridge UDS: auth_none, CreateSession subsystem, exec scoped to the session, stdout + exit status propagated. Deps already present as dev-dependencies.Acceptance
cargo test -p minvmd -- --include-ignored: 57 passedcargo clippy -p minvmd --all-targets -- -D warnings: cleancargo fmt: applied🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Improvements
Tests