Skip to content

fix(pod): scope pod kiro-cli oauth grants to the pod home - #8528

Merged
iamwhatever merged 1 commit into
mainfrom
fix/conn-pod-grant-isolation
Sep 7, 2026
Merged

fix(pod): scope pod kiro-cli oauth grants to the pod home#8528
iamwhatever merged 1 commit into
mainfrom
fix/conn-pod-grant-isolation

Conversation

@pepmach

@pepmach pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Problem

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. Two consequences, both observed live:

  • A pod reused the operator's machine-level grants — a provider card read Connected from a grant minted on the real machine, so a pod could not honestly test a connect flow.
  • A grant minted inside a pod outlived pod down as 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, and mcp_discovery's remote probe) — 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. acp.client._apply_pod_home_remap(), applied identically at both spawn transports, makes that same directory the child's literal HOME/USERPROFILE, which is the only lever that moves kiro-cli's own WRITES: it offers no env override for just its OAuth cache. 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). 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.

2. The relocated tree is fenced as an ALTERNATE HOME, not left at a path no matcher covers. security.py anchors KIROCREW_OS_HOME as an alternate $HOME and re-anchors every fenced home_dirs entry under it, so a tool-shaped read of a pod-minted grant is refused in-band. The sandbox mask carves .aws out of the re-anchoring (_POD_OS_HOME_GRANT_STORE_LEAVES) because the child must WRITE its grants there, and re-anchors .aws/config, .aws/credentials and .aws/cli explicitly (_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.

path under <os-home> strict cc standard
.aws (grant store) readable readable readable
.aws/sso/cache (corridor) readable readable readable
.aws/config EMPTY-MASKED EMPTY-MASKED readable
.aws/credentials EMPTY-MASKED EMPTY-MASKED readable
.aws/cli EMPTY-MASKED EMPTY-MASKED readable
.gnupg (sibling control) EMPTY-MASKED EMPTY-MASKED EMPTY-MASKED
.ssh readable readable readable

The carve-out applies only to tiers that actually mask .aws: _STRICT_DIRS and _CC_DIRS list it, _STANDARD_DIRS does 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. .ssh is 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_FILE and AWS_SHARED_CREDENTIALS_FILE are 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/credentials and .aws/cli are empty-masked under the pod home; and no name-based deny rule for those two variables shipssecurity.py records 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_home create-only mirrors the agent runtime's own identity store — the per-platform paths derived from identity_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/cache is 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/cache is 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-cli is a shim that prefers aim sandbox, whose mount plan is built around the real user home; under the remapped HOME it fails Device or resource busy (os error 16) before kiro-cli starts, so the pod-isolation change alone would have left every pod agent turn dead.

argv[0] HOME result
shim real home launches
shim real home, canonical spelling launches
shim empty dir outside the home tree EBUSY (os error 16)
shim empty dir inside the home tree same EBUSY
shim + .toolbox symlink to the real tree same EBUSY
shim + real .toolbox/{bin,tools,registries} skeleton same EBUSY
bundle binary remapped os-home starts, reaching error: You are not logged in

So for a pod child only, acp.client.apply_pod_bundle_spawn spawns the bundle binary the shim itself falls back to (an executable KIRO_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 an open() that never routes through the tool gate. Three of this PR's seven security.py hunks extended exactly that leg and are therefore dropped, not re-expressed: re-adding them would reinstate machinery main's own TestTraversalSimulationIsGone asserts is absent by name. What survives is the fence itself, unchanged and byte-identical to the reviewed revision -- _ResolvedRoots.os_home, _resolved_root_key's KIROCREW_OS_HOME resolution, and _home_dir_targets_uncached's re-anchoring of every home_dirs entry (both separator joins, plus the realpath spelling) -- which is the is_sensitive_path layer #9089 explicitly keeps, alongside this PR's own OS-layer mask (sandbox._pod_os_home_targets). Measured, not assumed: with KIROCREW_POD=1, is_sensitive_path still denies <os-home>/.aws, <os-home>/.aws/sso/cache/<sha256>.token.json and the staged identity store, while the surviving bash matcher now allows the absolute pod-home spelling -- at parity with KIROCREW_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 in docs/system-specs/modules/security.md, so it reads as #9089's posture rather than a gap this mechanism opens. Consequently test_pod_home_remap_security_floor.py loses its Windows-native bash-tokenization class and re-points two bash assertions at the resolving layer; a per-file git patch-id --stable comparison 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:

  1. Toolbox's sandbox is HOME-dependent — closed by item 5.
  2. The re-anchored mask emptied the pod's own grant store. Round 10 re-anchored every tier leaf under the pod home, .aws among 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 so grant_presence answered "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.
  3. The seeder staged the wrong store. kiro-cli resolves its access token from its dirs-crate data dir, not from .aws/sso/cache. This repo already said so — test_the_agent_runtime_auth_stores_stay_visible pins .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 remapped HOME the 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 /health 200 while every agent turn failed, surfacing only as agent_unreachable on each provider — which reads as a Connections bug rather than a boot failure. agent_sdk.pod_child_probe is now the last gate before serving: it resolves the executable the same way, passes it through apply_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:

  • still serving when the bound expires, or a clean rc=0 (the probe hands the child a CLOSED stdin, so a signed-in child reads EOF and exits 0) — viable;
  • exited naming its own login gate — accepted with a warning, seeding is best-effort;
  • no kiro-cli on the host — skipped;
  • could not bootstrap, or could not be SANDBOXED on this hostrefused, through _refuse with EXIT_REFUSED_UNRECOVERABLE and the reason recorded where pod status shows 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 via sandbox_allow_unsandboxed_exec; transient → the same pod up can 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).

  • Pod grant scoping, resolver defaults, and the four call sites: test_mcp_grant.py, test_pod.py.
  • Alternate-home fencing at both matcher surfaces, plus a non-pod target set proven unchanged: test_pod_home_remap_security_floor.py.
  • Corridor + compensating masks + standard-tier invariance: test_sandbox_pod_grant_corridor.py, test_sandbox_governance_mask.py.
  • Identity-store staging derived from store_mappings, macOS/POSIX layouts, XDG_DATA_HOME redirect: test_pod_runtime_auth_store.py.
  • Bundle-binary spawn decision and its non-pod invariance: test_acp_pod_bundle_spawn.py, test_acp_pod_home_remap.py.
  • Probe verdicts per outcome — sandbox-OK (wrap REQUESTED with production's own arguments), sandbox-unavailable → recorded terminal refusal, unsealable ceiling, missing binary, signed-out, clean exit — and the refusal note read no-follow: test_pod_child_viability.py. The probe tests fake the wrap seam rather than exercising the host's sandbox, because asserting through the real wrap_argv passes on a dev box and fails on every CI runner where unshare(CLONE_NEWUSER) returns EPERM under the AppArmor userns restriction.

Drift guards test_spawn_audit.py and test_security_posture.py run 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

# Item Verdict Evidence
i ACP child spawns and survives PASS no AcpRuntimeDead, no login error in the boot; the child served a Test request
ii Test verdict off agent_unreachable PASS notion and linear return code: 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 agent
iii Six providers not_connected on clean seed PASS all six not_connected, grantPresent=False (host grants invisible)
iv Child authenticates from the staged store PASS the not logged in marker is GONE and the probe's signed-out branch does not fire
v Teardown residue / protected pods PASS "isolated HOME nuked (zero residue), live plane untouched"; no orphan added; 7975 untouched

Still open, disclosed rather than implied

  • .ssh is not re-anchored. The alternate-home anchoring covers the fenced home_dirs set; .ssh sits 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.
  • Upstream revocation on pod down. mcp_grant.revoke_local_grant exists and is wired to the Disconnect flow, but not to pod teardown: cleanup_home reclaims 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.
  • A same-UID process can still swap a pinned target between the lstat and 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 the sandbox_allow_unsandboxed_exec opt-in from config, so a hand-rolled copy silently diverges on exactly the decisions that matter. Any new spawn under src/kiro_crew should 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_HOME in the ambient environment at call time, so a check performed before exec-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_reason by-name read following a planted symlink. The class (a by-name read/write on an agent-influenced path) is already covered by the pinned_fs chokepoints 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 pod boot/status output. No dashboard surface changes.

@pepmach
pepmach requested a review from a team as a code owner September 4, 2026 19:03
@pepmach
pepmach requested a review from Zedmor September 4, 2026 19:03
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 4, 2026
@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5.1) — ✅ PASS

Design-level review of 2623c73036ef8742652d23b574f3761acbf9f773 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 (kiro_oauth_cache_home), the child's write side (_apply_pod_home_remap), the sensitive-path fence re-anchored under KIROCREW_OS_HOME, the sandbox mask, best-effort identity-store seeding for sign-in, and a viability probe that converts an unbootable child into a recorded terminal refusal. Every failure path degrades safely (missing bundle → shim → probe refuses; unbuildable os-home → fatal; unreadable store → signed-out boot). The one-way-door surface is minimal and Crew-owned. The residual (a pod-shell raw open() can read the pod-minted grants and the staged token) is structurally forced by harness/tool namespace sharing, is strictly narrower than both baselines it replaces, and is documented and test-pinned in the same commit — not a design gap.

[DESIGN-REVIEWED] 2623c73

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5.1) — 🟡 CONCERNS

