Repository navigation
fix: limit failed logins per source address and bound bcrypt verification - #718
Open
LeeroyHannigan wants to merge 17 commits into
Open
LeeroyHannigan wants to merge 17 commits into
LeeroyHannigan wants to merge 17 commits into
Conversation
…source Every backend verified passwords with its own spawn_blocking call and no bound on how many ran at once; only hashing in the server took a permit. Move verification into extenddb_storage::password::verify_password, which shares one process-wide semaphore of four slots with hashing, waits at most ten seconds for a slot, and answers OpError::Internal when none frees so the caller returns a server error instead of queueing without bound. RateLimitStore::count_principal_failures now takes the source address and counts only failures from that address, on all four backends and the test double, so failures from one source do not lock the principal out of others.
…pts before the password check The management API and the console refused a principal after five failed logins counted over every address, and every management handler except role assumption passed no source address, so five wrong passwords for a known admin name from anywhere answered 429 to that admin for fifteen minutes, from every address, on both surfaces. The limit check also ran before the password check and the failure was recorded after it, so a burst of concurrent wrong passwords ran one bcrypt verification each, past the cap of five. Limits now apply per principal and peer address (5 failures in 15 minutes) and per peer address (20 failures in 15 minutes). Every management handler passes the connection's peer address through a ConnectInfo extractor; the console already had it. An attempt reserves its slot in a process-wide registry before any password is checked and holds it until the failure is recorded, so a burst is admitted up to the cap and the rest answer 429. A storage error before or during the password check answers 500 and records nothing against the caller. Hashing in the server takes its permit from the bcrypt semaphore shared with verification in the storage crate.
Add a Login Rate Limiting section to the security model manual with the limits, the reservation before the password check, the bcrypt bound, the source address rule and what a reverse proxy means for it. Point the architecture guide at it, describe what RateLimitStore counts in the storage design doc, and list the trait's real methods in the storage extension manual.
verify_password took a slot from the bcrypt semaphore in the async caller, and the slot was released when the caller's future was dropped. A login request cancelled while bcrypt ran (a client that closed the connection) freed its slot at once while the blocking call kept running, so more than four bcrypt calls could run at the same time. The permit now moves into the spawn_blocking closure and is dropped there after bcrypt returns. Hashing moves into extenddb_storage::password beside verification and takes the same path, and the server's hash_password calls it. The new tests cancel four callers while bcrypt runs, start four more verifications, and check that no more than four bcrypt calls ran at once.
The login limits were keyed on the peer's full address. An IPv6 client can send from any address in the /64 assigned to it, so it could use a new address for every attempt and never reach either limit. begin_login now keys a non-mapped IPv6 peer by its /64 prefix (2001:db8:1:2::/64) for the limiter. IPv4 peers and IPv4-mapped IPv6 peers keep the full IPv4 address, and aws:SourceIp still sees the full address. The security model states the /64 rule.
rate_limit.rs holds 267 lines of code and 285 lines of tests. The tests move unchanged to rate_limit/tests.rs, following leading_keys.rs, so the tests added next do not take the file past 600 lines.
An admitted attempt keeps its reservation until its failure row is written. The new test holds one row write open and checks that an admission for the same principal and source made during the write is refused.
A login attempt whose principal is longer than 256 bytes is refused before the rate limiter reads the store or reserves an attempt. The management API and /metrics answer 401 Invalid credentials, the console shows its invalid credentials page, and nothing is recorded. Admin names are at most 64 bytes and account_id/user_name at most 141, so no principal that can log in is affected. Each surface now builds its answer to a refused attempt in one function, and unit tests check each answer: 429 for a limit, 500 when the attempts store cannot be read, and the wrong password answer for a principal over the length limit.
The console login handler now checks the credentials in one function and settles the attempt in another, so both can be tested without a catalog. Unit tests check that a store error from the admin password check is returned as an error and not read as an unknown admin, that a store error during the password check answers 500 and records no failure, and that a wrong password answers the login form's error and records one.
The line logged when a principal is refused for a source address printed
the principal as the caller sent it, so a principal containing a newline
could start a forged log line. The principal is now printed with {:?},
which escapes newlines and other control characters. A principal that
reaches this line is at most 256 bytes long.
A password check that waits more than 10 seconds for a bcrypt slot is refused, and the storage crate logs the refusal at WARN under its own message. The management API and the console then logged the same refusal again at ERROR as "DB error during auth". They now skip that ERROR line for the overload and log every other storage error as before.
…n addresses tests/test_rate_limiting.py sent every failed login from 127.0.0.1, and the limit of 20 failures per source address now covers the management API, so a third run within 15 minutes refused the admin on 127.0.0.1 and failed in setup. Each test now sends from two random addresses in 127.0.0.0/8, so the file can be run again at once and records nothing against 127.0.0.1. A test skips when the endpoint is not a loopback address or the address cannot be bound. New tests check, on each backend the suite runs against, that failures for a user from one address do not refuse that user from another, that an address is refused after 20 failures across users, and that the admin password check on the management API, the console login form and /metrics each record failures and refuse after five.
The security model now lists the callers that reach ExtendDB from one address and so share one set of login limits: a proxy or load balancer that does not preserve the client address, the server host itself (extenddb manage and a /metrics scraper), and a NAT gateway. It says how to recover an address that is refused, and advises putting a proxy or load balancer in front of ExtendDB only when it preserves the client address, or when callers behind it are trusted not to lock each other out. The deployment checklist's proxy line says the same. Docs and comments that described the limits as account lockout now say login rate limiting per user and source address.
The burst and cancellation tests queue wrong-password checks behind running bcrypt calls. When the host is slow enough that no slot frees within the 10 second wait, a queued check is refused as overloaded, and the tests failed on that refusal although no bcrypt call ran past the bound. A queued check may now be refused as wrong or as overloaded; the high-water mark checks are unchanged.
The warning written when a principal reaches its limit from one source address began "Account lockout:". With the per-address limits the principal is refused from that address only and no account is locked, so the line now begins "Login refused:". The unit test that checks the line escapes the principal is renamed to match.
LeeroyHannigan
requested review from
amrith,
c33howard,
jcshepherd,
pdf-amzn,
robinnsc and
yesyayen
as code owners
November 1, 2026 01:24
6 of 8 tasks
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
test_admin_wrong_password_is_counted_on_the_management_api sent its requests to GET /management/admins. On Cassandra that route answers 500 to every caller, because the bootstrapper writes the first admin without created_at and list_admins cannot read the row. The test's last request, the right password from the second address, therefore got 500 instead of 200 on Cassandra. The test now uses GET /management/accounts, which also requires an admin and lists rows that carry created_at on every backend. The Cassandra route is #643.
6 of 8 tasks
4 of 8 tasks
hitesh96db
approved these changes
Nov 3, 2026
This branch has not been deployed
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.
What
Login limits are now per source address, and an attempt reserves its slot before the password is checked.
Limits (values unchanged):
RateLimitStore::count_principal_failurestakes the source address and counts only that (principal, address) pair. All four backends (SQLite, PostgreSQL, Cassandra, MongoDB) filter on thesource_ipcolumn they already stored.Every management handler now takes the connection's peer address through axum's
ConnectInfoextractor and passes it toauthenticateandauthenticate_admin, which take aSocketAddrinstead ofOption<&str>. The console login and/metricsuse the same path. The limiter counts an IPv4 peer, and an IPv6 peer carrying an IPv4-mapped address, under the IPv4 address. Any other IPv6 peer counts under its /64 prefix, because a client can send from any address in the /64 it holds and would otherwise get a fresh set of limits for every address it picks.aws:SourceIpstill sees the full address.rate_limit::begin_loginreplacescheck_login_allowedandrecord_failed_login. It increments an in-process count of attempts in flight for the pair and for the address, then reads the two recorded counts, and admits the attempt only if recorded plus in-flight is within both limits. The returnedLoginAttemptholds the reservation;record_failurewrites the row and then releases it, and dropping the attempt (success, storage error, cancelled request) releases it without a row. An earlier attempt is therefore visible to a concurrent admission check either in flight or in the table, and a burst is admitted up to the cap with the rest answered 429 before any password check. A principal longer than 256 bytes is answered as a wrong password before the store is read or anything is reserved; admin names are at most 64 bytes andaccount_id/user_nameat most 141.bcrypt verification moves into
extenddb_storage::password::verify_passwordand the server's hashing intoextenddb_storage::password::hash_password. Both take a permit from one process-wide semaphore of four slots and move it into the blocking bcrypt call, which releases it when bcrypt returns. A request cancelled while bcrypt runs, such as a client that disconnects mid-login, keeps its slot until bcrypt finishes, so hashing and verification together never run more than four bcrypt calls at once per process. All eight verify sites in the four backends callverify_password. A verification that waits more than 10 seconds for a slot is answered 500 and not recorded as a failure, since no password was checked; the storage crate logs it once at WARN, and the handlers no longer log it again as a database error. Hashing waits without a timeout, as before.Waits added by this change:
Source address and shared addresses
The source is the peer address of the TCP connection. ExtendDB does not read
X-Forwarded-Foror any other forwarded-address header; they are caller-supplied. This is the existing policy foraws:SourceIpand is unchanged. Callers that reach ExtendDB from one address share one set of limits: five failures for a user from any of them, or twenty in total, refuse that user or every user to all of them for fifteen minutes. This covers callers behind a reverse proxy or load balancer that opens its own connections to ExtendDB,extenddb manageand a/metricsscraper running on the server host, and clients behind one NAT gateway. The security model manual lists these cases, advises putting a proxy or load balancer in front of ExtendDB only when it preserves the client address or when callers behind it are trusted not to lock each other out, and gives the recovery: wait fifteen minutes, or delete the address's rows fromlogin_attempts.A password can still be guessed against one principal from many addresses at five attempts per address per fifteen minutes; the per-address limit and the password's strength are the bounds on that. A legitimate user is no longer affected by anyone else's failures; their own address is refused for a principal after five wrong passwords from it, for fifteen minutes, as before.
Docs
docs/manuals/10-security-model.mdgains a Login Rate Limiting section with the limits, the reservation, the bcrypt bound, the source address rule with the /64 prefix for IPv6, and the callers that share an address (a proxy or load balancer, the server host, a NAT gateway) with the recovery. The architecture guide, the storage design doc, the storage extension manual (whose method table listed methods the trait never had) and the deployment checklist's reverse-proxy line are updated, and the docs and comments that called the limits account lockout now say login rate limiting per user and source address.Not covered by this change
login_attemptstable keeps one row per principal and millisecond, so failures recorded in the same millisecond collapse into one row, and its per-address count scans the whole table, which this change now runs on every management request and/metricsscrape.login_attemptscollection, while the store writes and counts failures infailed_logins.initseeds an admin row and a default account that the management API can't read #643: on Cassandra,GET /management/adminsanswers 500 on every cataloginitcreated, because the first admin row has nocreated_at. The management API test oftests/test_rate_limiting.pytherefore logs in onGET /management/accounts, which also requires an admin.Merging with #704
#704 (console session revocation) verifies console passwords in its own
verify_loginincrates/server/src/console/pages/auth_pages.rs, which takes no bcrypt slot. The two changes conflict incrates/server/src/console/pages/auth_pages.rsandcrates/server/src/management/password.rs. Whichever of the two merges second:verify_loginthroughextenddb_storage::password::verify_password, answering its error with a 500 that records nothing, so console logins stay within the four-slot bound.crate::management::password::verify_password, which runsbcrypt::verifyon the blocking pool with no bound and has no caller onceverify_loginis rerouted.CredentialStampthrough this change'scheck_credentialsandsettle_attempt, which return the identity only, and callsstate.sessions.create(identity, stamp).auth_pages.rsaPasswordHashStorefake in place ofFailingAdmins, anAdminStorefake, because fix(console): end console sessions when the principal is deleted or its password changes #704'stry_admin_logintakes&dyn PasswordHashStore. The console bcrypt-timeout test can then reach the log helper only through the store read, because the overload arises insideverify_login; the management testa_bcrypt_slot_timeout_answers_500_and_is_not_logged_as_a_store_errorstill covers the helper.A trial merge done this way passed the server crate's 270 tests, and
tests/test_rate_limiting.pywithtests/test_console_session_revocation.pyagainst a SQLite build of it passed 10, with 1 skipped that needs a second server.Why
On main, the limit of five failed logins for a principal counted that principal's failures from every address. The management API handlers passed no source address to the limiter, so the limit of 20 failures per address applied only to the console login and to role assumption. Five wrong passwords for a known admin name, sent from anywhere, made ExtendDB answer
429 Too Many Requeststo that admin for fifteen minutes on every surface, even with the correct password, and the sender could repeat this indefinitely.The limit was also checked before the password was verified and the failure recorded after, with no bound on how many bcrypt verifications ran at once (the server's semaphore covered hashing only). A burst of concurrent wrong passwords ran one verification each, past the cap of five.
Reproduction on main, against a SQLite server with the default
adminuser:GET /management/adminswithAuthorization: Basic admin:<wrong password>.Observed: 401 for all 80, 80 rows in
login_attempts, and the next request with the correct password from any address answers 429.admin. From address B, send the correct password.Observed: 429 from B.
Testing done
Tests
Unit,
crates/storage/src/password.rs: the right password is accepted and a wrong one refused; a malformed hash counts as a wrong password and returns no error; a hash verifies against its password; 32 concurrent verifications on 8 worker threads never run more than 4 at once (high-water mark); four verifications, and separately four hashes, whose callers are cancelled while bcrypt runs keep their slots, so four verifications started after the cancellation never push the high-water mark past 4. On a slow host a queued check in these tests may be refused as overloaded after its 10-second wait; the high-water mark is checked either way.Unit,
crates/server/src/rate_limit/tests.rs, with an in-memory attempts store: five failures from A refuse A for that principal and B is admitted with no row written; A is still admitted for another principal; 20 failures over 20 names from one address refuse the 21st name there and not elsewhere; 40 concurrent attempts against a store with a 50 ms read delay admit exactly 5, with 5 in flight and 0 rows, and after they are recorded the next attempt is refused and the registry entry is gone; an admission made while a failure's row write is held open is refused; a dropped attempt leaves no row and no reservation; a store error answers Internal and holds no reservation;[::ffff:198.51.100.60]shares the limit of198.51.100.60; an IPv6 peer is keyed by its /64, and addresses in one /64 share the pair and per-address limits while the next /64 does not; a principal of 257 bytes is refused before the store is read and one of 256 bytes reaches the store; the refusal log line escapes a newline in the principal.Unit,
crates/server/src/management/auth.rsandcrates/server/src/console/pages/auth_pages.rs: each surface answers a refused attempt with 429 for a limit, 500 when the attempts store cannot be read, and its wrong-password answer for a principal over 256 bytes; a store error from the admin password check is returned as an error and logged at ERROR, and a bcrypt slot timeout is returned as an error and not logged as a database error; on the console, a store error during the password check answers 500 and records nothing, and a wrong password answers the form's error and records one failure.Integration,
tests/test_rate_limiting.py, which CI runs against SQLite, PostgreSQL, Cassandra and MongoDB. Each test sends from two random addresses in 127.0.0.0/8, so the file records nothing against 127.0.0.1 and can be run again at once; a test skips when the endpoint is not a loopback address or the address cannot be bound. Five failures for a user from one address refuse that user there and not from the other address; 20 failures over 20 names from one address refuse a 21st name there and not from the other; the admin password check on the management API, the console login form and/metricseach answer 429 to the right password after five failures from one address and accept it from the other. Against one SQLite server the file passed three times back to back (7 tests each, about 22 seconds a run), left 150 rows from 24 addresses and none from 127.0.0.1, and the admin then logged in from 127.0.0.1. Against one PostgreSQL server it passed twice back to back (7 tests each) and left 100 rows from 16 addresses and none from 127.0.0.1.Live, SQLite, management API and console, A = 127.0.0.1 and B = 127.0.0.2: five wrong
adminpasswords from A, then 429 for the sixth and for the correct password from A; 200 (management) and 303 (console) for the correct password from B; 401 (management) or the form's error (console) for a wrong password as another principal from A; rows(admin, 127.0.0.1) 5.Live, 80 concurrent wrong
adminpasswords: 429 for 75, 401 for 5, 5 rows, 429 for the next attempt. Refusals took 38 to 175 ms including the TLS handshake.Negative controls
Each on the committed tree and reverted with git afterwards:
{}: the log test fails and its output shows the forged line.tests/test_rate_limiting.py, with four changes at once: the admin's wrong password not recorded on the management API (the management API and/metricstests fail), the SQLite per-user count taken over every address (the two-address test fails), the SQLite per-address count forced to 0 (the 20-name test fails), and the console's wrong password not recorded (the console test fails). The two older tests pass.Commands run
cargo +1.97.0 fmt --all -- --check: clean.cargo +1.97.0 clippy --locked --workspace --all-targets -- -D warnings: clean.cargo test --locked -p extenddb-storage -p extenddb-storage-sqlite -p extenddb-serveron Rust 1.99.0 and on 1.88.0: server 257, storage 145 and storage-sqlite 163 passed with 1 ignored, on each.cargo +1.88.0 check --locked --workspace --all-targets: clean.tests/test_rate_limiting.pywith the console, management, metrics, auth and cache test files, 9 files, 161 passed and 1 skipped.tests/test_rate_limiting.pyagainst a PostgreSQL build, twice: 7 passed each.These ran before the last two commits. The first changes the refusal log line's first words and the name of the test that checks it; after it,
cargo +1.97.0 fmt --all -- --check,cargo +1.97.0 clippy --locked -p extenddb-server --all-targets -- -D warningsand the server crate's rate limit unit tests ran again. The second moves the management API test fromGET /management/adminstoGET /management/accounts(see #643), and CI runs it on all four backends.Checklist
cargo test --workspace)cargo fmt --check)cargo clippy -- -W clippy::pedantic)Storagetrait, auth model, on-diskformat, or public CLI surface, an RFC has been accepted or is linked
below. Otherwise, an ADR captures the decision (link below).
CI ran the workspace tests on Rust 1.88.0 and on stable, and the integration suites on all four backends, and all of them passed. CodeQL reports 13 hard-coded cryptographic value alerts, each on a password literal inside a test module. Clippy ran with
-D warnings, without the pedantic group.RateLimitStore::count_principal_failuresgains a source address parameter, which all four backends implement. It has no RFC or ADR.ADR / RFC: none
Breaking changes
There are no configuration or schema changes. These behaviours change:
Internal error)./metrics. On main it applied to the console login and to role assumption, the one management handler that passed an address./metricscarry no source address and stop counting toward any limit after upgrade, a one-time loosening of at most fifteen minutes. Console and role-assumption rows for an IPv4 peer carry the address and keep counting. Measured on SQLite with three console rows and two management rows that main wrote foradminfrom 127.0.0.1: after upgrade, wrong management passwords foradminfrom 127.0.0.1 answered 401, 401, then 429. From the code, not measured: rows main stored for an IPv6 peer, and console rows it stored as::ffff:a.b.c.don a dual-stack listener, stop matching, because the limiter now keys those peers by /64 prefix or by IPv4 address./metricsand with the login form's error on the console, and nothing is recorded for it.Login refused:(it beganAccount lockout:) and prints the principal with{:?}, so newlines and control characters in it are escaped.By submitting this pull request, I confirm that my contribution is made under
the terms of the Apache License 2.0 and I agree to the Developer Certificate of
Origin (DCO). See CONTRIBUTING.md for details.