vrr/wirenboard-agent-vm
2
0
Fork 0
mirror of https://github.com/wirenboard/agent-vm.git synced 2026-07-11 17:01:29 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

post-review-2: tighten chrome MCP wrapper + log hints + opt-out cleanup + JWT sig length

Browse source 
Folds in 13 findings from the second-pass max-effort review of
ce745ec. (The 14th — "var_os().is_some() opt-out semantics flipped" —
is actually a false positive: both old and new code treat
AGENT_VM_NO_CHROME_MCP=anything as opt-out. The unconventional
"empty value also opts out" behavior is consistent with
AGENT_VM_PROFILE / AGENT_VM_DEBUG_CONFIG elsewhere; documented
inline so the next reviewer doesn't re-flag it.)

**run.rs**

- `sandbox_log_dir()` replaces `runtime_log_path()`. Returns the log
  *directory* (containing `runtime.log` + `kernel.log` +
  `boot-error.json` — verified all three are written by vendor's
  `vm.rs::setup_kernel_log` and `boot_error.rs`). The previous helper
  pointed at `runtime.log` only, which is zero bytes for the most
  common boot-stage failures where the cause sits in kernel.log or
  boot-error.json instead. The doc comment now lists all three files
  and acknowledges that the helper deliberately diverges from
  upstream's `microsandbox_utils::resolve_home()` in one place: when
  `MSB_HOME` and `HOME` are both unset, we canonicalize the relative
  `./.microsandbox` fallback against the current working directory
  so the hint string is absolute (upstream's fallback ships into a
  subprocess where CWD is more controlled; embedding `.`-prefixed
  paths into launcher-side error messages is confusing).

- Error-context wording changed from `(see <path>/runtime.log for the
  in-VM tail)` to `(full logs: <dir>)`. Vendor's `Sandbox::create`
  already inlines the runtime.log tail into BootError messages via
  `tail_runtime_log`; the prior wording duplicated the suggestion
  ("look at the file you just saw the tail of"). The new wording
  positions the directory pointer as the *full* log location, useful
  in both the boot-failure case (vendor already showed you a tail)
  and the post-boot attach failure case (vendor didn't).

- Certutil prelude no longer silently swallows failures. Sudo /
  certutil stderr is captured to a per-invocation tempfile; on
  non-zero exit the launcher prints a 4-line warning that names the
  user-visible symptom (`HTTPS in chrome-devtools MCP will fail with
  ERR_CERT_AUTHORITY_INVALID`) and tail-prefixes the stderr so the
  user can see the actual sudoers / NSS error. Without this, every
  failure mode (dropped sudoers, world-writable sudoers, locked NSS
  DB, missing chrome user, unreadable CA file) was invisible from
  the launcher — chrome MCP boots normally, then every navigate
  silently fails with an opaque CDP error.

- AGENT_VM_NO_CHROME_MCP opt-out semantics commented inline at the
  point where the env-var is consulted, so the "set with any value
  including empty = opt out" rule (consistent with the rest of the
  agent-vm env-var family) doesn't re-trip future reviewers.

**secrets.rs**

- `OPENCODE_OPENAI_ACCESS_PLACEHOLDER` JWT signature length fixed:
  was `msb-opencode-placeholder-a-v2` (29 chars, `len % 4 == 1`
  which is structurally impossible for unpadded base64url and would
  trip any strict JWT parser); now `msb-opencode-placeholder-av2`
  (28 chars, `% 4 == 0`, valid). Sibling `OPENAI_ID_PLACEHOLDER` sig
  is already 28 chars — the rename in the previous commit produced a
  29-char OPENCODE sig by accident. `placeholders_are_pairwise_distinct`
  test continues to pass.

- Always-own-the-mcpServers-key block now tolerates a pre-existing
  `mcpServers: null` (resets to `{}` rather than bailing) and cleans
  up an empty `mcpServers: {}` left over after opt-out. With this,
  `AGENT_VM_NO_CHROME_MCP=1` from a fresh state dir produces a
  `claude.json` with the `mcpServers` key absent (the same shape it
  had pre-Phase-7), rather than the empty placeholder map that
  surprised users opting out from day one.

- Pinned `chrome-devtools-mcp@1.0.1` in the MCP config (was
  `@latest`). The image's pre-warm step now also pins the same
  version; without coupled pins, `npx -y @latest` re-resolves
  against the registry on every launch and invalidates the pre-warm
  cache as soon as upstream cuts a release. Bump both together when
  you want to track a newer release.

**Dockerfile**

- Wrapper script: replace `cd /home/chrome` with `cd /home/chrome
  2>/dev/null || { echo "...image misconfigured?" >&2; exit 1; }`.
  Without this, a missing /home/chrome (derivative image strips it,
  runtime mount fails) silently exits the wrapper from claude's MCP
  transport perspective with no actionable breadcrumb.

- Wrapper script doc: drop the false claim that agentd boot-sets
  `HTTP_PROXY` / `HTTPS_PROXY` / `NO_PROXY` (it doesn't — vendor
  agentd/lib/tls.rs sets only the CA env vars). The proxy env vars
  remain in the preserve list because user MCP `env:` blocks or
  `.agent-vm.runtime.sh` may set them, but the *reason* is
  documented honestly.

- Wrapper script doc: explicitly note that USER and LOGNAME are NOT
  in the preserve list, because sudo's default `env_reset` policy
  re-derives them from the target passwd entry; adding them would
  forward root's USER into the chrome session.

- Pre-warm RUN now `chrome-devtools-mcp@1.0.1 ... 2>&1 || echo
  "==> pre-warm skipped"`. Was an unguarded `npx -y @latest --help
  >/dev/null` that hard-failed the entire `agent-vm setup` on any
  npm registry blip — the opposite of its stated intent of
  insulating launches from registry availability.

Verified e2e in a fresh sandbox:
- pre-warm cache at /home/chrome/.npm/_npx/... is hit by the runtime
  npx invocation (same hash for matching @1.0.1 pin); reports
  app-version=1.0.1
- chromium runs as chrome (UID 9999) with its sandbox active; NSS
  DB trusts only the microsandbox CA with attributes `C,,`
- navigate_page https://example.com + take_snapshot return real
  "Example Domain" content
- AGENT_VM_NO_CHROME_MCP=1: jq 'has("mcpServers")' returns false
  (key entirely absent, not empty {})
- 73 cargo unit tests pass (placeholders_are_pairwise_distinct
  green with the new 28-char OPENCODE sig)

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Evgeny Boger 2026-05-25 12:03:49 +03:00
parent ce745ece6d
commit 43203fe3a9
3 changed files with 151 additions and 44 deletions
Download patch file Download diff file Expand all files Collapse all files

98
crates/agent-vm/src/run.rs
View file

@ -613,6 +613,11 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
// var keeps the opt-out actually opt-out — no sudo, no
// certutil fork.
let project_guest_path_escaped = shell_escape(&project_guest_path);
// Env-var semantics: `AGENT_VM_NO_CHROME_MCP` set to *any* value
// (including empty) opts out. Matches `AGENT_VM_PROFILE` /
// `AGENT_VM_DEBUG_CONFIG` in the same codebase. Unconventional vs
// "VAR=0 means off" — documented here so the next reader doesn't
// re-flag it.
let chrome_mcp_prelude = if std::env::var_os("AGENT_VM_NO_CHROME_MCP").is_some() {
String::new()
} else {
@ -620,15 +625,30 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
// upper layer is regenerated by `Sandbox::builder.replace()`),
// so `certutil -A` always runs against the same baseline; no
// chance of accumulating duplicate trust entries across boots.
// `2>/dev/null || true` keeps a transient missing chrome user
// or unreadable CA file from breaking the prelude.
//
// Failure modes worth surfacing (vs silently swallowing with
// `|| true` like the original Phase 7 patch): sudoers
// dropped/world-writable, NSS DB corrupted, CA file missing
// because agentd's TLS init didn't run, or chrome user
// removed in a downstream image. Without a warning, every
// chrome MCP HTTPS request would return
// `ERR_CERT_AUTHORITY_INVALID` with no breadcrumb back to the
// launcher. Stderr is captured in a temp file and tail'd on
// failure so the user sees the actual certutil/sudo error
// rather than a generic "non-zero exit".
String::from(
"if [ -f /usr/local/share/ca-certificates/microsandbox-ca.crt ] \\\n\
&& [ -d /home/chrome/.pki/nssdb ]; then\n\
\tsudo -u chrome -n -- certutil -d sql:/home/chrome/.pki/nssdb -A \\\n\
\t_cu_err=$(mktemp)\n\
\tif ! sudo -u chrome -n -- certutil -d sql:/home/chrome/.pki/nssdb -A \\\n\
\t\t-t C,, -n microsandbox \\\n\
\t\t-i /usr/local/share/ca-certificates/microsandbox-ca.crt \\\n\
\t\t2>/dev/null || true\n\
\t\t-i /usr/local/share/ca-certificates/microsandbox-ca.crt 2>\"$_cu_err\"; then\n\
\t\techo \"==> warning: failed to install microsandbox CA into chrome NSS DB;\" >&2\n\
\t\techo \"==> HTTPS in chrome-devtools MCP will fail with ERR_CERT_AUTHORITY_INVALID.\" >&2\n\
\t\techo \"==> certutil/sudo stderr:\" >&2\n\
\t\tsed 's/^/==> /' \"$_cu_err\" >&2\n\
\tfi\n\
\trm -f \"$_cu_err\"\n\
fi\n",
)
};
@ -660,7 +680,12 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
sandbox
.attach(cmd, agent_args)
.await
.with_context(|| format!("attaching to {inner_cmd} (see {} for the in-VM tail)", runtime_log_path(&session.sandbox_name).display()))?
.with_context(|| {
format!(
"attaching to {inner_cmd} (full logs: {})",
sandbox_log_dir(&session.sandbox_name).display()
)
})?
} else {
// No host TTY (piped, redirected, smoke-tested under `sg`/`sudo` etc.).
// attach() needs a real /dev/tty for raw-mode stdin, so use the
@ -679,8 +704,8 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
.await
.with_context(|| {
format!(
"running {inner_cmd} in sandbox (see {} for the in-VM tail)",
runtime_log_path(&session.sandbox_name).display()
"running {inner_cmd} in sandbox (full logs: {})",
sandbox_log_dir(&session.sandbox_name).display()
)
})?;
let mut stdout = tokio::io::stdout();
@ -708,8 +733,8 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
}
ExecEvent::Failed(payload) => {
anyhow::bail!(
"exec session failed: {payload:?} (see {} for the in-VM tail)",
runtime_log_path(&session.sandbox_name).display()
"exec session failed: {payload:?} (full logs: {})",
sandbox_log_dir(&session.sandbox_name).display()
);
}
ExecEvent::Started { .. } | ExecEvent::StdinError(_) => {}
@ -718,8 +743,8 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
match exit_code {
Some(c) => c,
None => anyhow::bail!(
"exec session event stream ended without Exited (agentd disconnect or microsandbox bug; partial output above; see {} for the in-VM tail)",
runtime_log_path(&session.sandbox_name).display()
"exec session event stream ended without Exited (agentd disconnect or microsandbox bug; partial output above; full logs: {})",
sandbox_log_dir(&session.sandbox_name).display()
),
}
};
@ -747,22 +772,45 @@ pub async fn launch(agent: Agent, args: Args) -> Result<i32> {
Ok(exit)
}
/// Best-effort hint at where msb writes a sandbox's `runtime.log` (the
/// only post-mortem file vendor actually creates today — `vm.rs`'s
/// `setup_log_capture` redirects the subprocess stderr there). Used
/// solely in the `with_context` of attach errors, so a wrong guess just
/// dilutes the hint message rather than breaking anything. Matches
/// upstream's `microsandbox_utils::resolve_home()` fallback order
/// (`$MSB_HOME` → `$HOME/.microsandbox` → `./.microsandbox`).
fn runtime_log_path(sandbox_name: &str) -> PathBuf {
/// Best-effort hint at where msb writes a sandbox's per-launch
/// logs. The directory contains three files worth checking on a
/// failed launch:
///
/// - `runtime.log` — msb tracing + Rust panic from the sandbox
/// subprocess (vendor `vm.rs::setup_log_capture` redirects its
/// stderr here).
/// - `kernel.log` — kernel printk / early-init panic (vendor
/// `vm.rs::setup_kernel_log`).
/// - `boot-error.json` — structured cause when the VM fails to come
/// up far enough to write into the two above (vendor
/// `boot_error.rs` is the canonical source of "couldn't boot
/// because X").
///
/// Returning a *directory* rather than a single file lets the user
/// `ls` it and pick whichever is non-empty, instead of following a
/// `runtime.log` hint that's zero bytes on early-boot failures.
///
/// Resolution mostly matches upstream's
/// `microsandbox_utils::resolve_home()` (`$MSB_HOME` →
/// `$HOME/.microsandbox` → `./.microsandbox`), with one deliberate
/// difference: when both `MSB_HOME` and `HOME` are unset (cron,
/// systemd-unit-without-Environment=HOME, `env -i`) we canonicalize
/// the relative fallback against the current working directory so
/// the hint string is absolute. Upstream does the same lookup
/// inside the msb subprocess where CWD is more controlled; on the
/// launcher side, embedding `./.microsandbox` in a user-facing
/// error message rendered far from where the launcher ran is
/// confusing.
fn sandbox_log_dir(sandbox_name: &str) -> PathBuf {
let home = env::var_os("MSB_HOME")
.map(PathBuf::from)
.or_else(|| env::var_os("HOME").map(|h| PathBuf::from(h).join(".microsandbox")))
.unwrap_or_else(|| PathBuf::from("./.microsandbox"));
home.join("sandboxes")
.join(sandbox_name)
.join("logs")
.join("runtime.log")
.unwrap_or_else(|| {
env::current_dir()
.unwrap_or_else(|_| PathBuf::from("."))
.join(".microsandbox")
});
home.join("sandboxes").join(sandbox_name).join("logs")
}
/// One `--mount HOST[:GUEST]` argument resolved into separate paths.

51
crates/agent-vm/src/secrets.rs
View file

@ -61,16 +61,23 @@ pub const OPENAI_ID_PLACEHOLDER: &str = "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJ
/// header = base64url('{"alg":"none","typ":"JWT"}')
/// payload = base64url('{"exp":9999999999,
/// "chatgpt_account_id":"00000000-0000-0000-0000-000000000000"}')
/// sig = "msb-opencode-placeholder-a-v2" (non-token-shaped to
/// match the rest of the placeholder family; see the warning
/// on `OPENAI_ID_PLACEHOLDER`)
/// sig = "msb-opencode-placeholder-av2" (28 chars; non-token-
/// shaped to match the rest of the placeholder family; see
/// the warning on `OPENAI_ID_PLACEHOLDER`). Length is
/// deliberately ≡ 0 mod 4 so the segment is still a valid
/// unpadded base64url string — strict JWT parsers
/// (`jose` v6 in strict mode, `jsonwebtoken`) reject the
/// 29-char `…-a-v2` form with "invalid base64" because
/// `len % 4 == 1` is structurally impossible. OpenCode's
/// current parser is lax and tolerates that, but the
/// defensive length is one char away.
///
/// **Kept short on purpose:** an earlier ~480-char payload (with
/// iss/aud/scp/email/sub claims) triggered upstream issue #8 — long
/// placeholders fail sandbox boot with `handshake read id_offset:
/// timed out before relay sent bytes`. Add fields here only if
/// OpenCode actually parses them and chokes on absence.
pub const OPENCODE_OPENAI_ACCESS_PLACEHOLDER: &str = "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJleHAiOjk5OTk5OTk5OTksImNoYXRncHRfYWNjb3VudF9pZCI6IjAwMDAwMDAwLTAwMDAtMDAwMC0wMDAwLTAwMDAwMDAwMDAwMCJ9.msb-opencode-placeholder-a-v2";
pub const OPENCODE_OPENAI_ACCESS_PLACEHOLDER: &str = "eyJhbGciOiJub25lIiwidHlwIjoiSldUIn0.eyJleHAiOjk5OTk5OTk5OTksImNoYXRncHRfYWNjb3VudF9pZCI6IjAwMDAwMDAwLTAwMDAtMDAwMC0wMDAwLTAwMDAwMDAwMDAwMCJ9.msb-opencode-placeholder-av2";
pub const OPENCODE_OPENAI_REFRESH_PLACEHOLDER: &str = "msb-opencode-placeholder-r-v2";
/// Placeholder for the host's `gh auth token`. The in-guest `gh` /
/// git credential helper sees this string; the proxy substitutes the
@ -680,12 +687,23 @@ fn write_default_claude_root_state(path: &Path, project_guest_path: &str) -> Res
// AGENT_VM_NO_CHROME_MCP after a launch without it would leave
// the stale entry in the on-disk claude.json and the MCP would
// keep spawning. We always own this key.
let mcp = obj
//
// `mcpServers: null` (left by an earlier buggy write, or a
// hand-edit) is treated as "no MCP servers" — we reset to {}
// rather than bail. The old `as_object_mut().context(...)?` form
// would have errored out the entire launch over a recoverable
// shape mismatch.
let mcp_entry = obj
.entry("mcpServers".to_string())
.or_insert_with(|| serde_json::json!({}))
.or_insert_with(|| serde_json::json!({}));
if !mcp_entry.is_object() {
*mcp_entry = serde_json::json!({});
}
let mcp = mcp_entry
.as_object_mut()
.context("~/.claude.json mcpServers is not an object")?;
if std::env::var_os("AGENT_VM_NO_CHROME_MCP").is_some() {
.expect("mcp_entry coerced to object above");
let opting_out = std::env::var_os("AGENT_VM_NO_CHROME_MCP").is_some();
if opting_out {
mcp.remove("chrome-devtools");
} else {
mcp.insert(
@ -695,13 +713,28 @@ fn write_default_claude_root_state(path: &Path, project_guest_path: &str) -> Res
"args": [
"npx",
"-y",
"chrome-devtools-mcp@latest",
// Pinned. The image's pre-warm RUN step in
// images/Dockerfile bakes THIS version into
// /home/chrome/.npm/_npx so first launch is a
// cache hit; bump both together. Without a pin,
// npx re-resolves `@latest` against the registry
// on every launch and invalidates the cache as
// soon as upstream publishes anything new.
"chrome-devtools-mcp@1.0.1",
"--headless=true",
"--isolated=true",
],
}),
);
}
// If we ended up with an empty mcpServers map *and* the user is
// opted out, drop the key entirely — don't materialise an empty
// object on disk that would surprise the user inspecting the
// file and could trip future Claude Code schema validation that
// requires absent-or-non-empty.
if opting_out && mcp.is_empty() {
obj.remove("mcpServers");
}
atomic_write(path, serde_json::to_vec(&state)?.as_slice(), 0o644)?;
Ok(())

46
images/Dockerfile
View file

@ -89,20 +89,34 @@ RUN echo 'chrome:x:9999:9999:Chrome MCP:/home/chrome:/bin/bash' >> /etc/passwd \
'# node fails `npx -y` with SELF_SIGNED_CERT_IN_CHAIN.' \
'# - PATH: needed so npx is findable (sudo'\''s secure_path' \
'# would otherwise replace it).' \
'# - HTTP_PROXY / HTTPS_PROXY / NO_PROXY (both cases):' \
'# forward agentd-set proxy config so chrome routes traffic' \
'# the same way as the rest of the guest.' \
'# - HTTP_PROXY / HTTPS_PROXY / NO_PROXY (upper/lowercase):' \
'# NOT set by agentd (microsandbox uses transparent network' \
'# interception, no HTTP proxy env). Forwarded so that a' \
'# user-supplied MCP `env:` block or .agent-vm.runtime.sh' \
'# setting these is honoured by chromium.' \
'# - CHROME_DEVTOOLS_MCP_NO_USAGE_STATISTICS / CI / DEBUG:' \
'# user/MCP-config opt-outs and debugging knobs that' \
'# chrome-devtools-mcp documents.' \
'# - TZ / LANG / LC_ALL / TMPDIR: locale + sane temp paths.' \
'#' \
'# NOT in the preserve list (deliberate):' \
'# - USER / LOGNAME: sudo'\''s default `env_reset` policy re-' \
'# derives these from the target passwd entry. Adding them' \
'# here would forward root'\''s USER into the chrome session.' \
'# - HOME: set by `-H` from chrome'\''s passwd entry (not by' \
'# env passthrough).' \
'#' \
'# `cd /home/chrome` keeps chromium'\''s default CWD writable' \
'# (the agent runs as root in /workspace, where chrome (UID' \
'# 9999) cannot write; tools that emit relative paths' \
'# (./trace.json, ./screenshot.png) would otherwise EACCES).' \
'# Diagnostic on failure so Claude doesn'\''t just see the MCP' \
'# transport close before handshake with no breadcrumb.' \
'set -e' \
'cd /home/chrome' \
'cd /home/chrome 2>/dev/null || {' \
' echo "agent-vm-chrome-mcp: cannot cd to /home/chrome -- image misconfigured?" >&2' \
' exit 1' \
'}' \
'exec sudo -u chrome -H -n \' \
' --preserve-env=NODE_EXTRA_CA_CERTS,SSL_CERT_FILE,CURL_CA_BUNDLE,REQUESTS_CA_BUNDLE \' \
' --preserve-env=PATH,HTTP_PROXY,HTTPS_PROXY,NO_PROXY,http_proxy,https_proxy,no_proxy \' \
@ -130,12 +144,24 @@ RUN curl -fsSL https://deb.nodesource.com/setup_22.x | bash - \
# Pre-warm chrome-devtools-mcp's npm cache as the chrome user so the
# first browser tool call in any sandbox doesn't pay the multi-second
# `npx -y` fetch cost (and so cold launches don't fail entirely when
# registry.npmjs.org is briefly unreachable). `--help` exits cleanly
# after install/version-check; it's the cheapest exercise of the full
# install path. Must come AFTER the nodejs RUN above — npx didn't
# exist before it.
RUN sudo -u chrome -H npx -y chrome-devtools-mcp@latest --help >/dev/null
# `npx -y` fetch cost. `--help` exits cleanly after install/version-
# check; it's the cheapest exercise of the full install path.
#
# Pinned to a known-good version. The runtime MCP config in
# `crates/agent-vm/src/secrets.rs::write_default_claude_root_state`
# pins the SAME version — bump both together. Without the pin the
# cache stops being useful the first time upstream cuts a release:
# `npx -y` re-resolves `@latest` against the registry and re-
# downloads under the writable upper layer.
#
# Best-effort: `|| true` because npm-registry transient failure or a
# bad `--help` exit code shouldn't fail the whole `agent-vm setup`.
# A missing cache costs a multi-second fetch on first browser tool
# call; failing the image build is much worse.
#
# Must come AFTER the nodejs RUN above — npx didn't exist before it.
RUN sudo -u chrome -H npx -y chrome-devtools-mcp@1.0.1 --help >/dev/null 2>&1 \
|| echo "==> pre-warm skipped (will lazy-fetch chrome-devtools-mcp at first MCP call)"
# Claude Code official installer.
RUN curl -fsSL https://claude.ai/install.sh | bash