feat(auth): add email_password sign-in with reset and revocation - #7
Merged
Conversation
Adds HashPassword/VerifyPassword over golang.org/x/crypto/argon2, PHC-encoded, plus the shared length validator every entry point calls. Parameters are m=64 MiB, t=3, p=1; p is 1 because the lanes partition the same arena rather than buying parallel speedup, and a value tied to host CPU count would make a hash's cost depend on which replica computed it. Two properties are load-bearing and pinned by tests. Verification reads m/t/p out of the stored record rather than assuming the current constants, so retuning them later does not strand existing hashes. The test uses a literal PHC fixture written under other parameters, which cannot drift toward the constants and survives changing them. A record that is not a well-formed argon2id string -- a sha256 digest that reached the password column, a truncated row, an out-of-range cost -- returns ErrMalformedHash rather than a quiet mismatch. Collapsing the two would hide a wrong-hash-function bug behind what looks like a bad password. Parsed cost values are bounded, because the record is data: a row claiming a huge arena would otherwise be honored and exhaust the process. The PHC codec is hand-rolled because x/crypto/argon2 ships only the KDF and no encoder, and the libraries that parse PHC would be a new dependency in the authentication path. Promotes golang.org/x/crypto to a direct dependency; it was already indirect.
UpsertPassword writes or replaces a password in one statement, through the jet builder. The arbiter is a partial unique index, so the conflict target repeats its predicate -- ON CONFLICT (user_id) WHERE kind = 'password' -- which is what makes Postgres infer auth_credentials_password_uidx instead of looking for a total index on user_id. That predicate is a raw expression rather than a built one, and the reason is worth recording: postgres.String() renders a ::text cast, and `kind = 'password'::text` does not match the index's `WHERE kind = 'password'`. Inference then fails with 42P10 at runtime -- it compiles, the first insert succeeds, and only the second one, the one that conflicts, errors. The store tests catch it, verified by putting the cast back. The DO UPDATE clears attempts, expires_at, consumed_at and session_nonce: those belong to one-time codes sharing this table, and a row that held one would otherwise carry its state into a live password. Pinned by a test. Replacing in place rather than retiring the old row via consumed_at is deliberate: neither GetPasswordByUserID nor the unique index filters on it, so a soft-deleted password would still occupy the slot and still read as live. RevokeByUserIDExceptFamily backs a password change made from a live browser: every other session is evicted -- the point of changing a password after it leaked -- while the one doing the changing survives. FamilyByRefreshToken resolves which session that is, verifying ownership, because the family is not in the access token and TokenPair.SessionID never reaches the wire.
/login/password stops being hardwired to the break-glass provider. Resolution is now: a user with a stored password is verified against it; otherwise, if the submitted address is the configured bootstrap one, the break-glass password is tried; otherwise the attempt fails. Step order matters. Once the admin has a password of their own, the break-glass one is no longer consulted for them -- the personal credential wins as soon as it exists, and the shared one stays for emergencies. Two behavior changes worth naming. The request body email is no longer ignored -- it selects the method, so a break-glass password submitted against another address signs nobody in. And a credential written by the wrong hash function fails the login loudly rather than reading as a mismatch. Every failing branch pays exactly one argon2id verification. Where no stored password is found the handler verifies against a fixed decoy instead, and the break-glass branch pays it too: Authenticate is a constant-time compare over a config string, so without the decoy a wrong guess against the configured address answered orders of magnitude faster than against any other, locating the admin's address by timing alone. A test measures both branches. Adds the password.changed and password.reset audit actions, in both direction maps -- the reverse one feeds the category filter, so an action listed in only one is written correctly and then never appears when a user filters by its own category. A test derives that check from the maps themselves rather than a hand-kept list.
Three endpoints. POST /me/password sets the caller's own password;
/password/reset/{request,confirm} reuse the existing one-time code mechanism
unchanged.
installPassword holds what both writers share: the password write and the
session revocation in ONE transaction, with the revocation passed in as a
callback so it runs inside it. Splitting them would leave a window where the
password is new but the old sessions still work, which is exactly what someone
replacing a leaked password is closing. Tested in both directions: a revocation
that fails rolls the password back.
/me/password requires current_password when one is set and refuses it when none
is -- both directions are errors, so a client learns which state it is in.
refresh_token names the session to spare and is OPTIONAL; omitting it revokes
everything including the caller's, which is the only way an admin who has lost
it can still set a password. A reset revokes every session: the proof was
possession of the mailbox, and the person resetting may be recovering an account.
The reset policy check runs BEFORE the code is redeemed and answers identically
to a bad code. Checking after would tell a caller holding a guessed code that
the code was right, and would spend an attempt on a request the user is about to
retry. Pinned by handler tests asserting the failures are byte-identical.
A blocked user is refused explicitly -- Verify does not check blocking, which
lives in IssueAccessToken, and this path issues no tokens.
The transaction starts after Verify returns, leaving its no-transaction contract
intact. Accepted consequence: a write failing after redemption burns the code,
which is the safe direction.
Registers the three new routes, mounts the password gate at every RequireAccessToken site, and completes the wiring. The router file lands here rather than split across the earlier commits because its hunks interleave: the gate mount and the route registration sit in the same functions, and separating them would leave neither commit building on its own. The reset routes take the SAME three limiter tiers as the sign-in codes, because they issue the same codes through the same mailer: leaving them on the per-IP bucket would be a second, cheaper door to it. Rewrites BootstrapLoginFailing as PasswordLoginFailing. The old rule read every non-429 4xx on /login/password as a break-glass brute force and told the operator to set a strong bootstrap password. That route now serves ordinary users against their own hashes, so the advice is wrong for almost every firing, and a real security alert turned into noise is worse than none. The threshold rises accordingly, and the annotation says to check whether failures concentrate on the configured bootstrap address -- which is what an attempt on the admin credential looks like.
otp.Verify leaves the audit reason empty when it fails infrastructurally -- a database that did not answer while resolving the user, reading the live code, claiming an attempt, or consuming it. Those failures were dropped from the trail entirely, on the reasoning that a database error is not a judged credential. That reasoning is half right and the conclusion is wrong. An attempt was made and it was refused; a trail that omits it reads as though nothing happened, which is the wrong answer to give an operator reading it after an incident -- and this is the one record they can reach when sign-in is what broke. The distinction it was protecting survives: the event is recorded AS unknown rather than dressed up as an invalid code, so nothing claims a credential was judged when none was. The raw error stays out of the payload, which is a whitelist by design. Reverses the test that pinned the old rule.
The frontend needs to know which form to draw -- setting a first password or replacing an existing one -- and it cannot work that out on its own. Nothing else in the profile implies it, and a value inferred at sign-in does not survive a page reload. password_set mirrors what POST /me/password enforces: current_password is required when one exists and refused when none does, both a 400, so a client that guesses wrong gets an error rather than a form it can submit. A read failure degrades to false rather than failing the profile. The field steers a form; guessing wrong costs the user a retry, a 500 costs them the page. Also fixes a policy failure on /me/password answering 500. ErrPasswordPolicy comes from xcripto and was not wrapped in apperr.ErrValidation, so the mapper fell through to its default arm -- a password one character short returned an internal error. Unlike the reset path there is nothing to hide here: the endpoint is authenticated and the caller owns the account, so it now answers 400 and says the password was too short.
`go test -race` reports a data race between any two concurrent password writes, and it is not a test artifact: postgres.NULL is ONE package-level value shared by the whole process, and jet's typed wrappers mutate it in place by calling setRoot on it while building the statement. Two people changing their passwords at the same moment take that path in production, so this would have raced there too. It went unnoticed because the ordinary `make tloc` runs without -race; only `make tloc-cov` catches it. The nulled columns now go through CAST(NULL), which wraps the shared value in a fresh expression before the typed wrapper touches it, so the mutation lands on something this statement owns. The generated SQL carries an explicit ::timestamp with time zone, which is what a bare NULL would have been inferred as anyway. The concurrency test already covered this path; its comment now names what it guards, since a plain run passes either way. Verified by putting the bare wrapper back and watching -race report the race again.
POST /me/password with the wrong current_password returned an internal error. apperr.ErrInvalidCredentials was missing from the auth mapper entirely, so it fell through to the default arm -- a client could not classify the failure, and a BFF routing on status would bounce the operator to sign-in on what is really a refusal. The login routes never noticed: they answer failures themselves and bypass this mapper on purpose, to keep every rejection identical. Adds handler tests pinning all three statuses this endpoint can answer -- 401 for a wrong current password, 400 for one below the length floor, 204 on success. Unlike the login and reset routes these are told apart deliberately: the caller is authenticated and owns the account, so a form can say which of the two went wrong. Found by the frontend session building against this contract, which had assumed 401 and would have shipped against a 500.
GetMaints chose its two defaults independently: the lower bound from the caller, the upper one from the end of TODAY. A caller supplying only period_from -- tomorrow, say -- therefore built a range whose lower bound sat above its upper, and Postgres rejected the query outright with 22000, "range lower bound must be less than or equal to range upper bound". The request failed with a 500 where the honest answer is "the rest of that day". The upper default now comes from periodFrom instead, so an open-ended filter always describes a valid window. This is also why TestList/no_overlap looked flaky in CI. It asks about a maintenance starting two hours out, so it built an inverted range only when the run landed in the last two hours of a UTC day -- red overnight, green all morning. The behaviour was time-dependent in a way nothing declared, which is the part worth fixing rather than the test. Adds a regression test using an explicit far-future period_from, so it fails at any hour rather than reproducing the defect only after 22:00 UTC.
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
1 participant
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.
Makes a password the second built-in sign-in method: argon2id hashes in
auth_credentials, self-service reset over the existing one-time code flow, and
session revocation whenever a password changes. RUK-289.
stored record so retuning them never strands existing hashes; a record that is
not well-formed argon2id (a sha256 digest in the password column, a truncated
row) fails loudly instead of reading as a mismatch
is verified against it, and the submitted address now selects the method
rather than being ignored
keeps the calling session and evicts the rest, a reset evicts every session
verification, including the break-glass one, which previously answered orders
of magnitude faster and located the configured admin address
policy failure on /me/password answering 500 instead of 400
from the trail entirely; retarget the login alert, whose old remediation
advice no longer fits a route that serves ordinary users