Repository navigation
Conversation
There was a problem hiding this comment.
🔵 Needs a closer look
Peak RSS and GC headroom need deployment-level validation for maximum-memory stored hashes, since the gate permits 128 MiB of live KDF buffers in a 200 MiB pod but the reported concurrency check uses default-cost hashes.
No open issues 🎉
What changed in this PR
This PR adds HTTP and sign-in hardening to the self-hosted OIDC provider, building on the preceding credential and protocol fixes.
Changes:
- Adds server timeouts, request limits, security and cache headers, configurable listening, and scheme-aware secure cookies.
- Protects credential submissions against cross-origin requests, bounds password-hashing concurrency and stored-hash parameters, and adds login cooldowns and bounded failure logging.
- Expands regression coverage and documents compatibility changes, operational limits, and account recovery.
| File | Description |
|---|---|
scripts/e2e.py |
Binds the test server to loopback using LISTEN_ADDR. |
README.md |
Documents hardening behavior, upgrade requirements, and recovery. |
pkg/user/user.go |
Routes password operations through the gate and exposes verification errors. |
pkg/user/register_locked_test.go |
Updates locked-account checks for error-aware verification. |
pkg/user/password.go |
Bounds stored hashes and adds gated verification and dummy refusals. |
pkg/user/password_test.go |
Adapts verification tests and accepted hash parameters. |
pkg/user/kdf.go |
Adds bounded password-hashing admission and salt-length checks. |
pkg/user/kdf_test.go |
Tests concurrency limits, hash bounds, and refusal behavior. |
pkg/user/admin.go |
Gates administrative password hashing. |
main.go |
Wires HTTP limits, headers, secure cookies, and client-secret warnings. |
main_test.go |
Tests server limits, headers, cookies, origin handling, and logging wiring. |
internal/throttle/throttle.go |
Implements bounded username cooldowns and logging counters. |
internal/throttle/throttle_test.go |
Tests cooldown timing, eviction, retention, and counters. |
internal/routes/throttle.go |
Adds bounded login and client-authentication failure logging. |
internal/routes/throttle_test.go |
Tests cooldown enforcement, reset recovery, and logging bounds. |
internal/routes/routes.go |
Initializes origin protection and throttling state. |
internal/routes/root.go |
Enforces sign-in protection, body-only credentials, and cooldowns. |
internal/routes/password.go |
Uses body-only secrets and handles hashing refusals safely. |
internal/routes/password_test.go |
Initializes hardening components in route fixtures. |
internal/routes/oidc.go |
Validates authorization before login and records client-secret failures. |
internal/routes/middleware.go |
Adds security-header and no-store middleware. |
internal/routes/login_origin_test.go |
Tests origin protection, session preservation, and body-only fields. |
internal/routes/error.go |
Adds retryable busy responses and generic password-write errors. |
internal/routes/csrf.go |
Reads CSRF tokens from the body and checks sign-in origins. |
internal/routes/busy_test.go |
Tests overload and long-ID refusals without state changes. |
internal/routes/bootstrap_test.go |
Initializes origin protection and cooldowns in bootstrap fixtures. |
internal/routes/admin.go |
Uses body-only passwords, handles gate refusals, and clears cooldowns after resets. |
Review effort: Balanced
ahma
force-pushed
the
fix/oidc-lifecycle-and-protocol-on-chain
branch
from
October 29, 2026 20:51
0f5e56e to
3cd883c
Compare
ahma
force-pushed
the
fix/http-and-login-hardening-on-chain
branch
from
October 29, 2026 20:51
fbb12a7 to
360b49d
Compare
The Secure flag came from strings.HasPrefix(baseUrl, "https://"), but URL schemes are case-insensitive and Validate accepts "HTTPS://host", so such a deployment served its session cookie without Secure and a plain-http request could carry it. The flag now comes from the scheme url.Parse returns, which is lowercased. The issuer keeps the spelling the configuration gives. The routes configuration that follows from baseUrl moves into baseRoutesConfig, so a test can sign in through the same wiring main uses.
http.ListenAndServe ran a zero-value server: no read, write or idle timeout, so a client that trickles its headers or never reads the response holds a connection and its goroutine for as long as it likes, and a form body was bounded only by ParseForm's 10 MB default. newServer sets a 5s header timeout, 30s read and write timeouts, a 120s idle timeout, and caps the headers and, through http.MaxBytesHandler, every request body at 64 KiB, one much tighter bound for all routes and still far above the largest form the IdP takes. The listen address comes from LISTEN_ADDR (default ":8080"), so a deployment can bind one interface. The e2e suite binds 127.0.0.1.
No response said it must not be framed, so a page on another site could load the login or admin form in an invisible frame and clickjack it. A CSP frame-ancestors 'none' (with X-Frame-Options DENY for older browsers) and X-Content-Type-Options nosniff now go on every response, from a SecurityHeaders wrapper around the whole mux, so the mux's own 404s and redirects carry them too. The profile page, the admin pages and their JSON API, /password, /set-password, the admin forms that show a fresh reset link, and /oidc/userinfo could be kept by a browser or proxy cache and shown after sign-out or to the next user of a shared machine. They are now registered through NoStore, which sets Cache-Control: no-store before the handler writes, so their error responses carry it too. /token keeps the headers it already sets.
A page of another site could post the login or registration form and sign the victim's browser in to an account the attacker controls (login CSRF); what the victim then enters there is the attacker's to read. The credential-submitting POSTs, /login, /register and the login form of /oidc/auth, now run http.CrossOriginProtection and answer 403 without a cookie to a cross-origin request. A same-origin request passes, and so does one with neither Sec-Fetch-Site nor Origin, from a non-browser client such as the e2e suite. A relying party's direct POST authorization request carries no credentials and is not checked. The /oidc/auth login form signed the user in before the authorization request was validated, so a request refused anyway still left a session; the login now runs after the validation. The login fields and csrf_token are read from the body only: in the URL they reach access logs, proxies and the browser history.
Every login, registration, password change and reset derives an argon2id hash of ~15 MiB, and nothing bounded how many ran at once: a burst of sign-in attempts, valid usernames or not, could exhaust the 200 MiB memory limit of the chart's pod. Every derivation a request can cause, the verification of an argon2id, bcrypt or legacy hash, the dummy one for an unknown username, and the hash of a new password, now runs through a gate of 4 slots. Up to 32 more callers wait for a slot for at most 2s; the rest, and a waiter whose deadline passes, get ErrBusy before anything is allocated. No lock of the user store is held while a caller waits. The routes answer ErrBusy with 503 and Retry-After: 2; a refused reset leaves its link usable. A stored argon2id string was derived with whatever parameters it named, so an entry in users.json could make one check allocate gigabytes, spin for any number of passes, or panic argon2 with a time or parallelism of zero or an empty digest. Its parameters are now bounded before the gate is entered: m at most 32 MiB, t from 1 to 10, p from 1 to 4, version 19, salt of 1 to 1024 bytes, digest of 16 to 64 bytes. Anything else does not verify. An existing test's 64 MiB hash is now 32 MiB. User.Verify tells ErrBusy from ErrInvalidCredentials; Authenticate keeps its signature.
Nothing limited how fast one account's password could be guessed: each attempt cost the attacker a request and the server a derivation. A new internal/throttle Limiter counts failed sign-ins per username, in memory and for at most 10,000 usernames (the least recently failed is dropped), over a 10-minute window. After five, every attempt for that username is answered, without checking the password, with the same 401 page as a wrong password plus Retry-After, for a cooldown of 10s that doubles with each later failure up to 30s. No handler sleeps. A successful sign-in, or a new password set by an admin or through a reset link, clears the count. The README states the trade-off: who knows a username can keep it in cooldown while the attack lasts. A confidential client's wrong secret at /token or /revoke is counted and logged, at most once a minute per client with the count, but never refused: a lockout would let a guesser cut a relying party off. A public client, an unknown client id and grant, PKCE or redirect errors are not counted. Validate warns at start for a clientSecret under 16 characters, since its length is what bounds an online guesser.
A locked account ("!"), a malformed stored hash or one outside the
argon2id bounds was refused at once, while a wrong password costs a
derivation: the time of the answer told such an account apart from a
real one. These values now run the dummy argon2id derivation through
the gate before they are refused, as an unknown username already did.
bcrypt reads only the first 72 bytes of a password, so for a
bcrypt-stored user any suffix after the real password was accepted.
A password over 72 bytes is now refused for such a user (after the
dummy derivation); the README points them to a reset.
Hashes are still not upgraded on sign-in; the README documents the
timing a bcrypt or non-default argon2id hash still gives away and how
new passwords remove it.
The README gains the rest of its Hardening section: LISTEN_ADDR and the server's timeouts and size limits, the framing, sniffing and no-store headers, that a baseUrl path is no boundary against another application on the same host name, the cross-origin sign-in check and what a reverse proxy must keep for it, the derivation gate and the memory to plan for it, and an upgrade note listing the hashes and requests that are now refused.
The limiter kept each submitted username whole, as map key and entry, for up to 10,000 usernames, whether or not the account exists. A username is as long as the 64 KiB request body allows, so one failed sign-in per distinct 60 KiB name retained about 600 MiB, three times the pod's memory limit. Expired entries were never released either: they stayed until a success, an eviction or a restart. An entry is now keyed by the SHA-256 of the exact username bytes, the same byte-exact identity the password check uses, and holds no copy of the name. Every failure drops the entries whose window and cooldown have both passed before it evicts for capacity; Wait drops them from the least recently failed end. The cooldown warning logs the first 64 bytes of the username, marked when cut, and a 12-hex-digit digest prefix that tells apart two names sharing them.
Sign-in already read its credentials and every form its CSRF token from the body, but the other form writes still took their passwords from the merged form: /register, /password, the POST of /set-password (with its token), /admin/register and the admin reset. A same-origin or header-less POST with the secret in the query string was accepted, and the secret reached access logs, proxies and the browser history. A registration could also take its username from the query and its password from the body. All of them now read from req.PostForm: the registration's username, email, password and confirmation, the current and new password, the reset token and new password, the admin's initial and reset password. The link's GET keeps ?token=, and /oidc/auth keeps its authorization parameters and the relying party's credential-less POST as before.
A stored hash is read from users.json as it is. parseArgon2id split and decoded it before checking any length, and the legacy branch decoded before its 32-byte check, so a huge value allocated in proportion to its size on every attempt, outside the derivation gate. Sscanf ignored text after v=19 or p=1, so a misspelled hash still verified. bcrypt.Cost accepted any cost up to 31 and a salt that does not decode: four attempts against a cost-31 hash held every gate slot for hours, and the undecodable salt made the comparison return before any work, a fast answer for that account. And the writers salted with the full user ID, so a reset for an ID over the verifier's salt bound saved a hash that never verifies, locking out an account that could sign in before. Each form now has an encoded-length bound checked first: a PHC string at most 512 bytes, a bcrypt string exactly 60, a legacy value at most 64. The PHC version and parameters are parsed with strings.Cut and strconv.ParseUint, nothing before or after. A bcrypt string must name $2a$, $2b$ or $2y$, a cost from 4 to maxBcryptCost (15, about two seconds), and carry its salt and digest in bcrypt's alphabet. Anything else takes the gated dummy refusal, chosen before the gate is entered. The salt bound becomes 256 bytes so that every hash the writers produce fits the 512-byte bound, and the writers refuse a longer ID with ErrIDTooLong before any derivation; the reset link, the password change and the admin reset answer a generic 500, log the cause and leave the password, the session and the link as they were. README documents the bounds and the migration.
The sign-in check used one package-level CrossOriginProtection with no trusted origin. A browser that sends Origin but not Sec-Fetch-Site is judged by Origin against the request's Host, and behind a reverse proxy that rewrites Host (Origin https://console.example, Host axoidp:8080) that never matches: login, registration and the /oidc/auth form all answered 403. Each Routes now builds its own check and trusts the scheme and authority of its baseUrl, lowercased and without a default port, as a browser serializes Origin; no path, no forwarded headers, no bypass. A Sec-Fetch-Site the browser sent still decides on its own, because CrossOriginProtection would let a trusted Origin overrule a cross-site one. Two servers with different baseUrls do not accept each other's origin, and header-less clients pass as before.
The no-store wrapper covered the account, admin and reset pages, but not /oidc/auth, whose authenticated redirects carry an authorization code or an implicit ID token, nor /login, /register and /logout, whose responses set or clear the session cookie, nor the errors of /token, which set no-store only on success. Those responses could be kept by a browser or proxy cache. They now go through NoStore too, before anything is written; /token still adds its own no-store and Pragma to the tokens. The header matrix covers a code and an implicit redirect, sign-in and registration success and failure, logout and a /token error, and reads the headers from rec.Result(), the ones sent with the status, so a middleware that set them after its handler would fail it.
Several invariants the F4 tests advertised were not distinguished by them: a mutation that broke one still passed. Each test below fails under the mutation named, applied locally and reverted. - /oidc/auth with the right password and an invalid scope, nonce, resource or PKCE, the browser's session cookie attached: no Set-Cookie, the old session still resolves, one session. Controls: a public and a confidential login, the relying party's cross-site POST. Catches validation moved after the login, and the old session ended before validation (the session count alone missed both). - The production gate's 4 slots, 32 waiters and 2 s, read from kdf itself. Catches a changed initializer, which SetKDFGate hid. - Each refused argon2id bounds row is refused by the parser and runs the dummy, with a matching digest where affordable. Catches a dropped time or salt bound, which still returned false before. - ChangePassword's second derivation, a legacy base64 check and SetPassword each enter a slot. Catches either of them ungated. - A wrong confidential secret is logged at /token and at /revoke, each on its own server, with the clients passed by baseRoutesConfig, which main now uses for them; a right secret without a /revoke token is invalid_request and not counted. Catches either call removed, the clients not passed, and counting under the merged error condition. - The concurrency test counts a derivation until testOnDeriveDone, a new exit hook, so its maximum is of admitted derivations. Catches a slot released before the derivation, which the old test passed.
The README claimed more than the code does in places and was behind the fix-ups in others. The derivation gate refuses before the KDF buffer is allocated, not before anything is, and it bounds the derivations running at once: it is no limit on the request rate and no bound on the process's memory, so 4 x 32 MiB is a planning figure. The cooldown is kept per exact username under its digest, for at most 10,000 entries, idle ones dropped, and 10,000 other failed usernames push one out. An Origin-only browser is accepted when Origin is the baseUrl origin, so a proxy may rewrite Host. Every form secret is read from the body, no-store covers the sign-in, authorization and token routes, and a client secret is guessed by its predictability, not its length. Three comments that restated the name they stood on are removed, and the KDF gate's and the short-secret warning's comments say the same as the README.
The body-only rows moved several fields to the query together, so one reader put back on req.Form survived them: another missing body field refused the write anyway. Each named field now has its own query-only row with every other field valid in the body, so a single reader reverted to req.Form fails exactly its row. req.Form puts the body's value first, so a field in both places still reads the body there; the new conflicting rows assert the identity, password or reset token the write used, which pins out a reader of the URL query. The unregistered-redirect refusal of the /oidc/auth login form had no control with the browser's existing session attached. The new test checks the in-place 400 and that the same cookie still resolves, so a regression ending that session in the ValidateRedirect error branch fails.
The upgrade note told users with a salt over 256 bytes to get a new password before upgrading. When that salt is the user's ID, it does not work: the previous release writes another hash with the same long salt, which this release refuses to verify, and this release refuses the write itself (ErrIDTooLong) without changing anything. The short-ID migration stays an ordinary change or reset. The long-ID case gets its own section: inventory the IDs before rollout, replace the stored credential offline with a self-contained bcrypt or argon2id hash through the stop, edit or seed, start procedure, keep the ID (it is the subject identifier and a legacy hash's salt), and expect the same offline step for every later rotation, as this release does not change the bound.
The 176-byte cooldown warning measured an ASCII username, but the JSON handler escapes the 64-byte prefix: a control byte or an invalid UTF-8 byte becomes 6 bytes, so the same line can reach 496 bytes. The test now has a control-byte and an invalid-UTF-8 row, bounded by 6 x 64 bytes for the prefix plus 128 for the fixed fields (112 in the ASCII row), and checks that the line is one JSON object naming the expected prefix. The comment labels the ASCII figures as a sample and the 12-hex digest as a correlation aid that two usernames may share, not an identifier.
… name The cooldown warning carries the first 64 bytes of the username and a 12-hex-character prefix of its SHA-256. The comment said the prefix tells two names apart; 48 bits cannot promise that, and the limiter itself keys on the full digest. Say what the prefix is for so nobody treats the log line as an identifier.
ahma
force-pushed
the
fix/oidc-lifecycle-and-protocol-on-chain
branch
from
October 29, 2026 21:12
3cd883c to
15590a5
Compare
ahma
force-pushed
the
fix/http-and-login-hardening-on-chain
branch
from
October 29, 2026 21:12
360b49d to
8539cf6
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
The HTTP server has header, read, write and idle timeouts, a header and body size limit, and a listen address;
every response carries frame and content-type headers and the authenticated pages are not cached; a credential
POST from another site is refused; the session cookie is
Securewhenever the issuer ishttps, however thescheme is spelled; password hashing runs behind a fixed admission gate so a burst of logins cannot exhaust the
pod's memory; repeated failures for one account are met with a cooldown; and a locked or oddly stored account
takes as long to refuse as a normal one.
Why
Findings of the security review of the repository at
d515e17(main plus #29 to #32), all pre-existing onmain:Securewhen thehttpsbase URL was spelled with an upper-case scheme (low). Theflag now follows the parsed scheme.
(low). Those paths now run the same argon2id work before refusing.
each login held a 15 MiB argon2id buffer with no bound on concurrency; there was no attempt throttling and no
failure log; password login accepted cross-site form POSTs; no HTML response restricted framing. Each is fixed
in source (the owner checks of the deployment confirmed the exposure: the IdP shares the console's origin
behind Traefik with no rate limit of its own).
What changed
fix(config):SecureCookiesfrom the parsed scheme.fix(server): timeouts,MaxHeaderBytes,http.MaxBytesHandler,LISTEN_ADDR(default:8080).fix(routes):Content-Security-Policy: frame-ancestors 'none',X-Frame-Options: DENY,X-Content-Type-Options: nosniffon every response;Cache-Control: no-storeon authenticated andsecret-bearing responses.
fix(routes):http.CrossOriginProtectionon the credential-submitting POSTs (/login,/register, theusername branch of
/oidc/auth); the authorization request is validated before the login runs; form fieldsread from the POST body.
fix(user): a KDF admission gate (4 slots, bounded waiters, 2 s deadline, 503 withRetry-Afterbeyond it);parsed argon2id parameters bounded before use.
feat(throttle): per-username failure cooldown (5 failures in 10 minutes, 10 s doubling to 30 s, cleared bya success), failed client authentications logged at a bounded rate, a warning for short client secrets.
fix(user): locked and unparseable stored values take the dummy argon2id path; bcrypt users refuse passwordsover 72 bytes; no automatic rehash.
fix(throttle): the cooldown is keyed by the SHA-256 of the exact username, holds no copy of it, drops idle entries,and logs a bounded form (first 64 bytes plus a digest prefix).
fix(routes): every form reads its secrets and identity fields from the POST body (/register,/password,/set-password,/admin/register, the admin reset); the sign-in origin check trusts the configuredbaseUrloriginper
Routes;Cache-Control: no-storealso on/oidc/auth,/login,/register,/logoutand/token.fix(user): stored hashes are bounded before any decode (PHC at most 512 bytes, exact parameter grammar; bcryptexactly 60 bytes in its alphabet with cost 4 to 15; legacy at most 64 bytes); anything else takes the gated dummy
refusal without holding a slot; a user ID longer than the 256-byte salt bound can no longer receive a new hash.
256 bytes need migration).
body value winning over a query copy, and the old session surviving an authorization request with an unregistered
redirect; the cooldown log is bounded for escaped prefixes too; README gains the recovery for accounts whose ID exceeds
the salt bound (inventory with
jq, offline credential replacement, do not shorten the ID).Test plan
just verify,just test-race,just test-e2e(in a Linux container; the e2e client sends noSec-Fetchheaders and stays green) on
fbb12a7.TestFormWritesPreferTheBody,TestOidcAuth_UnregisteredRedirectKeepsTheSession,TestClientSecretFailures,TestClientSecretFailuresThroughTheWiring,TestCounter,TestFormSecretsFromTheBodyOnly,TestFormWritesReadTheBody,TestKDFBusy_Is503,TestKDFGate_AdmitsEveryDerivation,TestKDFGate_BusyRefusesEveryDerivation,TestKDFGate_ConcurrentLogins,TestKDFGate_ProductionDefaults,TestKDFGate_Waiters,TestLimiter,TestLimiterBounded,TestLimiterDropsIdleEntries,TestLimiterRetainsNoUsername,TestLoginCrossOrigin,TestLoginFailed_LogsABoundedUsername,TestLogin_Cooldown,TestLogin_CooldownEndsWithAReset,TestNewServer,TestOidcAuth_DirectPostFromTheRelyingParty,TestOidcAuth_InvalidRequestCreatesNoSession,TestPasswordWrite_LongIDKeepsThePassword,TestPasswordWrites_RefuseALongID,TestSecureCookieFromScheme,TestSecurityHeaders,TestSignInTrustsTheConfiguredOrigin,TestValidateWarnsShortClientSecret,TestVerifyArgon2id_ParameterBounds,TestVerifyPassword_BoundsBeforeDecoding,TestVerifyPassword_RefusalDerives.36 logins were served and 28 refused with 503 and
Retry-After: 2, the refusals answered within the 2 s deadline (theslowest in 1.3 ms with production-cost hashes).
ReadHeaderTimeoutdropped;X-Frame-Optionsdropped;no-storereplaced; cross-origin sign-in check disabled; login fields read from the mergedform; gate overflow deriving instead of refusing; a long bcrypt password accepted; the dummy derivation removed from
refusals; the argon2 memory bound raised; cooldown after 500 failures; cooldown not enforced at login.
dropped; the cooldown log printing the whole username; the trusted origin pointing at another host;
Sec-Fetch-Sitenolonger deciding; the bcrypt cost ceiling removed; the PHC length bound removed; the bcrypt alphabet check removed; a long
user ID still salting a new hash;
/oidc/authoutsideno-store;/registerfields read from the merged form: allkilled. One survived on the first fix-up head (the
/set-passwordtoken reader switched back to the merged form, becausethe earlier rows always sent the token in the body); the per-field rows added afterwards catch it (re-run: killed).
Compatibility
(no
Origin, noSec-Fetch-Site) are unaffected.trade-off (an attacker who knows a username can keep that account in cooldown) and the recovery are in the
README.
Retry-After: 2instead of waiting.LISTEN_ADDRcan bind the listener to one interface.Not in this PR
documented).
/tokenand/revoke(client_secret_postandnoneare the supportedmethods).
Stacked on #38 (
fix/oidc-lifecycle-and-protocol-on-chain), which stacks on #37, #36, #35 and #32. Draft until the stack is merged.