Repository navigation
Conversation
There was a problem hiding this comment.
🟡 Changes recommended
The throttle test returns without draining five pending requests, allowing password checks and logging to spill into subsequent tests and potentially race with replacement of the package-wide KDF gate.
1 open issue
What changed in this PR
This PR strengthens the identity provider’s seed-account handling, password-guess throttling, and token revocation persistence, building on the earlier HTTP and sign-in hardening.
Changes:
- Bind seed password updates to both account ID and username, with migration guidance.
- Reserve password-check attempts before verification and share the cooldown with
/password. - Add optional file-backed revocations with bounded loading, atomic writes, and regression tests.
| File | Description |
|---|---|
| README.md | Documents configuration, migration steps, and security limits. |
| pkg/user/user.go | Enforces seed identity matching and exposes a password-error sentinel. |
| pkg/user/setpassword_test.go | Checks the password-error sentinel. |
| pkg/user/seed_test.go | Tests seed identity matching and required IDs. |
| pkg/user/persist_test.go | Adds stable IDs to seed persistence fixtures. |
| main.go | Validates seed IDs and configures revocation persistence. |
| main_test.go | Tests revocations across restarts and seed configuration validation. |
| internal/tokenstore/tokenstore.go | Adds bounded file loading and atomic revocation persistence. |
| internal/tokenstore/tokenstore_test.go | Tests persistence, write failures, size limits, and concurrent writes. |
| internal/throttle/throttle.go | Reserves attempts and preserves entries with checks in progress. |
| internal/throttle/throttle_test.go | Tests burst limits, success handling, and eviction. |
| internal/routes/throttle.go | Updates cooldown logging and centralizes retry headers. |
| internal/routes/throttle_test.go | Tests shared cooldowns, concurrent bursts, and busy refusals. |
| internal/routes/routes.go | Adds password-check hooks for concurrency tests. |
| internal/routes/root.go | Reserves login attempts before password verification. |
| internal/routes/password.go | Applies the shared cooldown to current-password checks. |
| internal/routes/oidc_test.go | Tests revocation behavior when persistence fails. |
| CLAUDE.md | Updates contributor documentation for the security changes. |
Review effort: Balanced
ahma
force-pushed
the
fix/http-and-login-hardening-on-chain
branch
from
October 29, 2026 20:51
fbb12a7 to
360b49d
Compare
ahma
added a commit
that referenced
this pull request
Oct 29, 2026
TestLogin_CooldownBurstRefusesTheRightPassword released the five held password checks and returned without reading their answers, so their derivations and cooldown log lines ran on into later tests, two of which replace the KDF gate (SetKDFGate forbids derivations in flight) or capture the default logger. Copilot review on #40.
ahma
force-pushed
the
fix/seed-identity-cooldown-revocation
branch
from
October 29, 2026 20:51
368317c to
7388561
Compare
With users.updateSeedPasswords the start matched seed entries by username only. After an admin deleted a seed account and someone else registered the same username, the next start put the seed hash on the new owner's account, so whoever knew the seed password took it over. A seed entry now applies only to the account with both its ID and its username; any other match is skipped with a warning that names the username and seed ID, never a hash. A generated ID changes at every start, so the option now requires an ID on every seed entry: user.New refuses one without, and config validation refuses it before any store is built. Without the option ID-less entries still seed a new database as before. The README tells an operator where to find the ID.
The sign-in cooldown was checked with Wait before the password and recorded with Fail after it, so a concurrent burst of guesses passed Wait before the fifth failure was recorded: the audit saw 8 to 20 of 20 checked, and a right password inside the burst signed in. Limiter.Begin now admits an attempt under the lock and counts it in flight until its done reports whether it failed. Attempts in flight count as failures would, so a burst gets no more password checks than a sequential run: five before the first cooldown, one at a time after it. An entry with attempts in flight is neither dropped as idle nor evicted, and a success keeps it for them. A refusal by the derivation gate derived nothing and does not count. Wait and Fail have no caller left and are removed; their tests run through Begin.
POST /password verified current_password through ChangePassword with no cooldown and no log, so a session holder could guess the password at the derivation gate's pace, even while the account was in sign-in cooldown, and a hit took the account. The check now runs behind the sign-in cooldown of the account's username: user.ErrInvalidOldPassword marks a wrong current password (the verify miss and the compare-and-swap miss), each one counts as a failed sign-in and is logged with the user's ID, and a username in cooldown is answered 429 with Retry-After before any derivation. A successful change clears the count as a sign-in does. A busy refusal still counts for nothing.
Revocations lived only in TokenStore's map, while the signing key now always comes from a file. After a restart /oidc/userinfo served a revoked access token again until its exp, though the README promised the revocation was kept for 24 hours. An optional revocations.filePath keeps them: tokenstore.Open loads the file (a missing one holds none, a malformed one or a missing directory fails the start), drops entries older than the token lifetime, and every Revoke writes the whole jti -> revokedAt set back through an owner-only temp file, fsync and rename, as users.json is written. A failed write keeps the revocation in memory, is logged, and /revoke still answers 200. Without the path the store stays memory-only, the start warns about it, and the README promise is corrected. tokenstore.New keeps its signature as the memory-only store, so the callers that need no file are unchanged.
The README already says how each of the four fixes behaves where an operator configures it. What was left: the Quickstart config shows revocations.filePath, so a new deployment keeps its revocations from the start; "Failed sign-ins" says a concurrent burst gets no more password checks than a sequential run and that a busy refusal is not a failure; and the repository's maintainer notes carry the seed ID requirement, where the revocations are kept, and what the cooldown covers, so a maintainer does not undo them by accident.
Open read the whole file before looking at it and decoded it straight
into the map. A top-level null decoded to no map, and a null or zero
revocation time decoded to the zero time, which the prune drops at
once: valid JSON that silently started with fewer revocations, so the
revoked tokens were accepted again. A file of any size was read whole,
so a damaged or oversized one could exhaust memory at start.
Open now reads through io.LimitReader with a documented bound of
16 MiB (some 260,000 revocations) and fails the start on a file at or
over it, never loading part of it. A null root and an entry without a
revocation time fail the start as a malformed file does; {} is still
the empty set, and expired entries are still pruned on load.
The persistence tests checked a successful reload and an unwritable directory. Replacing the atomic write with os.WriteFile, or dropping saveMu so an older snapshot can land after a newer one, would have passed them both. writeFile now goes through a private fileSystem, as pkg/user's does, so a test fails the write, the fsync, the close or the rename against an existing file: the old bytes stay, the revocation stays in memory, one error is logged, no temp file is left, and the next Revoke writes everything. A directory sync that fails after the rename is one warning. Overlapping Revoke and CleanUp calls, with the first write held at its rename, all survive a reopen. At the route, /revoke still answers 200 when the write fails and /oidc/userinfo refuses the token.
The burst tests counted calls of a hook that ran after the cooldown admitted a request, and called that count password checks. Moving Verify above Begin in login let every request of a burst derive while only five reached the hook, and both login burst tests still passed. The hook is now the verification itself: hooks.verify and hooks.changePassword stand in for r.user.Verify and r.user.ChangePassword when a test sets them, so what is counted and held is a call that derives. pkg/user's API is unchanged. A new table asserts five derivations and the statuses for a burst at /login, at the /oidc/auth login form, at /password, and at /login and /password together for one username. With Verify moved above Begin, the two login burst tests and three of the four new rows fail.
The seed tests covered a username reused under another ID, and the ID-less refusal only against a missing database. Changing the mismatch check to skip only when the seed's ID is gone would have kept them green while putting the seed hash on an account that has the seed's ID but another username. A table now runs one seed entry against an existing database for each way it can name accounts: its ID and username on one account, the ID only, the username only, the two on different accounts, and neither. Only the first changes a hash; each mismatch logs one warning without a hash; neither is the deleted seed account and logs nothing. The ID-less refusal runs against a new database, which stays unwritten, and an existing one, whose bytes stay as they were; with the option off the entry still seeds a generated ID.
The upgrade note said to copy into each seed entry the ID of the account that has its username. If the seed account had been deleted and the username registered again, that copies the new owner's ID, the ID and the username then agree, and the next start puts the old seed password on the new owner's account. The note now says to confirm from a trusted record (provisioning record, a backup from before any deletion, creation history) that the account is the one the seed created, that a username alone is not enough, and to remove the seed entry or turn updateSeedPasswords off instead when the account was deleted, replaced, or of uncertain origin, and when no account has the seed's username.
Begin's comment said the KDF gate keeps the attempts in flight far below MaxKeys. It does not: Begin runs before the gate, so requests waiting for the gate, and a /password change still writing after its derivation, all hold an entry. Nothing bounds them below MaxKeys. The comment now says that when every tracked entry has an attempt in flight a new key is let through uncounted, an accepted residual; that an entry with an attempt in flight is never dropped or evicted; and that the entry is made at Begin, also for an attempt the derivation gate then refuses as busy. "Failed sign-ins" in the README says the same in one sentence.
Two comments added in lane F6 narrated what the code does, against the repository's why-only convention. setRetryAfter's now keeps only why the wait is rounded up, so a retry at Retry-After never lands before the wait ends; the fail test helper's comment, which restated its three lines, is gone.
Open refuses a revocations file of maxRevocationsFile bytes or more, but Revoke wrote a set of any size. Past roughly 262,000 live revocations the server wrote a file its next start refused, so a large but legitimate revocation set became a start failure. Revoke now applies the loader's bound. A set that would encode to the bound or more stays in memory, the old file is not replaced and an error is logged, so the earlier revocations still survive a restart and the new ones are refused until then. Once pruning brings the set under the bound, the next Revoke writes the file again. /revoke still answers 200.
The seed example showed a bcrypt hash, and the password-hashes section told operators to close the hash-type timing difference by giving affected users new passwords. With users.updateSeedPasswords that advice is undone at the next start, which restores the configured hash. The example now shows a default-parameter argon2id hash with an ID, the seeding section says why that format is preferred, and the timing paragraph names the seed exception and its fix: a default argon2id hash in the configuration.
TestLogin_CooldownBurstRefusesTheRightPassword released the five held password checks and returned without reading their answers, so their derivations and cooldown log lines ran on into later tests, two of which replace the KDF gate (SetKDFGate forbids derivations in flight) or capture the default logger. Copilot review on #40.
ahma
force-pushed
the
fix/http-and-login-hardening-on-chain
branch
from
October 29, 2026 21:12
360b49d to
8539cf6
Compare
ahma
force-pushed
the
fix/seed-identity-cooldown-revocation
branch
from
October 29, 2026 21:18
7388561 to
481cb42
Compare
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.

