Skip to content

feat(sandbox): opt in to Docker registry credentials - #6663

Open
mrbeag wants to merge 1 commit into
kirodotdev:mainfrom
mrbeag:feat/docker-registry-sandbox-opt-in
Open

feat(sandbox): opt in to Docker registry credentials#6663
mrbeag wants to merge 1 commit into
kirodotdev:mainfrom
mrbeag:feat/docker-registry-sandbox-opt-in

Conversation

@mrbeag

@mrbeag mrbeag commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Kiro Crew hides ~/.docker from 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.json stores 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_level and 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.

  • Rebased ACP, sandbox, pod, API and provider run: 1,771 passed and 10 skipped, with one new audit-test expectation failure. The fixture caller expectation was corrected; the final API/provider rerun passed all 28 tests.
  • Final focused Security/settings tests: 126 passed.
  • TypeScript and Vite production build passed. Backend lint/type checks, scoped formatting/import checks, and security/documentation gates passed during validation.
  • Added coverage for malformed grants, expiry/revocation at spawn, FIFO rejection, critical audit failure, duration display and cancelled consent.

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

Active grant with visible expiry

Consent at 320px

Related Issues

no linked issue: this PR proposes the owner opt-in directly.

Checklist

  • One commit with a Conventional Commits title
  • Existing tests pass and new tests added for new functionality — new coverage and scoped checks pass; full CI pending
  • Self-review completed; code follows project style guidelines
  • Documentation and threat model updated
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

@mrbeag
mrbeag requested a review from a team August 28, 2026 23:36
@mrbeag
mrbeag requested a review from a team as a code owner August 28, 2026 23:36
@mrbeag
mrbeag requested a review from dwu96 August 28, 2026 23:36
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 28, 2026
@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — 🔴 changes requested (blocking)

Reviewed 98d4ebff2bceda914acc5706216ce2db06851891 via the fork AI-review pipeline; updated in place on each push.

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
docker_config_data = fh.read()
Enabled grant -> ACP spawn -> launcher mounts the credential snapshot -> untrusted agent receives registry credentials.
Anchor: backend-security-controls
Fix: Keep ~/.docker masked and remove the credential snapshot exposure.

BLOCKING -- src/kiro_crew/config/loader.py:870 -- Deeply nested grant JSON prevents agent startup (origin: validation)
except (OSError, ValueError):
Deeply nested grant JSON -> docker_registry_access_state() -> uncaught RecursionError -> every ACP spawn fails.
Anchor: residual/crash-data-loss-corruption
Fix: Catch RecursionError and return the disabled state.

FINDING -- src/kiro_crew/dashboard/handlers/docker_registry_access.py:134 -- "permanent", False accepts {"enabled": true} without the promised explicit duration or acknowledgement, silently creating a six-hour credential grant -> Fix: require explicit duration and acknowledgement fields before enabling.

[BLOCK-MERGE] 98d4ebf
[GPT-REVIEWED] 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 _CREW_READONLY_LEAVES/_CREW_SECRET_LEAVES (sealed read-only in every sandbox mode) and to is_sensitive_write_path, so an agent cannot self-grant. The only path to credential exposure is a deliberate, owner-authenticated dashboard PUT (require_owner_dashboard_request, handler diff:338) with a critical SEL audit gate and platform check, re-read on every spawn (sandbox.py wrap_argv diff:672-677). The security spec explicitly records disclosure as accepted residual risk while enabled (security-deep-dive.md diff:23-28). Recovery: PUT enabled=false stops new snapshots; live processes revoked by stopping them. This is the textbook accepted-residual-risk opt-in — FLAG.

F2except (OSError, ValueError) in docker_registry_access_state(). RecursionError (a RuntimeError subclass) is genuinely not caught, and docker_registry_access_enabled() runs on every ACP spawn. But the sole writer of this keystone is the owner PUT handler, which emits flat JSON via json.dumps(state) with ≤3 scalar keys (handler diff:384-390); the file is a read-only, secret keystone the agent cannot write. Deeply nested JSON is thus an input the system's own writer cannot produce — only manual out-of-band tampering with a protected file yields it. Recovery is trivial and loud: repair/remove the file → state reads disabled → spawns resume. Condition unproducible by the system + trivial recovery — FLAG.

