Skip to content

feat(apps): replay-proof v2 proxy signature with a per-request nonce - #23937

Open
Pearcekieser wants to merge 1 commit into
kirodotdev:mainfrom
Pearcekieser:feat/proxy-nonce-v2
Open

Pearcekieser wants to merge 1 commit into
kirodotdev:mainfrom
Pearcekieser:feat/proxy-nonce-v2

Conversation

@Pearcekieser

@Pearcekieser Pearcekieser commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

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 seq to its POSTs for this reason. The opt-in backend.signedPrincipal claim has a replay cache, but only for apps that opt in, and only in Python.

Not a goal

  • Changing X-KiroCrew-Proxy in any way. Its name, value and signed string stay as they are.
  • Moving the builtin apps' verifiers to v2. They keep v1 and pass their current tests unchanged.
  • A published Node SDK package. The Node verifier is a documented reference snippet.

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-Nonce is 32 random hex characters. X-KiroCrew-Proxy-V2 is <ts>:<hmac> over v2:<ts>:<nonce>:<METHOD>:<raw target>:<sha256(body)>, with the same secret and timestamp as v1. The v2: 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_v2 in src/kiro_crew/apps/proxy_auth.py checks 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 existing ProxyPrincipalReplayCache, 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.signedPrincipal keeps its own random requestId, independent of the nonce. The routes.py facade re-exports the four new proxy_auth names 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.signedPrincipal claim 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's backend dict, 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.md documents the wire format, the Python helper and a dependency-free Node verifier using crypto.timingSafeEqual and a Map nonce cache. The trust model and the App Kit platform spec describe v2 and say that v1 still works.

Backwards compatibility

Compatible: X-KiroCrew-Proxy is 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.py locks 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.py extracts the Node verifier from the api reference and runs it under node against 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.py adds 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.
  • Existing test/test_app_proxy_auth.py and 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.py and 1 in test/test_dev_fleet_app_trusted_helpers.py that fail the same way on a clean origin/main worktree in my sandbox.

Related Issues

Fixes #23936

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|style|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

@Pearcekieser
Pearcekieser requested review from a team and KiroCrewCaptain as code owners October 25, 2026 07:17
@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 Oct 25, 2026
@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

UX Review (Opus 5.5, fork) — ⏭️ skipped

Revision e2b1a6f19739b46c4facc25f8e24cd0a5030d31a touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Does not block merge.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Design Review (Opus 5.5, fork) — ✅ PASS

Design-level review of 2e80cec4bfb50e4d2d2c3ed4626a86c76c12e555 via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS does not.

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

  • Once the replay cache holds 4096 live nonces, it refuses every new request. Above about 68 requests/s within the 60-second window, a v2 backend therefore starts returning 401 to real traffic. The docs should say how a busy app should size max_entries.

[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 .github/scripts/pr-description-capture.sh over the same inputs; if it differs, the description or that evidence changed after this verdict was formed and any finding drawn from either is unproven.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

First Principles Review (Opus 5.5, fork) — ✅ PASS

Premise-level review of 2e80cec4bfb50e4d2d2c3ed4626a86c76c12e555 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 does not.

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 ships

Inventory (8 items) — 8 justified

Intent: let an app backend refuse a replayed copy of a request the gateway forwarded, without breaking v1 verifiers. This is an ADDITION.

  1. Every proxied request, for every app, now carries a fresh X-KiroCrew-Proxy-Nonce — justified
  2. Every proxied request now carries an X-KiroCrew-Proxy-V2 HMAC that binds the nonce — justified
  3. The proxy strips caller-sent v2 and nonce headers, so a page cannot pick its own nonce — justified
  4. A new public Python helper, verify_proxy_request_v2, uses the existing ProxyPrincipalReplayCache — justified
  5. New gateway-side helpers sign_proxy_request_v2 and new_proxy_nonce each have one consumer, routes_proxy.py — justified
  6. The principal claim's request-id shape check now goes through the shared _is_request_id helper — justified
  7. The routes.py facade re-exports the four new names, following the existing _facade_mirror convention — justified
  8. The api reference gains a Node reference verifier, and a test runs it under node against the gateway's signer — justified

[FIRST-PRINCIPLES-REVIEWED] 2e80cec

Description read

[DESCRIPTION-READ] 15e522e74890da47e330f2b0931d0af3bc26ad3f5464d6c20e457003e2e03114

That sha256 names the PR description this verdict read. Recompute it with .github/scripts/pr-description-capture.sh; if it differs, the description changed after this verdict was formed and any finding drawn from the description is unproven.

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

GPT 6.1 Review (fork) — ✅ no blocking findings

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

Review details

No findings.
[GPT-REVIEWED] 2e80cec

@github-actions

github-actions Bot commented Oct 25, 2026 •

Copy link
Copy Markdown
Contributor

Opus 5.5 Review (fork) — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 2e80cec

@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 Oct 25, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

praise (non-blocking): all three subtractions taken in the next push

fixed: PROXY_HEADER had no reader and is deleted; the gateway keeps writing the v1 literal it wrote before.
fixed: the ProxyReplayCache alias is deleted; code, docs, spec and tests name ProxyPrincipalReplayCache only.
fixed: _NONCE_HEX_LENGTH and _is_proxy_nonce are deleted; the v2 nonce and the claim requestId both go through one helper, _is_request_id, keyed on _REQUEST_ID_HEX_LENGTH.

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): PASS acknowledged, nothing to change

