fix(api-gateway): enforce wallet challenge expiry server-side - #581
Open
Andreschuks101 wants to merge 1 commit into
Open
fix(api-gateway): enforce wallet challenge expiry server-side#581Andreschuks101 wants to merge 1 commit into
Andreschuks101 wants to merge 1 commit into
Conversation
Wallet auth challenges lived in a per-process Map. Nothing evicted an entry that was never verified, no other gateway instance could see it, and a challenge survived every failed verification attempt — so one outstanding challenge could be guessed at indefinitely, and the only expiry check ran against state that a restarted or peer instance did not share. - Add src/wallet-challenge-store.ts: challenges are stored in Redis under a TTL matching their recorded deadline, so an unclaimed challenge is reaped instead of accumulating forever. - consume() reads and deletes in one atomic GETDEL, so a challenge is usable exactly once whether or not the signature turns out to be valid, and two concurrent verifications cannot both claim it. - Re-check the recorded expiresAt against the server clock, so a record that outlives its TTL (clock skew, an unreaped key, a restored snapshot) is rejected rather than accepted. - POST /api/auth/challenge and POST /api/auth/verify use the store and report 503 when it is unreachable, so a store failure is never mistaken for a valid or an invalid challenge. - Add src/wallet-challenge-expiry.test.ts: TTL storage, expiry by TTL and by deadline, deadline boundary, single use, atomic consumption, corrupt records, and the routes end to end.
Contributor
|
kindly resolve conflict |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
closes #554
Problem
wallet-auth-challenge.test.tsdescribes a Redis-backed challenge flow with a TTL, but the server did not implement one. Challenges lived in a per-processMap:Three things followed from that:
Date.now() > challengeInfo.expiresAtis the right check, but it ran against a Map that was neither durable nor shared, so in a multi-instance deployment it was routinely bypassed by simply not being reachable.Solution
A new module,
services/api-gateway/src/wallet-challenge-store.ts, backs challenges with Redis:issue(address)mints a 32-byte challenge and writes it withPXset to the same TTL as the recordedexpiresAt. Redis reaps unclaimed challenges on its own, so nothing accumulates and a stale challenge disappears even if nobody comes back for it. Re-issuing replaces the outstanding challenge for that address.consume(address)reads and deletes in one atomicGETDEL. The challenge is gone whatever happens next — valid signature, invalid signature, malformed request — so it is single-use and there is nothing left to guess against. Two concurrent verifications cannot both claim it.expiredrather than being accepted. Defence in depth: Redis expiry is the primary mechanism, the deadline check is the one that cannot be defeated by the store misbehaving.In
index.ts,POST /api/auth/challengeandPOST /api/auth/verifyuse the store. Both return503 Authentication service unavailablewhen Redis is unreachable, so a store failure can never be mistaken for a valid challenge (fail-open) or for an invalid one (a confusing 400). The verify route's response codes and messages are otherwise unchanged:400 Challenge not found or expired,400 Challenge expired,401 Invalid signature.The clock and the Redis surface are both injectable, so expiry is tested without waiting five minutes and without a live Redis.
Expired-challenge rejection
Two distinct expiry paths, both proven. First, the deadline check — the record is deliberately left in place while the clock moves past its deadline:
Second, the Redis TTL:
And through the real routes:
Single use
Assertions 28-31 are the behaviour change that closes the guessing window: previously a wrong signature left the challenge sitting there for the next attempt.
TTL storage, and store failures
Verification
Also re-ran the suites that touch this area:
wallet-auth-challenge(7/7),auth-security(45/45),rate-limit-headers(9/9),error-response(7/7).The new test file is wired into the
api-gatewaytest script so it runs in CI.Notes for reviewers
GETDELneeds Redis 6.2 or newer. If older servers are still in the fleet, the same atomicity can be had with a two-commandMULTIor a one-line Lua script — say the word and I will swap it.wallet-auth-challenge.test.tsbuilds its own miniature app rather than exercising the gateway's routes, so it keeps passing unchanged. Its premise — a Redis-backed, TTL-bearing store shared across instances — is now what the server actually does, so it could be folded into the new file in a follow-up if you would like the duplication gone./api/auth/wallet/verifyroute already has its own replay protection (used_nonce:*) and is untouched by this change.