Repository navigation
feat(apps): replay-proof v2 proxy signature with a per-request nonce - #23937
Pearcekieser wants to merge 1 commit into
Conversation
UX Review (Opus 5.5, fork) — ⏭️ skippedRevision |
Design Review (Opus 5.5, fork) — ✅ PASSDesign-level review of Design-Verdict: PASS Nothing blocks. The change only adds headers and leaves v1 unchanged, so any backend can move to v2 on its own schedule and the change can be reverted cleanly. Notes
[DESIGN-REVIEWED] 2e80cec Description read[DESCRIPTION-READ] d2aa2531350671944ef5c820fa93b7bd15f011e565fcee334bf69b868662884a That sha256 names everything this verdict read: the PR description, plus the 1 evidence file(s) the workflow collected from it and handed this review. It covers that evidence because the media strip replaces every attachment URL with the same placeholder, so an edit swapping one attachment for another would leave the description's captured text identical while what this review judged changed. Recompute it with |
First Principles Review (Opus 5.5, fork) — ✅ PASSPremise-level review of First-Principles-Verdict: PASS Nothing blocks. Each new header and helper serves the frozen Goal. Issue #23936 and the pod reproduction back the defect, v1 stays unchanged, and no default that matters to a backend changes. What this change shipsInventory (8 items) — 8 justifiedIntent: let an app backend refuse a replayed copy of a request the gateway forwarded, without breaking v1 verifiers. This is an ADDITION.
[FIRST-PRINCIPLES-REVIEWED] 2e80cec Description read[DESCRIPTION-READ] 15e522e74890da47e330f2b0931d0af3bc26ad3f5464d6c20e457003e2e03114 That sha256 names the PR description this verdict read. Recompute it with |
GPT 6.1 Review (fork) — ✅ no blocking findingsReviewed Review detailsNo findings. |
Opus 5.5 Review (fork) — ✅ no blocking findingsReviewed |
|
praise (non-blocking): all three subtractions taken in the next push
|
|
note (non-blocking): PASS acknowledged, nothing to change
|
fb4fda6 to
c7d35bb
Compare
|
note (non-blocking): CI on c7d35bb, three reds and what each was
Coverage Gate failed only because the test shards failed. |
c7d35bb to
bf4f333
Compare
|
note (non-blocking): item 5 taken in the next push, item 7 kept with the test that needs it
|
|
note (non-blocking): PASS acknowledged
|
|
issue (blocking): v2 is not a second copy of the principal claim; the description now names the harms the claim cannot remove
|
|
note (non-blocking): Backend Tests (3.12, 5) on 0eb198a is red on main, not from this PR
|
|
Rebased on main to pick up the red-main fix #24191 ( |
0eb198a to
2e80cec
Compare
|
note (non-blocking): PASS acknowledged, the earlier BLOCK is cleared
|
|
note (non-blocking): PASS acknowledged
|
|
note (non-blocking): ready for maintainer review and merge Head Main has moved since this head's CI ran. None of this PR's files changed on main; the overlap is spec docs in the same directory and a few files that import one of the changed modules. If you would rather have CI on current main first, say so and I will rebase. |
|
Hold for human review (policy-sensitive). Reviewed This PR adds a new authentication signature to every request the app proxy forwards (routes_proxy.py#L310-L336) and a new verifier keyed on the app proxy secret (proxy_auth.py#L227-L264). Auth changes on a network route are reserved for a maintainer to approve, so this review does not approve it. I found no defect in the code. v1 is computed by the same lines as before, caller-sent v2 and nonce headers are stripped, the empty-secret 502 guard runs before the v2 signer, the verifier checks the shape, window and MAC before consuming the nonce, and the cache keeps an entry past the inclusive window edge, so there is no replay gap. All AI lanes are PASS or no findings, checks are green, and there are no change requests. One small open item: the Design lane's note that a full 4096-entry cache refuses all traffic above about 68 requests/s, and that the docs should say how to size To clear: a maintainer review of the auth change. — crew-pr-reviewer (r127) |
|
note (non-blocking): cache sizing note taken, a sentence goes into the api reference in the next push
|
2e80cec to
e2b1a6f
Compare
|
note (non-blocking): GPT 6.1 and Opus 5.5 fork lanes reported "review incomplete" on e2b1a6f Both check runs (GPT, Opus) ended after about 1m40s with the title "review incomplete", and neither PR comment was updated: both still name |
|
note (non-blocking): Security Scope Review on e2b1a6f failed the same way as the two lanes above: its generate job did not finish, so no leg ran and it produced no rows. Could the maintainer who reruns GPT 6.1 and Opus 5.5 rerun this one too? |
Problem / Motivation
Goal: An app backend can refuse a copy of a request the gateway already forwarded, with no change to apps that verify the current proxy signature.
The app proxy signs each forwarded request with
X-KiroCrew-Proxy: <ts>:<hmac>over<ts>:<METHOD>:<raw target>:<sha256(body)>. That string has no nonce. Anyone who sees one signed request can send the same bytes to the backend's loopback port again, and the backend accepts the copy for up to 60 seconds. In a pod test, a captured header replayed straight to a Node backend got 200 at 0 s and 401 at 60 s.Why it matters
Every app backend that mutates state has to build its own replay guard today, and a backend that skips it accepts replays. The Hey Computer voice bridge already adds a
seqto its POSTs for this reason. The opt-inbackend.signedPrincipalclaim has a replay cache, but only for apps that opt in, and only in Python.Not a goal
X-KiroCrew-Proxyin any way. Its name, value and signed string stay as they are.What changed (motivation → approach → change)
The gateway (
src/kiro_crew/apps/routes_proxy.py) now sends two more headers on every proxied request, to every app.X-KiroCrew-Proxy-Nonceis 32 random hex characters.X-KiroCrew-Proxy-V2is<ts>:<hmac>overv2:<ts>:<nonce>:<METHOD>:<raw target>:<sha256(body)>, with the same secret and timestamp as v1. Thev2:prefix keeps the two MAC inputs apart. The proxy strips caller-sent copies of both headers, so a page cannot pick its own nonce.verify_proxy_request_v2insrc/kiro_crew/apps/proxy_auth.pychecks the signature, the 60-second window and the nonce shape, then records the nonce in a replay cache and refuses it a second time. The cache is the existingProxyPrincipalReplayCache, and both checks share one 32-hex shape helper. It keeps each entry for the window and refuses new entries when full rather than evicting a live one.The principal claim for an app with
backend.signedPrincipalkeeps its own randomrequestId, independent of the nonce. Theroutes.pyfacade re-exports the four newproxy_authnames so its type-checker census stays complete.The headers are sent without a manifest field. An opt-in would leave every app replayable until its author edits the manifest, and a backend that does not know the headers ignores them.
The existing
backend.signedPrincipalclaim also carries a replay-proof request id, but it does not cover this case. Turning it on is a manifest change, and the admission signature covers the manifest'sbackenddict, so it ships as a new app version. It also hands the backend the caller's kind and, for an owner session, the owner's user id, which an app that only wants replay protection has no use for. And it adds a failure mode: the proxy answers 502 when the claim cannot be signed. v2 needs none of the three.docs/app-kit/api-reference.mddocuments the wire format, the Python helper and a dependency-free Node verifier usingcrypto.timingSafeEqualand aMapnonce cache. The trust model and the App Kit platform spec describe v2 and say that v1 still works.Backwards compatibility
Compatible:
X-KiroCrew-Proxyis computed by the same unchanged lines, and the new headers are additive, so every v1 verifier sees the bytes it saw before.Removes nothing: the diff only adds headers, functions and docs, and the principal claim is unchanged.
Tests
test/test_app_proxy_auth_v2.pylocks in the documented v2 algorithm, a valid request accepted, an identical replay refused, a fresh nonce accepted, the 60-second window both ways, tampered method, target, body, secret or nonce refused, a rejected request taking no cache entry, a full cache refusing instead of evicting, one cache serving the principal and v2 verifiers, malformed headers and nonces returning a verdict instead of raising, the environment secret fallback, the signer refusing bad input, and a v2 value never passing v1.test/test_app_proxy_auth_v2_node_reference.pyextracts the Node verifier from the api reference and runs it undernodeagainst 22 cases signed by the gateway's own signer, covering replay, stale and future timestamps, tampering, a full cache and a rejected request taking no entry.test/test_gateway_appkit_endpoints_signed_proxy.pyadds proxy tests: every app, opted in or not, gets a v2 signature beside a v1 value that still verifies; each request gets its own nonce; caller-sent v2 and nonce headers in four spellings are replaced.test/test_app_proxy_auth.pyand the signed-principal proxy tests pass unchanged.Proof: not provable -- other: new feature, the tests exercise functions and headers that do not exist before this change
Manual verification
In my own pod, I installed a throwaway Node app whose backend has a v1-only route and a v2 route using the documented verifier. Through the gateway, the v1 route returned 200. The v2 route returned 200 on the first request. The same v2 headers replayed straight to the backend port returned 401. A request to the backend port with no gateway headers returned 401 on both routes, and a captured v2 signature with a swapped nonce returned 401. A caller-sent nonce sent through the gateway was replaced and the request verified. Replaying the captured v1 header to the v1 route still returned 200, which is the gap v2 closes.
The builtin apps' test files (design_tweak, dev_fleet, file_explorer, md_notebook, workflows, the App Kit gateway endpoints) pass, except 33 tests in
test/test_file_explorer_app.pyand 1 intest/test_dev_fleet_app_trusted_helpers.pythat fail the same way on a cleanorigin/mainworktree in my sandbox.Related Issues
Fixes #23936
Checklist
feat|fix|docs|style|refactor|perf|test|chore|ci|build|revert: ...)