From 4463d148ac68e7077117b0ddbc7060c4768d6e46 Mon Sep 17 00:00:00 2001 From: dcccrypto Date: Wed, 2 Sep 2026 22:48:33 +0100 Subject: [PATCH] fix(#2476): bind the HTTP method and path into the keeper request HMAC MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The signed string was `"."` — no method, no path. Every route sharing KEEPER_REGISTER_SECRET therefore accepted every other route's signatures: the credential authenticated "someone who knows the secret", not "this request". A signature minted for one endpoint verified on another with the same body. Now signs `[timestamp, METHOD, path, rawBody].join("\n")`. Newline-separated rather than dot-separated: a dot can appear in a path, so "POST./a.b" and "POST./a" + ".b" would be ambiguous under the old delimiter. A newline cannot appear in a method or a URL path. Method is upper-cased and path excludes origin and query, so the two ends cannot disagree over casing or a host. ONE ENDPOINT IS DELIBERATELY TRANSITIONAL, and this is the part worth reading. `PATCH /api/markets/[slab]` verifies signatures from a caller that is NOT in this repo — the keeper service has no createHmac at all, so nothing I can see signs it. Making that verifier strict could break a live hop I cannot inspect, so it accepts the legacy unbound form as well AND LOGS EVERY ACCEPTANCE. That is not a fix; an unbound signature is still valid for any endpoint sharing the secret, which is the whole of #2476. The log exists so the transition can be finished rather than forgotten — once it goes quiet, drop the flag. The internal markets -> oracle-keeper hop has both ends in this repo and both are updated here, so that verifier is STRICT immediately. A test pins that it rejects the legacy form. WHAT REMAINS, exactly as #2476 says: there is still no nonce, so a captured signature is replayable against the SAME endpoint within MAX_SIGNATURE_AGE_MS. A nonce needs a store shared across serverless instances — the same constraint that shaped the keeper-register proof in #2505 — so it is a separate design problem, not a line change. FOUND WHILE DOING THIS, filed as #2533: the launch app sends HMAC headers to the keeper's /register, but that endpoint requires `x-shared-secret`, which the app never sends. Hot-registration 401s and fails silently behind "Keeper unreachable — market will auto-discover on next cycle". LAUNCH-16 migrated the sender and never the receiver, which is also why #2233 is still accurate. An existing test signed the unbound message; split into sign() and signUnbound() so the legacy form is still constructible and is now used to assert REJECTION. Negative control: removing the binding fails 2 of the 9 tests in that file. Launch suite: 3138 passed / 16 skipped / 0 failed. Refs: dcccrypto/percolator-launch#2476 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01NgoNgagkvw7i5SSRC3FJ8D --- .../gh1692-timing-safe-keeper-auth.test.ts | 63 ++++++++++++++ app/app/api/markets/[slab]/route.ts | 10 +++ app/app/api/markets/route.ts | 5 +- app/app/api/oracle-keeper/register/route.ts | 11 ++- app/lib/keeper-hmac.ts | 86 +++++++++++++++++-- 5 files changed, 168 insertions(+), 7 deletions(-) diff --git a/app/__tests__/api/gh1692-timing-safe-keeper-auth.test.ts b/app/__tests__/api/gh1692-timing-safe-keeper-auth.test.ts index 5b7dc51d9..ce3883160 100644 --- a/app/__tests__/api/gh1692-timing-safe-keeper-auth.test.ts +++ b/app/__tests__/api/gh1692-timing-safe-keeper-auth.test.ts @@ -53,7 +53,17 @@ describe("GH#1692/LAUNCH-16: oracle-keeper/register HMAC auth", () => { mainnetCA: "22222222222222222222222222222222", }); + // #2476: the signed string now binds the METHOD and PATH, not just + // ".". This helper signs what a MIGRATED caller signs; + // signUnbound below is the pre-#2476 form, kept so the legacy-rejection test + // below has something real to reject. function sign(secret: string, rawBody: string, timestamp = Date.now().toString()) { + const message = [timestamp, "POST", "/api/oracle-keeper/register", rawBody].join("\n"); + const signature = createHmac("sha256", secret).update(message).digest("hex"); + return { timestamp, signature }; + } + + function signUnbound(secret: string, rawBody: string, timestamp = Date.now().toString()) { const signature = createHmac("sha256", secret).update(`${timestamp}.${rawBody}`).digest("hex"); return { timestamp, signature }; } @@ -142,4 +152,57 @@ describe("GH#1692/LAUNCH-16: oracle-keeper/register HMAC auth", () => { expect(res.status).not.toBe(401); expect(res.status).not.toBe(503); }); + + // ── #2476: the signature binds the endpoint ──────────────────────────────── + // + // Before this, the signed string was "." — no method, no + // path — so every route sharing KEEPER_REGISTER_SECRET accepted every other + // route's signatures. The credential authenticated "someone who knows the + // secret", not "this request". + + it("rejects a signature that was not bound to this endpoint (#2476)", async () => { + // A signature minted for a DIFFERENT path, same secret, same body, same + // minute. Under the old scheme this verified here. + const rawBody = RAW_BODY; + const timestamp = Date.now().toString(); + const otherPath = createHmac("sha256", CORRECT_SECRET) + .update([timestamp, "POST", "/api/markets/SomeSlab", rawBody].join("\n")) + .digest("hex"); + + const { POST } = await import("@/app/api/oracle-keeper/register/route"); + const res = await POST( + new NextRequest("http://localhost/api/oracle-keeper/register", { + method: "POST", + headers: { + "content-type": "application/json", + "x-keeper-timestamp": timestamp, + "x-keeper-signature": otherPath, + }, + body: rawBody, + }), + ); + expect(res.status).toBe(401); + }); + + it("rejects the legacy UNBOUND signature on this endpoint (#2476)", async () => { + // This route's signer lives in the same repo and was migrated in the same + // commit, so it is strict — no legacy acceptance. (markets/[slab] is the one + // that still accepts the legacy form, because its caller is external.) + const rawBody = RAW_BODY; + const { timestamp, signature } = signUnbound(CORRECT_SECRET, rawBody); + + const { POST } = await import("@/app/api/oracle-keeper/register/route"); + const res = await POST( + new NextRequest("http://localhost/api/oracle-keeper/register", { + method: "POST", + headers: { + "content-type": "application/json", + "x-keeper-timestamp": timestamp, + "x-keeper-signature": signature, + }, + body: rawBody, + }), + ); + expect(res.status).toBe(401); + }); }); diff --git a/app/app/api/markets/[slab]/route.ts b/app/app/api/markets/[slab]/route.ts index 0dc27e021..c600ee0db 100644 --- a/app/app/api/markets/[slab]/route.ts +++ b/app/app/api/markets/[slab]/route.ts @@ -434,11 +434,21 @@ export async function PATCH( // Read the body ONCE as raw text — the HMAC covers the exact bytes sent, so it // must be verified before parsing, and re-reading the stream is not possible. const rawBody = await req.text(); + // #2476: bound to this method and path. `req.nextUrl.pathname` rather than a + // literal, because this route is parameterised by slab — a literal would be + // wrong, and reconstructing it would duplicate what the router already has. const signed = verifyKeeperSignature( secret, req.headers.get("x-keeper-timestamp"), rawBody, req.headers.get("x-keeper-signature"), + { method: req.method, path: req.nextUrl.pathname }, + // Transition: this endpoint's signer is NOT in this repo — the keeper + // service has no createHmac at all, so the caller is external and could + // not be updated in the same commit. Accepting the legacy form keeps that + // hop alive across the rollout and logs every use. Drop this argument once + // the warnings stop. See #2476. + true, ); if (!signed) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); diff --git a/app/app/api/markets/route.ts b/app/app/api/markets/route.ts index 1973033bf..3513cbafe 100644 --- a/app/app/api/markets/route.ts +++ b/app/app/api/markets/route.ts @@ -1928,7 +1928,10 @@ export async function POST(req: NextRequest) { const keeperBody = JSON.stringify({ slabAddress: slab_address, mainnetCA: canonicalMainnetCa }); // LAUNCH-16: sign instead of forwarding KEEPER_REGISTER_SECRET as a raw header — // /api/oracle-keeper/register verifies this same HMAC-SHA256 scheme. - const { timestamp, signature } = signKeeperRequest(process.env.KEEPER_REGISTER_SECRET, keeperBody); + const { timestamp, signature } = signKeeperRequest(process.env.KEEPER_REGISTER_SECRET, keeperBody, { + method: "POST", + path: "/api/oracle-keeper/register", + }); const res = await fetch(keeperRegisterUrl, { method: "POST", headers: { diff --git a/app/app/api/oracle-keeper/register/route.ts b/app/app/api/oracle-keeper/register/route.ts index 802046814..2f36cab0d 100644 --- a/app/app/api/oracle-keeper/register/route.ts +++ b/app/app/api/oracle-keeper/register/route.ts @@ -49,11 +49,14 @@ export async function POST(req: NextRequest) { // LAUNCH-16: HMAC-SHA256(REGISTER_SECRET, ".") replaces the raw // x-keeper-secret header — the credential itself never appears on the wire. Verification // is timing-safe and rejects stale/replayed signatures (see verifyKeeperSignature). + // #2476: the signature is bound to THIS method and path, so one issued for + // another route sharing the secret no longer verifies here. const signed = verifyKeeperSignature( REGISTER_SECRET, req.headers.get("x-keeper-timestamp"), rawBody, req.headers.get("x-keeper-signature"), + { method: "POST", path: "/api/oracle-keeper/register" }, ); if (!signed) { return NextResponse.json({ error: "Unauthorized" }, { status: 401 }); @@ -131,7 +134,13 @@ export async function POST(req: NextRequest) { // The keeper verifies: HMAC-SHA256(REGISTER_SECRET, ".") == x-keeper-signature. // This means the raw credential is never sent on the wire regardless of URL scheme. const keeperPayload = JSON.stringify({ slabAddress, mainnetCA }); - const { timestamp: keeperTimestamp, signature: keeperSig } = signKeeperRequest(REGISTER_SECRET, keeperPayload); + const { timestamp: keeperTimestamp, signature: keeperSig } = signKeeperRequest(REGISTER_SECRET, keeperPayload, { + // #2533: the keeper service verifies x-shared-secret, NOT this HMAC, so + // this binding is currently INERT on this hop. Bound anyway, so the + // sender is already correct when the keeper adopts HMAC verification. + method: "POST", + path: "/register", + }); const keeperResp = await fetch(`${KEEPER_URL}/register`, { method: "POST", diff --git a/app/lib/keeper-hmac.ts b/app/lib/keeper-hmac.ts index e232049ae..ff5907d8d 100644 --- a/app/lib/keeper-hmac.ts +++ b/app/lib/keeper-hmac.ts @@ -12,12 +12,54 @@ import { createHmac, timingSafeEqual } from "node:crypto"; /** Reject signatures older than this — bounds the replay window. */ const MAX_SIGNATURE_AGE_MS = 5 * 60_000; +/** The request this signature is for. #2476: without it, one signature is valid + * for the same body sent to any endpoint sharing the secret. */ +export interface KeeperRequestBinding { + /** HTTP method, any casing — normalised below. */ + method: string; + /** Path only: no origin, no query string. */ + path: string; +} + +/** + * The exact string both ends HMAC. + * + * Newline-separated rather than dot-separated: a dot can appear in a path, so + * `"POST./a.b"` and `"POST./a" + ".b"` would be ambiguous under the old + * delimiter. A newline cannot appear in a method or a URL path. + */ +function signedString(timestamp: string, rawBody: string, b: KeeperRequestBinding): string { + return [timestamp, b.method.toUpperCase(), b.path, rawBody].join("\n"); +} + +/** + * #2476: the signed string now covers the HTTP METHOD and PATH, not just the + * timestamp and body. + * + * Without them a signature is valid for the same body sent to a DIFFERENT + * endpoint — every route sharing KEEPER_REGISTER_SECRET accepted each other's + * signatures, so the credential authenticated "someone who knows the secret" + * rather than "this request". Binding the target makes a captured signature + * usable only against the endpoint it was issued for. + * + * NOT fixed here, and #2476 is right that it remains: there is still no nonce, so + * a captured signature is replayable against that same endpoint within + * MAX_SIGNATURE_AGE_MS. A nonce needs a store shared across serverless instances + * — the same constraint that shaped the keeper-register proof (#2505) — so it is + * a separate design problem, not a line change. + * + * `method` is upper-cased and `path` is taken WITHOUT query or origin, so the two + * ends cannot disagree over casing or a trailing host. + */ export function signKeeperRequest( secret: string, rawBody: string, + binding: KeeperRequestBinding, ): { timestamp: string; signature: string } { const timestamp = Date.now().toString(); - const signature = createHmac("sha256", secret).update(`${timestamp}.${rawBody}`).digest("hex"); + const signature = createHmac("sha256", secret) + .update(signedString(timestamp, rawBody, binding)) + .digest("hex"); return { timestamp, signature }; } @@ -25,18 +67,52 @@ export function signKeeperRequest( * Verify a signed request. Timing-safe comparison prevents a signature-length/byte * timing oracle; the timestamp bound prevents replay of a captured signature. */ +/** + * @param allowLegacyUnbound Accept the pre-#2476 `"."` form + * as well as the bound one. + * + * Set this ONLY where the signer is outside this repo and therefore cannot be + * updated in the same commit. It keeps such a hop working across the rollout, + * and it is NOT a fix — an unbound signature is still valid for any endpoint + * sharing the secret, which is the whole of #2476. Each acceptance is logged so + * the transition can actually be finished rather than forgotten; once the logs + * go quiet for a caller, drop the flag at that call site. + */ export function verifyKeeperSignature( secret: string, timestamp: string | null, rawBody: string, signature: string | null, + binding: KeeperRequestBinding, + allowLegacyUnbound = false, ): boolean { if (!timestamp || !signature) return false; const ts = Number(timestamp); if (!Number.isFinite(ts) || Math.abs(Date.now() - ts) > MAX_SIGNATURE_AGE_MS) return false; - const expected = createHmac("sha256", secret).update(`${timestamp}.${rawBody}`).digest("hex"); - const aBytes = Buffer.from(signature, "utf8"); - const bBytes = Buffer.from(expected, "utf8"); - return aBytes.length === bBytes.length && timingSafeEqual(aBytes, bBytes); + const matches = (expected: string): boolean => { + const aBytes = Buffer.from(signature, "utf8"); + const bBytes = Buffer.from(expected, "utf8"); + return aBytes.length === bBytes.length && timingSafeEqual(aBytes, bBytes); + }; + + const bound = createHmac("sha256", secret) + .update(signedString(timestamp, rawBody, binding)) + .digest("hex"); + if (matches(bound)) return true; + + if (!allowLegacyUnbound) return false; + + // Pre-#2476 form. Deliberately checked SECOND, so a caller that has migrated is + // never evaluated against the weaker string. + const legacy = createHmac("sha256", secret).update(`${timestamp}.${rawBody}`).digest("hex"); + if (matches(legacy)) { + console.warn( + "[keeper-hmac] Accepted a LEGACY unbound signature (#2476). The caller has not " + + `migrated; it is not bound to ${binding.method.toUpperCase()} ${binding.path}. ` + + "Remove allowLegacyUnbound at this call site once these stop appearing.", + ); + return true; + } + return false; }