v1 stays byte-identical and the v2 headers are additive, as the punchline says; the next push only removes three unused names and adds encoding="utf-8" to one test subprocess call.

@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 Oct 25, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): CI on c7d35bb, three reds and what each was

  • Backend Tests (3.12, 5): mine. test/test_routes_facade.py::test_every_served_name_is_named_for_type_checkers wanted the four new proxy_auth names in the routes.py facade import. Fixed in the next push; every test/test_*facade*.py passes locally (3,884 tests).
  • Backend Tests (3.12, 2): test/test_native_skill_projection_rewrites.py::test_two_concurrent_releases_each_prune_their_own_view, the known flake in Flaky: test_two_concurrent_releases_each_prune_their_own_view sees a lingering view since #23913 #23965. Reported there with this run.
  • Frontend Tests (1): website/src/guide/findByName.test.tsx > probeForName > opens a popover without its autofocus taking focus from the person, expected 'none' to be 'found' (run). This PR changes no file under website/, and the file passes 3 of 3 runs locally on this branch. I found no flake issue for it.

Coverage Gate failed only because the test shards failed.

@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 Oct 25, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): item 5 taken in the next push, item 7 kept with the test that needs it

fixed (next push): request_id=nonce, _V2_CACHE_KEY_PREFIX and the shared-cache paragraph are deleted; the principal claim keeps its own random requestId, and the two tests that pinned the shared id are gone with it.
rebutted: the four routes.py imports are not dead. test/test_routes_facade.py::test_every_served_name_is_named_for_type_checkers requires every name the facade serves from routes_proxy to be bound or named in routes.py; dropping them failed Backend Tests (3.12, 5) on c7d35bb.

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): PASS acknowledged

The shape the punchline describes stands. The next push only removes the shared nonce/requestId id, which First Principles marked as a rider.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Oct 25, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

issue (blocking): v2 is not a second copy of the principal claim; the description now names the harms the claim cannot remove

  • Second replay-proof signature beside the principal claim

rebutted: the claim is gated on backend.signedPrincipal, which sits in the manifest backend dict that the admission signature covers, so turning it on ships as a new app version; v2 reaches every installed app with no manifest change, which issue #23936 requires.
rebutted: the claim hands the backend the caller kind and, for an owner session, the owner user id; an app that wants only replay protection should not have to accept identity data to get it.
rebutted: an opted-in request fails with 502 proxy_principal_sign_failed when the claim cannot be signed; v2 adds no new refusal path at the proxy.
The same diff passed this lane on fb4fda6, bf4f333, eb08bd4 and 69dcd4e, and this head is a no-change rebase. The description's What changed section now names the three harms, which is this finding's Clears-when.

@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 Oct 25, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): Backend Tests (3.12, 5) on 0eb198a is red on main, not from this PR

test/test_memory_files_substitution.py::TestTheInjectedImplementationAnswers::test_pruning_removes_from_the_seam_and_leaves_local_files_alone fails with assert 2 == 1 at line 233 (run). It fails the same way, 2 of 2 runs, on a clean worktree at this PR's base 8edd26d9 without this diff. This PR touches no memory code. I found no open issue or PR for it.

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

Rebased on main to pick up the red-main fix #24191 (test_memory_files_substitution); no behaviour change, CI re-running.

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): PASS acknowledged, the earlier BLOCK is cleared

This head is the same diff rebased on main to pick up #24191. The lane now marks all 8 inventory items justified, including the v2 headers the previous head's BLOCK asked to delete. No code changes in response.

@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 Oct 26, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): PASS acknowledged

No change of shape. v1 stays byte-identical and each backend adopts v2 when it chooses, as the punchline says.

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): ready for maintainer review and merge

Head 2e80cec4b has readiness: passed. GPT 6.1 and Opus report no findings, Design and First Principles pass, and every lane item has a reply above. I can't arm auto-merge from the fork, because EnablePullRequestAutoMerge needs write access. A maintainer approval and merge is the remaining step.

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.

@iamwhatever

Copy link
Copy Markdown
Contributor

Hold for human review (policy-sensitive). Reviewed 2e80cec4bfb50e4d2d2c3ed4626a86c76c12e555.

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 max_entries, has only a generic acknowledgement. A sentence in the api reference would close it.

To clear: a maintainer review of the auth change.

— crew-pr-reviewer (r127)

@Pearcekieser

Copy link
Copy Markdown
Contributor Author

note (non-blocking): cache sizing note taken, a sentence goes into the api reference in the next push

  • Full replay cache refuses real traffic above about 68 requests/s

fixed (next push): the v2 section of docs/app-kit/api-reference.md now says each nonce stays cached for 60 seconds, that the default max_entries=4096 holds about 68 requests a second, that above that rate the verifier returns 401 to real traffic, and that a busier backend sets max_entries to at least 60 times its peak requests per second.
The refuse-when-full behaviour itself stays: evicting a live nonce would reopen the replay this PR closes.

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

Copy link
Copy Markdown
Contributor Author

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 2e80cec4b, where each reported no findings. The only change since 2e80cec4b is one sentence in docs/app-kit/api-reference.md on sizing max_entries, plus a rebase on main. A fork author cannot rerun these lanes; could a maintainer rerun them?

@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 Oct 26, 2026
@Pearcekieser

Copy link
Copy Markdown
Contributor Author

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?

This branch has not been deployed

No deployments
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.

feat(apps): replay-proof app proxy signature with a per-request nonce

2 participants