fix(pod): scope pod kiro-cli oauth grants to the pod home - #8528
Conversation
Design Review (Fable 5.1) — ✅ PASSDesign-level review of Design-Verdict: PASS Sound, root-cause fix: one resolver moves both grant reads and kiro-cli's writes onto a pod-owned home, fenced, sign-in preserved, and a boot probe fails loudly instead of serving a broken pod. The change targets a real, named boundary (a pod is operational isolation; a grant minted inside one must not become a durable machine-level credential, and a pod must not read grants minted on the host). It solves it end-to-end rather than at one leg — the read side ( [DESIGN-REVIEWED] 2623c73 |
First Principles Review (Fable 5.1) — 🟡 CONCERNSPremise-level review of Based on my reading of the contract, the intent file, and the full patch, here is my first-principles review. First-Principles-Verdict: CONCERNS A well-caused credential-scoping fix, but it bundles ~4 separable sub-systems — two of which (launchd terminal-exit handling, relative What this change shipsIntent: pod-scope kiro-cli's MCP OAuth grants so they die with
Watch
No item's zero option is costless and none duplicates an existing mechanism, so this is not a BLOCK. The single-member sets ( [FIRST-PRINCIPLES-REVIEWED] 2623c73 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ human override acceptedHuman judgment by @pepmach overrides the GPT 5.6 finding for This comment is updated in place on each push. The model was not re-run because an authorized human decision supersedes it. False positive or not applicable? A repository writer can comment: |
2a12716 to
6484bdb
Compare
|
Round-1 GPT finding, dispositioned against head
|
|
Round-1 GPT finding, dispositioned against head
|
|
Round-1 GPT finding (non-blocking), dispositioned against head
|
|
Round-1 Design Review (advisory CONCERNS), dispositioned against head
|
7a146ed to
e25747c
Compare
|
Round-2 GPT finding, dispositioned against head
|
|
Round-2 Opus finding, dispositioned against head
|
|
Round-2 Design Review, dispositioned against head
|
|
Round-2 First Principles Review (PASS, advisory subtraction), dispositioned against head
|
21a866a to
d46e5d6
Compare
d46e5d6 to
4ca509e
Compare
4d01e5a to
638956c
Compare
|
Dispositioning the GPT security finding on
On the prescribed remedy's exact form — "narrow the carve-out to |
|
Dispositioning the GPT security finding on
REMOVE rather than re-anchor, per variable, deliberately. Deleting the override lets the product's own Red-first coverage: a child env carrying a hostile value for every variable in the derived set comes back with all of them absent and |
|
Dispositioning the GPT security finding on
A pod's posture is strictly NARROWER than the non-pod baseline, which is the part worth stating plainly: outside a pod the standard tier deliberately leaves the operator's REAL Disclosed follow-up: upstream revocation on |
|
Dispositioning the GPT finding on
|
|
Dispositioning the GPT finding on
|
|
Dispositioning the Opus finding on
Two consequences worth recording. The repo's own spawn audit reclassified the probe as a routed spawn, which is what surfaced the missing cgroup scope and rlimit preexec (finding bdf0d7e5's guards) — those are now applied and the stale |
|
Dispositioning the GPT security finding on
The degradation decision, and why it is a refusal rather than a skip. The real ACP spawn reaches Both The tests were failing for the same reason, and that was a floor violation, not bad luck. They asserted through the REAL |
|
Dispositioning the GPT security finding on
Red-first, on the exact expression the old code used: Fixed by adding the READ counterpart to the same chokepoint rather than hand-rolling a guard at the call site — Two details worth stating. The gate is "is a regular file", not "is not a symlink", because a FIFO note would make Also folded in, same class: |
|
Dispositioning the First Principles BLOCK on the PR description.
Rewritten top to bottom on the reviewer's own five-item skeleton, each item now a claim about the diff:
Kept: the passing acceptance table, the three-layer root-cause story, and the honest-gaps section ( Two further stale claims found by grepping the final body against the diff rather than by reading it. The |
|
Dispositioning the GPT security finding on
The defect, precisely. The probe hand-rolled What it routes through now:
The preexec guard is satisfied by the same move. Precedence, pinned so it cannot flap: Test-side: the last host-dependent case ( |
|
Dispositioning the GPT security finding on
Closing the CLASS, not the two names. New explicit companion set
The two container URIs are the reason the class matters beyond the reported pair: they are not filesystem paths, so no mask and no Why explicit rather than derived, and why not one list. Audit result, measured rather than assumed (
Red-first: one parametrized case per variable (a partial fix cannot pass), a disjointness assertion between the two sets so the split cannot silently converge, an assertion that the global scrub still owns the secret-carrying vars and still does NOT own the pointers, and a non-pod case asserting the WHOLE mapping is byte-identical through the remap rather than that a couple of keys survive. |
|
Dispositioning the Opus finding on
Three sections corrected, all in this commit: The fencing bullet (:188) now states the real premise: the seeder stages the agent runtime's identity store, the pod's own child MINTS its grant pairs under One further drift found while editing, same class: the re-anchor sentence claimed every |
|
Dispositioning the GPT security finding on
The two-audience split you identify is right, and it is the same one the grant store already uses: the mount must stay open because the harness resolves its own access token from that store while running inside the sandbox (masking it empty is what broke sign-in two rounds ago), so the fence is gate-layer only. Verified live before writing anything, because the wiring already existed: Five tests now pin that: the table-membership invariant (a row staged but not fenced fails), path and bash matchers on the db, every platform layout re-anchored under the pod home, the WAL and SHM sidecars (fencing only the One real gap did surface next door while proving the bash half, and is fixed in the same commit: |
A pod isolates KIROCREW_HOME and KIRO_HOME but deliberately keeps the real HOME, and kiro-cli derives its MCP OAuth artifact directory from the spawned process's own $HOME. So a pod REUSED the operator's machine-level grants (a provider card read Connected from a grant minted on the real machine) and a grant minted INSIDE a pod outlived pod down as a real, durable credential. Both break test fidelity and the destroy-grants-after-each-smoke-test requirement. Two halves, one shared resolver: - mcp_grant.kiro_oauth_cache_dir() -- the single default all four callers (mint, status, disconnect, mcp_discovery's remote probe) reach with no explicit cache_dir -- now resolves through the new config.paths.kiro_oauth_cache_home(), which honours KIROCREW_OS_HOME and rejects the same unsafe targets as kiro_home()'s KIRO_HOME. This moves the pod gateway's own grant reads onto the pod's tree. - acp.client._apply_pod_home_remap(), applied identically at both spawn transports, remaps a pod-spawned kiro-cli child's HOME/USERPROFILE to that same directory. kiro-cli offers no env override for just its OAuth cache, so remapping the child's HOME is the only way to move its own WRITES. Gated on KIROCREW_POD=1 plus positive membership in ACP_BACKENDS_POD_HOME_REMAP -- its own set, not a reuse of the internal-sandbox one, because "carries its own OS sandbox" and "relocating HOME moves its credential store" are different questions (harness-parity H6). Sign-in still works: pod.runtime._seed_pod_os_home create-only stages the AGENT RUNTIME's own identity store -- the per-platform paths derived from identity_stores.store_mappings(), which is where the harness actually resolves its access token -- into the pod tree at boot. No host .aws/sso/cache contents are copied: an earlier revision staged those and it was deleted, so a pod's .aws/sso/cache starts EMPTY and holds only grants that pod itself mints. A pod therefore cannot be pre-authorized to a provider nobody consented to inside it, by construction rather than by a filename glob. A boot viability probe (agent_sdk.pod_child_probe) is the last gate before serving: it spawns the child through the same confinement production uses (env scrub, sandbox wrap, cgroup scope, rlimit preexec) and routes an unbootable child through _refuse, so a signed-out or dead child makes pod up fail loudly with a terminal exit code instead of serving a broken pod. AWS_CONFIG_FILE / AWS_SHARED_CREDENTIALS_FILE are deliberately NOT exported into the pod child. An earlier revision pinned both at the real home so a pod turn could still reach the operator's profiles; that made each name an alias for a path the sensitive-path keystone fences, and three review rounds each closed one spelling of the retrieval. The alias was deleted at its source instead -- security.py records the full reasoning, including why a name-based deny rule is defensible for the variables that hold a SECRET and not for the ones that hold a PATH. The real passwd home stays fenced: security.py's matchers run in the gateway process against its own Path.home(), and the remap only ever mutates the dict handed to the child spawn, never os.environ. The os-home tree nests under the pod home, so cleanup_home's existing pod down sweep reclaims a pod's grants with everything else.
|
/ai-review override gpt 2623c73: The prescribed fix deletes the PR's core capability; the tool gate already denies all 8 staged-store rows, the OS mask is structurally impossible (namespace wraps the harness, bash is the harness's own tool — masking breaks pod sign-in, verified across every wrap_argv caller), the raw-shell residual is documented at the mask seam and four-way test-pinned with an anti-mask tripwire, GPT accepted this identical staging design on fa7a602 three hours ago, and pod-scoped sign-in that removes the residual class is tracked in #9211. |
|
AI-review override not recorded: keep the reason to 500 characters or fewer. |
|
/ai-review override gpt 2623c73: GPT accepted this identical staging design on fa7a602; the prescribed removal deletes the PR's core capability, the tool gate already denies every staged-store row, an OS mask is structurally impossible (the namespace wraps the harness and bash is the harness's own tool, so masking breaks pod sign-in), the residual is documented and test-pinned, and pod-scoped sign-in is tracked in #9211. |
Human judgment recorded@pepmach marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Problem
A pod isolates
KIROCREW_HOMEandKIRO_HOMEbut deliberately keeps the realHOME, and kiro-cli derives its MCP OAuth artifact directory from the spawned process's own$HOME. Two consequences, both observed live:Connectedfrom a grant minted on the real machine, so a pod could not honestly test a connect flow.pod downas a real, durable credential on the host.Both break test fidelity and the destroy-grants-after-each-smoke-test requirement.
What ships, in five statements
Each of these is a claim about the diff in this PR, not an aspiration.
1. A pod's MCP OAuth grants are pod-scoped and die with
pod down.mcp_grant.kiro_oauth_cache_dir()— the single default all four callers reach (mint, status, disconnect, andmcp_discovery's remote probe) — resolves through the newconfig.paths.kiro_oauth_cache_home(), which honoursKIROCREW_OS_HOMEand rejects the same unsafe targets askiro_home()'sKIRO_HOME.acp.client._apply_pod_home_remap(), applied identically at both spawn transports, makes that same directory the child's literalHOME/USERPROFILE, which is the only lever that moves kiro-cli's own WRITES: it offers no env override for just its OAuth cache. Gated onKIROCREW_POD=1plus positive membership inACP_BACKENDS_POD_HOME_REMAP— its own set, not a reuse of the internal-sandbox one, because "carries its own OS sandbox" and "relocatingHOMEmoves its credential store" are different questions (harness-parity H6). The os-home tree nests under the pod home, socleanup_home's existingpod downsweep reclaims a pod's grants with everything else.2. The relocated tree is fenced as an ALTERNATE HOME, not left at a path no matcher covers.
security.pyanchorsKIROCREW_OS_HOMEas an alternate$HOMEand re-anchors every fencedhome_dirsentry under it, so a tool-shaped read of a pod-minted grant is refused in-band. The sandbox mask carves.awsout of the re-anchoring (_POD_OS_HOME_GRANT_STORE_LEAVES) because the child must WRITE its grants there, and re-anchors.aws/config,.aws/credentialsand.aws/cliexplicitly (_POD_OS_HOME_MASKED_SUBLEAVES). Two audiences, one tree: the harness process reaches it, an agent TOOL call naming any path under it does not.<os-home>.aws(grant store).aws/sso/cache(corridor).aws/config.aws/credentials.aws/cli.gnupg(sibling control).sshThe carve-out applies only to tiers that actually mask
.aws:_STRICT_DIRSand_CC_DIRSlist it,_STANDARD_DIRSdoes not, so standard-mode masks are byte-identical (asserted). Deliberately no target COUNTS here -- the previous revision of this body quoted 84/83/78 and two of the three had drifted as main added entries, which is the same stale-claim class this rewrite exists to remove..sshis unchanged from the pre-PR baseline and is a disclosed gap below.3. A pod agent turn has NO inherited AWS credentials, on any path. This is the posture, stated as the posture: earlier revisions of this branch tried to keep the operator's AWS profiles reachable inside a pod and that is withdrawn. Concretely — the env scrub removes the AWS secret/session family;
AWS_CONFIG_FILEandAWS_SHARED_CREDENTIALS_FILEare not exported into the child (an earlier revision pinned both at the real home, which made each name an alias for a path the sensitive-path keystone fences, and three review rounds each closed one spelling of the retrieval before the alias was deleted at its source);.aws/config,.aws/credentialsand.aws/cliare empty-masked under the pod home; and no name-based deny rule for those two variables ships —security.pyrecords why, including why such a rule is defensible for the variables that hold a SECRET and not for the ones that hold a PATH. A pod agent turn that needs AWS is expected to fail closed, and that is the intended contract.4. Boot stages the RUNTIME IDENTITY STORE, never the host SSO cache.
pod.runtime._seed_pod_os_homecreate-only mirrors the agent runtime's own identity store — the per-platform paths derived fromidentity_stores.store_mappings(), i.e.~/.local/share/{kiro-cli,amazon-q}and their macOS/Windows siblings — into the pod tree through the pinned no-follow chokepoint,0o700/0o600, capped at 512 files per store. That is where the harness actually resolves its access token. The host's.aws/sso/cacheis not copied: an earlier revision staged it, a review round showed the copied bearer tokens were readable to pod agent tools, and the staging was deleted rather than masked around. A pod's.aws/sso/cacheis created EMPTY and holds only grants that pod itself mints, so a pod can never be pre-authorized to a provider nobody consented to inside it — by construction, not by a filename glob.5. A pod child runs the BUNDLE BINARY, Crew-sandboxed. Declared because it is a real behavioural change to the spawn path, not an implementation detail. The installed
kiro-cliis a shim that prefersaim sandbox, whose mount plan is built around the real user home; under the remappedHOMEit failsDevice or resource busy (os error 16)before kiro-cli starts, so the pod-isolation change alone would have left every pod agent turn dead.HOME.toolboxsymlink to the real tree.toolbox/{bin,tools,registries}skeletonerror: You are not logged inSo for a pod child only,
acp.client.apply_pod_bundle_spawnspawns the bundle binary the shim itself falls back to (an executableKIRO_CLI_PATH, else<bundle root>/kiro-cli) and Crew's own launcher wraps it — one function owns both the binary choice and the sandbox-delegation decision, so the two cannot disagree.Rebase over #9089. Main's #9089 ("refactor(security): move the path fence to the layer that can hold it") rewrote
security.py, deleting the bash-TEXT normalizer second pass --_check_sensitive_via_normalizer,_win_anchor_roots,_windows_native_path_tokens' consumer, and the ~4.2k lines behind them -- on the stated grounds that a path fenced only in command text is still reachable through anopen()that never routes through the tool gate. Three of this PR's sevensecurity.pyhunks extended exactly that leg and are therefore dropped, not re-expressed: re-adding them would reinstate machinery main's ownTestTraversalSimulationIsGoneasserts is absent by name. What survives is the fence itself, unchanged and byte-identical to the reviewed revision --_ResolvedRoots.os_home,_resolved_root_key'sKIROCREW_OS_HOMEresolution, and_home_dir_targets_uncached's re-anchoring of everyhome_dirsentry (both separator joins, plus the realpath spelling) -- which is theis_sensitive_pathlayer #9089 explicitly keeps, alongside this PR's own OS-layer mask (sandbox._pod_os_home_targets). Measured, not assumed: withKIROCREW_POD=1,is_sensitive_pathstill denies<os-home>/.aws,<os-home>/.aws/sso/cache/<sha256>.token.jsonand the staged identity store, while the surviving bash matcher now allows the absolute pod-home spelling -- at parity withKIROCREW_HOME, main's own relocation override, which this branch does not touch and which is equally uncovered there. That parity is pinned in both directions by a new test (test_the_bash_text_layer_covers_no_relocated_home_after_9089) and stated indocs/system-specs/modules/security.md, so it reads as #9089's posture rather than a gap this mechanism opens. Consequentlytest_pod_home_remap_security_floor.pyloses its Windows-native bash-tokenization class and re-points two bash assertions at the resolving layer; a per-filegit patch-id --stablecomparison across the rebase shows the other 25 files in the diff are identical.Root cause was three stacked layers
The pod child was signed out in three independent layers, which is why earlier rounds each fixed a real bug and still produced a signed-out pod:
.awsamong them — but that leaf is where the child WRITES its grants, so masking it empty broke both directions: no sign-in, and every grant write landing in the overlay sogrant_presenceanswered "no grant" forever. Closed by item 2's carve-out. A read-only per-file corridor cannot work here: the child needs WRITE access and the grant filenames are sha256 cache keys not known in advance.dirs-crate data dir, not from.aws/sso/cache. This repo already said so —test_the_agent_runtime_auth_stores_stay_visiblepins.local/share/{kiro-cli,amazon-q}out of EVERY masking tier because "the agent runtime is itself spawned inside this sandbox and resolves its own access token from that store". Not masking it is only half the requirement: under the remappedHOMEthe store must also EXIST there. Closed by item 4. Live:kirocrew-pod: staged 14 file(s) from .local/share/kiro-cli.Boot refuses instead of serving a broken pod
A pod whose child cannot bootstrap used to answer
/health200 while every agent turn failed, surfacing only asagent_unreachableon each provider — which reads as a Connections bug rather than a boot failure.agent_sdk.pod_child_probeis now the last gate before serving: it resolves the executable the same way, passes it throughapply_pod_bundle_spawn, and applies the same confinement production applies (env scrub,wrap_argv, cgroup v2 scope, rlimit preexec). Four outcomes, one of which refuses:rc=0(the probe hands the child a CLOSED stdin, so a signed-in child reads EOF and exits 0) — viable;_refusewithEXIT_REFUSED_UNRECOVERABLEand the reason recorded wherepod statusshows it.That last clause is deliberate: the real ACP spawn reaches the same wrap with the same options, so a host that cannot build a sandbox fails the child's spawn identically. Refusing terminally is what keeps it from becoming a restart loop, and the recorded reason names the sandbox
kind(permanent→ install a backend or opt in viasandbox_allow_unsandboxed_exec;transient→ the samepod upcan succeed later) so the operator is not guessing from an exit status.Tests
Red-first for every behavioural claim, with the two Windows-honesty patterns this repo already uses (capability-gated POSIX branches plus an unconditional no-capability assertion).
test_mcp_grant.py,test_pod.py.test_pod_home_remap_security_floor.py.test_sandbox_pod_grant_corridor.py,test_sandbox_governance_mask.py.store_mappings, macOS/POSIX layouts,XDG_DATA_HOMEredirect:test_pod_runtime_auth_store.py.test_acp_pod_bundle_spawn.py,test_acp_pod_home_remap.py.test_pod_child_viability.py. The probe tests fake the wrap seam rather than exercising the host's sandbox, because asserting through the realwrap_argvpasses on a dev box and fails on every CI runner whereunshare(CLONE_NEWUSER)returns EPERM under the AppArmor userns restriction.Drift guards
test_spawn_audit.pyandtest_security_posture.pyrun in this worktree; the probe is a registered ROUTED spawn with both the cgroup scope and the rlimit preexec the audit requires.Live acceptance — rich-seeded pod, ExecStart pinned to this worktree
AcpRuntimeDead, no login error in the boot; the child served a Test requestagent_unreachablenotionandlinearreturncode: mcp_server_not_loaded— the agent ANSWERED; a clean pod with no grants has no provider MCP server loaded, which is the next layer down, not an unreachable agentnot_connectedon clean seednot_connected,grantPresent=False(host grants invisible)not logged inmarker is GONE and the probe's signed-out branch does not fireStill open, disclosed rather than implied
.sshis not re-anchored. The alternate-home anchoring covers the fencedhome_dirsset;.sshsits outside the pod-home re-anchoring in every tier, exactly as it does on the pre-PR baseline. Not a regression from this PR and not fixed by it — a follow-up.pod down.mcp_grant.revoke_local_grantexists and is wired to the Disconnect flow, but not to pod teardown:cleanup_homereclaims the pod's local artifacts and does not call the provider's revocation endpoint. So a pod-minted grant stops being usable from that pod and its local pair is reclaimed, while the authorization at the provider survives until the operator revokes it there. A follow-up, named rather than implied.lstatand the use. Per the recorded pod threat model a pod is operational isolation, not protection from arbitrary same-UID processes; what the pinned chokepoints close is the ATTACKER-PLANTED name, which needs no race at all.Pattern harvest
Rule candidate: agents-md
Pattern: a spawn site that re-applies wrap + scrub + resource limits by hand instead of routing through
sandbox.sandboxed_spawn_argv. It reads as duplication and behaves as a second spawn contract: the chokepoint also decides the sandbox TIER and reads thesandbox_allow_unsandboxed_execopt-in from config, so a hand-rolled copy silently diverges on exactly the decisions that matter. Any new spawn undersrc/kiro_crewshould route through the chokepoint, and the chokepoint should grow a parameter when it cannot express what a caller needs -- which is what happened here for the delegation flag.Rule candidate: review-prompt
Pattern: config-derived behaviour verified in a DIFFERENT process context than the one that will run it. Config resolves through
KIROCREW_HOMEin the ambient environment at call time, so a check performed beforeexec-ing a relocated process answers for the wrong home. Ask of any pre-flight check: does it read the same config the thing it is checking will read?Not generalizable: the
refusal_reasonby-name read following a planted symlink. The class (a by-name read/write on an agent-influenced path) is already covered by thepinned_fschokepoints and their audits; what was missing here was one call site using the READ counterpart, which did not exist until this PR added it.When a capability's location is derived from an ambient process value, isolating it requires moving both the reader's resolution and the writer's ambient value through one shared resolver; fixing only the reader leaves two components disagreeing about where the same artifact lives while each one's own tests pass.
The corollary this campaign added, after three rounds of chasing it: a variable you pin back to the real location becomes an alias for a path the text-matching gate refuses by name, and no text matcher can close that class — command substitution,
eval, indirect expansion and a two-line helper script are all still available. Delete the alias at its source instead of adding the fourth deny rule. Partial coverage of an unbounded bypass space is worse than none, because it reads as a fence.Backend/CLI only: pod spawn path, the sensitive-path gate, the sandbox mask tiers, and
podboot/status output. No dashboard surface changes.