A bound on every redis call, a login that answers at a fixed time, and an admin test that boots the admin - #379
Conversation
444c91e to
add9b89
Compare
Herstelronde 2: beide blokkers dicht, met meetbewijsRebased op Blokker 1 - een teller die zijn tijdvak kwijtraakt, weigert voorgoedBevestigd en breder dan gemeld: 23 call-sites, niet 14. Alle 23 lopen nu via async function incrInWindow(client, key, windowSec) {
const count = await client.incr(key);
await ensureWindow(client, key, windowSec); // EXPIRE ... NX, onvoorwaardelijk
return count;
}
Sabotage. Tegen de vorige revisie ( Tegen deze revisie: 3/3 groen. De derde test pint de Blokker 2 - de vloer dichtte het orakel nietTerecht, en het gemiste deel is drie ordes van grootte breder dan het deel dat wél dicht was. Ik heb de stub-relay de échte throttle uit Vóór (
Ná, zelfde instrument, zelfde 100 requests:
De vertraging is verhuisd naar de enige plek die niet weet of het account bestaat.
Tegen Wat níét dicht is, en in SECURITY.md staat: een adres boven de drempel antwoordt 428 waar een adres eronder 401 antwoordt. Dat blijft een onderscheider voor wie tien mislukkingen plus een proof-of-work per adres wil betalen. Dat is de prijs van beprijzen in plaats van weigeren, en het is een bewuste ruil, geen omissie. Nits
Vervolg, vastgelegd in SECURITY.md
Beide vragen hetzelfde werk (Lua of WATCH/MULTI) en horen in een eigen wijziging. Suites
De drie sabotage-suites ruimen nu op via |
…d an admin test that boots the admin Three findings from the review of #368. 1. #368 bounded one redis read on the TOTP verify path and wrote the rest up in SECURITY.md as open: "every other redis-backed route in relay.js still inherits it and will still hang in an outage". It does not any more, and it turned out to be three problems rather than one. node-redis holds commands on an offline queue while it reconnects and never retries out of it, so against a store that is gone a command neither resolves nor rejects. disableOfflineQueue refuses it instead. That covers a socket that died; it cannot see a connection that stays open and goes silent, where isReady stays true and the reply simply never comes, and that is the outage a dropped firewall rule or a wedged proxy actually produces. Only a per-command deadline catches that one. And a deadline makes the caller safe, not the client: after a command is lost that way node-redis goes on waiting for its reply and holds every later command behind it, so the connection never recovers even once the network does. Measured on 5.12.1 and 6.2.1, it reports itself ready and answers nothing until the process restarts. The guard rebuilds the connection after two unanswered commands in a row, not one, because a single slow command is a large SCAN and not a wedge. The bound sits on the client (lib/redis-deadline.js) rather than on the call sites, because there are about 300 of them across relay.js, relay/lib, envelope.js, admin/server.js and admin/lib, including the sMembers and sRem pair behind /v2/user/consume-backup that #368 bounded on the route next to it and not on this one. A per-call-site list is wrong the first time somebody adds a route. An exceeded deadline, a closed socket and an offline client all arrive as one RedisUnavailableError; redisOutage503 answers it on the 31 route-level catches that used to report an outage as 400 bad_request or 500 internal, a new top-level catch on the request handler answers it for everything else, and the admin's error middleware does the same. That top-level catch is a fix in its own right: http.createServer was handed a 4000 line async callback with nothing behind it, so any throw was an unhandled rejection, which is no answer to the client and a process exit on Node 22. PARAMANT_REDIS_DEADLINE_MS keeps the name #368 gave it and is now the single configuration source for both services. What is given up: during an outage every request pays the deadline once before it is refused, and a deployment whose redis routinely takes longer than a second must raise the knob rather than remove it. The health routes were part of the same blind spot. /v2/health/deep listed eight checks and never mentioned the store that holds every TOTP secret, so it stayed green through an outage. It now carries a redis check. The admin had no health route at all: its container probe was GET /api/auth/check, which answers 401 when nobody is signed in, so healthy meant the process still refuses me. It now has GET /health, always 200, saying degraded when redis cannot be reached. The relay's own /health touches no redis and still answers 200, which is correct. 2. POST /api/user/login did more work for an address that exists than for one that does not: a second relay call and one more redis read. The status codes were already identical, deliberately, but the clock was not. Measured on a booted admin over 200 requests per case, interleaved, one source address each, with the relay stub given a realistic 3 ms cost for the verify call, the median was 6.44 ms against 2.20 ms and the two ranges did not overlap at all, so one request classified an address with no credentials at all. Every credential answer is now held to t0 plus PARAMANT_LOGIN_MIN_ANSWER_MS, default 250 ms, and the not-found branch makes the same number of redis calls as the found one. The same measurement afterwards reads 251.37 against 251.27 ms with the ranges fully overlapping. The 429 and the 428 are not padded: their status codes tell them apart whatever the clock says, and holding the 428 back would only delay the page that is waiting to start hashing. 3. admin/test/login-ratelimit.test.js drove the limiter module through an attemptLogin() helper written inside the test file and then read server.js as a string to check the order of three calls. Both halves are worth keeping and neither ever ran the handler, so the suite could pass while the route was wrong. admin/test/_admin-server.js is the counterpart of relay/test/_relay-server.js: it spawns the real admin/server.js against a stub relay and speaks HTTP to it. login-http.test.js runs the reviewer's scenario on that, ten wrong codes on one address from three source addresses followed by the owner solving a real 2^18 proof-of-work, plus the per-IP refusal, the IP refund on a priced attempt and the relay 503 being passed through. Checked against the code it is meant to catch: three of its five tests fail against the pre-#368 admin, and so does login-timing.test.js. The admin CI job now gets a redis service and npm ci, because until now it installed nothing, which is the reason every admin suite was a lib test or a source-text assertion in the first place.
…, and a delay that only existing accounts paid 1. The redis deadline made a latent bug in every rate limiter reachable in a single request. All of them were INCR followed by a conditional expiry, `if (count === 1) await expire(key, WINDOW)`, which is correct only while those two commands always happen together. If the INCR outlives the deadline while the server still executes it, the caller gets an outage and the expiry is never sent; the next INCR returns 2, so no later call sets it either, and TTL -1 is for ever. Measured on a booted admin with the replies from redis dropped for the duration of one login: the per-IP counter stood at 9 with TTL -1 and that source address kept getting 429 until the key was deleted by hand. The same shape stranded the login failure counter, which is a proof-of-work bill that never lifts on an address anybody may name, and the monthly counters in relay/lib/quota.js, which is an account permanently over its limit. All 23 INCR call sites in both services now go through lib/redis-counter.js, which sets the expiry unconditionally after every INCR, with NX so it can only create a window and never slide one. The healthy case is unchanged; a lost window is repaired by the first request that lands after it instead of never. EXPIRE NX needs Redis 7.0 and docker-compose pins 7.4.8 by digest, but a server that refuses the option makes the helper fall back to a TTL read for the life of the process, because an error on every rate-limited route would be worse than the bug. ratelimit-ttl.test.js reproduces it against a real admin behind a proxy that delivers commands and drops replies, which is the only sabotage that strands a counter: a full outage never executes the INCR either. Against the previous revision both counters come back TTL -1. 2. The fixed floor did not close the existence oracle, and the part it missed was three orders of magnitude wider than the part it caught. relay.js sleeps userMfaDelayMs before it checks a code, 250 ms per failure past ten up to two seconds, and only a request naming an account that exists ever reaches that sleep. Twelve wrong codes from rotating source addresses put an address there, and nothing refuses them, because the per-address counter deliberately prices rather than denies. Measured over 100 requests per case with the stub relay charging the same throttle: at twelve prior failures 509.91 ms against 251.61, at twenty 2010.23 against 251.82, ranges not overlapping either time. The 503 totp_unavailable answer was unfloored as well and is only reachable for an address that has an account. The delay now belongs to the admin, which owns a counter keyed on the hashed address and increments it for a miss exactly as for a hit, so it cannot know whether the account exists. loginRate.mirrorThrottleMs reproduces the relay's curve on that counter, the result is added to the floor under every credential answer on both login routes including the 503, and the admin tells the relay it has already paid. The relay still counts the failure, still reports what it would have charged as throttle_ms, and still charges any caller that does not set the flag; the route is X-Internal-Auth only, so the callers who can set it are callers who could already name any account they like. Same instrument after: 251.86 against 251.87 at zero failures, 751.84 against 751.76 at twelve, 2251.90 against 2252.11 at twenty, all overlapping. login-timing.test.js now runs at all three levels with the shipped default floor and the real throttle values, pays a real proof-of-work for the levels above the threshold, and pins the flag itself; it fails against the previous revision on the twelve-failure level. The .env.example entry says what the default covers and what is added on top, and an answer that overruns its floor is logged, because at that point the floor has stopped being one. /health no longer repeats the deadline error, which carried the configured bound in its text on an unauthenticated route. SECURITY.md carries both findings and two follow-ups that are recorded rather than fixed here: consumeBackupCode can spend a code while the caller sees 503, because the read, the argon2 verification and the removal are three bounded calls and not one atomic one, and regenerateBackupCodes is a DEL followed by a SADD with nothing between them.
add9b89 to
4191f18
Compare
Herstelronde 3: de backup-code-route, en de twee kleinere puntenRebased op Blokker -
|
| p50 | min | max | |
|---|---|---|---|
| één verificatie | 49.7 ms | 44.7 ms | 95.9 ms |
| tien (één foute code) | 494.2 ms | 462.3 ms | 870.9 ms |
Een vloer van 250 ms zit daar dus ver onder. Jullie meting klopt, en de admin schreeuwde het al: answer overran its floor, 40 van de 40.
Twee wijzigingen.
- Een eigen vloer:
PARAMANT_LOGIN_BACKUP_MIN_ANSWER_MS, default 1500 ms. Dat is 1,7x de traagste tien-hash-misser die ik hier meet en 3x de mediaan. Een tragere machine heeft meer nodig en zegt dat ook, want elke overschrijding wordt gelogd. - De mirror-throttle op de bestaande
bk:email:<hash>-teller, bovenop de vloer. Die teller wordt opgehoogd vóórfindUserByEmail, dus hij is per definitie hetzelfde getal voor een treffer en een misser.
Waarom die route een eigen curve krijgt en niet mirrorThrottleMs. De route weigert bij vijf pogingen per adres per venster, dus de drempel van tien uit auth-throttle.js is er onbereikbaar: een mirror op die drempel zou nul zijn voor élke poging die de route toestaat, en dat is geen mirror. backupThrottleMs start daarom na de eerste poging, met dezelfde stap (250 ms) en hetzelfde plafond (2000 ms). webauthn.rateHit heeft er een rateHitCounted naast gekregen die de telling teruggeeft; rateHit delegeert.
Meetbewijs. login-timing.test.js boot voor deze route een echte relay in plaats van de stub, want het orakel ís de argon2-kost en een stub zou de stub meten. Hij enrolt een account, controleert dat er echt tien hashes staan (sCard), en meet dan een foute code tegen bestaand en afwezig, op 0 en 4 eerdere pogingen. Vijf eerdere pogingen is een 429 voor béíde gevallen, dus geen credential-antwoord; daarom 4 en niet 5.
Tegen de vorige revisie (57866599):
not ok 3 - the backup-code route answers in the same time whether the address has an account or not
error: 'exists must be held to its floor of 1500ms; at 0 prior attempts:
exists p50=491.99 [487.85, 625.41], absent p50=253.16 [251.39, 253.49]'
Tegen deze revisie: 4/4 groen, 19 checks. De test faalt ook als de admin ook maar één keer answer overran its floor logde, dus de vloer wordt niet stilzwijgend te laag.
login-http.test.js dekt de route nu ook: verdicten, de per-adres-limiet (zesde poging 429), de sessiecookie, en de assertie dat élke consume-backup-aanroep throttled_upstream meestuurt.
Kleinere punten
email als object gaf 500 in 1 ms. {"email": {}} is truthy, dus if (!email) liet het door naar String(email).trim().toLowerCase(), wat gooide. Beide loginroutes controleren nu het type (ook totp en backup_code) en antwoorden 400 missing_fields, hetzelfde antwoord voor iedereen. Sabotage tegen de vorige revisie:
not ok 7 - a body that is not what it says it is gets a 400, not a 500
error: '/user/login with email={} must be a 400, got 500 {"error":"internal_error"}'
"23" gecorrigeerd naar 18 in SECURITY.md en de changelog, met de uitsplitsing: admin/lib/login-ratelimit.js (2), admin/lib/webauthn.js (1), admin/server.js (8), relay/lib/quota.js (6), relay/relay.js (1). De 23 telde de twee kopieën van de helper mee en telde admin/server.js verkeerd. De PR-body hierboven is bijgewerkt.
Het 503-restorakel, vastgelegd
Staat nu in SECURITY.md als eigen kopje. Kort: als alléén de relay-store stuk is, antwoordt een adres mét account 503 totp_unavailable (de fail-closed replay-guard, doorgegeven) en een adres zónder account 401, omdat dat tweede de relay nooit bereikt. De vloer maakt ze even snel; de statuscodes verschillen. In de verscheepte topologie is dat niet bereikbaar, want docker-compose.yml geeft beide diensten dezelfde redis en dan kan de admin sowieso geen login afhandelen. Het wordt bereikbaar zodra iemand ze splitst, en dat hoort geen verrassing te zijn voor wie dat doet.
Suites
| suite | ronde 2 | nu |
|---|---|---|
tests/static-sanity.sh |
PASS, 11 checks, 127 suites | PASS, 11 checks, 130 suites |
npx eslint@9 . |
schoon | schoon |
| relay unit | 199 | 199 pass, 0 fail |
| relay route (echte redis) | 91 | 91 pass, 0 fail |
| admin (echte redis) | 63 | 67 pass, 0 fail (~1m50s) |
| root integration | 176 | 190 pass (2 skipped), 0 fail |
| relay boot | 11 | 11 pass, 0 fail |
De admin-CI-job draait dit met een redis-service en npm ci; login-timing heeft daarnaast relay/node_modules nodig voor de echte argon2 en zegt dat bij naam als het ontbreekt (ADMIN_TEST_SKIP=relay).
…loor of 250 ms was nowhere near it
The blocker from the third review. /api/user/login-with-backup got the floor
from the previous round and nothing else, and on that route the work is not four
milliseconds of redis reads. consumeBackupCode verifies the provided code
against every stored hash until one matches, and a wrong code matches none, so a
miss costs ten full argon2id verifications at 64 MiB and timeCost 3. Measured on
this machine with ten codes really stored: one verification p50 49.7 ms, ten of
them p50 494.2 ms with a max of 870.9 ms. An address with no account pays none
of it. Through the route that read as 472.7 ms against 251.6 ms with the ranges
not overlapping, and the admin logged "answer overran its floor" on forty
requests out of forty, which is the code saying out loud that the number it had
been given was not a floor.
Two changes. The route has a floor of its own,
PARAMANT_LOGIN_BACKUP_MIN_ANSWER_MS, 1500 ms by default, which is 1.7x the
slowest ten-hash miss measured here and 3x the median; a slower machine needs
more and says so on every overrun. And the per-address throttle is mirrored onto
this route as well, on the counter it already keeps for the address, which is
incremented before the account lookup and is therefore the same number for a hit
and a miss. It needs its own curve rather than mirrorThrottleMs: the route
refuses at five attempts per address per window, so the relay's threshold of ten
is unreachable through it and a mirror built on that threshold would be zero for
every attempt the route allows. It starts after the first attempt instead, with
the same step and the same ceiling.
login-timing.test.js measures this case against a REAL relay rather than the
stub, because the oracle here is the argon2 cost and a stub would be measuring
the stub. It enrols an account, checks that ten hashes are really stored, and
compares a wrong code against an address that exists and one that does not, at
zero and four prior attempts. Five prior attempts is a 429 for both cases, which
is not a credential answer. It also fails if the admin logged a single floor
overrun. Against the previous revision it reads 491.99 ms against 253.16 ms with
no overlap. login-http.test.js now covers the route's wiring as well: the
verdicts, the per-address ceiling, the session cookie, and the flag that keeps
the relay from charging its account-keyed delay a second time.
Two smaller things from the same review. A request body whose email is not a
string answered 500 in about a millisecond, because {"email": {}} is truthy and
`if (!email)` waved it through into String(email).trim().toLowerCase(), which
threw: an unhandled throw on an unauthenticated route, and the fastest answer
either login handler had. Both routes check the type now and answer 400. And the
count of INCR call sites in SECURITY.md and the changelog said 23, which counted
the two copies of the helper itself and miscounted admin/server.js; it is 18.
SECURITY.md also records a residual that belongs to the deployment rather than
to the code: if the relay's redis is unreachable while the admin's is not, an
address with an account answers 503 and one without answers 401, because the
second never reaches the relay. docker-compose gives both services the same
store, so it is not reachable as shipped, but splitting them turns an outage
into an enumeration oracle and that should not be a surprise to whoever splits
them.
The real relay that case boots will not start without @paramant/core, which is a
file-link to the sibling repo and is built in exactly one CI job. So the admin
job declares it as a skip by name and the relay-crypto job, which has the
binding, runs that suite for real and fails if it skips there. Without the
declaration the case is a hard failure, so it cannot quietly stop running in
both jobs at once.
4191f18 to
2eb5ce6
Compare
Meetbewijs
|
| eerdere pogingen | geval | p50 | min | max | status |
|---|---|---|---|---|---|
| 0 | bestaand | 507.33 ms | 476.02 | 536.93 | 401 |
| 0 | afwezig | 252.03 ms | 251.24 | 253.96 | 401 |
| 0 | delta p50 255.30 ms, ranges overlappen niet | ||||
| 4 | bestaand | 483.21 ms | 474.10 | 489.40 | 401 |
| 4 | afwezig | 252.30 ms | 250.75 | 252.64 | 401 |
| 4 | delta p50 230.91 ms, ranges overlappen niet | ||||
| 5 | beide | 2.19 / 2.41 ms | 429 |
answer overran its floor gelogd: 18 keer (van de 32 credential-antwoorden).
Ná, zelfde instrument:
| eerdere pogingen | geval | p50 | min | max | status |
|---|---|---|---|---|---|
| 0 | bestaand | 1502.38 ms | 1501.23 | 1503.47 | 401 |
| 0 | afwezig | 1503.31 ms | 1500.56 | 1503.67 | 401 |
| 0 | delta p50 -0.92 ms, ranges OVERLAPPEN | ||||
| 4 | bestaand | 2252.16 ms | 2251.29 | 2253.03 | 401 |
| 4 | afwezig | 2253.15 ms | 2250.65 | 2253.62 | 401 |
| 4 | delta p50 -0.99 ms, ranges OVERLAPPEN | ||||
| 5 | bestaand | 4.01 ms | 2.30 | 4.51 | 429 |
| 5 | afwezig | 3.42 ms | 2.28 | 4.73 | 429 |
| 5 | delta p50 0.59 ms, ranges overlappen |
answer overran its floor gelogd: 0.
Waarom 4 en niet 5 als "eerdere mislukkingen" in de assertie: bij vijf eerdere pogingen is de zesde een 429 voor béíde gevallen, dus geen credential-antwoord. De tabel laat dat niveau zien zodat het zichtbaar is dat ook dáár niets uiteenloopt. 1502 en 2252 zijn de vloer (1500) plus de mirror-throttle (0 respectievelijk 750 ms bij vier eerdere pogingen).
CI
Alle checks groen op 2eb5ce67. De backup-case draait écht in relay - crypto suite, de enige job met @paramant/core gebouwd:
12 success Install admin dependencies
13 success Admin login timing against a real relay
ok 3 - the backup-code route answers in the same time whether the address has an account or not
# login-timing: 19 checks passed # pass 4 # fail 0
De admin - unit suite job declareert die ene case als ADMIN_TEST_SKIP=relay (relay.js start niet zonder de binding) en de crypto-job faalt expliciet als hij dáár zou skippen, zodat hij niet in beide jobs tegelijk stil kan vallen.
…e bar is measured on three pages Review of #392 found the hole in the first round. developer.html keeps its own navigation (KEEP_OWN_NAV in frontend/apply-nav.py), is stamped by hand, and carries a hamburger. The round moved Sign in and Help out of the bar below 700px and into a strip under the drawer, and that page never got a strip. So on a 390px screen both links measured 0x0 there, where main still had them in the bar. A page the generator does not own is a page a generated fix does not reach. Three things, because one of them alone would leave the same gap open. The markup. developer.html carries the strip now, like every stamped page. The script. nav.js builds the strip when a page has a menu button and no strip. A page can forget the markup; this cannot forget to build it. That is a floor under the live site, not a substitute for the markup, which is why it comes with the check below rather than instead of it. The check. tests/navigation-shell reads every .html in frontend/, not only the 51 the generator writes, and fails on any page that has id="nav-hamburger" without id="nav-mobile-tail". 52 pages carry the button. Taking the strip back out of developer.html turns it red and names the file. And the measurement moved. The three phone-bar checks ran on the homepage alone, which is exactly why they were green while /developer was broken. They run on /, /pricing and /developer now, each with a fourth check that the strip really holds /auth/login and /help at 44px or more. /developer measures logo x16-95, Create account x181-318, menu x330-374, gaps 86 and 12, all heights 44. 39 checks, all green. Rebased on main with #379, #390 and #391.
2 participants
Three findings from the security review of #368. Built on top of it, rebased onto
mainafter it merged.1. No redis call in either service can hang
#368 bounded one read on the TOTP verify path and recorded the rest in SECURITY.md as open: "every other redis-backed route in relay.js still inherits it and will still hang in an outage". Closing it turned out to need three things, not one.
The offline queue. node-redis holds commands in memory while it reconnects, and the default strategy retries for the life of the process. Measured against redis 5.12.1 (relay) and 6.2.1 (admin), with a live connection cut underneath the client:
disableOfflineQueue: trueA connection that goes silent. Socket stays open, bytes stop. Nothing fails:
isReadystaystrue, the command goes out, the reply never comes.disableOfflineQueuecannot see this, and it is the outage a dropped firewall rule or a wedged proxy actually produces. Measured on both versions, withdisableOfflineQueueon: pending after five seconds, and after any bound worth measuring. Only a per-command deadline catches it. So both, and neither is redundant.A bound makes the caller safe, not the client. Third measurement, and the one I did not expect: after a command is lost to a silent connection, node-redis keeps waiting for its reply and holds every later command behind it. Hole the connection, let one command be swallowed, then let the bytes flow again, and the client answers nothing, ever, while reporting itself ready.
destroy()+connect()rebuilds it in about 3 ms. The guard does that after two unanswered commands in a row, not one, because a single slow command is a bigSCANor a loaded server and tearing the socket down for it would turn a slow minute into a broken one.Where the bound lives. On the client, not on the call sites:
lib/redis-deadline.jsexportsguardRedisClient, a proxy that puts the deadline on every command, everyMULTIchain and everyscanIteratorstep. There are roughly 300 redis call sites acrossrelay.js,relay/lib/*,envelope.js,admin/server.jsandadmin/lib/*-- including thesMembers/sRempair inconsumeBackupCodebehind/v2/user/consume-backup, which #368 bounded on the route next to it and not on this one. A per-call-site list is wrong the first time somebody adds a route.An exceeded deadline, a closed socket and an offline client all arrive as one
RedisUnavailableError, and three places turn it into 503:redisOutage503in relay.js, one line at the top of the 31 route-level catches that used to answer 400bad_requestor 500internalfor an outage;http.createServerwas handed a 4000-line async callback with nothing behind it, so a throw was an unhandled rejection, which is no answer to the client and a process exit on Node 22. It was survivable only because a dead redis hung instead of throwing;PARAMANT_REDIS_DEADLINE_MSkeeps the name #368 gave it and is now the single configuration source for both services.The health routes were part of the same blind spot.
/v2/health/deeplisted eight checks and never mentioned the store that holds every TOTP secret, so it reported the same green as a healthy relay. It carries aredischeck now. The admin had no health route at all: its container probe isGET /api/auth/check, which answers 401 when nobody is signed in, so "healthy" meant "the process still refuses me". It now hasGET /health, always 200, withstatus: "degraded"when redis cannot be reached. The relay's own/healthtouches no redis and still answers 200, which is correct.Sabotage evidence.
relay/test/route-redis-outage.test.jsandadmin/test/redis-outage.test.jsboot a real relay and a real admin against a real redis behind a TCP proxy the suite owns, then break it two ways. Deadline set to 400 ms for the suites, which also pins the knob.What was given up. During an outage every request pays the deadline once before it is refused, where before it paid nothing and answered nothing. A deployment whose redis routinely takes longer than a second must raise the knob rather than remove it. There is no retry and no open circuit: this buys a bounded answer, not throughput during an outage.
2. The login page told you which addresses were customers
POST /api/user/logindid strictly more work for an address that exists: a second relay call (/v2/user/verify-totp) and one more redis read. The status codes were already identical on purpose -- #368 folded three 403s into one 401 for exactly this reason -- but the clock was not.Measured on a booted admin, 200 requests per case, interleaved, one source address each,
admin/test/login-timing.bench.js. The stub relay is given a cost for the verify call, because the size of the oracle is exactly the cost of that call.Before (
ADMIN_SERVER_JSpointed at the mergedmain):In the 3 ms row the distributions do not overlap: the fastest hit (4.59 ms) is slower than the slowest miss (3.73 ms). One request classifies an address, under the rate limit, with no credentials. The review measured 5.40 against 2.95 ms on the production pair; same channel, slower machine here.
After, same instrument, same 200 requests:
No credential answer leaves before
t0 + PARAMANT_LOGIN_MIN_ANSWER_MS(default 250 ms), so what an attacker times is a constant this code sets. The not-found branch also makes the same number of redis calls as the found one, so the floor is a margin rather than the only thing holding the two paths together. A fixed added delay would not have worked: it moves both curves and keeps the distance between them.The 429 and the 428 are deliberately not padded. They are not credential answers, their status codes tell them apart whatever the clock says, and holding the 428 back would only delay the login page that is waiting to start hashing.
What this does not fix, said out loud: an address over the failure threshold answers 428 where one under it answers 401, so an attacker willing to burn ten failures per address can still tell them apart by status code. That is the price of pricing an attempt rather than refusing it, and it is a far more expensive oracle than 4 ms.
admin/test/login-timing.test.jsmeasures both cases against a booted admin and asserts the medians do not separate and the ranges overlap. Against the pre-fix admin it fails withexists p50=7.86 [6.62, 12.40], absent p50=3.22 [1.81, 6.19].3. A test that reimplemented the handler it was testing
admin/test/login-ratelimit.test.jsdrovelib/login-ratelimit.jsthrough anattemptLogin()helper written inside the test file, then readserver.jsas a string to assert the order of three calls. Both halves are worth keeping. Neither ever ran the handler, so the suite could pass while the route was wrong, which is the reviewer's objection.admin/test/_admin-server.jsis the admin counterpart ofrelay/test/_relay-server.js: it spawns the realadmin/server.js, points it at a stub relay answering the two routes a login touches, and speaks HTTP to it. Redis is real; the admin process is real and unmodified; only the relay is stubbed, and it can be made to answer 503 on demand.admin/test/login-http.test.jsruns the reviewer's scenario on that:GET /api/captcha/challenge, and gets a session with a cookie;totp_unavailableand is not counted against the address.Checked against the code it is meant to catch. Run against the pre-#368 admin (
b913f9e0), three of its five tests fail:The admin CI job gets a redis service and
npm ci. It installed nothing until now, which is the reason every admin suite was a lib test or a source-text assertion in the first place, and it now fails on a silent suite the way the relay jobs do.Test runs
tests/static-sanity.shnpx eslint@9 .Notes for the reviewer
relay/lib/redis-deadline.jsandadmin/lib/redis-deadline.js, byte-identical. The two services are separate npm projects on different major versions of the redis client and each Dockerfile copies only its ownlib/, so a shared module one directory up would resolve in a checkout and be missing in the container.tests/redis-deadline-parity.test.mjsfails if the copies drift and also asserts both services really wrap their client.relay/test/auth-throttle.test.jshad two source assertions pinningPARAMANT_REDIS_DEADLINE_MSand the fallback rule insiderelay.js. The declaration moved into the module, so the pin follows it; the property asserted is unchanged.unref'd, and there is a comment saying why. It was, briefly, andrelay/test/redis-deadline.test.jscaught it: a deadline that does not hold the event loop open does not fire when nothing else is running, which reintroduces the exact hang inside the mechanism meant to prevent it.deploy/.env.examplewithread in:pointers, whichtests/env-documented.test.mjsverifies.admin/test/login-timing.bench.jsis the instrument, not a test.ADMIN_SERVER_JS=/path/to/checkout/admin/server.js node admin/test/login-timing.bench.jsreproduces every number in section 2.