Skip to content

feat(pod): add the authenticated pod api command - #8218

Merged
pepmach merged 1 commit into
mainfrom
feat/pod-api
Sep 5, 2026
Merged

feat(pod): add the authenticated pod api command#8218
pepmach merged 1 commit into
mainfrom
feat/pod-api

Conversation

@pepmach

@pepmach pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Add the authenticated pod api command

Slice 3 of the verification-controller plan, and the last pod verb in it. A pod's HTTP API is reachable today only by reading its credential out of the pod's env file and hand-assembling a URL — the step that made every earlier verification recipe fragile. This adds one verb that does it correctly and nothing else.

kirocrew pod api <worktree> <METHOD> <path> [--data JSON] [--allow-write]

Output is a fixed-key JSON document on every path — {name, method, path, status, ok, body} — so a caller never parses prose. A non-2xx response prints the same shape and exits 1, which is what lets a recipe assert on status instead of on stderr text.

Why the original implementation was replaced

The version carried on the closed combined branch was dead on this host, and the live smoke on #7841 is what proved it:

  • Ownership was attested by lsof alone. On a host without a listener tool the proof could never be produced, so the credential was withheld and the verb always failed. Ownership is now the agreement between the pod's own PID record and systemd's MainPID; a listener tool, when present, only corroborates. A listener attribution alone never vouches for a pod, and a listener that disagrees with the attested PID marks the port foreign.
  • It authenticated with a Bearer header. The dashboard does not accept that. It now uses the query-credential contract in token_auth.py, pinned by a test that asserts the header form is not used.

Missing, malformed, or stale records stay unproven rather than degrading to a guess, and a non-POSIX host reaches that verdict without touching host tools at all.

Credential hygiene

The credential is minted only after ownership is attested, and it is treated as unprintable from that point:

  • A caller-supplied credential query parameter is refused without echoing it — and the refusal reaches neither stderr nor the audit record.
  • A malformed credential URL is never quoted back, and transport errors never include the authenticated URL.
  • Response echoing cannot surface the minted value; the audit record names a credential-free target.

Safety surface

  • GET and HEAD are the default surface; POST/PUT/PATCH/DELETE require --allow-write, and the refusal happens before any host access.
  • Response reads are capped at 32 MiB: one byte over is refused with an instruction to narrow the request, exactly at the cap is returned, and a truncated body is reported without a traceback.
  • Existing query parameters on the path are preserved; /api/ is prepended when absent; methods are case-insensitive.

Verification

  • Live acceptance on a real pod, which is the acceptance bar for this program rather than supporting evidence: booted from its own worktree, GET health returned 200 through the new verb, a caller-supplied credential was refused without echo, the --allow-write gate was exercised in both directions, teardown left zero residue, and three unrelated pods running on the host were untouched.
  • 24 focused tests in test/test_pod_api.py covering every claim above, red-first.
  • 812 focused tests green across the pod, seed, scenario, CLI-help, and instances suites; Black ratchet, isort, flake8, subprocess-encoding, mypy (1,281 files, --platform linux), harness parity, brand, and docs lint all green.

Scope and rebase

Command, ownership attestation, run marker, focused tests, and the two command-doc surfaces. 10 files, +938/−65. No lifecycle changes, no seeding changes, no recipes.

The run-marker reader shared with host gateway ownership now caps PID sidecars at 64 bytes and accepts only positive ASCII decimal. The gateway writes os.getpid() in that sidecar, so valid gateway lifecycle behavior is unchanged; only malformed or non-contract records are newly refused.

Rebased onto current main after #8127 (pod scenarios) and #8125 (manifest publication) merged. The four conflicts were all union merges in verb tables and doc rows — the subcommand metavar, the dispatch table, the usage string, and the CLI spec rows — resolved so both verbs are listed; no behavior was renegotiated in the rebase.

With this, the pod verb surface planned by the controller judgment is complete: no further verbs. The remaining slice is recipes and discoverability, which composes these verbs with the existing /api/session-control/* routes.

Follow-ups

  • #8552 — route mint_token over the pod's unix socket too. Deliberately not a rider here: api_token_local refuses AF_UNIX peers today (is_loopback("") is False) and sits in token_auth._BYPASS_EXACT, so admitting unix peers widens a middleware-bypassed auth gate that deserves its own review. Its residual leaks no pod credential; what a port-squatter could capture during the mint window is the host-side .local_secret, which is pod token's pre-existing exposure on main.

@pepmach
pepmach requested a review from a team as a code owner September 3, 2026 17:32
@pepmach
pepmach requested a review from iamwhatever September 3, 2026 17:32
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 3, 2026
@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound verb and attestation redesign, but the durable .local_secret still travels the exact TCP race this PR built the unix socket to eliminate.

Watch

  • pod_api refuses TCP for the short-TTL minted token because "a pod that exits between the mint and the send leaves the port free for any local user to bind" — yet one step earlier mint_token sends the pod's durable .local_secret over http://127.0.0.1:{port} guarded only by the TOCTOU-prone attestation. The stronger credential gets the weaker transport. Since pod api already requires the socket to exist, routing the mint over it (for this caller at least) closes the window entirely; as-is the socket work protects the derivative credential but not its parent.
  • build_pod_env now pins KIROCREW_BIND=127.0.0.1 for every pod at every pod up — load-bearing for the attestation's soundness (the ::1/0.0.0.0 divergence), but a real behavior change for operators whose environment exported a non-loopback bind, and the description states "No lifecycle changes." Name it in the PR so it isn't lost at release time.

Suggestions

  • Mint the per-request token with a one-shot short TTL instead of the 2h default: it is consumed once, immediately, and a scrubber miss should not leave a 2-hour credential live.

[DESIGN-REVIEWED] 5979f98

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 5979f981d261bdbf7d9f5fe505a1d694d21b33f7 — 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.

All evidence is in: the contract, the intent file, the full patch, and the repo greps for consumer counts, the existing session_pid._pid_start_token sibling, and the host-side _gateway_owns_port (which has its own recycling guard via uid + port-holding, so it is not an unfixed sibling). Producing the review.

First-Principles-Verdict: CONCERNS

Every pod now binds 127.0.0.1 and the whole unix-socket transport ships undeclared, in a description that claims "No lifecycle changes" — plus a Windows fallback whose beneficiary cannot exist.

What this change ships

Intent: let a recipe/agent call a running pod's HTTP API without ever touching its credential — an ADDITION.

  1. New pod api verb printing a fixed-key JSON envelope on every exit — justified
  2. Non-GET/HEAD refused before host access unless --allow-write — justified
  3. Caller token query params refused without echo; token scrubbed from body and audit — justified
  4. Response reads capped at 32 MiB — justified
  5. Request delivered only over the pod's private unix socket, no TCP fallback — justified, undeclared
  6. A missing socket refuses before minting — justified, undeclared
  7. Ownership now = PID-record/MainPID agreement; listener attribution demoted to corroboration — justified (declared cause: live smoke on feat: seed pod homes from named scenarios #7841)
  8. Every gateway writes a .start start-identity sidecar, pruned with the pid — justified
  9. PID sidecar reads capped at 64 bytes, ASCII decimal, first line only — justified (declared)
  10. Every pod gateway now binds 127.0.0.1, overriding an inherited KIROCREW_BIND — rides along, undeclared

Watch

  • Item 10 contradicts the stated scope: "No lifecycle changes, no seeding changes" vs build_pod_env now forcing "KIROCREW_BIND": "127.0.0.1" on every pod up, pod api or not. The change is derived (attestation vouches for a process, not an address) but a human should see it declared, not discover it.
  • Items 5–6 are the command's central security mechanism and its main new failure mode (refusal on absent socket), yet the description never mentions the socket at all.
  • pid_start_token's process_start_time fallback is justified as "what keeps Windows working: … a pod there could never prove ownership" — but this same diff writes "pod api is Linux-only like every systemd-touching pod verb" (pod/README.md) and opens port_owner with if not IS_POSIX: return OWNER_UNPROVEN, so the token is never compared on any host where the fallback is the only producer. I counted readers of the .start token: 1 (_pod_recorded_pid, POSIX-gated). It also makes run_marker.pid_start_token the second spelling of session_pid._pid_start_token (session_pid.py:115), which deliberately delegates to get_process_start_id alone.

Subtractions

  • Drop the process_start_time fallback leg (and its whitespace-collapse) from run_marker.pid_start_token — on the platforms pods run, get_process_start_id is the producer, matching session_pid._pid_start_token; the Windows scenario it defends is unreachable by this diff's own gates.

[FIRST-PRINCIPLES-REVIEWED] 5979f98

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 5979f981d261bdbf7d9f5fe505a1d694d21b33f7 — this comment is updated in place on each push.

Review details

I've analyzed the diff and the three candidates. Let me verify the key structural fact underpinning all three — whether the weakened port_owner and the KIROCREW_BIND pin ship together.

All three candidates share one root cause: port_owner was changed so a provably-fresh PID-record/MainPID agreement attests OWNER_POD without listener attribution, so mint_token sends .local_secret over TCP to 127.0.0.1:<port> — which a foreign process could hold if the pod bound ::1/0.0.0.0 instead.

Falsifying this:

  • The pin and the weakened check are the same file. build_pod_env (which adds "KIROCREW_BIND": "127.0.0.1") and port_owner/mint_token all live in src/kiro_crew/pod/runtime.py. They ship as one unit — you cannot have the new port_owner without the new build_pod_env. So the candidates' required input ("host tooling predates the pin but code is new enough to weaken the check") is contradictory: the code performing the attestation is the code that writes the pin.
  • The residual gap is upgrade-mid-lifetime only. The one non-contradictory path is: an old install runs pod up (pod inherits non-loopback bind, checkout new enough to write .start), the host install is then upgraded, and a foreign process binds 127.0.0.1:<port>. That is a multi-step "could" scenario, not an input that occurs in practice — exactly the speculative case the falsification bar and the base-rules ("do NOT reason from an assumed... future case") forbid. Each candidate self-rates confidence low and admits it "could not rule out a cross-version launch."
  • The foreign-responder path is still caught. When listener tools are present, if ours is None or ours not in pids: return OWNER_FOREIGN still fires — a foreign 127.0.0.1 listener is detected and the mint refuses, as in base.
  • The threat is documented and mitigated in-diff. The build_pod_env comment names this exact attack and pins the bind to close it; candidate 3's ask (have port_owner independently confirm the bound address) is defense-in-depth, not a concrete reachable defect at 80+.

None of (a)/(b)/(c) survives without assuming a specific version-transition sequence. All three drop. Step 2 turned up no independently groundable defect in the changed lines.

No findings.

[OPUS-REVIEWED] 5979f98

Verdict parsed from the review's SHA-scoped output markers for commit 5979f981d261bdbf7d9f5fe505a1d694d21b33f7.

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

@github-actions

github-actions Bot commented Sep 3, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ human override accepted

Human judgment by @pepmach overrides the GPT 5.6 finding for 5979f981d261bdbf7d9f5fe505a1d694d21b33f7; 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 5979f981d261bdbf7d9f5fe505a1d694d21b33f7: <one-sentence reason>

@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 3, 2026
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Review disposition — 9637d9f6a

  • TAKEN — JSON-escaped credential leak (runtime.py:1834): response JSON is parsed and every string key/value is scrubbed recursively before re-serialization; non-JSON text keeps the bounded string scrub. Red-first coverage drives the full _api output path and proves raw, URL-encoded, quote-plus, and \uXXXX forms cannot reconstruct the minted credential.
  • TAKEN — transport errors violated fixed-key output (pod/cli.py:869): every post-parse PodError now emits {name, method, path, status, ok, body} to stdout and exits 1; status 0 means no HTTP response. Invalid/credential-bearing paths use /api/<invalid> and audit only the error type.
  • TAKEN — METHOD enum duplication: six canonical choices plus type=str.upper; mixed-case Get is pinned.
  • TAKEN — duplicate PID parser: run_marker.read_pid_path owns the bounded 64-byte positive-ASCII-decimal contract; pod ownership composes the explicit path and delegates.
  • TAKEN — undeclared baseline formatting: restored the black-baseline entry and unrelated _prune formatting; the branch has no .github/black-baseline.txt diff.
  • DEFERRED — challenge-response / HMAC: Design review correctly identifies a possible defense-in-depth follow-up, but it is a new gateway protocol outside this verb slice. This head retains the declared PID-record + systemd MainPID proof, with listener tools as optional corroboration; listener evidence alone never grants a credential.

Validation: 811 passed, 1 skipped; Black ratchet, isort, flake8, subprocess encoding, mypy --platform linux (1,281 files), harness parity, brand, docs lint, and git diff --check all green.

@pepmach
pepmach enabled auto-merge (squash) September 3, 2026 18:21
@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 Sep 3, 2026
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — the host-side PID parser hardening is now declared span=07ab51761caa

    The PR description now names the shared host-gateway path, the 64-byte/ASCII-decimal acceptance change, and why valid lifecycle behavior is unchanged: the gateway writes os.getpid(), so only malformed or non-contract sidecars are newly refused. Fixed in 4fc72bb4201aefccbf7922ec734ed93a556cd540.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — removed the unused pod_api(timeout=…) extension point span=358b6166dc17

    pod_api now uses API_TIMEOUT_SECS directly. Red-first evidence: test_timeout_is_not_part_of_the_public_runtime_surface failed against 9637d9f6a because the keyword existed, then passed against 4fc72bb4201aefccbf7922ec734ed93a556cd540 after the subtraction.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • REBUTTED — pod ownership and host-gateway ownership are not duplicate mechanisms span=aa9e7cb9a94b

    The host gateway has no service manager identity and therefore requires listener attribution; the pod path can substitute the pod unit's MainPID, an independent pod-only fact. Unifying them would either discard that stronger proof or weaken the host path, so keeping two mechanisms is the smaller correct design.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — span=e23105e8bfba — missing listener attribution could disclose the pod secret

727e46547398a1727866b3eb5d5e62d4f8842bb4 now returns OWNER_UNPROVEN when listener attribution is unavailable, raises, or yields no loopback owner. The regression test covers unavailable and empty attribution and proves mint_token never calls the credential-bearing request path.

Stale sidecar + reused MainPID + foreign listener + unavailable attribution could let mint_token send .local_secret to the foreign responder.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — span=c7d8344b8408 — pod-api failures could escape the fixed JSON envelope

727e46547398a1727866b3eb5d5e62d4f8842bb4 moves name validation into the single envelope-producing boundary and funnels PodError plus unexpected runtime failures through that same output. A parameterized regression covers name/path/method validation, write refusal, inactive pod, port resolution, token ownership refusal, transport, read cap, and unexpected runtime errors.

Invalid names escaped before the JSON-producing handler and emitted stderr prose.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — span=e23105e8bfba — stale PID reuse can disclose the pod secret (re-fixed at the correct leg; the previous fix regressed pod api off entirely)

The earlier response to this finding made port_owner require listener attribution, returning OWNER_UNPROVEN whenever the lookup tool was absent, raised, or returned no PIDs. That closed the finding by removing a supported platform path, and it broke this PR's whole purpose. Verified live on this host at the pre-fix head:

pod status  -> up  port=7974  health=200      (the pod IS serving)
pod api kirocrew-wt-pod-api GET api/health
  -> status 0, ok=false, PodOwnershipUnproven

Branch taken, measured rather than read off the source: listening_pid_tool_available() is True (lsof resolves at /usr/bin/lsof), the sidecar PID and systemd MainPID both read 79984 so attested was True, and find_port_listeners(7974) returned [] — the if not pids leg. lsof works here; it just cannot see a socket held by a process outside an unprivileged caller's view, which is every pod gateway, because the user's service manager starts it and pod api asks from an agent process. Attribution came back empty for all 20+ loopback listeners on this host. So the requirement is not satisfiable here at all, and the credential would have been withheld from every healthy pod permanently.

The leg the chain actually depends on. This finding needs a stale sidecar — a record naming a PID that has since been recycled onto a foreign process. Listener attribution was only ever standing in for that. So the record now proves its own freshness instead: write_marker records the writing process's start-time identity beside its PID (platform_compat.process_start_time, which exists as a PID-reuse guard), and _pod_recorded_pid re-derives that identity live. A recycled PID answers with its own start time and cannot match, so a crash leftover can no longer attest — with no listener evidence involved.

Fails closed on every way of not knowing: no record, no start-time line (a pre-binding record), or a host that will not report a start time, which covers a Windows box whose query handle cannot be opened. OWNER_FOREIGN is unchanged and still outranks the record — a different PID on the reached listener is positive proof and vetoes. Attribution is still never sufficient alone: a PID holding the port with no fresh record behind it stays OWNER_UNPROVEN.

Line 1 of the sidecar is still the bare PID, so port_resolution._gateway_owns_port via run_marker.read_pid is unaffected.

Red-first. Three tests failed with assert 'unproven' == 'pod' against the unfixed port_owner (tool absent / lookup raised / empty attribution), then passed. Mutation-checked the other direction: deleting the freshness comparison turns the three refusal tests red with assert 'pod' == 'unproven', so they are not vacuous and this finding's chain is what they hold shut.

Two pre-existing cases encoded the invariant being replaced and were rewritten rather than deleted — test_a_throwing_listener_lookup_leaves_pid_attestation_unproven and test_no_visible_listener_leaves_pid_attestation_unproven now assert that a provably fresh record survives a failed or unattributable lookup. The credential-withholding sweep they anchored is preserved as test_an_unprovable_record_withholds_the_pod_secret, which drives the refusal through an unprovable record instead of through missing attribution.

Live acceptance at this head: pod api kirocrew-wt-pod-api GET api/healthstatus 200, ok=true, with find_port_listeners still returning []. Stale-record simulation against the same running pod (same PID, foreign start token) → unproven, mint refused. Pod torn down, zero residue.

Gates: black, subprocess-encoding, isort, flake8, mypy --platform linux (1281 files) all clean; 2014 passed / 25 skipped across the pod, CLI, seed, instances, platform-compat and ownership suites.

@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — span=c7d8344b8408 — deeply nested JSON responses crash the command (third round on this span, so this is a restructure of the exit path, not another instance patch)

Nested response -> json.loads(raw) raises RecursionError -> traceback replaces the promised JSON envelope. Fix: Catch RecursionError separately and use a safe omission message.

Confirmed reachable and fixed. Two things about the mechanism are worth stating, because they are why enumerating exception types kept failing here.

Why the previous round's fix did not cover this. The decode sat in the else: clause of the request try, and an else: clause is not protected by its own statement's handlers — so the broad except Exception two lines above could never see it. On top of that RecursionError is a RuntimeError, not the ValueError a malformed body raises, so the inner except ValueError let it through as well. Red-first: RecursionError: maximum recursion depth exceeded while decoding a JSON array escaped _api entirely at nesting depth 2,000 and 20,000, printing a traceback where the envelope belongs.

Catching RecursionError at the decode site would not have been sufficient. Measured on this branch: at depth 20,000 json.loads fails, but at depth 2,000 the decode SUCCEEDS and json.dumps then fails — the escape moves to the render, after the request has already returned 200. A decode-site-only fix leaves that case crashing. Which half gives way depends on how much stack is left, so it is not a threshold worth pinning.

The restructure. The envelope is now the single exit from _api:

  • The request, the decode and the render all sit inside one guarded region whose handlers cover any Exception. The else: clause is gone, so there is no longer an unguarded region to escape from.
  • _api_body() decodes without ever raising — any failure degrades the body to its already-scrubbed text rather than abandoning the envelope.
  • _api_envelope() renders without ever raising — a body that cannot be serialized is replaced with <body omitted: not serializable> in a fallback document of fixed shape and depth, which cannot fail in turn.

A successful request keeps its status and ok through either degradation, so a deep body is reported as a 200 with an unparsed body rather than as a synthetic failure.

Mutation-checked, so the guards are not vacuous. Narrowing the decode guard back to except ValueError turns 2 tests red (the body becomes pod api failed (RecursionError)); deleting the render fallback turns 2 tests red. Coverage: a parametrized end-to-end test asserting the six fixed keys survive both depths, a direct test that the decode boundary fails safe on an exception nobody enumerated, and a direct test that the render degrades an unserializable body.

Validation. 54 passed in test/test_pod_api.py; 2226 passed / 24 skipped across the pod, seed, scenarios, instances, run-marker and platform-compat suites. black (baseline pruned for the now-clean file), subprocess-encoding, isort, flake8, and mypy --platform linux (1281 files) all clean. Diff unchanged in scope at 11 files. No new flag, header, route, or protocol; no deferred scenario payload restored.

@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 3, 2026
@pepmach

pepmach commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — the persisted pid-marker format no longer changes (span=c16c49ad9e53)

The old read_pid strips the whole file and .isdigit()s it, so a two-line record written by a new gateway parses as None in any older client venv sharing the data home — _gateway_owns_port then denies a gateway that is genuinely ours. The _pid_record docstring's claim that "an older reader that takes the whole file as the pid keeps working" is false for the shipped old reader.

Correct, and the docstring claim was mine — I asserted in an earlier disposition on this PR that line 1 staying the bare pid left port_resolution._gateway_owns_port unaffected. That was true only of the reader in this branch. Verified against git show origin/main:src/kiro_crew/instances/run_marker.py: the shipped reader does raw.isdigit() on the whole file, so the two-line record answered None.

Rather than document the skew, the format change is withdrawn. The start identity moved into its own sidecar:

  • gateway-<port>.pid is again exactly f"{pid}\n" — byte-identical to what this module has always written, so every released reader keeps working with no version-skew story needed in either direction.
  • gateway-<port>.start holds the token (_START_SUFFIX, _start_path_for), 0600 through the same atomic_write path, inside the same 0700 run/ dir — so its trust basis is unchanged.
  • read_pid_record_path() keeps its (pid, start_token) signature, reading the pid from the path it was given and the token from the sibling beside it.
  • write_marker() writes .start first and unconditionally — first because both orders fail closed and this one narrows the window where a published pid has no identity, unconditionally so a predecessor's token can never be left in place for a successor to inherit.
  • prune_markers() and clear_marker() reap it with the pid it attests, so a token cannot outlive its record.

Proved cross-checkout rather than simulated. With a live pod on port 7974, the on-disk pid file was b'710860\n' and systemd MainPID=710860. An independent older checkout (kirocrew-wt-verification-docs, carrying main's whole-file isdigit() reader) parsed it as 710860. The counterfactual on that same reader: the old two-line record → None, the bare pid → 710860. That is this finding reproduced and then closed with the reviewer's own mechanism.

The _pid_record docstring now states why the file must stay single-line instead of claiming compatibility it did not have, and docs/system-specs/modules/instances.md §12 — which enumerates the run-dir files — gains §12.2 for the new sidecar, per the same-commit spec-sync rule.

Validation. 712 passed / 1 skipped across the pod, instances and CLI-help suites; 2105 passed / 24 skipped on the broader pod/seed/instances/platform-compat sweep. black, subprocess-encoding, isort, flake8, mypy --platform linux (1281 files), docs-lint (259 files) and the brand gate all clean.

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — the credential-bearing request now travels a transport no other process can answer (span=e23105e8bfba; implemented as prescribed)

pod api mints a token, then sends it on a separate connection. Pod exits between the calls → another user binds the port and captures a replayable token. Fix: send the authenticated request through the pod's private dashboard Unix socket with TCP fallback disabled.

The prescription is correct and was implemented, not rebutted. Earlier rounds on this span asked for exact IPv4 listener attribution, which is unsatisfiable on this host (find_port_listeners() returns empty for every loopback listener) and measurably made pod api permanently dead. This round names a different mechanism, and it is satisfiable: dashboard/server.py binds web.UnixSite at dashboard_socket_path(port) unconditionally on non-Windows (socket 0600, parent 0700), and pod api — like every systemd-touching pod verb — is already Linux-only. Requiring the socket therefore costs zero capability, which is exactly what attribution could not claim.

No fallback, structurally. loopback_urlopen(unix_socket_path=…) already existed but silently retries over TCP when the socket is missing, stale, or refusing — that retry is the race. Rather than add a require_unix flag (a conditional a later edit can invert), a sibling primitive unix_socket_urlopen(req, timeout, socket_path=…) builds the same opener with no TCP handler in it at all; a missing or dead socket raises URLError and the request goes nowhere. loopback_urlopen now delegates its own unix leg to it, so every existing caller's behaviour is byte-identical and none gained a parameter.

The pod's home, not the host's. dashboard_socket_path() resolves the calling process's config_dir(), which would have pointed at the host gateway's socket. The leaf name is split into dashboard_socket_name(port) so pod_socket_path() joins the one canonical name onto the pod's isolated home; its docstring records why composing dashboard_socket_path(port).name must not be "simplified" back.

mint_token deliberately stays on TCP, and this does not reopen the chain. It cannot move without a separate change: api_token_local gates on is_loopback(request.remote or ""), request.remote is empty for AF_UNIX, and is_loopback("") is False, so the endpoint answers 403 — and that endpoint is in token_auth._BYPASS_EXACT, so admitting unix peers means widening a middleware-bypassed auth gate, which is separately reviewable rather than a rider here. Its residual differs in kind: mint_token requires positive OWNER_POD proof immediately before sending, and if a squatter did win that gap we would receive their token and hand it to the pod's own socket, which rejects it — no pod credential leaks. What a winner could capture is the host's .local_secret, which is the exposure pod token already ships on main and is not introduced here; it is called out for its own follow-up. health() / _probe_health() stay on TCP legitimately — they carry no credential, and port_owner() already reports HEALTH_FOREIGN for who replied.

Red-first. Written before the fix. The harness is the load-bearing part: stub_gateway now starts the pod's unix socket and a TCP listener on the derived port standing in for a squatter, then asserts at teardown that the TCP peer was never contacted. That turned seven pre-existing tests which asserted "the request reached a TCP peer" — the defect encoded as an assertion — into proofs that it did not. Verbatim red included AssertionError: the squatter on the pod's port was contacted and Failed: minted before refusing; 15 failed / 9 errors before the fix.

Mutation-checked four ways, each restored after: plain TCP → 6 failed / 9 errors; fallback restored → 1 failed / 1 error (test_a_stale_socket_file_does_not_reopen_the_tcp_path, "the squatter … was contacted"); exists() guard disabled → 2 failed ("minted before refusing"); socket rooted outside the pod home → 9 failed. The fallback mutation was independently re-run at review: same single failure, same message.

Live acceptance (mandatory on this PR's attestation path). Pod up on 7974 with --seed minimal: GET /api/health → 200 ok, GET /api/sessions → 200 (proving the ?token= contract works over AF_UNIX), POST /api/sessions → envelope refusal. Socket observed at srw------- dashboard-7974.sock inside a drwx------ home. The counterfactual, run with the gateway provably answering on TCP at the same instant: socket renamed away → envelope refusal naming the path and stating it will not retry on 127.0.0.1:7974; a stale plain file left at the path → refusal citing URLError, again not retried; socket restored → 200. Before the fix both cases returned 200 over TCP. Teardown reported zero residue and the three unrelated pods were untouched.

Validation. test_pod_api.py 68 passed (61 → 68); 730 passed / 1 skipped across pod, pod-api, instances, cli-help and loopback-proxy; black, subprocess-encoding, isort, flake8, mypy --platform linux (1,288 files), docs-lint (261 files), brand and harness-parity all clean. Docs updated in the same commit: the pod api section of src/kiro_crew/pod/README.md and the kirocrew pod api row in docs/system-specs/modules/cli.md. Head is also rebased onto current origin/main, content-identical through the rebase (2,458 diff content lines before and after, zero added, zero dropped).

@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author

The mint_token / .local_secret residual named in the disposition above is now tracked as #8552 (owner-approved follow-up): admit verified unix-socket peers at api_token_local, then move mint_token onto unix_socket_urlopen. Deliberately not a rider here — it widens a _BYPASS_EXACT auth gate, which deserves its own review.

@pepmach pepmach changed the title feat: add the authenticated pod api command feat(pod): add the authenticated pod api command Sep 4, 2026
@pepmach

pepmach commented Sep 4, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — the refusal now gives the right remediation for a token-less record (span=a3449494eefb)

The mint's new remediation ("restarting it is what re-establishes the proof") is wrong for this case; only rebasing the worktree helps.

Correct. A pod's gateway is the checkout's own venv binary, so a worktree branched before this change writes no start identity — and restarting it writes none either, which made the advice actively misleading for the one case it was most likely to be read in.

_unproven_remedy() in src/kiro_crew/pod/runtime.py now splits the two causes: a missing, malformed, or stale record means the gateway is not the process that wrote it, so restarting the pod re-establishes the proof; a record with no start identity cannot be fixed by a restart, because the writer is the checkout's own binary — re-provisioning the worktree is what fixes it.

Grepped the repository for other places making the "until restart" claim about this case: line 1750 was the only one. (_POD_EQUIVALENT["restart"] is an unrelated verb map.)

Both texts were verified live, not just unit-tested. Against a running pod that was demonstrably serving: removing the .start sidecar produced the refusal carrying the "no start identity → re-provision the worktree" text, and a bogus .start produced the refusal carrying the restart text. Restoring the sidecar returned status 200, ok=true.

(Reposted verbatim from the original disposition of 2026-09-03 22:42 UTC, reformatted only: the readiness disposition gate counts top-level bold bullets as finding records, and the original's two explanatory sub-bullets made one record look like three. One finding, one record, same rationale.)

@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 Sep 4, 2026
@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — fixture setup leaked resources when acquisition failed (span=e1cd212a4c31; upheld by adjudication under no-test-side-effects)

Real: stub_gateway and port_squatter acquired the temp dir and servers before the try, so a bind refusal inside that window skipped teardown entirely. Both fixtures now register each cleanup on a contextlib.ExitStack the moment its acquisition succeeds — rmtree first, server_close per server, and the shutdown+join pair only after the serving thread starts, because BaseServer.shutdown() on a never-started serve_forever blocks forever. Happy-path teardown stays explicit after the yield so the TCP-sentinel assertions keep their ordering.

Same push also fixed the four Windows shard reds, which were ours: this test module failed collection on Windows (_RecordingUnixServer named socketserver.UnixStreamServer as a class base, evaluated at import time before any mark can skip — the trap test_socketsec_cov80.py documents). The base now resolves through getattr and both socket-binding fixtures skip on Windows; the pure path test still runs there.

@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author
  • FIXED — function-local import moved to the module import block (span=201ca675d00e)

The lazy import defended itself with dashboard/urls's own hot-path rationale, but that module is deliberately a stdlib-only leaf so that hot paths can import it — the justification pointed at the wrong direction. dashboard_socket_name now imports at top level; the docstring paragraph justifying laziness was removed with it.

@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author
  • REBUTTED — missing positionals are arity errors, and argparse usage is the CLI-wide contract for those (span=c7d8344b8408)

An invalid value for a supplied argument reaches the JSON envelope — that is the settled earlier fix on this file (an unrecognised METHOD prints fixed-key JSON). An absent positional is a different class: pod up without <wt>, pod token without a name, and every other kirocrew verb answer it with argparse usage + exit 2. Making name/method/path optional with sentinel defaults so _api can re-implement arity checking would fork pod api from the whole CLI surface and buy an agent nothing: an omitted argument is a deterministic caller programming error either way. The envelope governs execution once a syntactically complete invocation exists.

@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

The prescription ("mint through socket_path") cannot land as a rider: api_token_local gates on is_loopback(request.remote or ""), an AF_UNIX request has an empty request.remote, and is_loopback("") is False — so the socket path answers 403 today. That endpoint is in token_auth._BYPASS_EXACT, so admitting unix peers widens a middleware-bypassed auth gate, which deserves its own review; the owner ruled to route it to #8552. The residual differs in kind from the finding this PR fixed: a squatter winning the mint window receives no pod credential (its fabricated token is handed only to the pod's own socket, which rejects it) — what it could capture is the host-side .local_secret, the exposure pod token already ships on main.

@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

IPv6-bound pod + unavailable listener attribution + IPv4 port squatter → PID attestation passes → mint sends X-Local-Secret to the squatter.

Taking the premise seriously rather than by round-count: the widened window this round adds over the crash-race residual is a pod that is alive but IPv6-bound, leaving 127.0.0.1:<port> free for a squatter to hold indefinitely while PID attestation passes. That state cannot occur under this PR.

First leg: a pod booted by this code cannot be IPv6-bound. build_pod_env pins KIROCREW_BIND=127.0.0.1 (this PR's round-5 fix on this same span) — live-verified by booting a pod with KIROCREW_BIND=::1 deliberately exported and reading /proc/net/tcp: the gateway's listener sat at 127.0.0.1:7974 exactly, zero IPv6 listeners. While the pod holds that socket, a squatter cannot bind it; the address split the finding needs does not exist.

Second leg: a legacy pod that could be IPv6-bound cannot attest. A pod booted before this PR predates the .start sidecar, and the owner-confirmed migration rule (recorded on this PR) is that pre-sidecar pods are refusedmint_token requires positive OWNER_POD proof, the start-identity check fails closed, and no secret is sent. The refusal text tells the operator to re-provision.

What remains is exactly the crash-window residual dispositioned last round: pod exits between attestation and the mint's TCP connect. That is pre-existing (pod token on main shares the same leg), leaks no pod credential, and is tracked as #8552 — whose scope is precisely this round's prescription ("mint through the pod's private Unix socket and authenticate Unix peers at /api/token/local"): the second half widens api_token_local, a token_auth._BYPASS_EXACT endpoint, which is why the owner routed it to its own reviewed change rather than a rider on a PR whose diff never touches that gate.

Seventh consecutive blocking round on this span. Rounds 5–7 each prescribed a different remedy: round 5's was implemented (bind pin), round 6's was owner-ruled to #8552, round 7 re-prescribes round 6's. Per the recorded stall rule, further rounds re-prescribing #8552's content will be escalated to the owner rather than re-argued.

@pepmach

pepmach commented Sep 5, 2026

Copy link
Copy Markdown
Contributor Author

/ai-review override gpt 5979f98: mint-over-TCP residual is owner-ruled to #8552 (widening the token_auth._BYPASS_EXACT gate is its own reviewed change); the IPv6 premise is unreachable here — new pods are bind-pinned to 127.0.0.1 (live-verified) and pre-sidecar pods are refused by the owner-confirmed migration rule before any secret is sent.

@github-actions

github-actions Bot commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Human judgment recorded

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

mint-over-TCP residual is owner-ruled to #8552 (widening the token_auth._BYPASS_EXACT gate is its own reviewed change); the IPv6 premise is unreachable here — new pods are bind-pinned to 127.0.0.1 (live-verified) and pre-sidecar pods are refused by the owner-confirmed migration rule before any secret is sent.

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

@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 5, 2026

@buluoray buluoray left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed against source at 5979f98, weighted toward the authentication path since that is what this adds. 0 blocking findings.

The credential never reaches the caller, and that is the design's best property. The CLI surface is api <wt> <METHOD> <path> [--data JSON] [--allow-write] — no token argument, so nothing lands in ps or shell history. pod_api mints internally via mint_token(cfg, name) at runtime.py:1995. It is not logged either: the audit at :1867 records the credential-free normalized path, and the URLError handler at :2018 deliberately declines to render exception values because urllib would carry the token-bearing full_url. It is scrubbed from the response body in raw, percent, quote_plus and \uXXXX forms.

Transport is the pod's private unix socket with no TCP fallback (unix_socket_urlopen, runtime.py:2004), which is the structural version of the guarantee rather than a check. The code comment states the threat plainly — ordinary loopback can be held by a process that is not the pod, so a minted credential must never be sent there — and an absent socket refuses through the envelope before minting, since mint_token itself spends the pod's .local_secret to obtain one. Nothing to squat.

Fail-closed on ownership, mutation-verified. A missing record, missing start-identity or start-token mismatch all resolve to OWNER_UNPROVEN and mint_token raises before any secret moves. Non-POSIX returns OWNER_UNPROVEN without touching host tools.

Mutation (source) Guard Result
Remove the start-time freshness check in _pod_recorded_pid recycled PID cannot attest RED — assert 'pod' == 'unproven'
Disable the --allow-write gate write refused before host access RED — the test also catches "guard ran too late"
Drop the caller token= param refusal caller token refused and scrubbed from audit RED — 5 tests, including the URL-encoded %74oken evasion

Cross-platform and invariants both hold. The .start / .pid / .secret sidecars go through atomic_write(..., mode=0o600) (mkstemp O_EXCL 0600 + os.replace), not a bare os.chmod under if IS_POSIX — the diff adds zero os.chmod calls, and a code comment shows the author knew why. The keystone sensitive-path and denied-command matchers are untouched. cli.md:241, instances.md §12.2 and pod/README.md are all updated in the same commit.

It also closes an exposure rather than opening one: build_pod_env now pins KIROCREW_BIND=127.0.0.1 (runtime.py:2384), overriding an inherited 0.0.0.0/::1 from the official image. pod api adds no listener — it is a client.

On the MCP-first convention (AGENTS.md), which is the one repo rule this could plausibly trip: I checked rather than assumed, and it is not violated. Zero pod verbs ship as MCP tools — mcp_core.py and mcp_cron.py contain no pod reference at all — so pod is an established CLI-only operator surface and the recently merged pod scenarios (#8127) sets the precedent this verb follows. Worth a human sanity-check only if the team intends pod verbs to become MCP tools as a class; as things stand it is consistent with what ships.

Non-blocking:

  1. The residual you already ruled on and routed to #8552 is correctly scoped: mint_token still fetches over TCP to 127.0.0.1:<port>, so a squatter winning the crash window captures the host-side .local_secret — not a pod credential, since a fabricated token is only ever handed to the pod's own socket, which rejects it. Pre-existing on main via pod token, and no new fail-open.
  2. pid_start_token's Windows process_start_time fallback leg is currently dead-benefit for the pod path: its only reader _pod_recorded_pid is reached through port_owner, which returns OWNER_UNPROVEN when not IS_POSIX. Defensive, not defective.
  3. The body's "No lifecycle changes" line understates the KIROCREW_BIND pin, which is a real behavior change for an operator who exported a non-loopback bind. Documented in the code comment; the prose is just stale.
  4. Design's suggestion to mint with a one-shot short TTL instead of the 2h default is reasonable hardening.

Not verified: the socket 0600 + pod-home 0700 property this depends on lives in dashboard/server.py and pod provisioning, outside this diff — I confirmed the dependency but did not re-audit the pre-existing creation code. macOS/Windows pid_start_token behavior was not executed (both degrade to "" = unproven, so fail-closed). test_an_unrecognised_method_reaches_the_envelope_not_argparse_prose fails in my worktree on empty-stdout JSONDecodeError, which is a pristine-subprocess capture artifact — running the command by hand emits the correct fixed-key envelope and exits 1, matching your disposition.

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.

3 participants