Repository navigation
fix(auth): run one bcrypt check per login whether or not the name exists - #847
Open
LeeroyHannigan wants to merge 1 commit into
Open
LeeroyHannigan wants to merge 1 commit into
LeeroyHannigan wants to merge 1 commit into
Conversation
A login to the management API, the /metrics endpoint or the console ran
one bcrypt verification, about 100 ms at the default cost of 12, when
the name belonged to an admin or to an IAM user with a console password,
and none for any other name. The time a login took therefore told the
caller whether the name existed.
Every login now runs one bcrypt check. The admin stores are unchanged:
an existing admin's hash is checked as before, and an unknown name
returns not found without a check, after which the login runs the IAM
user step.
In that step, every backend's verify_iam_user_password passes the
user's hash, or None for an unknown account, an unknown user or a user
without a console password, to the new function
extenddb_storage::password::verify_stored_password. A name without a
slash names no IAM user, and the management API and the console pass
None for it themselves. With None, verify_stored_password checks the
password against a fixed hash at the default cost through
verify_password, so it waits for one of the four bcrypt slots as a
check against a stored hash does, and it refuses the login whatever
bcrypt returns.
bcrypt checks per login, on main and with this change:
main now
name without a slash, an admin has the name 1 1
name without a slash, no admin has the name 0 1
account_id/user_name, user with a console password 1 1
account_id/user_name, user without a console password 0 1
account_id/user_name, unknown user or unknown account 0 1
admin whose name contains a slash 1 1
A login the rate limiter refuses runs no check, on main and now.
Status codes, messages and what the rate limiter records are as before.
A check for a name that does not exist now waits for a bcrypt slot, so
after 10 seconds without one it is answered 500, as a check for an
existing name is.
Under the test-util feature of extenddb-storage, verify_password records
the bcrypt cost of each hash it checks, and
password::count_verifications returns those costs. The server tests run
nine logins through the management API's authenticate and through the
console's credential check against an in-memory SQLite catalog. They
assert one check per login, at the default cost for the four logins that
name no principal with a password and at the test hashes' cost for the
other five. The same assertions cover verify_iam_user_password in the
SQLite unit tests, in a new PostgreSQL integration test and in the
Cassandra direct integration tests. The server crate gains three
dev-dependencies: extenddb-storage with test-util,
extenddb-storage-sqlite, and sqlx with the sqlite feature.
The security model manual states the rule under Login Rate Limiting. The
storage extension guide states what each backend's verify_admin_password
and verify_iam_user_password must do.
LeeroyHannigan
requested review from
amrith,
c33howard,
eliseong,
hitesh96db,
jcshepherd,
navareamol,
pdf-amzn and
robinnsc
as code owners
November 5, 2026 03:17
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| let (right, right_costs) = count_verifications(catalog_store.verify_iam_user_password( | ||
| &account_id, | ||
| "alice", | ||
| "correct horse", |
| let (verified, costs) = count_verifications(catalog_store.verify_iam_user_password( | ||
| account, | ||
| user_name, | ||
| "battery staple", |
| store.create_user(ACCOUNT, "bob", None).await.unwrap(); | ||
|
|
||
| let (right, right_costs) = | ||
| count_verifications(store.verify_iam_user_password(ACCOUNT, "alice", "correct horse")) |
| let (verified, costs) = count_verifications(store.verify_iam_user_password( | ||
| account_id, | ||
| user_name, | ||
| "battery staple", |
| s.create_user(ACCOUNT, "bob", None).await.unwrap(); | ||
|
|
||
| let (right, right_costs) = | ||
| count_verifications(s.verify_iam_user_password(ACCOUNT, "alice", "correct horse")) |
| let (verified, costs) = count_verifications(s.verify_iam_user_password( | ||
| account_id, | ||
| user_name, | ||
| "battery staple", |
| let _counters = COUNTERS.lock().await; | ||
| let hash = bcrypt::hash("correct horse", COST).unwrap(); | ||
| let (wrong, wrong_costs) = count_verifications(verify_stored_password( | ||
| "battery staple".to_owned(), |
| assert!(!wrong.unwrap(), "a wrong password verified"); | ||
| assert_eq!(wrong_costs, [COST]); | ||
| let (absent, absent_costs) = | ||
| count_verifications(verify_stored_password("battery staple".to_owned(), None)).await; |
| let slots = u32::try_from(MAX_CONCURRENT_BCRYPT).unwrap(); | ||
| let held = BCRYPT_SEMAPHORE.acquire_many(slots).await.unwrap(); | ||
| let check = tokio::spawn(count_verifications(verify_stored_password( | ||
| "pw".to_owned(), |
| for hash in ["not a bcrypt hash".to_owned(), unreadable_salt] { | ||
| assert!(bcrypt::verify("x", &hash).is_err(), "{hash}"); | ||
| let (verified, costs) = | ||
| count_verifications(verify_password("x".to_owned(), hash)).await; |
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
A password login now runs one bcrypt check whether or not the name exists. This covers the management API (HTTP Basic auth),
/metricsand the console login./that an admin has, right or wrong password/that no admin hasaccount_id/user_name, the user has a console password, right or wrong passwordaccount_id/user_name, the user has no console passwordaccount_id/user_name, no such user or account/, right or wrong passwordextenddb_storage::password::verify_stored_passwordtakes the stored hash, orNonewhen there is none. WithNone, it checks the password against a fixed bcrypt hash at cost 12 throughverify_password. So the check waits for one of the four bcrypt slots, as a check against a stored hash does. It refuses the login whatever bcrypt returns.verify_iam_user_password(SQLite, PostgreSQL, MongoDB and Cassandra) passes the stored hash to the helper. It passesNonefor an unknown account, an unknown user and a user without a console password.verify_admin_passworddoes not change. For an unknown admin it returnsNonewithout a check. The login then goes to the IAM user step, which runs one check for any name. For a name without/, that step checks the fixed hash.docs/manuals/12-extending-extenddb-storage.md) describe the new rule.Why
Closes #688.
A wrong password for an existing admin or IAM user ran one bcrypt check, about 100 ms at cost 12. A name that did not exist ran no check. So the time of a login told the caller whether the name existed.
adminis the default admin name in the compose files.Testing done
The test-only
count_verificationsrecords the bcrypt cost of each check that bcrypt runs. A hash that bcrypt refuses records nothing. So a test sees both the number of checks and their cost.Two server tests run against an in-memory SQLite catalog.
every_login_path_runs_one_bcrypt_checkgoes throughauthenticate, andevery_credential_check_runs_one_bcrypt_checkgoes through the console'scheck_credentials. Each covers 9 cases:/team/rootwith a wrong passwordEach case runs exactly 1 check and ends with the expected outcome. A refused case records 1 failure, and an accepted case records none. The 4 cases with no stored hash run their check at cost 12. The other 5 run at the cost of the test hashes, 4.
Storage tests: the fixed hash has cost 12 and bcrypt decodes it. A check without a stored hash runs 1 check at cost 12 and refuses. With all four slots held, it waits and does not run. A hash that bcrypt refuses records no check.
IAM store tests cover a wrong password, a user without a console password, an unknown user and an unknown account:
crates/storage-postgres/tests/iam_user_password.rs: passed against a localpgvector/pgvector:pg16server, the image that CI uses. CI runs it in the PostgreSQL storage test step.crates/storage-cassandra/tests/direct/users.rs: it compiles. It has not run yet, so its first run is in the CIdirect_integrationstep.Unit tests: server 296, storage 218, SQLite 236, PostgreSQL 75, MongoDB 165 and Cassandra 125 passed.
cargo fmt --checkandcargo clippy --all-targets -- -D warningson the six changed crates pass.Negative controls, each one made a test fail:
try_iam_user_auth, or intry_iam_loginBehaviour under load
A login for a name that does not exist now waits for a bcrypt slot. When the server is overloaded and no slot frees up in 10 seconds, the answer is HTTP 500, as it is for an existing name. Before this change, the answer for such a name was 401 at once.
Merging with other PRs
try_iam_loginincrates/server/src/console/pages/auth_pages.rs. It reads the password hash itself. It returns before any bcrypt check in two places: for a name without/, and for a user without a hash. The PR that merges second must keep the rule of this PR there:verify_stored_password(password.to_owned(), None)at both early returns, and answer its error with 500, as for the hash read.check_credentialson fix(console): end console sessions when the principal is deleted or its password changes #704 returnsOption<(CallerIdentity, CredentialStamp)>. Adaptevery_credential_check_runs_one_bcrypt_checkto it, and keep all nine cases.verify_logindoc on fix(console): end console sessions when the principal is deleted or its password changes #704, and the last sentence of the backend guide indocs/manuals/12-extending-extenddb-storage.md. After the merge, the console reads the hash itself and runs the fixed-hash check when the read returnsNone.Cargo.lock, keep the lines from both sides.[dev-dependencies]incrates/server/Cargo.toml. Nine open PRs also add lines there. Keep every line, and keep one copy ofextenddb-storage-sqlite.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).
ADR / RFC: n/a. The trait signatures do not change. The change adds one bcrypt check on the login paths that ran none.
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.