[ADJUDICATION] 98d4ebf total=0 uphold=0 downgrade=0
[GPT-ADJUDICATED] 98d4ebf
[ADJUDICATION-FENCED] 98d4ebf fenced=2 flagged=2
FLAG F1 src/kiro_crew/sandbox.py:3771 -- Credential exposure requires a deliberate owner-only opt-in on a read-only keystone the agent cannot self-grant, with disclosure documented as accepted residual risk while enabled.
FLAG F2 src/kiro_crew/config/loader.py:870 -- The uncaught RecursionError requires deeply nested JSON that only manual tampering can place in this read-only keystone, since the sole writer emits flat JSON; failure is loud and recovered by repairing the file.
[GPT-ADJUDICATED-FENCED] 98d4ebf

🏷️ Fenced finding(s) machine-flagged as likely edge case

The 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.)

  • F1 src/kiro_crew/sandbox.py:3771 — Credential exposure requires a deliberate owner-only opt-in on a read-only keystone the agent cannot self-grant, with disclosure documented as accepted residual risk while enabled.
  • F2 src/kiro_crew/config/loader.py:870 — The uncaught RecursionError requires deeply nested JSON that only manual tampering can place in this read-only keystone, since the sole writer emits flat JSON; failure is loud and recovered by repairing the file.

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

Design-level review of 98d4ebff2bceda914acc5706216ce2db06851891 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • .docker/config.json is on the credential floor (security/paths.py:93) and .docker is masked in all three tiers; the only prior carve-out, .aws/config, is explicitly non-credential ("they carry no credential"). This grant exposes bearer material, and disclosure is unrecoverable once enabled — the PR itself defers that product/security call to maintainers.
  • The spec calls the grant "deliberately independent of sandbox.min_level", so an enterprise admin pinning strict confinement has no way to forbid it. The existing sandbox scope family already accepts a new governed child as a SCOPE_CATALOG data row.

Suggestions

  • Read credsStore/credHelpers at enable time and refuse or name the limitation, so an owner cannot accept irreversible credential exposure on a host where the pull still fails.
  • Add a per-entry strict flag (O_NOFOLLOW + regular-file) to _CC_EXPOSE_FILES rather than a second parallel pre-read/memfd/mount/seal path in the launcher — two exposure mechanisms in that file must now stay in sync.
  • Drop the committed temp-screenshots/ binaries; link them from the PR body.

[DESIGN-REVIEWED] 98d4ebf

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5, fork) — 🟡 CONCERNS

UX-level review of 98d4ebff2bceda914acc5706216ce2db06851891 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

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

  • Revocation instruction disappears exactly when it applies. docker_access_enabled_warning ("restart current sessions to revoke their existing access") renders only under {effective && …}; flipping the switch off unmounts it while the handler comment confirms "Existing live processes keep their namespace until the operator stops them" → the owner sees an off switch and believes credentials are unreachable. Every revocation × wrong security belief × always. Fix: keep that sentence (past tense) on the card after a disable, ideally with a link to Sessions.
  • Three "Allow…" controls, two of which only select. <Btn role="radio">{docker_access_six_hours} ("Allow for 6 hours") and docker_access_until_disabled ("Allow until turned off") sit above the footer's "Allow credential access", styled with the same Btn primitive and no visible group heading (docker_access_duration is aria-label only) → a first-timer clicks "Allow for 6 hours", nothing happens but a check icon. Every grant × dead-end click × always. Fix: render the "Access duration" heading visibly and drop the verb from the options, matching yolo_duration_6h ("6 hours") / yolo_duration_until_shutdown.
  • Silent expiry. The 6-hour grant returns enabled: false once expired and the card polls at refetchInterval: 30_000 → the switch turns itself off mid-view with no expired state anywhere; private pulls later fail with nothing in the UI explaining why. Every bounded grant × unexplained failure × always. Fix: an expired note in the role="status" row.

