fix(gateway): pin the MAC length, and stop refunding the credit pool - #1
Conversation
Two defects in the money path, both found by reading DESIGN.md's guarantees back against the code. **A one-byte signature verified any payload.** split() accepted a MAC of any length from 1 to 32 bytes and compared it against its own prefix of the real one, so 256 guesses forged a token for an attacker-chosen subject without the secret — 93 in practice. That bypasses the mint entirely, which makes the per-subject quota unenforceable, and because the subject is chosen it also defeats the user_id KV-cache isolation and safety attribution that DESIGN.md names as the reason user_id is overridden rather than honoured. The length tolerance existed only because challenges carry 16 bytes and tokens carry 32. Both callers already know which they expect, so the expected length is now a parameter and the comparison is equality. **The lifetime credit pool reset on restart.** priorSpend only advanced at an in-process rollover, so a gateway stopped on one day and started on the next came back believing that day's money was never spent. Every deploy or reboot across midnight silently refunded DSGATE_TOTAL_BUDGET_USD. The daily breaker was unaffected, so loss stayed bounded per day, but the credit pool — the thing meant to stop a bad invoice — was not enforced across restarts. state.json already recorded through_day; it was written but never used. Open() now folds in every journal from [through, today) before replaying today's, and the fold is idempotent, so repeated restarts and a pre-existing state file both come out right. Tests: 13 new cases. The forgery search is the one that matters — it walks all 256 one-byte MACs against a chosen subject and fails if any verifies. Journal folding is covered for multi-day downtime, repeated restarts, an already-folded state file, a missing state file, and a journal torn by a crash mid-write. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewBoth defects are real. I reproduced each one against the pre-fix source rather than taking the description on trust, and the fixes are correct. Verifying #1Wrote a throwaway probe in the The severity framing is right, and worth restating because it is easy to read "the budget breaker still bounds the money" as "so it is minor". The breaker bounds the invoice. What a forgeable subject destroys is everything layered on identity: proof-of-work becomes optional, per-subject quota becomes unenforceable, and — the part I would put first — Making the length a parameter is the right shape. Verifying #2The seven new The invariant holds where it matters: One thing this does not cover
func TestProbeBackwardClockDoubleCounts(t *testing.T) {
dir := t.TempDir()
writeJournal(t, dir, dayOffset(0), 3.00)
writeState(t, dir, 10.00, dayOffset(1)) // prior already covers today
l, done := open(t, dir, poolLimits())
defer done()
if got := l.Health().TotalSpendUSD; got != 10 {
t.Errorf("lifetime spend = $%.2f, want $10.00 — today's $3.00 counted twice", got)
}
}
// lifetime spend = $13.00, want $10.00
Not a blocker, and I would not hold the PR for it: it needs a snapshot restore or a bad NTP step to reach, and it errs toward refusing service rather than spending money — the opposite direction from the bug being fixed. A monotonic guard ( VerdictShip it. Both are genuine defects in the money path, the diagnosis is accurate in each case, and the tests ask the right questions rather than merely adding to the count — (Posted as a comment: GitHub will not let the PR author approve their own PR.) |
Two defects in the money path. Both are in code that already has good tests — the gaps were in what the tests asked, not how many there are.
1. A one-byte signature verified any payload
token.split()accepted a MAC of any length from 1 to 32 bytes and compared it against its own prefix of the real one:So a one-byte MAC only had to match one byte. Walking all 256 forges a token for an attacker-chosen subject, with no knowledge of the signing secret — 93 tries in practice:
Two consequences:
user_idis overridden rather than honoured; a forgeable subject defeats both.The length tolerance existed only because challenges carry a truncated 16-byte MAC and tokens carry the full 32. Both call sites already know which they expect, so the expected length is now a parameter (
challengeMACLen,tokenMACLen) and the comparison is equality.2. The lifetime credit pool reset on restart
priorSpendonly ever advanced at an in-process day rollover. A gateway stopped on one day and started on the next came back believing that day's money had never been spent:Every deploy or reboot crossing UTC midnight silently refunded
DSGATE_TOTAL_BUDGET_USD. The daily breaker was unaffected, so loss stayed bounded per day — but the credit pool, the thing meant to stop a bad invoice, was not enforced across restarts.state.jsonalready recordedthrough_day; it was written and never read.Open()now folds in every journal from[through, today)before replaying today's, and records how far it got. The existingTestLifetimeSpendSurvivesTheDayRollingpasses because the rollover it tests happens in-process — this is the other half.Tests
13 new cases, all failing before the fix.
TestCannotForgeTokenBySearchingShortMACsis the one that matters: it walks all 256 one-byte MACs against a chosen subject and fails if any verifies. Plus truncated and over-long MACs on both credential types, and domain separation.Journal folding is covered for multi-day downtime, repeated restarts (idempotence), an already-folded state file, a missing state file, and a journal torn by a crash mid-write.
make fmt-check vet gateway-test price-checkpasses, including-raceand the CLI-against-real-gateway interop test.🤖 Generated with Claude Code