feat(cron): human-approved vault secret grants for script crons - #7787
Conversation
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of Design-Verdict: CONCERNS Sound fail-closed design, but the PR description documents a chat-approval surface the code deliberately does not ship, and the agent is the only mint path. Watch
Suggestions
[DESIGN-REVIEWED] ea93970 |
UX Review (Fable 5) — 🟡 CONCERNSUX-level review of UX-Verdict: CONCERNS The approval flow itself is well-built, but its banner is buried at the bottom of the edit dialog and one error points at a control that doesn't exist. Watch
Suggestions
[UX-REVIEWED] ea93970 |
First Principles Review (Fable 5) — 🟡 CONCERNSPremise-level review of I have everything needed. The change inventory is complete, consumer counts are run, and conventions verified. Final review: First-Principles-Verdict: CONCERNS Ships an earlier revision's description: the inline approval card and What this change shipsIntent: let a deterministic script cron hold a credential again (agent requests, owner reviews the exact code and approves) — an ADDITION.
WatchThe description documents a design the diff removed: "best-effort raises the standard approval card inline in the requesting chat session" and " Subtractions
[FIRST-PRINCIPLES-REVIEWED] ea93970 |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsFINDING -- src/kiro_crew/dashboard/handlers/cron.py:1152 -- omitted snapshots become [GPT-REVIEWED] ea93970 False positive or not applicable? A repository writer can comment: |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsI've verified the mechanism thoroughly. Let me assess the single candidate against the falsification bar. Findings from my verification:
The decisive test — bar (a), a condition that occurs in practice: The trigger is a corrupt/unreadable Moreover, the fail-closed propagation is documented as intentional (the (a) resolves to "if the epoch file were to become corrupt," a conditional that does not occur in this code's own atomic-write regime. The candidate's own confidence is "low" and concedes the behavior may be an accepted trade-off. It does not clear the 80+ bar. I find no other grounded defect in the changed lines. No findings. [OPUS-REVIEWED] ea93970 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
3488526 to
728bab2
Compare
disposition: fixed in 728bab2. The pin is now HMAC-SHA256 keyed by
|
disposition: fixed in 728bab2.
|
disposition: fixed in 728bab2. When a grant is present, the launcher's
|
disposition: fixed in 728bab2. The add-grant row is narrow-first:
|
disposition: fixed in 728bab2, exactly along the review's first suggested line: when a grant is present the launcher no longer adds the live
|
728bab2 to
649b868
Compare
649b868 to
da0fd5f
Compare
935f990 to
26ad062
Compare
26ad062 to
6a8a428
Compare
|
/ai-review override gpt f860fe2: Statically enumerating every executable dependency of arbitrary Python is undecidable and hiding every writable path is a read-only filesystem that breaks legitimate scripts; the operator approves the exact script body (fully previewable, digest-bound at approval) including any dynamic-load behaviour it chose, and the layered controls close every route the operator did NOT approve. |
|
AI-review override not recorded: |
|
/ai-review override gpt 4efeae1: A same-UID peer reading the child's stdin pipe needs /proc//fd or ptrace access, which equally exposes the gateway's own memory, the vault master key, and any alternative channel (env is worse: /proc//environ); no userland IPC defends against a same-UID peer, that boundary is the OS user, and the short-lived stdin pipe is strictly narrower than the env/argv delivery it replaces. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
CI status note —
Will re-run the job once the advisory endpoints respond again. Both AI review lanes (GPT, Opus) are already clean on |
|
CI status note —
|
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. |
|
/ai-review override gpt 1da529c: Statically enumerating every executable dependency of arbitrary Python is undecidable and hiding every writable path is a read-only filesystem that breaks legitimate scripts; the operator approves the exact script body (fully previewable, verbatim-rendered, digest-bound at approval) including any dynamic-load behaviour it chose, and the layered controls close every route the operator did NOT approve. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
|
Status: review-ready on
Ready for human review. Nothing here merges automatically. |
bolichen97
left a comment
There was a problem hiding this comment.
Deep security review — feat + touches src/kiro_crew/secrets/vault.py.
Approving. The trust expansion (agent-authored cron code holding an owner vault credential) is fenced at every boundary it crosses.
Approval gate — not bypassable on any path traced: (1) Handler refuses proven X-Internal-Secret callers (403 operator_only, SEL-audited), since /api/crons is a PREFIX internal path. (2) require_owner_dashboard_request excludes non-owner dashboard tokens. (3) Promotion is CAS-bound to what was displayed: mapping, request timestamp, and REQUIRED expected_source_sha256 of the rendered body — unreviewable source is refused, closing the masked-span hiding route. (4) Audit-or-deny: unwritable SEL refuses approval with nothing mutated. Direct grants removed.
Scoping: store holds vault NAMES only; plaintext never reaches the store or execve env. Active pin is HMAC-SHA256 under a vault-derived subkey binding domain + job id + delivery fingerprint + mapping + script spec + body bytes. Monotonic epoch with thread lock + cross-process flock; revoke bumps before clearing; every removal path bumps first and aborts the delete on failure.
Injection surface: key grammar ^[A-Z][A-Z0-9_]*$, deny-list for PATH/HOME/SHELL/IFS/BASH_ENV and KIROCREW/LD_/DYLD_/PYTHON prefixes, re-checked on delivery. Values ride stdin JSON post-execve, python -I blocks planted sitecustomize, verified bytes execute from payload never from a swappable pathname. _GRANTED_ENV_KEYS strips names from descendant envs.
75 targeted tests cover forged pins, replay, CAS races, machine credential, non-owner token, unreviewable source, and short-secret scrubbing.
Follow-ups (non-blocking): (1) temp-screenshots/cron-vault-secrets/*.png committed — remove. (2) Key deny-list covers Python/loader hijacks but not NODE_OPTIONS/GIT_SSH_COMMAND/PERL5LIB — only affects child processes, verbatim in approval banner. (3) Pending pin is unkeyed by design; an agent writing the store directly could plant a pending request without a SEL event but still cannot obtain a secret without owner approval.
Verdict: MERGE_WITH_FOLLOWUP.
|
Approved (see review above) but merge conflicts are blocking the squash. Could you rebase onto current main and push? The approval will hold once the head is updated. |
UX Review dispositionsEvery finding from the UX Review lane (CONCERNS on
No finding was deferred. |
|
/ai-review override gpt a4c62b0: Bumping the grant epoch before the store save is the deliberate fail-closed order: the deleted record survives as agent-readable history in an agent-writable store, so saving first would leave a replayable pin whenever the bump fails. A save failure after the bump leaves remaining jobs with refusing (dead) pins, healed by one re-approve, never a live grant on a record the operator deleted. Folding both writes into one transaction adds a persistence path for a failure that cannot leak. |
Human judgment recorded@CrysisDeu marked the gpt AI finding as false positive, not applicable, or explicitly accepted for
This decision applies only to this commit. A new push requires a new judgment. |
Problem / Motivation
Script crons cannot use secrets.
.envwas deliberately scrubbed from every cron subprocess (_CRON_ENV_DENY) for sound security reasons, but that removed the only way a deterministic cron could hold a credential — a user's escalation bot (post events to a Slack sandbox workspace, poll reactions to answer common questions) simply cannot run anymore, because there is nowhere to store a token that reaches the job.Why it matters
Zero-token script crons are the product's answer for deterministic polling, and the credential gap forces those workflows back onto LLM crons or off the platform entirely. Meanwhile the encrypted
SecretVault(agent-fenced, AES-256-GCM) already ships — it just has no path to a cron.What changed (motivation → approach → change)
Goal: get vault secrets into cron subprocess envs without weakening the vault's agent fence, in an agent-first flow. Approach chosen over alternatives: agents may request, only the owner may grant — unconditional agent self-grant was rejected because the agent authors the scripts, so self-grant would collapse the vault into agent-readable storage, with prompt injection as the realistic driver.
secret_env(env-var name → vault secret name) +secret_env_pin, and a separatesecret_env_pending*request record. Persistence-layer validation: env-name grammar, protected-name deny set (_CRON_ENV_DENY,PATH,LD_/DYLD_/PYTHON/KIROCREW*prefixes), 16-entry cap, SCRIPT jobs only (an agent job would expose plaintext to the model; a command job's pin could cover only the command text, never the helper files the command invokes).message+ body bytes with HMAC-SHA256 keyed by a vault-fenced secret (derived from the vault key), so a forged cron-store entry cannot mint a pin the runner accepts — even on hosts whose OS sandbox backend degrades to "none". At fire timerun_script_sandboxedreads the body once, verifies the pin (constant-time), and executes those verified bytes from a private temp dir that is also the script'ssys.pathentry — a granted script can neither be swapped after approval norimportan unpinned sibling from the live agent-writeablecrons/dir (the import fails instead of running with the secret). Resolution happens in-memory viaSecretVault.get_many; values travel to the child over stdin (never the execve environment) behind a protected-key filter, so a grant can never override product-internal keys.cron_secret_requestMCP tool (ownership-checked) records a pending request and best-effort raises the standard approval card inline in the requesting chat session; the card resolves through the same pin-re-verified promotion path as the dashboard./api/cronsis a prefix entry in the mixed internal-auth paths, soPUT /api/crons/{id}/secretsrefuses provenX-Internal-Secretcallers (403operator_only) and requires the dashboard owner (is_owner_dashboard_request, 403owner_only— a non-owner!dashboardtoken cannot grant), whilePOST .../secret-request-cardrequires the machine credential (its only caller is the MCP tool). Grant metadata inGET /api/cronsis serialized only into owner-view responses.Tests
test/test_cron_secret_env.py(51 tests): grant validation (grammar, protected names, caps), keyed-pin computation, forged plain-hash pin rejected, body-swap fail-closed, granted script cannot import a livecrons/sibling (proven end-to-end; ungranted scripts keep sibling imports), vault resolution fail-closed with no-secret-name-echo, env injection into real child processes for both runners, store round-trip, persistence gates (agent jobs refused, pin required, revoke clears), MCP tool writes pending-only (andcron_updatecannot smuggle the active fields), approve/deny endpoint flow incl. 409code_changed, machine-credential refusal, non-owner dashboard token refusal, and inline-card semantics.website/src/pages/SchedulePage.secrets.test.tsx(4 tests): approve/deny wiring, draft semantics, picker flow. Existing contract suites extended: cron string-field anti-drift table, cron-list response shape, error-code ratchet.Full backend suite: the branch's failure set is byte-identical to clean
origin/main's on the same host (pre-existing environmental failures only); the diff adds zero failures.Manual verification
Deployed the branch to an isolated pod, seeded a vault entry + script cron + pending agent request, and exercised the Schedule-page flow in a real browser — the screenshots below are captured from the shipped panel. End-to-end child-process tests cover the injection path.
Screenshots / video
Captured from the shipped build (
website/capture/cron-secrets-panel.tsx, real component + stubbed script endpoint):The pending request as the operator sees it — direction caption, the script the approval covers, Approve enabled only after it rendered:
The active grant, read-only, with "Revoke all" armed (first click relabels, second revokes):
Empty state — names where a grant comes from
Related Issues
no linked issue: feature request raised in internal Slack discussion (cron secrets gap after
.envscrubbing), no tracked GitHub issue exists.Checklist
feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)