Evidence gaps

  • Both PR screenshots (temp-screenshots/docker-registry-access/*.png) are unreadable in this fork lane and no first-time reader has seen them: the toggle, the consent dialog, the duration options, the acknowledgement checkbox, the active-grant status row and the confirm action are all unverified for comprehension. Push the branch to this repo for a blind read.
  • No screenshot at all for the non-Linux docker_access_unsupported note, the docker_access_unavailable + Retry failure card, or the docker_access_owner_only error.

Suggestions

  • Add a distinct status string ("Allowed until turned off") instead of reusing the imperative docker_access_until_disabled inside the role="status" row.
  • Pass the warning/unsupported note through SettingsToggle's describedBy so a screen-reader user hears the consequence before flipping the switch.
  • Use fmtTimeNumeric for a ≤6h expiry, as YoloDurationCard does, rather than fmtDateTime.

[UX-REVIEWED] 98d4ebf

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed 98d4ebff2bceda914acc5706216ce2db06851891 via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 98d4ebf

@github-actions

github-actions Bot commented Aug 29, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5, fork) — 🟡 CONCERNS

Premise-level review of 98d4ebff2bceda914acc5706216ce2db06851891 via the fork AI-review pipeline — 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.

I've read the contract, the intent file, the full patch, and checked the base tree for existing mechanisms (EXPOSE_FILES/extra_visible_dirs exposure, the agent.sandbox mode knob, the keystone precedents, the vault, and the temp-screenshots/ convention — which is sanctioned by MANIFEST.in and CI, so not a finding).

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 ships

Intent: let an owner run one agent task that pulls private Docker images without turning the sandbox mask off. ADDITION.

  1. New Settings → Security → "Docker registry credentials" toggle, default off — justified
  2. Confirm modal with acknowledgement checkbox before enabling — justified
  3. Duration choice: 6 hours vs "until turned off" — inherited
  4. Owner-only GET,PUT /api/security/docker-registry-access — justified
  5. New keystone docker_registry_access.json, sealed read-only in every mode — justified
  6. Linux agent spawns get a read-only memfd copy of ~/.docker/config.json — second exposure path
  7. Enabling requires a durable critical SEL event; revocation stays best-effort — justified
  8. Card re-reads state every 30s, and every read is audited — oversized
  9. Saving best-effort drains the warm pool for future sessions — justified
  10. Section rail shows "Allowed" and a warn-tinted key icon while active — justified

Watch

  • security.md: "the credential-access analogue of declared permanent YOLO" is the only support for item 3; the six-hour grant already removes the stated harm (pull private images for a task).
  • The launcher now holds two exposure implementations — the EXPOSE_FILES restore (sandbox.py:3852) and the memfd block — taking _mount_or_die sites from 6 to 8. Only "no host-visible backing file" separates them.
  • "no linked issue: this PR proposes the owner opt-in directly": no reported defect anywhere behind a permanent credential-disclosure surface.

Subtractions

  • Drop permanent end to end: the duration radiogroup in DockerRegistryAccessCard.tsx, the permanent branch in docker_registry_access_state, the PUT field, and 5 keys × 12 locales. Expiry-only leaves one shape to reason about.
  • Delete the 4-line comment added at config/loader.py:4442 ("Read it when constructing each provider…"): the factory never reads the keystone, and test_agent_factory_does_not_capture_docker_authorization asserts exactly that.
  • Remove refetchInterval: 30_000 from the card's query. Each poll writes a SEL docker_registry_access.read record (120/hour per open tab) to refresh a label the spec says does not control enforcement.

[FIRST-PRINCIPLES-REVIEWED] 98d4ebf

@mrbeag
mrbeag force-pushed the feat/docker-registry-sandbox-opt-in branch from 44d99c0 to 8379f93 Compare August 29, 2026 08:01
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Aug 29, 2026
@mrbeag
mrbeag force-pushed the feat/docker-registry-sandbox-opt-in branch 2 times, most recently from 2e4247a to 5f3a962 Compare August 29, 2026 11:31
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@mrbeag
mrbeag force-pushed the feat/docker-registry-sandbox-opt-in branch from 5f3a962 to 3438f8c Compare August 29, 2026 12:26
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention labels Aug 29, 2026
@mrbeag

mrbeag commented Aug 29, 2026

Copy link
Copy Markdown
Contributor Author

Current head cbc54c41e45ae67938c4f84d9dbb73d737e9f223 is rebased onto current main and includes the current-head fixes: confirmation/acknowledgement, dedicated owner-only keystone API, governance clamps, fail-safe revocation, post-commit refresh handling, and the post-rebase frontend assertions. The description and PR Readiness gates are green; all fork pull_request workflows are action_required. Could a maintainer approve the workflow runs for this exact head?

@bolichen97

Copy link
Copy Markdown
Collaborator

Open PR relationship audit

This 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

  • PR #7963 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7963: MERGE_DISCUSSION. Complementary features on the same function whose interaction is undefined. Whichever lands second should state whether expose_docker_config applies to a harness whose reads bypass the PreToolUse gate, and ideally assert it. Files: src/kiro_crew/sandbox.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Current head 29c65aef7584162607532f1447c6776d32789cda is rebased onto current main and fixes every failure from the previous approved run: feature-map coverage, Black formatting, sandbox sealing of the operator keystone, Bengali numeral style, and deterministic settings-registry extraction. The affected backend, frontend, i18n, static, and type gates pass locally in resource-capped containers. The current fork workflows are action_required; could a maintainer approve them for this exact head?

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased and fixed the CI flake8 failure on exact head cc051ef86ec1c9017f859cbff4bd64a642a096f2. The failure was the missing second blank line before the module-level sentinel in test/test_sandbox_cc_mode.py; whole-tree flake8 and the changed-file Black ratchet scope were rechecked locally in a 4-core/4 GiB capped container. Please approve the fork workflows for this head.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the final per-file coverage failure and rebased onto current main on exact head 67e22d357a6250b9392173d1babb57c8883a330f. Added focused owner-GET and atomic-write-failure tests; the handler test file is 16 passed and measured coverage is now 87% (gate: 80%) in a 4-core/4 GiB capped Python 3.12 container. Please approve the fork workflows for this head.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the bundle gate without raising its budget on exact head 60674cbebbb6b0a39c46477316fb8479994568d5: the Docker registry card is now lazy-loaded only when its Security subsection opens. Locally in 4-core/4 GiB capped containers: bundle-size gate passes (808 chunks in budget), targeted frontend tests 115 passed, lazy component line coverage 88.57% (80% floor), TypeScript/ESLint/theme/phantom/feature-map gates pass, and backend handler tests remain 16 passed at 87% coverage. Please approve the fork workflows for this head.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Rebased onto current main and fixed the remaining CI failures on exact head 4c1bf62396c4542d430e039bb100265eb078e458: Black formatting now matches CI, the lazy Docker card is registered in settings extraction/command metadata, and the existing bare-checkbox waiver follows the extracted component. Local validation used 4-core/4 GiB capped containers: backend API 16 passed at 87% handler coverage; Security/settings UI 147 passed; TypeScript, exact feature-map gate, and analyze-mode bundle gate (808 chunks in budget) all pass. The prior head’s four full backend shards also passed. Please approve the fork workflows for this head.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Resolved the new upstream conflict and rebased onto current main at exact head ccda83f13f2c767809217714b5b8c0d34cd32eab. The sole conflict was the explanatory comment beside the namespace launcher raw chmod; upstream’s fuller portability wording was retained and behavior is unchanged. Revalidated in a 4-core/4 GiB capped CI-like container with systemd and repository fixtures: affected sandbox suite 724 passed, 7 skipped; Docker grant API 16 passed at 87% handler coverage; Black clean. Branch is one commit, 0 behind / 1 ahead. Please approve the fork workflows for this head.

@mrbeag

mrbeag commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

Fixed the current CI failures on head fcd8f6f351dcbd5130df8b19bf19979b1dfc82ae and rebased onto main.

  • Completed the slot-close test request fixture with can_read_body, fixing the deterministic exception behind both Linux and Windows shard timeouts.
  • Made the fork E2E test wait for turn completion before checking completed-message controls.
  • Isolated the streaming-response E2E test from shared session history and queued turns, and asserted its own assistant response.
  • Resolved the ACP spawn conflict by retaining upstream adapter restrictions alongside the Docker opt-in.

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.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Invalid UTF-8 grant — span=9ec07ed2ded9

The shared state reader catches ValueError (including UnicodeDecodeError and JSONDecodeError) and returns disabled; non-finite or overflowing numeric expiries also fail closed. Regression tests exercise these payloads. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Shared error rendering — span=9b75988cb39f

Both load and save failures use ErrorNotice with structured report lookup. Retry remains separate. Agent handoff is disabled while an unsaved consent choice is open, with that reason stated in the component. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Expiry/revocation and spawn-time authorization

Deleted the captured provider/runtime authorization fields and constructor threading. The two actual ACP spawn entrypoints identify an eligible agent spawn; wrap_argv re-reads the protected grant on every invocation, including background workers and crash respawns. A repeated-spawn regression covers expiry, permanent enable, and revocation. Existing running namespaces retain their copy until stopped; the documentation states that limitation explicitly. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • needs-a-decision — Governance ceiling for deliberate credential disclosure

Maintainers: should this owner-authorized credential capability require a separate governance scope before landing? The current documented contract deliberately keeps credential supply separate from sandbox.min_level; enabling does not lower confinement or change that bound. The setting is default-off, owner-only, protected from agent writes, and now requires a durable critical SEL audit before authorization. I have not interpreted strict confinement as implicit approval for credential disclosure, nor invented a new governance scope without a product decision.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Credential-helper limitation

The spawn diagnostic now explicitly states that only the config.json snapshot is exposed and helper stores remain hidden, so helper-based login may remain unavailable. The security documentation makes the same limitation explicit. This opt-in does not expose keychains, helper secrets, or the rest of ~/.docker. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Unrelated diff riders

Restored the Playwright chat/fork specs, slot-close race test, generic config-patch test and settings description emission to upstream. Only the Docker lazy-panel settings extraction mapping remains because the new security control must be discoverable. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Visible active grant duration

The API returns permanent and expires_at; the card displays an explicit standing-grant label or a locale-formatted date/time with a Clock icon. It refreshes every 30 seconds. Enforcement independently re-reads the grant at spawn, not on the UI timer. Component tests cover both active durations. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Non-colour duration selection

Duration radios now include CheckCircle2/Circle as well as aria-checked, and use the shared button primitive. They stack at narrow widths rather than squeezing two columns into 320px. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Unsupported-platform rail state

The rail now requires supported AND enabled before reporting Allowed, matching the card. A stored grant can still be switched off on unsupported platforms. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Cancelled standing-grant choice

Opening or closing confirmation resets both acknowledgement and the permanent selection, so cancellation cannot silently preselect the standing grant next time. The component regression selects it, cancels, reopens, and checks the bounded option. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • needs-a-decision — Grant duration versus YOLO duration

Maintainers: should Docker credential authorization retain its explicit bounded/standing choice, or must it use an approved shared capability-grant policy first? I have retained the requested six-hour and until-disabled choices. Deriving credential access from agent.yolo_duration would couple two different permissions: tool auto-approval and disclosure of host registry credentials. The protected owner-only grant, not ordinary agent configuration, remains the authorization source. I have not silently made a YOLO preference authorize or extend credential exposure.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • rebutted — Dedicated anonymous credential snapshot

The generic EXPOSE_FILES path restores ordinary configuration through filesystem-backed files. Docker config can contain registry secrets: this path must use an anonymous memfd and must not leave a credential-bearing backing file outside the namespace. Routing it through the existing generic loop unchanged would lose that security property; rewriting all generic exposure semantics is unnecessary to provide this one deliberately credential-bearing leaf. The dedicated block rejects symlinks/non-regular files and never exposes the host inode.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Background and review-pool ACP spawn coverage

Authorization no longer depends on callers remembering an AcpRuntime constructor keyword. It is checked at the actual shared AcpRuntime spawn wrapper (and AcpClient equivalent), covering session_background and ReviewPool alongside foreground and respawn paths. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author
  • fixed — Remove unrelated items 9 and 10

Dropped the unrelated chat/fork Playwright changes, config-patch fixture adjustment, slot-race adjustment and description spread. The remaining settingsExtract addition maps the new lazy Docker card, directly supporting this feature. Fixed in 98d4ebf.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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.

@mrbeag

mrbeag commented Sep 7, 2026

Copy link
Copy Markdown
Contributor Author

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.

@bolichen97

Copy link
Copy Markdown
Collaborator

@mrbeag Thanks for this. The 2026-09-08 relationship audit found nothing on main implementing docker_registry_access or expose_docker_config, so this PR is not superseded. It does overlap three open PRs.

#9131 (@chenmingwei23) is the substantive one. It threads forward_ssh_auth_sock through the same four functions this PR threads expose_docker_config through, sandbox._build_launcher_script, namespace_argv, wrap_argv and wrap_argv_async, and sets it at the same two ACP spawn sites in src/kiro_crew/acp/client.py and src/kiro_crew/acp/runtime.py. Both also add a paragraph to the sandbox-mode section of docs/system-specs/modules/security.md and a TestWrapArgv case in test/test_sandbox_argv.py. The real difference is the authorization surface: yours is the owner-only keystone docker_registry_access.json, re-read inside wrap_argv on every spawn; #9131 uses the config field agent.sandbox_forward_ssh_auth_sock. Your own test/test_config_loader.py::test_sandbox_expose_docker_config_uses_operator_keystone asserts an agent-writable config key must not carry such a grant, which is exactly how #9131 authorizes its opt-in. Please settle on one surface with @chenmingwei23 first; whichever lands second should rebase onto that surface instead of adding a second mechanism.

#7140 (@GoZippy) rewrites the same expose_files and expose-json path join in _build_launcher_script for WSL2. No behavioural overlap, but whoever lands second must carry the new DOCKER_CONFIG_PATH join through that rewrite.

#9010 (@chenmingwei23, draft) collides only on the SecuritySectionKey union and SECURITY_SECTIONS in website/src/pages/settings/SecurityPanel.tsx. That is a mechanical merge for the later PR.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor) merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants