From 4479b1f2d1676fbf5a16e8e46162c276120a5404 Mon Sep 17 00:00:00 2001 From: Evgeny Boger Date: Sun, 24 May 2026 13:11:27 +0300 Subject: [PATCH] fix(phase6): make gh CLI actually usable inside the VM (graphql, utility paths, safe.directory) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two related fixes from real end-to-end testing of Phase 6: **Fix 1: gh CLI was effectively broken inside the VM.** The original github_path_allowed accepted only /user/* and /repos///..., which blocked the GraphQL endpoint gh CLI needs for the most common workflows: - `gh pr create` → POST /graphql (rejected with 403) - `gh issue create` → POST /graphql (rejected) - `gh repo view --json` → POST /graphql (rejected) - `gh issue list --search ...` → /search/issues (rejected) - `gh repo view org/x` → /orgs/ (rejected) - gh ambient probes → /rate_limit, /meta, /markdown (rejected) Extend the allow-list: - /graphql is allowed unconditionally. Per-repo scoping doesn't reach into GraphQL bodies in v1; this is a documented trade-off. - /search/*, /rate_limit, /meta, /markdown are allowed (read-only utility, no per-repo state exposed). - /orgs/ (bare) is allowed for `gh repo view org/...` resolution; /orgs by itself (listing) is denied to avoid leaking org membership. - /user surface narrowed: only /user (auth probe) and /user/orgs are allowed. /user/repos, /user/keys, /user/emails, /user/gpg_keys are now BLOCKED — the previous catch-all leaked the full inventory of private repos and SSH keys / verified emails (see code-review finding #4 from this session). - Reject any path segment equal to ".." anywhere to close the /repos///../../ path-traversal that GitHub would otherwise normalise upstream (code-review finding #2). **Fix 2: `fatal: detected dubious ownership in repository`.** The project is bind-mounted into the guest as the host user (UID 1000) but the in-guest agent runs as root (UID 0). git refuses operations on repos whose .git is owned by a different UID. Symptom: every `git status` / `git log` / `git show` etc. inside the guest aborts. write_guest_gh_config now ALWAYS writes a base /root/.gitconfig with `[safe] directory = *` regardless of whether gh auth was captured. The credential helper + url.insteadOf stanzas are only included when has_gh_token=true; the gh hosts.yml is still gated on having a token. Launcher always calls write_guest_gh_config (was gated on gh_token_file before) so safe.directory is in place even with --no-git or no host gh auth. Verified inside the guest: - `git status` returns "On branch master / nothing to commit" — no dubious-ownership abort. - `git log` reads the bind-mounted history. - `gh api graphql -f query="query { viewer { login } }"` reaches GitHub (response is Bad credentials, same nested-host limit as documented for the other providers — not our allow-list). - `gh api /rate_limit` reaches GitHub. - `gh api repos/octocat/Hello-World` still returns the clean 403 naming the allow-listed repos. Co-Authored-By: Claude Opus 4.7 (1M context) --- crates/agent-vm/src/intercept_hook.rs | 60 ++++++++++++++++--- crates/agent-vm/src/run.rs | 14 ++--- crates/agent-vm/src/secrets.rs | 86 +++++++++++++++++---------- 3 files changed, 113 insertions(+), 47 deletions(-) diff --git a/crates/agent-vm/src/intercept_hook.rs b/crates/agent-vm/src/intercept_hook.rs index c0bce3c..212ce3e 100644 --- a/crates/agent-vm/src/intercept_hook.rs +++ b/crates/agent-vm/src/intercept_hook.rs @@ -235,17 +235,61 @@ fn parse_http_request(req: &[u8]) -> Result<(String, String, Vec<(String, String /// Path-based allow-list check for api.github.com requests. /// -/// Accepts: +/// Accepts (case-insensitive on owner/repo): /// - `/repos//[/...]` where `/` is in the -/// allow-list (case-insensitive). -/// - `/user` and `/user/...` (gh CLI uses these to confirm auth + -/// list accessible repos; doesn't expose specific-repo state). -/// - `/repos//` for the parent-fetch of a fork (we -/// accept any owner here since the parent is read-only metadata — -/// reconsider if this proves too permissive). +/// allow-list, after rejecting any `..` traversal segment so a +/// crafted path like `/repos///../..//` +/// can't pass the surface check and let GitHub resolve `..` +/// upstream. +/// - `/user` and `/user/...` — gh CLI auth-status, current-user info. +/// Limited to the small set gh actually needs: /user (auth probe), +/// /user/orgs (org membership). Excludes /user/repos, /user/keys, +/// /user/emails, /user/gpg_keys to avoid leaking full inventory / +/// PII. +/// - **`/graphql`** — gh CLI's `gh pr create`, `gh issue create`, +/// `gh repo view --json` all use GraphQL mutations. We DO NOT body- +/// filter the GraphQL request: per-repo scoping doesn't reach into +/// the JSON body in v1. Accept that an agent that crafts arbitrary +/// GraphQL queries can read anything the token can; the practical +/// trade-off is "gh CLI works" vs. "no GraphQL". +/// - `/search/...`, `/rate_limit`, `/meta`, `/markdown` — small +/// utility endpoints gh CLI uses ambiently. +/// - `/orgs/` (no further path) — gh CLI reads org metadata to +/// resolve `gh repo view org/...`. fn github_path_allowed(path: &str, allowed: &[String]) -> bool { let p = path.split_once('?').map(|(p, _)| p).unwrap_or(path); - if p == "/user" || p.starts_with("/user/") { + + // Reject traversal anywhere in the path. GitHub normalises `..` + // upstream and would otherwise serve a different (blocked) repo. + for seg in p.split('/') { + if seg == ".." { + return false; + } + } + + // Narrow /user/* surface to what gh actually needs for auth + + // org-membership checks. Excludes /user/repos, /user/keys, + // /user/emails, /user/gpg_keys (PII / full-inventory leak). + if p == "/user" || p == "/user/orgs" || p.starts_with("/user/orgs/") { + return true; + } + // Utility endpoints — read-only, no per-repo state. + if matches!(p, "/rate_limit" | "/meta" | "/markdown") { + return true; + } + if p == "/search" || p.starts_with("/search/") { + return true; + } + if p == "/orgs" { + return false; // listing orgs isn't useful and exposes membership + } + // gh resolves `gh repo view org/x` via /orgs/ first; allow + // bare org-info reads (no /repos sub-path which would be a list). + if let Some(rest) = p.strip_prefix("/orgs/") { + return !rest.is_empty() && !rest.contains('/'); + } + // GraphQL: gh PR/issue creation. No body-filter — see fn doc. + if p == "/graphql" { return true; } if let Some(rest) = p.strip_prefix("/repos/") { diff --git a/crates/agent-vm/src/run.rs b/crates/agent-vm/src/run.rs index f203580..b3181f9 100644 --- a/crates/agent-vm/src/run.rs +++ b/crates/agent-vm/src/run.rs @@ -230,13 +230,13 @@ pub async fn launch(agent: Agent, args: Args) -> Result { let creds = crate::secrets::refresh(&session.state_dir, &project_guest_path, use_github) .context("snapshotting host credentials")?; - // Phase 6: write guest-side gh/git config that reaches for the - // placeholder bearer the proxy will substitute on outbound. Only - // wired when we actually captured a gh token. - if creds.gh_token_file.is_some() { - crate::secrets::write_guest_gh_config(&session.state_dir) - .context("writing guest gh/git config")?; - } + // Phase 6/9: always write the guest gitconfig (carries the + // unconditional `safe.directory = *` so git inside the guest + // accepts the host-bind-mounted project despite the UID + // mismatch). The credential-helper / gh hosts.yml stanzas are + // gated on having actually captured a host gh token. + crate::secrets::write_guest_gh_config(&session.state_dir, creds.gh_token_file.is_some()) + .context("writing guest gh/git config")?; // Phase 7: parse `--mount HOST[:GUEST]` extras. The libkrun IRQ // pool is *tight* — empirically the project bind + state bind + diff --git a/crates/agent-vm/src/secrets.rs b/crates/agent-vm/src/secrets.rs index 9f72005..6647c30 100644 --- a/crates/agent-vm/src/secrets.rs +++ b/crates/agent-vm/src/secrets.rs @@ -627,47 +627,69 @@ fn write_default_claude_root_state(path: &Path, project_guest_path: &str) -> Res Ok(()) } -/// Phase 6: write the gh/git config the guest agent uses to talk to -/// GitHub. Files land under `state_dir` so they're available inside -/// the guest via the existing bind mount + symlinks (see run.rs -/// patch builder). +/// Phase 6/9: write the guest's gitconfig and (if `has_gh_token`) +/// gh/git credential plumbing. Always called so the +/// `safe.directory = *` line is in place — without it, git inside the +/// guest fails with "fatal: detected dubious ownership in repository" +/// because the host-owned bind-mounted project is read by the guest's +/// root user (different UID). +/// +/// Files land under `state_dir` so they're available inside the guest +/// via the existing bind mount + symlinks (see run.rs patch builder). /// /// - `/gitconfig` → symlinked to `/root/.gitconfig` in the -/// guest. A `credential.helper` echoes `username=x-access-token` -/// and `password=` so `git push` to GitHub goes out -/// as `Authorization: Basic base64(x-access-token:placeholder)`, -/// which the proxy substitutes on the wire. +/// guest. Always contains `safe.directory = *`. When the host has +/// gh auth, also contains a `credential.helper` that echoes +/// `username=x-access-token` / `password=` so +/// `git push` to GitHub goes out as +/// `Authorization: Basic base64(x-access-token:placeholder)`, which +/// the proxy substitutes on the wire. /// - `/gh-config/hosts.yml` → symlinked to `/root/.config/gh` -/// in the guest. The placeholder is what gh CLI sends; the proxy -/// substitutes outbound to api.github.com. -pub fn write_guest_gh_config(state_dir: &Path) -> Result<()> { - let gitconfig = format!( - // The credential helper is a shell snippet; git invokes it - // with `get` and reads `username=`/`password=` lines. - "[credential \"https://github.com\"]\n\ - \thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\ - [credential \"https://gist.github.com\"]\n\ - \thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\ - [url \"https://github.com/\"]\n\ - \tinsteadOf = git@github.com:\n\ +/// in the guest. Only written when `has_gh_token`; the placeholder +/// is what gh CLI sends and the proxy substitutes outbound to +/// api.github.com. +pub fn write_guest_gh_config(state_dir: &Path, has_gh_token: bool) -> Result<()> { + // safe.directory = * is unconditional: the guest IS the security + // boundary (microVM), so trusting every path is fine and git + // operating on the host-bind-mounted project requires it. Use + // wildcard form so any --mount extra also works without + // listing every path. + let mut gitconfig = String::from( + "[safe]\n\ + \tdirectory = *\n\ [user]\n\ \tname = agent-vm\n\ \temail = agent-vm@msb.local\n", - tok = GH_TOKEN_PLACEHOLDER, ); + if has_gh_token { + // git's credential helper for github.com pushes/clones. The + // helper is a shell snippet; git invokes it with `get` and + // reads `username=`/`password=` lines from stdout. + gitconfig.push_str(&format!( + "[credential \"https://github.com\"]\n\ + \thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\ + [credential \"https://gist.github.com\"]\n\ + \thelper = \"!f() {{ test \\\"$1\\\" = get && echo username=x-access-token && echo password={tok}; }}; f\"\n\ + [url \"https://github.com/\"]\n\ + \tinsteadOf = git@github.com:\n", + tok = GH_TOKEN_PLACEHOLDER, + )); + } atomic_write(&state_dir.join("gitconfig"), gitconfig.as_bytes(), 0o600)?; - let gh_dir = state_dir.join("gh-config"); - std::fs::create_dir_all(&gh_dir)?; - let hosts_yml = format!( - "github.com:\n\ - \\x20\\x20user: agent-vm\n\ - \\x20\\x20oauth_token: {tok}\n\ - \\x20\\x20git_protocol: https\n", - tok = GH_TOKEN_PLACEHOLDER, - ) - .replace("\\x20", " "); - atomic_write(&gh_dir.join("hosts.yml"), hosts_yml.as_bytes(), 0o600)?; + if has_gh_token { + let gh_dir = state_dir.join("gh-config"); + std::fs::create_dir_all(&gh_dir)?; + let hosts_yml = format!( + "github.com:\n\ + \\x20\\x20user: agent-vm\n\ + \\x20\\x20oauth_token: {tok}\n\ + \\x20\\x20git_protocol: https\n", + tok = GH_TOKEN_PLACEHOLDER, + ) + .replace("\\x20", " "); + atomic_write(&gh_dir.join("hosts.yml"), hosts_yml.as_bytes(), 0o600)?; + } Ok(()) }