Premise-level review of 2623c73036ef8742652d23b574f3761acbf9f773 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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 POD_ROOT anchoring) repair pre-existing bugs unrelated to grant scoping.

What this change ships

Intent: pod-scope kiro-cli's MCP OAuth grants so they die with pod down and a pod can't reuse the operator's host grants. FIX.

  1. Pod's grant reads + child writes resolve to a pod-owned os-home (KIROCREW_OS_HOME, kiro_oauth_cache_home, _apply_pod_home_remap) — justified (the fix)
  2. Relocated tree fenced as alternate $HOME in security/paths.py + sandbox mask — justified (keystone invariant)
  3. Boot stages the runtime identity store into the pod tree — justified (moving HOME breaks kiro-cli sign-in)
  4. Pod child runs the bundle binary + Crew sandbox, not the toolbox shim — justified (shim EBUSYs under remapped HOME)
  5. AWS credential pointers scrubbed from child env; 2 deny rules removed — justified (subtraction)
  6. Boot-time viability probe spawns kiro-cli, refuses a dead pod — rides along (guards a different failure class: health-lies, not grant leakage)
  7. Terminal-exit/refusal-note subsystem (RestartPreventExitStatus, launchd exit-0, refusal file, pod ls section, _REQUIRED_DIRECTIVES) — rides along
  8. KIROCREW_POD_ROOT relative overrides now anchored via abspath (pod/config, session_storage) — rides along
  9. pinned_fs write/read/unlink/lstat helpers; unit.py collapsed onto them — justified (consolidation)
  10. is_kiro_cli threaded through sandboxed_spawn_argv — justified (1 consumer: the probe; routes it through the chokepoint)

Watch

  • Items 7 and 8 fix bugs that pre-date this change: the launchd 5s restart-loop already existed for the standing refusals exit 3 and exit 70 (the diff itself extends handling to them), and the CWD-mismatch on a relative KIROCREW_POD_ROOT is independent of OAuth grants. Both are legitimate, but they are separate logical changes riding on a fix(pod) commit — against AGENTS.md's "one logical change per commit, at most two commits per PR."
  • Item 6's motivating breakage (remap kills every ACP spawn) is itself removed by item 4; the probe now guards the general "up but dead" case rather than the reported defect. Real harm (observed live), so it earns its place, but note the aim shifted.

No item's zero option is costless and none duplicates an existing mechanism, so this is not a BLOCK. The single-member sets (ACP_BACKENDS_POD_HOME_REMAP) are mandated by harness-parity H6 and are not flagged.

[FIRST-PRINCIPLES-REVIEWED] 2623c73

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 2623c73036ef8742652d23b574f3761acbf9f773 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 2623c73

Verdict parsed from the review's SHA-scoped output markers for commit 2623c73036ef8742652d23b574f3761acbf9f773.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 2623c73036ef8742652d23b574f3761acbf9f773: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @pepmach overrides the GPT 5.6 finding for 2623c73036ef8742652d23b574f3761acbf9f773; the recorded reason is authoritative for this commit.

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:
/ai-review override gpt 2623c73036ef8742652d23b574f3761acbf9f773: <one-sentence reason>

@pepmach
pepmach force-pushed the fix/conn-pod-grant-isolation branch from 2a12716 to 6484bdb Compare September 4, 2026 19:51
@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 GPT finding, dispositioned against head 6484bdbaa.

  • AWS pointer bypasses sensitive-path enforcement (src/kiro_crew/acp/client.py, span a1c4f7e9b2d0) — ACCEPTED, fixed. The finding is correct and the exposure was introduced by this PR: security.py matches command TEXT and expands no variables by design, so exporting AWS_SHARED_CREDENTIALS_FILE handed an agent shell a working alias for a path is_sensitive_path refuses by name. Before this change the variable was unset, so cat "$AWS_SHARED_CREDENTIALS_FILE" read nothing.

    Fixed by denying the dereference rather than dropping the export, because the export is a stated requirement (a pod's agent turns need credential_process to keep resolving once HOME moves). New built-in credential-exfil-aws-credential-path-var refuses $NAME, ${NAME}, %NAME%, !NAME! and $env:NAME for both AWS_CONFIG_FILE and AWS_SHARED_CREDENTIALS_FILE; setting or exporting either, and prose or code merely naming one, stay allowed. Pinned red-first by 8 denied spellings and 4 still-allowed forms in test_pod_home_remap_security_floor.py, plus the catalog count and golden-manifest entry.

    Residual, stated rather than implied: an indirection that copies the value first (X=$AWS_CONFIG_FILE; cat "$X") spells neither name in the read. That is the same open-ended class every text-matching deny rule carries, and redact_credentials on output remains the floor. Recorded in the rule's own comment and in security.md.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 GPT finding, dispositioned against head 6484bdbaa.

  • OAuth seeding follows an agent-controlled symlink (src/kiro_crew/pod/runtime.py, span d7b3e1a5c806) — ACCEPTED, fixed. Correct as written: _seed_pod_os_home used cache_dir.mkdir(parents=True, exist_ok=True) and a by-name atomic_write, so a link planted at os-home — or at any component beneath it — was followed and the operator's host SSO token was copied wherever it pointed. It re-runs on every boot, so the window recurred.

    This is not covered by the standing ruling that pods provide operational isolation rather than an adversarial same-UID boundary: the artifact crossing the fence here is the HOST's credential moving OUT into an agent-readable location, not one pod's state reaching another's.

    Fixed with the discipline this same module already applies in seed_home_from_scenario: every component is created and opened through pinned_fs.create_and_open_dir_pinned (parent chain pinned, final component O_NOFOLLOW), the source cache is opened with open_dir_pinned, and the copy goes through copy_file_pinned with both ends pinned, skip_existing=True and force_mode=0o600 — so the inode written is the inode checked and no by-name window remains. A refused component leaves the pod booting signed-out, which is the safe outcome. Pinned red-first by two regressions: a link at os-home and a link at the inner .aws component, each asserting the host token never lands in the target directory.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 GPT finding (non-blocking), dispositioned against head 6484bdbaa.

  • "false" in KIROCREW_POD is truthy and remaps HOME outside a pod (src/kiro_crew/acp/client.py, span f2e8c40b91da) — ACCEPTED, fixed. The gate is now env.get("KIROCREW_POD") != "1", the exact value build_pod_env writes. Pinned by 7 parametrised cases ("false", "0", "no", "off", "", "true", "2", " 1") asserting none of them remaps HOME or pins the AWS pointers.

    This also answers First Principles' subtraction, which asked for the opposite — drop the KIROCREW_POD condition and let KIROCREW_OS_HOME presence alone scope the remap. Declined, with reason: dropping it widens when a credential store is relocated (any KIROCREW_OS_HOME in the environment would qualify), whereas the exact-match narrows it. Both changes target the same read/write asymmetry the lane flagged; the strict comparison closes it in the conservative direction, and build_pod_env remains the only setter of either variable so the latent split the lane describes has no live path.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-1 Design Review (advisory CONCERNS), dispositioned against head 6484bdbaa.

  • Suggestion: gate the remap on a dedicated membership set rather than reusing ACP_BACKENDS_INTERNAL_SANDBOX (span 3c9a5f2e7b14) — ACCEPTED, taken. New ACP_BACKENDS_POD_HOME_REMAP in acp_backends.py, re-exported through acp/types.py, used at both spawn sites. Membership is identical to the sandbox set today and the set still earns its own existence for exactly the reason given: "carries its own OS sandbox" and "relocating HOME moves its credential store" are different properties. Pinned by TestTheCapabilitySetIsItsOwnDecision, including a structural assertion that neither spawn site names the sandbox set in the remap call. Indexed on the H6 row of harness-parity.md.

  • Watch: the remap moves the whole ambient HOME, and the disclosed AcpRuntimeDead / toolbox: Unable to run aim is the predictable first casualty; the author's own remedy — hand MCP-server children the real HOME — should be resolved, not deferred (span 8d1f6b03ae52) — ACKNOWLEDGED; the named remedy is not reachable from this layer, and the blast radius is stated instead. MCP servers are spawned by kiro-cli, not by Kiro Crew: they inherit kiro-cli's own environment, and Crew's only lever is the environment it hands kiro-cli. There is no seam here that lets Crew give the parent a pod HOME and its grandchildren a different one, so "hand MCP-server children the real HOME" cannot be implemented in this PR — it needs harness support. The alternatives are therefore (a) accept that a real-home-dependent MCP server degrades inside a pod, or (b) abandon grant scoping; this PR takes (a).

    What I can state precisely: the degradation is bounded to pods (throwaway instances), never the operator's live gateway, and the pod still boots and serves health 200. What I could not establish is whether the aim entry is caused by the remap at all — a control boot of the identical pod on a pristine tree never exercised the ACP spawn path in its window, and both kiro-cli acp and the aim shim run correctly under a remapped HOME in direct tests. So I have not shown the casualty the finding predicts actually occurred, and I have not shown it did not. Left in the PR body's "did NOT prove" list rather than claimed either way. If a reviewer wants (b) or a narrower remap before pods carry real agent turns, that is a product call I will take rather than decide unilaterally.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@pepmach pepmach changed the title fix: scope pod kiro-cli oauth grants to the pod home fix(pod): scope pod kiro-cli oauth grants to the pod home Sep 4, 2026
@pepmach
pepmach force-pushed the fix/conn-pod-grant-isolation branch 2 times, most recently from 7a146ed to e25747c Compare September 4, 2026 21:20
@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-2 GPT finding, dispositioned against head 6484bdbaa.

  • Pod credential home is agent-readable and replaceable (src/kiro_crew/pod/runtime.py:2231, span b5e1d9c37a02, anchor backend-security-controls) — ACCEPTED, fixed. Correct, and it is the sharpest finding of the round: the previous head closed the $AWS_CONFIG_FILE alias while the relocated tree itself was the larger alias. _SENSITIVE_HOME_DIRS leaves are anchored under the real $HOME plus KIROCREW_HOME / KIRO_HOME, never KIROCREW_OS_HOME, so the seeded copy of the operator's SSO bearer credential sat at a path no matcher covered.

    Fixed exactly as prescribed: KIROCREW_OS_HOME is now anchored as an alternate home root in _resolved_root_key and _home_dir_targets_uncached, and folded into the target cache key so changing it invalidates the memo. Every home_dirs entry re-anchors under it rather than only .aws — the variable relocates the whole home, so .ssh and every other fenced leaf move with it. The real home's anchors are added to, never replaced.

    The second half of the finding (replaceable os-home) was already closed on the previous head by the pinned no-follow descriptor rewrite; the boot path additionally now refuses to seed at all when pinned_fs.supports_pinned_walk() is false, rather than degrading to a by-name copy — moving a host credential through an unpinned write is precisely the redirect the pinning exists to prevent.

    Verified directly, not only by unit test: with the pod markers unset, is_sensitive_path(<pod home>/os-home/.aws/sso/cache/kiro-auth-token.json) is False; with them set it is True, and the real ~/.aws/credentials stays True either way.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-2 Opus finding, dispositioned against head 6484bdbaa.

  • Pod OAuth-grant seeding copies the operator's real SSO identity credential into an os-home/.aws tree the sensitive-path fence never anchors (src/kiro_crew/pod/runtime.py:2233, span c4f70b2e8135) — ACCEPTED, fixed. The three-element derivation is correct end to end and I re-walked it: boot()_seed_pod_os_home copies from Path.home()/.aws/sso/cache; _apply_pod_home_remap makes that tree the child's HOME; the read reaches hooks.on_tool_callis_sensitive_path_path_in_home_dirs, whose anchors came from _home_dir_targets_uncached and did not include KIROCREW_OS_HOME, so the prefix match failed and the read was allowed. Your "before this PR the credential existed only at the fenced real ~/.aws" framing is the right one — the seeding created an unfenced second copy, so the override of the untouched-code demotion is correct too.

    Fixed as prescribed: KIROCREW_OS_HOME added to _resolved_root_key (so it participates in the cache key) and re-anchored in _home_dir_targets_uncached. I widened your remedy in one respect and will name it: rather than re-anchoring .aws and the SSO identity leaves specifically, every home_dirs entry re-anchors under the pod root, because the variable relocates the whole home — anchoring only the subtree the credential happens to occupy today would leave .ssh and the crew-home leaves exposed the moment anything else is seeded or written there.

    One part of your fix list I did not take: extending sandbox_credential_targets. No such symbol exists in this tree (grep -rn "def sandbox_credential_targets" src/kiro_crew/*.py → no match), so there was nothing to extend; if you meant sandbox.py's bind-mount target derivation, note that the Kiro spawn delegates to kiro-cli's internal sandbox on this path (H7), so the hook-layer fence is the enforcement point and it is now closed. Happy to extend an OS-layer list too if you can name the symbol.

    My own test that PINNED the gap as expected behaviour (is_sensitive_path(pod-os-home/.ssh/id_ed25519) is False) was itself part of the defect and is now inverted: test_the_relocated_pod_home_is_fenced_too asserts True for the seeded credential path, a pod-minted grant pair, and .ssh.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-2 Design Review, dispositioned against head 6484bdbaa.

  • The pod os-home/.aws tree is a copy of fenced credentials at an unfenced path (span 9a2c6f14be7d) — ACCEPTED, fixed. Same defect GPT and Opus reached independently, and your framing is the one I took: the PR "converts the lifecycle bug into a confidentiality regression against the keep-listed ~/.aws blocking." You also named the fix pattern precisely — re-anchor the override the way the existing overrides are re-anchored — and I took the wholesale option you offered in parentheses ("or fence os-home wholesale") rather than the .aws-only one, so every fenced leaf follows the relocated home instead of just the subtree the credential occupies today. Fence coverage verified directly: the pod-path credential spelling flips FalseTrue with the anchor while the real home stays True.

  • Watch: the remapped HOME is inherited by every MCP server kiro-cli spawns, and the undiagnosed toolbox: Unable to run aim boot failure is exactly that shape — root-cause before more pin-backs accumulate (span e83b5a01df46) — ACKNOWLEDGED, not fixed in this PR, and the pin-back concern is now bounded rather than open-ended. The inheritance is real and I cannot break it from this layer: MCP servers are spawned by kiro-cli, which passes its own environment down, so Crew has no seam to give the parent a pod HOME and its grandchildren a different one. On the aim failure specifically I have evidence but not attribution — kiro-cli acp and the aim shim each run correctly under a remapped HOME in direct tests, and a control boot on a pristine tree never exercised the ACP spawn path in its window, so I have shown neither that the remap causes it nor that it does not. It stays in the body's "did NOT prove" list.

    What has changed since you raised the pin-back concern: the fence anchor means a future pin-back does not each need its own deny rule. The AWS_CONFIG_FILE rule was needed because that variable points at the REAL home, outside the pod root; anything living under KIROCREW_OS_HOME is now fenced by the anchor itself. So the accumulation you were warning about is capped at pointers that escape the pod tree, and there is exactly one such pair today.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Round-2 First Principles Review (PASS, advisory subtraction), dispositioned against head 6484bdbaa.

  • Shrink kiro_oauth_cache_home() to honor KIROCREW_OS_HOME only when KIROCREW_POD == "1" (config/paths.py:585, span 2f6b8d05ca91) — ACCEPTED, taken. I declined this in round 1 on the grounds that dropping the marker gate on the write side would widen when a credential store relocates; that reasoning does not apply to the subtraction you actually asked for, which narrows the read side to match the write side rather than widening anything. Your counts settle it: 1 writer (build_pod_env), which sets both variables together, and 0 consumers of the override outside a pod — so the paired gate costs nothing and removes the last place where reader and writer could disagree. Pinned by test_the_override_is_inert_outside_a_pod.

    Concretely: kiro_oauth_cache_home() now returns Path.home() unless KIROCREW_POD is exactly "1", the same comparison _apply_pod_home_remap makes. The four resolver tests that exercise the override were updated to declare the marker, which is the honest shape — they were previously testing a wider contract than the pod ever needs.

    Note the interaction with this round's security fix, since it cuts the other way and is worth stating: the security.py fence anchor deliberately does not take the pod-marker gate. The fence must cover the relocated tree whenever KIROCREW_OS_HOME names one, because a stray override there is a confidentiality question rather than a resolution question — fencing a path that turns out not to hold credentials costs a refused read, while failing to fence one that does costs the operator's credential. So: resolution is gated, fencing is not.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 4, 2026
@pepmach
pepmach force-pushed the fix/conn-pod-grant-isolation branch 2 times, most recently from 21a866a to d46e5d6 Compare September 4, 2026 23:00
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 4, 2026
@pepmach
pepmach force-pushed the fix/conn-pod-grant-isolation branch from d46e5d6 to 4ca509e Compare September 4, 2026 23:17
@github-actions github-actions Bot added the merge conflict Branch has merge conflicts with its base — author must resolve before merge label Sep 6, 2026
@pepmach
pepmach force-pushed the fix/conn-pod-grant-isolation branch from 4d01e5a to 638956c Compare September 6, 2026 07:37
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: passed Eligible automated validation passed for the current revision labels Sep 6, 2026
@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/sandbox.py:1109.

  • FIXED (span=fa7ab943e399) — the finding is correct and its premise is now removed at the source rather than masked. The .aws carve-out did expose copied HOST SSO bearer tokens, because _seed_pod_os_home staged the host's ~/.aws/sso/cache into the pod corridor. That staging leg is deleted. Live acceptance on the fixed shape proves it was never load-bearing: the pod child signs in from the agent runtime's own data store (~/.local/share/kiro-cli, staged separately and left unmasked by this repo's own design — see test_the_agent_runtime_auth_stores_stay_visible), and the boot now reports staged 14 file(s) from .local/share/kiro-cli with no login gate, while <os-home>/.aws/sso/cache comes up empty. The corridor therefore contains only grant material the pod itself mints, and pod down reclaims it. Red-first: TestSeedPodOsHome inverts the three tests that previously pinned the copy, and a new suite asserts an empty corridor against a fixture host home that does hold token-shaped files.

On the prescribed remedy's exact form — "narrow the carve-out to .aws/sso/cache only, preserve the .aws mask" — that shape is not expressible with the launcher's bind-mask and I did not fake it. A mask on .aws bind-mounts an empty directory over the whole subtree, including sso/cache; the only re-expose primitive is per-FILE (expose_files), and grant filenames are sha256 cache keys that do not exist until the child mints them, so there is nothing to enumerate at launcher-build time. The mask keeps the same shape as before (.aws carved out; .aws/config, .aws/credentials, .aws/cli re-anchored explicitly), and what changed is that the tree it exposes no longer holds anything host-derived. That is the finding's actual objective — no host bearer token readable from inside the pod — reached by deleting the credential rather than hiding it from the process that must write beside it. Both constants now carry that reasoning in-code.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/acp/client.py:948.

  • FIXED (span=0dfda262228b) — the finding is correct. _apply_pod_home_remap set HOME/USERPROFILE only, and identity_stores.IDENTITY_STORE_ROOTS resolves each store from StoreRoot.env_var before falling back to $HOME. So an inherited data-root override pointing at a host path made the pod's kiro-cli read and write the HOST identity store, and that sign-in state survives pod down — the escape the remap exists to prevent, reached around the side. The remap now pops every data-root override from the child env. The set is IDENTITY_STORE_ROOT_ENV_VARS, derived from the store table rather than restated, so a platform or product row added there is scrubbed without a second edit: today XDG_DATA_HOME (POSIX rows) plus LOCALAPPDATA and APPDATA (Windows rows); the macOS rows carry env_var=None (fixed anchor) so nothing is scrubbed for them, and no XDG_CONFIG_HOME / XDG_STATE_HOME / XDG_CACHE_HOME or KIRO-specific data-dir variable appears in the table.

REMOVE rather than re-anchor, per variable, deliberately. Deleting the override lets the product's own $HOME-relative default resolve under os_home — the exact path the seeder already stages into and the tests already pin — whereas re-anchoring would invent a second spelling of "where the store lives" that must stay in sync with identity_stores forever, and whose failure mode is fail-OPEN (a live store somewhere unintended) rather than fail-closed. A removed variable cannot point anywhere.

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 HOME under the pod; a second test pins the set against IDENTITY_STORE_ROOTS itself; and a third asserts the non-pod env is returned byte-identical in all three no-remap shapes (no marker, marker without KIROCREW_OS_HOME, pod_home_remap=False), so an operator's own override is never touched outside a pod. Live re-acceptance on the new env shape: the pod boots signed in (staged 14 file(s) from .local/share/kiro-cli, no login gate) and POST /api/connections/test answers mcp_server_not_loaded for notion and linear rather than agent_unreachable.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/sandbox.py:1025.

  • FIXED (span=fa7ab943e399) — the exposure is closed at the layer that can close it, and the class is closed rather than the instance. Two audiences, one tree. The pod child's mount namespace must keep <KIROCREW_OS_HOME>/.aws readable and writable: kiro-cli mints its MCP OAuth grants there and no env lever relocates them, so masking the tree empty discards every grant the pod creates. That leaves exactly one audience to fence — an agent TOOL call — and it is fenced in-band by KiroCrew's own gate, not by the sandbox mask. security._resolved_roots anchors KIROCREW_OS_HOME as an alternate $HOME and re-anchors every fenced entry under it, .aws included, so a tool-shaped read of a pod-minted grant is refused before it runs. Verified live at both matcher surfaces: is_sensitive_path returns True for <os-home>/.aws, <os-home>/.aws/sso/cache and a <sha256>.token.json under it; is_sensitive_bash_command returns Blocked: command accesses sensitive credential path (resolved via normalizer: …/os-home/.aws/sso/cache/…) for cat, cp and ls forms. Four red-first tests in test_pod_home_remap_security_floor.py now pin that (path form, bash form, and a non-pod gateway target set left unchanged); separator handling is the builder's and stays covered by the existing gateway-home test, built with the running OS's separator per this suite's convention rather than a hand-mangled spelling.

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 ~/.aws visible to tool subprocesses (sandbox.py:884.aws is absent from _STANDARD_DIRS so credential_process can reach Bedrock auth). A pod adds a gate-layer denial of its own credential tree on top of that. GPT's prescribed broker-outside-the-sandbox would be the stronger design and is out of this PR's scope; nothing here weakens the baseline it replaces.

Disclosed follow-up: upstream revocation on pod down. mcp_grant.revoke_local_grant exists and is wired to the Disconnect flow (connections/ownership.py), but not to pod teardown — cleanup_home deletes the pod's local artifacts and does not call the provider's revocation endpoint. So a grant minted inside a pod 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. That is a real residual, named rather than implied, and it is the one leg a broker would also have to own.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT finding on src/kiro_crew/pod/runtime.py:2506.

  • FIXED (span=201ca675d00e) — correct, and the failure mode was worse than a miss: _RUNTIME_AUTH_STORES hardcoded the two POSIX .local/share paths, so a macOS host (~/Library/Application Support/...) or one with a redirected XDG_DATA_HOME staged nothing, and the boot viability probe then accepted the resulting signed-out pod, because signed-out is a legitimate boot state. The hardcoded tuple is replaced by _runtime_auth_store_mappings(), which returns identity_stores.store_mappings(sys.platform, Path.home(), os.environ) — the same authoritative-table discipline the env scrub already uses. The table follows the override on the SOURCE side and keeps the fixed default layout on the STAGED side, which is exactly a pod's need: read wherever the operator's store really is, write where the child will look. Red-first: a derivation test pins the set against store_mappings itself; parametrized macOS/POSIX fixtures stage each platform's real layout; and an XDG_DATA_HOME-redirected source stages from the redirect while landing at the default layout.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT finding on src/kiro_crew/acp/client.py:900.

  • FIXED (span=a63bd9ceeb00) — all three sites claimed host SSO tokens are staged into the pod home, which stopped being true when that staging was deleted. acp/client.py now says the seeder mirrors the AGENT RUNTIME's identity store (derived from identity_stores) and that the pod's .aws/sso/cache starts empty and holds only pod-minted grants. security.py's os_home rationale is restated the same way and additionally records why the gate is the only layer that can fence that tree (the child's namespace must keep it writable). sandbox.py's _pod_os_home_targets docstring no longer says "stages the host's SSO tokens into it". A stale comment that names a deleted mechanism is exactly what makes the next reviewer re-derive the wrong threat model, so these were corrected in the same push as the code they describe.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the Opus finding on src/kiro_crew/agent_sdk/pod_child_probe.py:83.

  • FIXED (span=f28d826691c9) — the finding is right on both halves: the probe spawned the resolved binary raw, so an unconfined child holding the gateway's own AWS_* material ran on the boot path, while its docstring claimed to be "the REAL spawn path" precisely by skipping the legs that make that path safe. It now applies the same four legs production applies — scrub_agent_subprocess_env on the env, sandbox.wrap_argv on the argv (is_kiro_cli carried through from apply_pod_bundle_spawn so the delegation decision is not re-derived), cgroup_scope_argv OUTERMOST for the pids/memory ceiling, and resource_limit_preexec() post-fork — with the launcher temp file released on every exit path. Confinement degrades per wrap_argv's own contract on a host with no backend rather than being dropped silently, and the docstring now describes what it does instead of what it claimed.

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 BENIGN_SPAWNS entry is removed rather than left to mask a future regression. And a missing binary now refuses through the exit path (the launcher starts, fails to exec, exits non-zero) instead of raising OSError at Popen; the test was corrected to assert the contract — a PodError naming the child with a reason — instead of one message spelling. Red-first: a spy on the probe's Popen proves no AWS_SECRET_ACCESS_KEY / AWS_SESSION_TOKEN reaches the child, and the classification tests neutralise the confinement seams explicitly so that what they grade is the verdict logic, not the wrapper.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/agent_sdk/pod_child_probe.py:111.

  • FIXED (span=3684ad650bbe) — correct, and it explains the four shard reds in the same run. wrap_argv was called unguarded, so on a host with no sandbox backend it raised SandboxUnavailableError, escaped the probe, and boot exited 1 — a restart-eligible code, which is the exact restart loop the _refuse/terminal-exit work in this PR exists to prevent. CI runners are that host: unshare(CLONE_NEWUSER) returns EPERM under the AppArmor userns restriction.

The degradation decision, and why it is a refusal rather than a skip. The real ACP spawn reaches wrap_argv_async with the same options (mode="auto", strip_python_env=True, is_kiro_cli from apply_pod_bundle_spawn), so a host that cannot build a sandbox fails the child's actual spawn identically. The pod is therefore genuinely non-viable, not merely unprobeable — every agent turn would fail while /health answered 200, which is the condition this probe was added to catch. So the sandbox-preparation surface (SandboxUnavailableError, SandboxCeilingUnsealable, and OSError from launcher staging) now converts to PROBE_DEAD, which routes through the caller's existing PodError_refuseEXIT_REFUSED_UNRECOVERABLE path. Terminal, recorded, and visible in pod status.

Both kinds refuse, deliberately: serving a pod whose agent cannot spawn is the failure mode, not the retry budget. What differs is the recorded reason, which carries the error's own kind and detailpermanent tells the operator to install a backend or set sandbox_allow_unsandboxed_exec, transient (namespace exhaustion, ENOSPC) tells them the same pod up can succeed later. That distinction comes from SandboxUnavailableError's own classification rather than from pattern-matching its prose, which is what the type exists for.

The tests were failing for the same reason, and that was a floor violation, not bad luck. They asserted through the REAL wrap_argv, so they passed on a developer box and failed on every runner — a unit test pinning host capability instead of contract. They now fake the wrap seam and assert it was requested with production's own arguments (including is_kiro_cli carried through from the bundle-spawn decision rather than re-derived), plus one test per outcome: sandbox-OK (wrapped + scoped argv is what reaches Popen, no AWS_SECRET_ACCESS_KEY/AWS_SESSION_TOKEN in the child env), sandbox-unavailable → PROBE_DEAD naming kind and mechanism with nothing spawned unconfined as a fallback, unsealable ceiling → same mapping, and the pre-existing missing-binary refusal. One end-to-end test drives an unsandboxable host through boot and asserts EXIT_REFUSED_UNRECOVERABLE with the reason readable through refusal_reason.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/pod/runtime.py:2780.

  • FIXED (span=e23105e8bfba) — real, and it is this PR's own machinery pointed the wrong way. _record_refusal publishes the note through pinned_fs.write_file_pinned, but refusal_reason read it back with a bare target.read_text(...), which follows a link at the final component. So the planted-name attack the write side already refuses still worked in the disclosure direction: a <pod>.refused symlink pointed at any file the gateway can read made kirocrew pod ls print that file's contents under the note's own label.

Red-first, on the exact expression the old code used:

OLD by-name read  -> 'SECRET-MATERIAL'
LEAKED            -> True
NEW pinned read   -> refused: refusing to read pod refusal note: …/pod.refused is not a regular file
LEAKED            -> False

Fixed by adding the READ counterpart to the same chokepoint rather than hand-rolling a guard at the call site — pinned_fs.read_file_pinned, with write_file_pinned's contract mirrored exactly: the ancestor chain resolved ONCE by the caller then pinned component-by-component with O_NOFOLLOW, the target lstat-ed through that descriptor and refused when it is not a regular file, and the final component never resolved. Windows degrades identically and by name: the lstat refusal is KEPT (it is portable, and it is the leg that stops a planted name, which needs no race), ancestor pinning is LOST, and no supported configuration relies on it because pods are systemd/launchd only.

Two details worth stating. The gate is "is a regular file", not "is not a symlink", because a FIFO note would make pod ls block forever waiting for a writer rather than leak — same check catches both, and there is a test for it. And the refusal preserves the function's documented contract that the file EXISTING is itself the signal: a planted note still reports a refusal, just as boot refused (reason file is not a regular file; refusing to read it) instead of as clean, and a missing note is still None.

Also folded in, same class: _seed_pod_os_home's summary line still read "seed os_home's .aws/sso/cache from the REAL host's" while its own body three lines down said that tree is created empty and never populated from the host. A docstring that contradicts itself is how the next reader re-derives the wrong threat model, so the summary now describes what the function does.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the First Principles BLOCK on the PR description.

  • FIXED (span=47eed90e1b1e) — upheld without qualification. The body had accreted as a round-by-round log, so superseded narratives sat beside current ones and two claims were flatly reversed by the shipped diff: "AWS agent turns still work" with AWS_CONFIG_FILE/AWS_SHARED_CREDENTIALS_FILE pinned at the real home (that export was deleted at its source rounds ago) and "the two variable NAMES are themselves denied when dereferenced" (those deny rules were removed, deliberately, with the reasoning recorded in security.py). A human approving from that text would have approved a posture the diff does not implement — which is the whole objection.

Rewritten top to bottom on the reviewer's own five-item skeleton, each item now a claim about the diff:

  1. pod grants are pod-scoped and die with pod down (one shared resolver, both transports);
  2. the relocated tree is fenced and masked as an alternate home, with the two-audience split stated;
  3. a pod agent turn has NO inherited AWS credentials on any path — the true posture, stated as the posture, with the earlier attempt explicitly withdrawn and the absence of a name-based deny rule declared rather than implied;
  4. boot stages the runtime identity store, never the host SSO cache, which starts empty and holds only pod-minted grants;
  5. pod children run the bundle binary Crew-sandboxed — previously present only in a mid-body section, now declared up front with the reason (toolbox's own sandbox EBUSYs under the remap) because it is a real change to the spawn path.

Kept: the passing acceptance table, the three-layer root-cause story, and the honest-gaps section (.ssh re-anchoring and upstream-revoke-on-pod down, plus the same-UID race the pinned chokepoints do not close).

Two further stale claims found by grepping the final body against the diff rather than by reading it. The kiro-auth-token*.json glob sentence is gone — that glob was replaced by an exclusion rule and the host copy deleted entirely. And the mask table's "strict (84 targets) | cc (83) | standard (78)" header was measured live at 82 / 81 / 78: two of three had drifted as main added entries, so the counts are dropped in favour of the qualitative fact the tests actually assert (_STRICT_DIRS and _CC_DIRS list .aws, _STANDARD_DIRS does not, so standard-mode masks are byte-identical). An unpinned number in a PR body is the same defect class in a smaller font.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/agent_sdk/pod_child_probe.py:114.

  • FIXED (span=3684ad650bbe) — the finding is right and its fix line resolves a second guard at the same time. Three guards were pulling the probe in different directions and they converge on one design rather than a compromise.

The defect, precisely. The probe hand-rolled wrap_argv + scrub + cgroup_scope_argv + Popen. The chokepoint it was bypassing does more than compose those: it decides the sandbox TIER and reads the sandbox_allow_unsandboxed_exec opt-in from CONFIG, and config resolves through KIROCREW_HOME in the ambient environment at call time (config_dir() re-reads the override "the moment it changes"). This probe runs in the process that is about to exec the pod gateway, holding pod_env as a dict — so its ambient KIROCREW_HOME was still the HOST's while the real spawn, which happens inside the pod gateway process, reads the POD's. An operator with the opt-in set in HOST config and not in POD config got a probe that PASSED and an agent that could never spawn: healthy status, dead pod. Exactly as reported.

What it routes through now: sandbox.sandboxed_spawn_argv — the module's own documented "single chokepoint for agent-influenced subprocess spawns", which the spawn audit already requires of every such spawn and which applies wrap_argv, scrub_env, the cgroup v2 scope and the spawned-identity marker in one place. Two supporting changes:

  • The preparation runs inside a narrow _process_env(pod_env) scope, so the tier and the opt-in resolve under the POD home. It wraps preparation only, never the child's lifetime, and the pod boot path is single-threaded at that point (the gateway is not up yet) — which is what makes swapping the process environment safe there and would not make it safe in a served gateway.
  • sandboxed_spawn_argv gained an is_kiro_cli passthrough to wrap_argv. Without it a DELEGATING spawn had to choose between the chokepoint (silently losing the delegation decision, asking for a tier the child cannot honour) and its own hand-rolled wrap (drifting from the contract). Growing the chokepoint is the right side of that trade; nothing else changes, None keeps wrap_argv's own classification.

The preexec guard is satisfied by the same move. test_no_sync_spawn_forks_python_in_the_child correctly flagged resource_limit_preexec() — a synchronous preexec_fn forks the threaded gateway and runs Python before exec — and its shrink-only ratchet admits no new entries. Its own prescribed remedy is popen_limited, which applies the same limits AFTER exec where the process is single-threaded, so the probe now spawns through that. The cgroup scope the spawn audit wants is applied INSIDE the chokepoint, so the guard that wanted routing and the guard that forbids preexec are both satisfied without weakening either.

Precedence, pinned so it cannot flap: unavailable > sandbox-unavailable > dead(binary). No kiro-cli at all stays a SKIP, because a host without it is a supported configuration. When both a backend-less sandbox and an unspawnable binary hold, the sandbox reason is recorded: both refuse, so precedence decides only the reason, and the sandbox fact is a property of the HOST that blocks every spawn regardless of which binary is resolved — an operator sent to reinstall kiro-cli would fix the named problem and still get a dead pod.

Test-side: the last host-dependent case (test_an_unspawnable_child_refuses_rather_than_tracebacks) now fakes the seam like its siblings, because on a backend-less runner the sandbox classification fired before its own subject was reachable. Two new tests: one asserts the chokepoint was REQUESTED with production's arguments, and one is red-first on the config context — it sets a HOST KIROCREW_HOME, a different POD one, and asserts every config-reading call saw the POD's (verified failing with the scope removed) plus that the host environment is restored afterwards.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/acp/client.py:981.

  • FIXED (span=0dfda262228b) — upheld, and the reason it took a second round is worth naming: a test asserted the vulnerability. Round 1 removed the pointer export this code manufactured; test_an_operator_set_pointer_is_left_exactly_as_it_was then pinned the inherited one in place, reasoning that an operator-set value "is their own named file, not an alias this function manufactured". That reasoning is wrong about who reads it. build_pod_env keeps AWS_* on purpose, so an absolute host pointer reaches a child whose HOME has moved and whose .aws/config / .aws/credentials / .aws/cli are empty-masked under the new home — the pointer walks around the relocation and the agent obtains the operator's real credentials by dereferencing it. Whose file it is does not change what following it yields. That test is now inverted, with the mistake recorded in it.

Closing the CLASS, not the two names. New explicit companion set CREDENTIAL_POINTER_ENV_VARS, popped alongside IDENTITY_STORE_ROOT_ENV_VARS:

variable why it belongs
AWS_CONFIG_FILE credential/config file the SDK reads directly
AWS_SHARED_CREDENTIALS_FILE same
AWS_WEB_IDENTITY_TOKEN_FILE OIDC token file exchanged for role credentials
AWS_CONTAINER_CREDENTIALS_RELATIVE_URI URL the SDK GETs for credentials
AWS_CONTAINER_CREDENTIALS_FULL_URI same
AWS_CONTAINER_AUTHORIZATION_TOKEN_FILE bearer authorizing that GET

The two container URIs are the reason the class matters beyond the reported pair: they are not filesystem paths, so no mask and no HOME remap can reach them — removal from the env is the only control that touches them at all. The extension rule is stated in code rather than implied: a variable belongs here when its value is a LOCATION that yields credentials when followed.

Why explicit rather than derived, and why not one list. IDENTITY_STORE_ROOT_ENV_VARS is derived from identity_stores.IDENTITY_STORE_ROOTS and correctly misses these — a store ROOT is a directory the product's layout hangs off, while these name a credential file or endpoint directly. No table in this repo enumerates them, so "deriving" them would mean inventing a table whose only consumer is this scrub. And they are deliberately NOT added to sandbox._SENSITIVE_ENV_PREFIXES, the repo's one global scrub: that set covers variables which CARRY a secret and stops there, because the standard sandbox tier leaves the real ~/.aws visible so the AWS CLI and credential_process keep working for non-pod turns. Adding pointers there would break a supported path everywhere to fix a pod-only exposure. One philosophy, two scopes — secrets scrubbed globally, pointers scrubbed where the thing they point at has been relocated.

Audit result, measured rather than assumed (scrub_agent_subprocess_env probed with each name set to a host path):

AWS_SECRET_ACCESS_KEY        DROPS   (global scrub owns it)
AWS_SESSION_TOKEN            DROPS   (global scrub owns it)
AWS_CONFIG_FILE              keeps -> pointer set
AWS_SHARED_CREDENTIALS_FILE  keeps -> pointer set
AWS_WEB_IDENTITY_TOKEN_FILE  keeps -> pointer set
AWS_CONTAINER_*  (x3)        keeps -> pointer set
overlap between the two sets: none

AWS_PROFILE / AWS_DEFAULT_PROFILE are excluded deliberately: they name a profile, not a location, and with the config file gone they resolve nothing. AWS_ACCESS_KEY_ID survives the global scrub but a key id without its secret is not a credential.

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.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the Opus finding on docs/system-specs/modules/security.md:188.

  • FIXED (span=aec828f504b1) — upheld. The spec stated as current fact that _seed_pod_os_home "copies the operator's durable SSO bearer token into <pod home>/os-home/.aws/sso/cache", and at :190 that it "create-only copies the real host's SINGLE-FILE AWS SSO tokens (kiro-auth-token*.json)" behind a "glob deliberately narrower than *.json". That mechanism was DELETED earlier in this campaign — the copied bearer tokens were readable to pod agent tools, and the staging was removed rather than masked around — so both sentences described code this PR does not ship.

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 .aws/sso/cache, and those are what the alternate-home anchoring fences. The old premise made the fence look like it existed to protect a copied host token, which is no longer what is there.
The seeding bullet (:190) now describes what runs: create-only staging of the per-platform identity-store paths derived from identity_stores.store_mappings() (capped at 512 files per store), with the host .aws/sso/cache explicitly NOT copied and the pod's cache created EMPTY. The "cannot pre-authorize a pod to a provider nobody consented to" property still holds and is now attributed correctly — by construction, because nothing is copied, rather than by a filename glob that no longer exists.
The pointer bullet closed with "An operator who sets either pointer variable in their own environment still has it inherited; that is their own named file, not an alias this codebase manufactured." That is the exact claim the GPT blocker on this same head overturned, so it is replaced with the corrected posture and the full pointer family named.

One further drift found while editing, same class: the re-anchor sentence claimed every home_dirs entry re-anchors under the pod root without saying HOW the target is spelled. The re-anchor now emits both separator joins, because the bash surface normalises a token's backslashes to forward slashes before comparing — with the running OS's join alone the Windows target and every Windows candidate never compared equal, which is what failed shard 3 there while passing on POSIX. The spec says so now, since "every entry re-anchors" was true and still insufficient.

@pepmach

pepmach commented Sep 6, 2026

Copy link
Copy Markdown
Contributor Author

Dispositioning the GPT security finding on src/kiro_crew/pod/runtime.py:2909.

  • FIXED (span=e23105e8bfba) — the exposure the finding names is closed, and it was closed by the derivation the finding asks for rather than by a new list; what this round adds is the pinning that proves it and keeps it true for a future store row.

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: identity_stores.fenced_home_dirs() — the SAME table store_mappings() seeds from — is spliced into security._SENSITIVE_HOME_DIRS, and _home_dir_targets_uncached re-anchors every home_dirs entry under KIROCREW_OS_HOME. So all eight rows (both products, all three platform layouts) are already in the os-home re-anchor family, and a row added to that table gains the real-home fence and the pod-home fence with no second edit. Probe on a pod-shaped env:

staged store dir   is_sensitive_path -> True
bearer-token db    is_sensitive_path -> True
db WAL sidecar     is_sensitive_path -> True
bash cat <db>      -> DENIED
bash cp <db> /tmp  -> DENIED
bash sqlite3 <db> .dump -> DENIED
_STRICT_DIRS / _CC_DIRS / _STANDARD_DIRS: entries covering the store -> NONE (mount open)

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 .sqlite3 name would leave the whole database readable in practice), and the harness-access assertion — the pod-home variant of the repo's existing test_the_agent_runtime_auth_stores_stay_visible, asserting no mask tier covers the store so the launcher mounts stay unchanged.

One real gap did surface next door while proving the bash half, and is fixed in the same commit: _win_anchor_roots() carried Path.home() and KIROCREW_HOME but not KIROCREW_OS_HOME. Its own docstring states the rule — a root whose keystone leaves are re-anchored must have its DRIVE recognised, or a backslash token on that drive never reaches is_sensitive_path. This PR made the pod home such a root. Benign while the pod home sits on the user's drive, a hole the moment it does not.

@pepmach

pepmach commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

Refreshing CI against healed main (#9182 merged): close/reopen fires a fresh full run on the identical head a4bb083, since rerun --failed would re-execute the pre-heal merge ref.

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.
@pepmach

pepmach commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

AI-review override not recorded: keep the reason to 500 characters or fewer.

@pepmach

pepmach commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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

@github-actions

github-actions Bot commented Sep 7, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

@pepmach marked the gpt AI finding as false positive, not applicable, or explicitly accepted for 2623c73036ef8742652d23b574f3761acbf9f773.

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.

This decision applies only to this commit. A new push requires a new judgment.

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