feat(sandbox): opt in to Docker registry credentials - #6663
Conversation
GPT 5.6 Review (fork) — 🔴 changes requested (blocking)Reviewed 2 of 2 blocking finding(s) are security-class and were withheld from adjudication, so the blocking verdict stands. BLOCKING -- src/kiro_crew/sandbox.py:3771 -- Reads a credential path forbidden by the backend security rule BLOCKING -- src/kiro_crew/config/loader.py:870 -- Deeply nested grant JSON prevents agent startup (origin: validation) FINDING -- src/kiro_crew/dashboard/handlers/docker_registry_access.py:134 -- [BLOCK-MERGE] 98d4ebf Adjudication (Opus 4.8) — is blocking on each finding proportionate?F1 — sandbox.py credential snapshot exposure. This is the feature the PR exists to add: an operator opt-in, disabled by default, stored in a keystone that the diff adds to F2 — [ADJUDICATION] 98d4ebf total=0 uphold=0 downgrade=0 🏷️ Fenced finding(s) machine-flagged as likely edge caseThe security fence keeps these findings blocking regardless of adjudication; the only clearance path is a human override recorded by a repository writer, who must independently verify a rationale before recording it — it is machine-authored, and a wrong override on a security-class finding ships exactly the class the fence exists to stop. (This lane's comment deliberately carries no override command.)
|
Design Review (Fable 5, fork) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS First deliberate hand-off of credential-floor bytes to the untrusted agent, with no administrator lever and a stated helper gap that can make the disclosure buy nothing. Watch
Suggestions
[DESIGN-REVIEWED] 98d4ebf |
UX Review (Fable 5, fork) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS Turning the grant off hides the one line that says running agents still hold the credentials, and the duration picker's "Allow…" buttons don't allow anything. Watch
Evidence gaps
Suggestions
[UX-REVIEWED] 98d4ebf |
Opus 4.8 Review (fork) — ✅ no blocking findingsReviewed |
First Principles Review (Fable 5, fork) — 🟡 CONCERNSPremise-level review of I've read the contract, the intent file, the full patch, and checked the base tree for existing mechanisms ( First-Principles-Verdict: CONCERNS The 6-hour grant is the whole fix; the "until turned off" variant is justified only by analogy to permanent YOLO and carries a third of the diff. What this change shipsIntent: let an owner run one agent task that pulls private Docker images without turning the sandbox mask off. ADDITION.
Watch
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 98d4ebf |
44d99c0 to
8379f93
Compare
2e4247a to
5f3a962
Compare
5f3a962 to
3438f8c
Compare
|
Current head |
Open PR relationship auditThis is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion. Relationship findings
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
|
Current head |
|
Rebased and fixed the CI flake8 failure on exact head |
|
Fixed the final per-file coverage failure and rebased onto current main on exact head |
|
Fixed the bundle gate without raising its budget on exact head |
|
Rebased onto current |
|
Resolved the new upstream conflict and rebased onto current |
|
Fixed the current CI failures on head
Local verification in CPU/RAM-limited Docker: 58 targeted backend tests passed with CI's pinned Python/pytest versions; TypeScript passed; the complete offline E2E gate passed (18 pytest checks including the browser suite, 375.99 seconds). CPU was capped at four cores and memory at 4 GiB throughout. The branch remains one commit. |
|
|
|
|
|
|
|
|
|
|
|
|
|
|
|
Follow-up to the relationship audit with #7963: 98d4ebf checks the protected grant at both actual ACP spawn wrappers, including adapted harnesses whose reads do not pass through the Kiro tool hook. Provider factories no longer capture authorization. Generic sandbox callers still default to no exposure. The documentation explicitly calls out credential disclosure and existing-process lifetime; the owner must stop existing processes to remove their already-mounted snapshot. |
|
The revision is now one commit, 98d4ebf, rebased onto a468766. GitHub reports it as mergeable. The review concerns have individual responses above, and the PR body includes current scoped test results and screenshot evidence. Update: the fork workflows moved from awaiting approval into queued/running while this comment was being posted; the approval request is no longer needed. CI and fresh reviews are now pending. The two product-policy questions above still need a maintainer ruling; passing CI is not yet being claimed. |
|
The fresh GPT review now blocks the core feature itself under backend-security-controls (F1: remove the credential snapshot exposure), while Opus reports no findings. Its own adjudication recognizes that this is the documented default-off, owner-authorized capability, but cannot clear the security fence. Maintainers: is this explicit credential opt-in acceptable upstream, and if so must it first be constrained by a new governance scope? Removing exposure would remove the purpose of this PR. I am pausing further redesign/re-push rounds for that ruling rather than weakening the review rules or claiming readiness. The new malformed-JSON RecursionError edge case and UX follow-ups are recorded in the latest reviews and remain open; they are not being represented as fixed. |
|
@mrbeag Thanks for this. The 2026-09-08 relationship audit found nothing on #9131 (@chenmingwei23) is the substantive one. It threads #7140 (@GoZippy) rewrites the same #9010 (@chenmingwei23, draft) collides only on the This also still needs the maintainer security ruling you are waiting on before it can land. Posted from the 2026-09-08 open-PR relationship audit (read-only, one auditor per PR); reply here if any of this is wrong. |
Problem / Motivation
Kiro Crew hides
~/.dockerfrom sandboxed agents. That safe default also blocks private image pulls when the owner wants to allow registry access for a task.Why it matters
Owners need a narrow, visible way to grant access without weakening the whole sandbox. Docker config can contain secrets. This feature is default-off and requires maintainer product/security approval.
What changed (motivation → approach → change)
Adds Settings → Security → Docker registry credentials and an owner-only API. The owner can allow access for six hours or until turned off. A protected
docker_registry_access.jsonstores the grant, not the credentials. Malformed and expired grants fail closed. Enabling requires a durable SEL audit before the grant is written; owner reads are audited too.Each actual ACP spawn checks the grant again. This covers native and adapted harnesses, background workers and respawns without caching permission in a provider. On Linux, an anonymous read-only snapshot exposes only Docker's
config.json; the host inode and helper stores remain hidden. Other sandbox callers remain unchanged. Existing processes keep their snapshot until stopped. Pool refresh is best effort after the save.The card shows the expiry or standing-grant label, uses shared error reporting, and resets consent after cancellation. Duration choices have icons and stack on narrow screens. Documentation states the risks and helper limitation. The grant remains separate from
sandbox.min_leveland YOLO settings; the review thread asks maintainers whether a shared governance policy is required before landing. Unrelated test and UI changes were removed. The rebase preserves upstream pod protections.Tests
Validation ran in a resource-capped container. Final checks used one CPU and 4 GiB RAM, sequentially.
The full local matrix was not completed to avoid host pressure. Full cross-platform CI is pending; the counts above are scoped checks, not a claim that every test passed locally.
Manual verification
Used scripted Playwright against the built SPA with fixture API responses in the isolated container, not a live gateway. Inspected active-expiry and 320px consent screenshots. Confirmed the duration choices stack and the acknowledgement and action stay visible.
Screenshots / video
Related Issues
no linked issue: this PR proposes the owner opt-in directly.
Checklist
Contribution License Agreement