Summary
A seed user's configured password now follows the account the seed entry created, never a later account that reuses
its username; the sign-in cooldown counts an attempt from the start of its password check, so a concurrent burst
gets no more guesses checked than sequential attempts; the current-password check of
/passwordruns behind thesame cooldown; and revocations can be kept in a file, so a restart still refuses a revoked token.
Why
Findings of the security review of this fix stack (#35 to #39) at
8f37696:users.updateSeedPasswords, the start-time seed pass matched accounts by username only (medium, pre-existingon
main). After an admin deleted a seed account and the same username was registered by someone else, the nextstart put the seed password on the new account and the owner's own password stopped working. Seed entries are now
bound to their account by
IDand username together.for one username got 8 to 20 of 20 checked against 5 sequentially, and a right password inside the burst signed
in. An attempt is now reserved under the limiter's lock before the check.
POST /passwordverified the current password with no cooldown and no log (low). A holder of a stolen sessioncould guess the password at the hashing gate's pace, even while the account was in sign-in cooldown, and a hit
replaced the password. The check now runs behind the same cooldown, keyed by the account's username.
/oidc/userinfoaccepted a revoked access token again until its
exp. Withrevocations.filePathset they are written at everyrevocation and reloaded at start; without it the server warns at start and the README says what a restart does.
What changed
nullroot or null timestamps fail the start)./login, the/oidc/authform and/password; seed identity rows.updateSeedPasswordsa new password is undone at the next start, so the configured hash itself must be default argon2id.Test plan
just verify,just test-race,just test-e2e(25 tests, Linux container) green on186d2e5; the last commit is documentation only.TestSeedPasswordFollowsTheAccountNotTheName,TestSeedPasswordsRequireAnID,TestLimiterBurst,TestLimiterSucceedWithAttemptsInFlight,TestLimiterEvictionKeepsAttemptsInFlight,TestLogin_CooldownBurst,TestLogin_CooldownBurstRefusesTheRightPassword,TestLogin_BusyIsNotAFailure,TestChangePassword_Cooldown,TestOpenKeepsRevocationsAcrossARestart,TestOpenDropsRevocationsOlderThanTheTokenLifetime,TestOpenRefusesWhatItCannotUse,TestRevokeKeepsARevocationItCannotWrite,TestRevocationSurvivesARestart,TestRevokeKeepsTheOldFileWhenAWriteFails,TestRevokeOnlyWarnsWhenTheDirectoryCannotBeSynced,TestOverlappingWritesKeepEveryRevocation,TestOidcRevoke_WriteFails,TestCooldownBurst_EveryDoor,TestSeedPasswordNeedsTheIDAndTheName, and the at-bound writer tests.Begin; awrong current password not counted at
/password; revocations not reloaded at start;Succeeddropping an entry withattempts in flight (run against the final head by the reviewer); the password check moved before the reservation;
os.WriteFilein place of the atomic write; the write lock removed; the writer's bound removed (run and recorded bythe author while adding the corresponding tests).
Compatibility
users.updateSeedPasswords: true, every entry ofusers.usersneeds anID; the server refuses to startotherwise. Before upgrading, copy each seed user's
"ID"fromusers.filePathinto its seed entry (README,"Seeding the user database").
revocations.filePath. Without it, behaviour is as before and the start logs a warning; the chartshould set it on the persistent volume in a follow-up.
/passwordanswers429 Too Many RequestswithRetry-Afterwhile the account's username is in cooldown.Residuals carried on purpose
entries are never evicted or dropped. Documented in the README.
across a crash in that instant is not guaranteed.
Not in this PR
email_verifiedclaim, the publish credential scope and limits on self-registration (deployment decisions).Stacked on #39 (
fix/http-and-login-hardening-on-chain). Draft until the stack is merged.