Skip to content

fix(minvmd): bridge-socket permissions, boot bench hardening, exec example - #382

Merged
norrietaylor merged 5 commits into
mainfrom
minvmd-bridge-followups
Jun 15, 2026
Merged

fix(minvmd): bridge-socket permissions, boot bench hardening, exec example#382
norrietaylor merged 5 commits into
mainfrom
minvmd-bridge-followups

Conversation

@norrietaylor

@norrietaylor norrietaylor commented Jun 11, 2026

Copy link
Copy Markdown
Member

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_permissions chmods 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)

  • Boot invocations redirect 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 — boots looked hung for 20s and failed silently.
  • MINVMD_INITRAMFS required; all three artifact paths validated up front.
  • Teardown SIGKILLs leaked __krun-vmm (a VM in krun_start_enter ignores SIGTERM and holds the bridge socket, failing every subsequent boot).
  • INT/TERM traps tear down on Ctrl-C.
  • Warmup failure prints the boot log tail and exits; per-run timings printed.

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 passed
  • cargo clippy -p minvmd --all-targets -- -D warnings: clean
  • cargo fmt: applied

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Added a Rust exec demo to run user commands inside a guest via the host↔guest bridge.
  • Improvements

    • Hardened bridge socket permissions on macOS to enforce owner-only access.
    • Strengthened benchmark script with additional preflight checks, per-run temp output, tighter timeouts, and clearer diagnostics.
  • Tests

    • Added a unit test verifying socket-permission tightening.

norrietaylor and others added 3 commits June 11, 2026 14:26
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>
@coderabbitai

coderabbitai Bot commented Jun 11, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 0225542e-ce1e-487d-b9dd-c7c599975806

📥 Commits

Reviewing files that changed from the base of the PR and between b5eef9c and ac51a2d.

📒 Files selected for processing (2)
  • crates/minvmd/examples/exec.rs
  • scripts/bench-minvmd-boot.sh
🚧 Files skipped from review as they are similar to previous changes (2)
  • scripts/bench-minvmd-boot.sh
  • crates/minvmd/examples/exec.rs

📝 Walkthrough

Walkthrough

Adds 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.

Changes

Bridge Socket Hardening and Demo Infrastructure

Layer / File(s) Summary
Socket Permission Enforcement
crates/minvmd/src/sock.rs, crates/minvmd/src/cmd/boot.rs
enforce_socket_permissions (public) sets socket mode to owner-only 0600; unit test added; boot now calls enforcement then verifies permissions after READY and warns if it cannot secure to 0600.
Exec Example Client Setup
crates/minvmd/examples/exec.rs
Adds CreateSession JSON types and a russh Handler, main entrypoint that assembles CLI command, resolves bridge UDS (with MINVMD_BRIDGE_SOCK override), connects with retries, and runs authenticate_none.
Exec Session and Command Execution
crates/minvmd/examples/exec.rs
Implements CreateSession RPC over an SSH session channel (JSON request/response), opens an exec channel with MINIMAL_SESSION_ID env and channel.exec, and consumes ChannelMsg events to collect stdout and exit status, failing on Failure.
Boot Benchmark Harness Improvements
scripts/bench-minvmd-boot.sh
Adds MINVMD_INITRAMFS requirement and preflight checks, uses a per-run temp output file, reduces BOOT_TIMEOUT to 12s, strengthens teardown() with pkill -9, adds EXIT and INT/TERM traps, captures warmup output and validates vm-up, and records per-run timings only for successful boots.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~45 minutes

Possibly related issues

Possibly related PRs

Suggested labels

needs-human

Suggested reviewers

  • evanspearman
  • jtnkminimal
  • twitchyliquid64

Poem

🐰 I tightened the bridge with careful paws,
I hopped through rust code and russh applause,
The bench now captures each boot's tiny beat,
Sessions spawn commands, stdout is sweet,
A little rabbit cheers: the flow is complete.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title directly summarizes the three main changes: bridge-socket permissions hardening, boot benchmark script improvements, and a new exec example.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 win

Validate N as a positive integer before entering the loop.

Invalid N values 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 win

Use 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

📥 Commits

Reviewing files that changed from the base of the PR and between 56f9449 and b5eef9c.

📒 Files selected for processing (4)
  • crates/minvmd/examples/exec.rs
  • crates/minvmd/src/cmd/boot.rs
  • crates/minvmd/src/sock.rs
  • scripts/bench-minvmd-boot.sh

Comment thread crates/minvmd/examples/exec.rs
Comment thread scripts/bench-minvmd-boot.sh
Comment thread scripts/bench-minvmd-boot.sh Outdated
norrietaylor and others added 2 commits June 12, 2026 09:05
- 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
@norrietaylor
norrietaylor enabled auto-merge (squash) June 12, 2026 17:44
@norrietaylor
norrietaylor merged commit 0600eb0 into main Jun 15, 2026
21 checks passed
@norrietaylor
norrietaylor deleted the minvmd-bridge-followups branch June 15, 2026 16:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants