Skip to content

fix(pod): send the token-mint secret over the pod's unix socket, never TCP - #9051

Open
javenciu wants to merge 2 commits into
kirodotdev:mainfrom
javenciu:fix/pod-mint-token-unix-socket
Open

fix(pod): send the token-mint secret over the pod's unix socket, never TCP#9051
javenciu wants to merge 2 commits into
kirodotdev:mainfrom
javenciu:fix/pod-mint-token-unix-socket

Conversation

@javenciu

@javenciu javenciu commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

mint_token reads the pod's .local_secret and sends it in an X-Local-Secret header over loopback TCP to http://127.0.0.1:<port>/api/token/local. Ownership is attested (OWNER_POD positive proof) and then a separate TCP connection carries the secret — a pod exiting inside that window frees the port for any local user to bind, and loopback TCP has no peer-credential API, so nothing on the wire can tell the squatter from the pod's gateway. #8218 closed exactly this class for the token-bearing request; mint_token is the residual its review disposition called out, and it could not ride #8218 because the server refuses the better transport: api_token_local gates on is_loopback(request.remote), and an AF_UNIX request has an empty request.remote, so the endpoint answers 403 to the transport that is strictly harder to reach than loopback TCP.

Why it matters

The captured value is the pod's per-gateway-start .local_secret. The impact is bounded exactly as the issue bounds it (capture-now / replay-before-any-gateway-restart, and #8218's socket transport means a fabricated token is only ever handed to the pod's own socket) — but pods carry ~2h TTLs and are torn down routinely, so the port-release window is a normal lifecycle event, not an anomaly. A secret-bearing request should not depend on winning a race against ordinary pod churn.

What changed (motivation → approach → change)

All three steps of the issue's proposal, with the admission taken at the stronger of the two options the issue offered:

  • Client: mint_token now sends the mint over the pod's private AF_UNIX socket via unix_socket_urlopen(req, timeout=5, socket_path=pod_socket_path(cfg, name, port)). That opener has no TCP handler, so "no fallback" is structural rather than a flag: a missing, stale, or refusing socket raises instead of handing the header to whatever answered — the error message says explicitly that the call is not retried on 127.0.0.1:<port> and why. The URL keeps the loopback host so the gateway's Host validation sees exactly what it saw on TCP; the socket path is derived, never caller-supplied. The OWNER_POD pre-check stays (the issue's own suggestion): it costs one process lookup and buys refusal messages that name why the pod cannot answer, instead of a bare connection error.
  • Server: api_token_local admits kernel-verified unix peers via a new _unix_peer_is_self helper — request_is_unix_socket(request) AND check_peer_is_self(sock) is PeerCredResult.MATCH. Because this endpoint is token_auth-bypassed, admission is deny-by-default: MISMATCH (another principal reached our socket — precisely when the 0700 directory gate has failed and refusing matters most) and UNVERIFIABLE (no platform mechanism, failed syscall) are both refused, so a platform without peer credentials never silently widens the gate. This admits a transport, never a caller: the X-Local-Secret check downstream is unchanged on both transports. Function-local imports mirror the sibling admission in handlers/updates.py.
  • Deliberately unchanged: health() / probe stay on TCP — they carry no credential, and port_owner() already reports who replied. The docker guide's local-bootstrap paragraph now describes the two admitted transports.

Tests

Server half (test/test_dashboard_handlers_core_coverage.py, TestLocalToken):

  • test_unix_peer_match_with_valid_secret_issues_a_token — kernel MATCH + valid secret mints (403 → 200 before the fix; the benign path is admitted, not just attacks refused).
  • test_unix_peer_still_needs_the_secret — admitted transport, wrong secret → 403 invalid secret (not loopback only), pinning that admission never weakens the secret check and that the request genuinely passed the transport gate.
  • test_unix_peer_uid_mismatch_is_refused_even_with_the_secret — kernel says another principal: refused with the audit log recording non-loopback.
  • test_unix_peer_unverifiable_is_refused_even_with_the_secret — no peer-credential mechanism: refused (deny-by-default pin).

Client half (test/test_pod.py):

  • test_mint_token_sends_the_secret_only_over_the_pod_socket — asserts the TCP opener list stays empty, the unix opener got pod_socket_path(...), and the X-Local-Secret header rode the socket.
  • test_mint_token_reads_secret_and_posts re-pointed at the socket transport; test_mint_token_refuses_a_foreign_port_holder passes unchanged on both sides (control: the pre-check's refusal behavior is untouched).

Fails-before at unfixed fd0fb5dbb (committed tests copied onto a pristine worktree): server 2 failed / 2 passed — admission 403-not-200 and the needs-secret refusal citing the transport (loopback only) instead of the secret; the two refusal pins pass both sides by design (deny-by-default is not new behavior). Client 2 failed / 4 passed — the secret rode TCP and no socket call existed. After the fix: 4/4 and 6/6. Full targeted files 502 passed / 1 skipped; seam neighbors (test_pod_api.py, test_socketsec*.py) 135 passed / 2 skipped; mypy clean over 1302 files; flake8 / isort / black-baseline / docs-lint all pass.

Manual verification

N/A — unit coverage sufficient: the two new seams are unit-pinned with the kernel verdict and transports faked, and both underlying primitives carry their own pre-existing real-socket suites (test_pod_api.py binds a live AF_UNIX server against unix_socket_urlopen; test_socketsec*.py covers check_peer_is_self including real-socket SO_PEERCRED). A live gateway round-trip was not run in this environment (no running pod); CI exercises the composed path.

Related Issues

Fixes #8552

Pattern harvest

Rule candidate: review-prompt
Pattern: "attest-then-send across separate connections is a TOCTOU on the transport — a credential-bearing request must ride a connection whose peer is verified at send time (unix socket + peer creds), not a port whose owner was checked moments earlier". Flag any request that sends a secret over plain loopback TCP when a peer-verified unix transport exists for the same server.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (if applicable)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

Per the template placeholder (CLA text pending): offered under the same terms as my prior merged contributions to this repository (#8835).

Round 2: connect-time peer verification (review finding)

Review flagged a residual window on the socket itself: the unix socket
path lives in an owner-writable directory, so a same-UID process can
unlink it and bind its own listener there — and the mint request would
hand that listener the pod's .local_secret. A fresh pid record cannot
see this; the record proves the gateway is alive, not that it is the
process answering the file.

Commit 2 closes it with the kernel's own answer: unix_socket_urlopen
grows a verify_peer callback that runs on the connected socket before
any HTTP bytes, and _attested_gateway_verifier builds that check from
the freshness-proven recorded gateway pid vs the kernel's peer
credentials (SO_PEERCRED on Linux, LOCAL_PEERPID on macOS).
Deny-by-default: a mismatched peer, an unreadable peer, and an
unprovable pid record all refuse before the request line goes out. Both
send sites are covered (token mint and pod api — a rebind between the
two sends would otherwise capture a live credential), and the
stub-gateway fixture now attests itself, so every pod-api test drives
the real kernel check end to end.

New tests pin the window shut from both sides: a live in-process
listener answering the pod's derived socket path with a mismatched
attested pid observes ZERO bytes (the refusal precedes the request
line), and the same listener attested as this process completes the
mint through the real peer-credential read — no monkeypatch on the
kernel path.

@javenciu
javenciu requested a review from a team as a code owner September 6, 2026 15:54
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 6, 2026
@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

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

Premise-level review of 3e401895108cde25d6a6d607bd76c3b347360974 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.

All evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

Round 2's rebind attacker can already read the secret it supposedly captures — the peer-pid verifier defends a boundary the codebase doesn't have.

Not justified as shipped

  • Item 4 — rides along; parameter surface added only for item 5's premise.
  • Item 5 — inherited: "Review flagged a residual window" is an unlinked assertion (contract: unverifiable review claims are inherited). The named attacker ("a same-UID process can unlink it and bind its own listener") must own the 0700 pod home to rebind — the same ownership reads the 0600 .local_secret beside it and can rewrite the pid record (_pod_recorded_pid reads the same owner-writable home) to attest itself. No sandbox mask covers pod homes (grepped pod_root|kirocrew-pods in sandbox.py: 0 hits; the .local_secret mask at sandbox.py:352 is crew-home-only). Zero option: nothing a nameable principal loses.

What this change ships

Intent: stop the pod's .local_secret from riding a squattable loopback TCP port when minting a token — a FIX (#8552).

  1. pod token mint rides the pod's unix socket, with no TCP fallback — justified
  2. /api/token/local also answers kernel-verified same-user unix peers — justified
  3. Mint failure text names the socket and the no-TCP-retry policy — justified
  4. unix_socket_urlopen grows a verify_peer connect-time callback — rides along, surface for item 5 only
  5. Mint and pod api now refuse when the socket's peer pid ≠ recorded gateway pid — inherited, attacker already holds the asset
  6. Docker guide's local-bootstrap paragraph names both transports — justified
  7. Pod-api stub fixture attests itself so tests drive the peer check — rides along with 5

Watch

  • Point patch: 4 sibling sends of X-Local-Secret over plain loopback TCP remain against the main gateway (cli_server.py:175, 301, 343, 860 — grepped X-Local-Secret), the exact attest-then-send TOCTOU the PR's own "pattern harvest" rule flags, and item 2 already opens the server's unix transport for them. Clears when: a linked follow-up issue names those sites, or the description states why the main gateway is out of scope.
  • Item 5 stays if a real split principal exists (bind-in-pod-home but cannot-read-secret). Clears when: the author names that principal or links the review finding establishing it; otherwise delete commit 2.

Subtractions

  • _unix_peer_is_self (core.py) re-implements token_auth._unix_request_socket (token_auth.py:1612–1631) line-for-line (2 spellings counted); reuse it: sock = _unix_request_socket(request); return sock is not None and check_peer_is_self(sock) is PeerCredResult.MATCH.
  • If item 5's principal cannot be named, delete commit 2 whole: the verify_peer parameter, _attested_gateway_verifier, both call-site threads, and the fixture attestation — round 1 alone closes pod: mint_token sends the pod's .local_secret over TCP loopback — route it over the pod's unix socket #8552.

[FIRST-PRINCIPLES-REVIEWED] 3e40189

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review (fork) — ✅ no blocking findings

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

Review details

The discovery pass produced no candidates, and my independent verification confirms the design holds: check_peer_is_self/get_peer_pid are deny-by-default (MATCH only on positive kernel confirmation); the widened api_token_local condition still 403s genuine remote TCP (request_is_unix_socket false → _unix_peer_is_self false); PodError/PodOwnershipUnproven are RuntimeError subclasses, so a raising verify_peer propagates unwrapped through urllib's except OSError and mint_token's except (URLError, OSError, ValueError), preserving the "refusing to send" message the tests pin; the except BaseException in connect() closes the fd before re-raising. No groundable defect on the changed lines.

No findings.

[OPUS-REVIEWED] 3e40189

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5, fork) — 🟡 CONCERNS

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

Both premises I probed check out in the base tree: pods are POSIX-only by design (runtime.py:776 — "pods are Linux-only" for the runtime dir; port_owner returns OWNER_UNPROVEN on non-POSIX), so the structural loss of the TCP path breaks no supported platform; and the pod unit is Type=simple execing the gateway in place, so the recorded pid is the process that binds the socket — the kernel peer-pid comparison matches the real deployment, and the repo has already accepted fail-closed peer-cred checks on all three platforms (PEER_IDENTITY_SUPPORTED, socketsec docstring).

The one design risk that survives falsification is version skew: _unproven_remedy's own text establishes that a pod's gateway is its worktree's venv binary, so a HEAD CLI minting against a pod built between #8218 (socket exists, serves routes) and this PR gets a permanent 403 ("loopback only") over the socket — the only transport the new client will use — and the new PodError text ("did not complete over its API socket… Not retried on 127.0.0.1") explains the no-fallback policy but not the actual cause or remedy (update that worktree and restart the pod). Since dev_fleet/worktree_ops.py calls mint_token and older worktrees are a normal dev-fleet state, this is a foreseeable operational break with a misdirecting error, though fully recoverable.

Design-Verdict: CONCERNS

Sound root-cause fix — credential moved onto a peer-verified transport — but the structural no-fallback hard-fails against pod gateways built before this PR, with a misdirecting error.

Watch

  • Version skew: a pod worktree pinned between feat(pod): add the authenticated pod api command #8218 and this change serves the socket but 403s AF_UNIX on /api/token/local (is_loopback on empty request.remote); the new socket-only client then fails every mint with the generic "did not complete over its API socket" message, which names neither the older-gateway cause nor the remedy — and _unproven_remedy establishes older worktree gateways are a normal state, not crash residue.
    Clears when: the mint error on an HTTP 403 from the socket distinguishes "older gateway refused the transport" (remedy: update the pod's worktree, then restart) from a genuine refusal, or the author states pod worktrees cannot lag this change.

Suggestions

  • The round-2 rebind defense stops a same-UID process that can already read the owner-readable .local_secret; a one-line note in _attested_gateway_verifier naming the confined-process (sandboxed same-UID) scenario it actually defends would stop a future reviewer from removing it as dead weight.

[DESIGN-REVIEWED] 3e40189

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review (fork) — ✅ no blocking findings

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

Review details

FINDING -- src/kiro_crew/dashboard/handlers/core.py:2525 -- function-local from ... import statements violate top-level-imports -> Fix: move both imports to the module import section.
[GPT-REVIEWED] 3e40189

@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 6, 2026
@javenciu
javenciu force-pushed the fix/pod-mint-token-unix-socket branch from a617b3b to 0242dd9 Compare September 6, 2026 18:55
@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 6, 2026
@javenciu
javenciu force-pushed the fix/pod-mint-token-unix-socket branch from 0242dd9 to e4bb192 Compare September 6, 2026 20:32
@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 6, 2026
…r TCP (kirodotdev#8552)

mint_token attested pod ownership and then opened a SEPARATE loopback TCP
connection carrying X-Local-Secret: a pod exiting inside that window frees
the port for any local user to bind, and loopback TCP has no peer-credential
API to tell the squatter from the gateway. Route the mint over the pod's
private AF_UNIX socket -- unix_socket_urlopen has no TCP handler, so no
fallback is structural rather than a flag.

Server side, admit kernel-verified unix peers at /api/token/local: the
endpoint is token_auth-bypassed and gated on is_loopback(request.remote),
which 403s AF_UNIX requests (empty remote) -- the transport that is strictly
harder to reach than loopback TCP. Admission is deny-by-default via
check_peer_is_self: MISMATCH and UNVERIFIABLE both refuse, and the
X-Local-Secret check is unchanged on both transports.

health()/probe stay on TCP deliberately: they carry no credential, and
port_owner() already reports who replied.
…ng the secret (kirodotdev#8552)

The unix-socket transport removed the TCP rebind window, but the socket
FILE has one of its own: its directory is owner-writable, so a same-UID
process can unlink the path and bind its own listener there -- and the
mint request would hand that listener the pod's .local_secret.

The kernel can prove who answered. unix_socket_urlopen grows a
verify_peer callback that runs on the connected socket BEFORE any HTTP
bytes; _attested_gateway_verifier builds it from the freshness-proven
recorded gateway pid vs the kernel's peer credentials (SO_PEERCRED on
Linux, LOCAL_PEERPID on macOS). Deny-by-default: a mismatched peer, an
unreadable peer, and an unprovable pid record all refuse before the
request line goes out. Both send sites are covered (token mint and pod
api), and the stub-gateway fixture now attests itself, so every pod-api
test exercises the real kernel check end to end.
@bolichen97
bolichen97 force-pushed the fix/pod-mint-token-unix-socket branch from e4bb192 to 3e40189 Compare September 8, 2026 10:11
@bolichen97

Copy link
Copy Markdown
Collaborator

Rebased onto main 6fbb06bcd by a maintainer as part of the 2026-09-08 open-PR audit.

Clean rebase — no conflicts, despite #8528 (ee3b8ebb3) having rewritten large parts of src/kiro_crew/pod/runtime.py after this branch's old merge base. Both commits replayed unchanged and the diff is byte-identical in shape to before (7 files, +494/-28).

Gates run locally on the changed files only: black --check, isort --check-only, flake8 all clean; pytest test/test_pod.py test/test_pod_api.py test/test_dashboard_handlers_core_coverage.py → 667 passed, 1 skipped.

Please review the rebased branch. Note that a maintainer push makes the maintainer the last pusher, so under this repo's last-push rule a second approver is needed before merge. The two open review items (the GPT lane's test-cleanup finding at test/test_pod.py and the First Principles _unix_request_socket reuse suggestion) are untouched and still yours. Reply if anything looks wrong.

@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 readiness: checking Automated validation is still running labels Sep 8, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention labels Sep 8, 2026
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) readiness: action required A blocking check or review needs attention

Projects

None yet

Development

Successfully merging this pull request may close these issues.

pod: mint_token sends the pod's .local_secret over TCP loopback — route it over the pod's unix socket

2 participants