From 97470ff827699adaeabbcee455da9c1a567df727 Mon Sep 17 00:00:00 2001 From: Lee Hannigan <29015211+LeeroyHannigan@users.noreply.github.com> Date: Thu, 5 Nov 2026 01:05:59 +0000 Subject: [PATCH] fix(auth): run one bcrypt check per login whether or not the name exists 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. --- Cargo.lock | 2 + crates/server/Cargo.toml | 5 + crates/server/src/console/pages/auth_pages.rs | 64 ++++- crates/server/src/management/auth.rs | 255 ++++++++++++++++-- .../src/management_store/users.rs | 17 +- .../storage-cassandra/tests/direct/users.rs | 78 ++++++ .../storage-mongodb/src/management_store.rs | 15 +- .../src/management_store/users.rs | 10 +- .../tests/iam_user_password.rs | 121 +++++++++ .../src/management_store/users.rs | 85 +++++- crates/storage/Cargo.toml | 5 +- crates/storage/src/management_store/mod.rs | 9 +- crates/storage/src/password.rs | 147 +++++++++- docs/manuals/10-security-model.md | 2 + docs/manuals/12-extending-extenddb-storage.md | 2 +- 15 files changed, 742 insertions(+), 75 deletions(-) create mode 100644 crates/storage-postgres/tests/iam_user_password.rs diff --git a/Cargo.lock b/Cargo.lock index 6cb6f19e4..5c101d9cf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1799,6 +1799,7 @@ dependencies = [ "extenddb-core", "extenddb-engine", "extenddb-storage", + "extenddb-storage-sqlite", "futures", "hyper", "libc", @@ -1807,6 +1808,7 @@ dependencies = [ "rustls", "serde", "serde_json", + "sqlx", "syslog-tracing", "time", "tokio", diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index 00814e1a2..cad7ba2c6 100755 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -50,3 +50,8 @@ libc = { workspace = true } [dev-dependencies] extenddb-cache = { workspace = true, features = ["test-util"] } +# The login tests count bcrypt checks with `count_verifications` against an +# in-memory SQLite catalog. +extenddb-storage = { workspace = true, features = ["test-util"] } +extenddb-storage-sqlite = { workspace = true } +sqlx = { workspace = true, features = ["sqlite"] } diff --git a/crates/server/src/console/pages/auth_pages.rs b/crates/server/src/console/pages/auth_pages.rs index f590cf1a4..a272a1490 100755 --- a/crates/server/src/console/pages/auth_pages.rs +++ b/crates/server/src/console/pages/auth_pages.rs @@ -213,26 +213,35 @@ async fn try_admin_login( }) } -/// `Ok(None)` is a wrong password or an unknown user; `Err(())` is a storage -/// error, which the caller answers 500 and records as nothing. +/// `Ok(None)` is a wrong password, an unknown user or a name without `/`; +/// `Err(())` is a storage error, which the caller answers 500 and records as +/// nothing. A name without `/` names no IAM user. It is checked against a +/// fixed hash, so that it runs one bcrypt check, as a login for an existing +/// admin or IAM user does. async fn try_iam_login( username: &str, password: &str, store: &dyn extenddb_storage::management_store::ManagementStore, ) -> Result, ()> { - let Some((acct_id, uname)) = username.split_once('/') else { - return Ok(None); + let user = username.split_once('/'); + let checked = match user { + Some((acct_id, uname)) => { + store + .verify_iam_user_password(acct_id, uname, password) + .await + } + None => extenddb_storage::password::verify_stored_password(password.to_owned(), None).await, }; - let ok = store - .verify_iam_user_password(acct_id, uname, password) - .await - .map_err(|e| { - log_password_check_error("Console login: DB error during IAM user auth", &e) - })?; - Ok(ok.then(|| CallerIdentity::IamUser { - account_id: acct_id.to_owned(), - user_name: uname.to_owned(), - })) + let ok = checked.map_err(|e| { + log_password_check_error("Console login: DB error during IAM user auth", &e) + })?; + Ok(match user { + Some((acct_id, uname)) if ok => Some(CallerIdentity::IamUser { + account_id: acct_id.to_owned(), + user_name: uname.to_owned(), + }), + _ => None, + }) } #[cfg(test)] @@ -323,4 +332,31 @@ mod tests { assert!(body_text(page).await.contains("Invalid credentials")); assert_eq!(attempts.rows_for("admin", "198.51.100.91"), 1); } + + #[tokio::test] + async fn every_credential_check_runs_one_bcrypt_check() { + use extenddb_storage::password::count_verifications; + + use crate::management::auth::tests::{LOGIN_CASES, Outcome, login_catalog}; + + let catalog = login_catalog().await; + for login in LOGIN_CASES { + let (checked, costs) = + count_verifications(check_credentials(login.username, login.password, &catalog)) + .await; + assert_eq!( + costs, + [login.cost], + "{}: bcrypt checks and their cost", + login.case + ); + let identity = checked.unwrap_or_else(|()| panic!("{}: storage error", login.case)); + assert_eq!( + Outcome::of(identity.as_ref()), + login.outcome, + "{}", + login.case + ); + } + } } diff --git a/crates/server/src/management/auth.rs b/crates/server/src/management/auth.rs index 521f7bbec..01a2f991b 100755 --- a/crates/server/src/management/auth.rs +++ b/crates/server/src/management/auth.rs @@ -66,7 +66,11 @@ pub enum CallerIdentity { /// Extract and verify credentials from the `Authorization: Basic ...` header. /// /// Tries admin auth first (`name:password`). If the admin name is not found, -/// tries IAM user auth (`account_id/user_name:password`). +/// tries IAM user auth (`account_id/user_name:password`). An admitted +/// attempt runs one bcrypt check whatever the name. When an admin has the +/// name, the check is against the admin's hash. Otherwise the IAM user step +/// runs it, against the user's hash, or against a fixed hash when no IAM user +/// with a console password has the name. /// /// Returns the caller identity on success, or an error response on failure. /// @@ -220,8 +224,12 @@ async fn try_admin_auth( } /// Check `password` for IAM user `account_id/user_name`. `Ok(None)` is a -/// wrong password or an unknown user; `Err` is the 500 answered to a storage -/// error. +/// wrong password, an unknown user or a name without `/`; `Err` is the 500 +/// answered to a storage error, including a timed-out wait for a bcrypt slot. +/// +/// A name without `/` names no IAM user. It is checked against a fixed hash, +/// so that it runs one bcrypt check, as a login for an existing admin or IAM +/// user does. #[allow(clippy::similar_names)] #[allow(clippy::result_large_err)] // axum Response as rejection is this module's idiom async fn try_iam_user_auth( @@ -229,37 +237,240 @@ async fn try_iam_user_auth( password: &str, store: &dyn extenddb_storage::CatalogStore, ) -> Result, Response> { - let Some((acct_id, uname)) = username.split_once('/') else { - return Ok(None); + let user = username.split_once('/'); + let checked = match user { + Some((acct_id, uname)) => { + store + .verify_iam_user_password(acct_id, uname, password) + .await + } + None => extenddb_storage::password::verify_stored_password(password.to_owned(), None).await, }; + let valid = checked.map_err(|e| { + log_password_check_error("Management API: DB error during IAM user auth", &e); + StatusCode::INTERNAL_SERVER_ERROR.into_response() + })?; - let valid = store - .verify_iam_user_password(acct_id, uname, password) - .await - .map_err(|e| { - log_password_check_error("Management API: DB error during IAM user auth", &e); - StatusCode::INTERNAL_SERVER_ERROR.into_response() - })?; - - if valid { - Ok(Some(CallerIdentity::IamUser { + Ok(match user { + Some((acct_id, uname)) if valid => Some(CallerIdentity::IamUser { account_id: acct_id.to_owned(), user_name: uname.to_owned(), - })) - } else { - Ok(None) - } + }), + _ => None, + }) } #[cfg(test)] pub(crate) mod tests { - use extenddb_storage::management_store::{AdminEntry, OpResult}; - use extenddb_storage::password::VERIFY_OVERLOADED; + use extenddb_storage::management_store::{AdminEntry, ManagementStore, OpResult}; + use extenddb_storage::password::{VERIFY_OVERLOADED, count_verifications}; use futures::future::BoxFuture; use super::*; use crate::rate_limit::PRINCIPAL_LIMITED; - use crate::rate_limit::tests::capture_logs; + use crate::rate_limit::tests::{MemoryStore, capture_logs, peer}; + + /// The password of every principal in [`login_catalog`]. + const PASSWORD: &str = "correct horse"; + + /// A password no principal in [`login_catalog`] has. + const WRONG: &str = "battery staple"; + + /// bcrypt's lowest cost, for the hashes in [`login_catalog`]. It keeps the + /// tests fast and tells a check against a stored hash apart from one + /// against the fixed hash, which is at the default cost. + const TEST_COST: u32 = 4; + + /// An in-memory SQLite catalog with the admins `alice` and `team/root`, + /// and in account 123456789012 the IAM user `bob`, who has a console + /// password, and `carol`, who has none. Every password is [`PASSWORD`], + /// hashed at [`TEST_COST`]. + pub(crate) async fn login_catalog() -> extenddb_storage_sqlite::SqliteCatalogStore { + // One connection keeps the in-memory database alive for the test. + let pool = sqlx::sqlite::SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .expect("open in-memory sqlite"); + let migrations = extenddb_storage_sqlite::migrations::SqliteMigrations::on_pool( + pool.clone(), + ":memory:", + ) + .expect("one connection"); + extenddb_storage::migrations::Runner::new(&migrations, "test") + .migrate() + .await + .expect("migrate the catalog"); + let catalog = extenddb_storage_sqlite::SqliteCatalogStore::new(pool); + let hash = bcrypt::hash(PASSWORD, TEST_COST).unwrap(); + for admin in ["alice", "team/root"] { + catalog.create_admin(admin, &hash).await.unwrap(); + } + catalog + .create_account("123456789012", "logins") + .await + .unwrap(); + catalog + .create_user("123456789012", "bob", Some(&hash)) + .await + .unwrap(); + catalog + .create_user("123456789012", "carol", None) + .await + .unwrap(); + catalog + } + + /// How a login attempt in [`LOGIN_CASES`] ends. + #[derive(Clone, Copy, Debug, PartialEq, Eq)] + pub(crate) enum Outcome { + Admin, + IamUser, + Refused, + } + + impl Outcome { + pub(crate) fn of(identity: Option<&CallerIdentity>) -> Self { + match identity { + Some(CallerIdentity::Admin(_)) => Self::Admin, + Some(CallerIdentity::IamUser { .. }) => Self::IamUser, + None => Self::Refused, + } + } + } + + /// A login attempt against [`login_catalog`]. + pub(crate) struct LoginCase { + pub(crate) case: &'static str, + pub(crate) username: &'static str, + pub(crate) password: &'static str, + pub(crate) outcome: Outcome, + /// The bcrypt cost of the one check the login runs: [`TEST_COST`] + /// against a stored hash, bcrypt's default cost against the fixed + /// hash. + pub(crate) cost: u32, + } + + /// One login attempt for each path through the admin and IAM user checks. + pub(crate) const LOGIN_CASES: &[LoginCase] = &[ + LoginCase { + case: "admin, right password", + username: "alice", + password: PASSWORD, + outcome: Outcome::Admin, + cost: TEST_COST, + }, + LoginCase { + case: "admin, wrong password", + username: "alice", + password: WRONG, + outcome: Outcome::Refused, + cost: TEST_COST, + }, + LoginCase { + case: "unknown name without a slash", + username: "nobody", + password: PASSWORD, + outcome: Outcome::Refused, + cost: bcrypt::DEFAULT_COST, + }, + LoginCase { + case: "IAM user, right password", + username: "123456789012/bob", + password: PASSWORD, + outcome: Outcome::IamUser, + cost: TEST_COST, + }, + LoginCase { + case: "IAM user, wrong password", + username: "123456789012/bob", + password: WRONG, + outcome: Outcome::Refused, + cost: TEST_COST, + }, + LoginCase { + case: "IAM user without a console password", + username: "123456789012/carol", + password: PASSWORD, + outcome: Outcome::Refused, + cost: bcrypt::DEFAULT_COST, + }, + LoginCase { + case: "unknown IAM user", + username: "123456789012/nobody", + password: PASSWORD, + outcome: Outcome::Refused, + cost: bcrypt::DEFAULT_COST, + }, + LoginCase { + case: "unknown account", + username: "111122223333/bob", + password: PASSWORD, + outcome: Outcome::Refused, + cost: bcrypt::DEFAULT_COST, + }, + LoginCase { + case: "admin named with a slash, wrong password", + username: "team/root", + password: WRONG, + outcome: Outcome::Refused, + cost: TEST_COST, + }, + ]; + + /// An `Authorization: Basic` header for `username` and `password`. + fn basic_auth(username: &str, password: &str) -> HeaderMap { + let token = + base64::engine::general_purpose::STANDARD.encode(format!("{username}:{password}")); + let mut headers = HeaderMap::new(); + headers.insert("authorization", format!("Basic {token}").parse().unwrap()); + headers + } + + #[tokio::test] + async fn every_login_path_runs_one_bcrypt_check() { + let catalog = login_catalog().await; + let attempts = MemoryStore::default(); + // Each case uses its own source address: the in-flight registry is + // process-wide. + for (n, login) in (1..).zip(LOGIN_CASES) { + let ip = format!("192.0.2.{n}"); + let headers = basic_auth(login.username, login.password); + let (result, costs) = + count_verifications(authenticate(&headers, &catalog, &attempts, peer(&ip))).await; + assert_eq!( + costs, + [login.cost], + "{}: bcrypt checks and their cost", + login.case + ); + let identity = match result { + Ok(identity) => Some(identity), + Err(response) => { + assert_eq!( + response.status(), + StatusCode::UNAUTHORIZED, + "{}", + login.case + ); + None + } + }; + assert_eq!( + Outcome::of(identity.as_ref()), + login.outcome, + "{}", + login.case + ); + let failures = i64::from(login.outcome == Outcome::Refused); + assert_eq!( + attempts.rows_for(login.username, &ip), + failures, + "{}: failures recorded", + login.case + ); + } + } /// An admin store whose password check fails with `OpError::Internal` /// carrying `message`. Logins call nothing else on it. diff --git a/crates/storage-cassandra/src/management_store/users.rs b/crates/storage-cassandra/src/management_store/users.rs index d076aef40..7c8bf3c9f 100644 --- a/crates/storage-cassandra/src/management_store/users.rs +++ b/crates/storage-cassandra/src/management_store/users.rs @@ -391,17 +391,12 @@ impl CassandraCatalogStore { ) .await?; - let Some(row) = row else { - return Ok(false); - }; - - let hash_opt: Option = row.get_by_name("password_hash").ok().flatten(); - - let Some(hash) = hash_opt else { - return Ok(false); - }; - - extenddb_storage::password::verify_password(password.to_owned(), hash).await + // `None` when the user is absent or has no console password, which + // still runs one bcrypt check (see `verify_stored_password`), as a + // wrong password does. + let hash: Option = + row.and_then(|row| row.get_by_name("password_hash").ok().flatten()); + extenddb_storage::password::verify_stored_password(password.to_owned(), hash).await } pub(crate) async fn change_user_password_impl( diff --git a/crates/storage-cassandra/tests/direct/users.rs b/crates/storage-cassandra/tests/direct/users.rs index b427aad20..3214f5049 100644 --- a/crates/storage-cassandra/tests/direct/users.rs +++ b/crates/storage-cassandra/tests/direct/users.rs @@ -423,4 +423,82 @@ mod tests { other => panic!("Expected NotFound, got: {:?}", other), } } + + /// Every check runs one bcrypt verification: against the user's hash + /// when the user has a console password, and against the fixed hash at + /// the default cost for a user without one, an unknown user and an + /// unknown account. + #[tokio::test] + async fn an_unknown_user_or_one_without_a_password_runs_one_bcrypt_check() { + use extenddb_storage::password::count_verifications; + + if crate::helpers::skip_without_cassandra() { + return; + } + let engine = setup_engine().await; + let config = test_config(); + let catalog_store = crate::helpers::create_catalog_store(&engine, &config); + + let account_id = unique_test_account(); + let missing_account = unique_test_account(); + catalog_store + .create_account(&account_id, &format!("TestAccount_{}", unique_test_id())) + .await + .expect("Failed to create account"); + // bcrypt's lowest cost keeps the test fast and tells a check against + // this hash apart from one against the fixed hash. + let test_cost = 4; + let hash = bcrypt::hash("correct horse", test_cost).expect("Failed to hash password"); + catalog_store + .create_user(&account_id, "alice", Some(&hash)) + .await + .expect("Failed to create user with password"); + catalog_store + .create_user(&account_id, "bob", None) + .await + .expect("Failed to create user without password"); + + let (right, right_costs) = count_verifications(catalog_store.verify_iam_user_password( + &account_id, + "alice", + "correct horse", + )) + .await; + assert!(right.expect("Failed to verify password")); + assert_eq!(right_costs, [test_cost]); + + // The cost of the one check each refused login runs: alice's hash + // for her wrong password, the fixed hash for the others. + let cases = [ + (account_id.as_str(), "alice", test_cost, "wrong password"), + ( + account_id.as_str(), + "bob", + bcrypt::DEFAULT_COST, + "user without a console password", + ), + ( + account_id.as_str(), + "nobody", + bcrypt::DEFAULT_COST, + "unknown user", + ), + ( + missing_account.as_str(), + "alice", + bcrypt::DEFAULT_COST, + "unknown account", + ), + ]; + for (account, user_name, cost, case) in cases { + let (verified, costs) = count_verifications(catalog_store.verify_iam_user_password( + account, + user_name, + "battery staple", + )) + .await; + assert!(!verified.expect("Failed to verify password"), "{case}"); + assert_eq!(costs, [cost], "{case}: bcrypt checks and their cost"); + } + } } diff --git a/crates/storage-mongodb/src/management_store.rs b/crates/storage-mongodb/src/management_store.rs index 79ba85d7e..305ffb0fe 100644 --- a/crates/storage-mongodb/src/management_store.rs +++ b/crates/storage-mongodb/src/management_store.rs @@ -634,16 +634,11 @@ impl ManagementStore for MongoCatalogStore { OpError::Internal("Database error".to_owned()) })?; - let Some(user_doc) = doc else { - return Ok(false); - }; - - let Some(hash) = user_doc.get_str("password_hash").ok() else { - return Ok(false); - }; - - let hash = hash.to_owned(); - extenddb_storage::password::verify_password(password, hash).await + // `None` when the user is absent or has no console password, which + // still runs one bcrypt check (see `verify_stored_password`), as a + // wrong password does. + let hash = doc.and_then(|d| d.get_str("password_hash").ok().map(str::to_owned)); + extenddb_storage::password::verify_stored_password(password, hash).await }) } diff --git a/crates/storage-postgres/src/management_store/users.rs b/crates/storage-postgres/src/management_store/users.rs index 5314b70a6..341f8142c 100755 --- a/crates/storage-postgres/src/management_store/users.rs +++ b/crates/storage-postgres/src/management_store/users.rs @@ -223,11 +223,11 @@ impl PostgresCatalogStore { OpError::Internal("Database error".to_owned()) })?; - let Some((hash,)) = row else { - return Ok(false); - }; - - extenddb_storage::password::verify_password(password.to_owned(), hash).await + // `None` when the user is absent or has no console password, which + // still runs one bcrypt check (see `verify_stored_password`), as a + // wrong password does. + let hash = row.map(|(hash,)| hash); + extenddb_storage::password::verify_stored_password(password.to_owned(), hash).await } pub(crate) async fn change_user_password_impl( diff --git a/crates/storage-postgres/tests/iam_user_password.rs b/crates/storage-postgres/tests/iam_user_password.rs new file mode 100644 index 000000000..e9d01fc5c --- /dev/null +++ b/crates/storage-postgres/tests/iam_user_password.rs @@ -0,0 +1,121 @@ +// Copyright 2026 ExtendDB contributors +// SPDX-License-Identifier: Apache-2.0 + +//! `verify_iam_user_password` runs one bcrypt check for every name: against +//! the user's hash when the user has a console password, and against the +//! fixed hash at the default cost for a user without one, an unknown user and +//! an unknown account. The test works in a scratch database on the server +//! that `EXTENDDB_TEST_PG_CONNECTION_STRING` names, and drops it at the end. + +use extenddb_storage::management_store::ManagementStore; +use extenddb_storage::password::count_verifications; +use extenddb_storage_postgres::{PostgresCatalogStore, apply_all_migrations}; +use sqlx::postgres::PgPoolOptions; + +const ACCOUNT: &str = "123456789012"; + +/// bcrypt's lowest cost, for the test user's hash. It keeps the test fast and +/// tells a check against a stored hash apart from one against the fixed hash, +/// which is at the default cost. +const TEST_COST: u32 = 4; + +/// The server's base URL, or `None` to skip. With `EXTENDDB_TEST_PG_REQUIRED` +/// set to any value but `0`, a job that must reach a server fails here +/// instead. +fn base_conn() -> Option { + let conn = std::env::var("EXTENDDB_TEST_PG_CONNECTION_STRING").unwrap_or_default(); + if conn.is_empty() { + let required = + std::env::var("EXTENDDB_TEST_PG_REQUIRED").is_ok_and(|v| !v.is_empty() && v != "0"); + assert!( + !required, + "EXTENDDB_TEST_PG_CONNECTION_STRING is not set, and EXTENDDB_TEST_PG_REQUIRED says \ + this job must reach a PostgreSQL server" + ); + return None; + } + Some(conn.trim_end_matches('/').to_owned()) +} + +#[tokio::test] +async fn an_unknown_user_or_one_without_a_password_runs_one_bcrypt_check() { + let Some(base) = base_conn() else { + eprintln!("SKIP: EXTENDDB_TEST_PG_CONNECTION_STRING is not set"); + return; + }; + let db_name = format!( + "eddb_iampw_{}", + &uuid::Uuid::new_v4().simple().to_string()[..12] + ); + let admin = PgPoolOptions::new() + .max_connections(1) + .connect(&format!("{base}/postgres")) + .await + .expect("connect to the postgres maintenance database"); + sqlx::query(&format!("CREATE DATABASE \"{db_name}\"")) + .execute(&admin) + .await + .expect("create the scratch database"); + let db = PgPoolOptions::new() + .max_connections(2) + .connect(&format!("{base}/{db_name}")) + .await + .expect("connect to the scratch database"); + apply_all_migrations(&db, &db) + .await + .expect("apply the crate's migrations"); + + let store = PostgresCatalogStore::new(db.clone()); + store.create_account(ACCOUNT, "acct").await.unwrap(); + let hash = bcrypt::hash("correct horse", TEST_COST).unwrap(); + store + .create_user(ACCOUNT, "alice", Some(&hash)) + .await + .unwrap(); + store.create_user(ACCOUNT, "bob", None).await.unwrap(); + + let (right, right_costs) = + count_verifications(store.verify_iam_user_password(ACCOUNT, "alice", "correct horse")) + .await; + assert!(right.unwrap()); + assert_eq!(right_costs, [TEST_COST]); + + // The cost of the one check each refused login runs: alice's hash for her + // wrong password, the fixed hash for the others. + let cases = [ + (ACCOUNT, "alice", TEST_COST, "wrong password"), + ( + ACCOUNT, + "bob", + bcrypt::DEFAULT_COST, + "user without a console password", + ), + (ACCOUNT, "nobody", bcrypt::DEFAULT_COST, "unknown user"), + ( + "111122223333", + "alice", + bcrypt::DEFAULT_COST, + "unknown account", + ), + ]; + for (account_id, user_name, cost, case) in cases { + let (verified, costs) = count_verifications(store.verify_iam_user_password( + account_id, + user_name, + "battery staple", + )) + .await; + assert!(!verified.unwrap(), "{case}: verified"); + assert_eq!(costs, [cost], "{case}: bcrypt checks and their cost"); + } + + drop(store); + db.close().await; + sqlx::query(&format!( + "DROP DATABASE IF EXISTS \"{db_name}\" WITH (FORCE)" + )) + .execute(&admin) + .await + .expect("remove the scratch database"); + admin.close().await; +} diff --git a/crates/storage-sqlite/src/management_store/users.rs b/crates/storage-sqlite/src/management_store/users.rs index c28f35a12..69c9fd6c7 100644 --- a/crates/storage-sqlite/src/management_store/users.rs +++ b/crates/storage-sqlite/src/management_store/users.rs @@ -219,11 +219,11 @@ impl SqliteCatalogStore { OpError::Internal("Database error".to_owned()) })?; - let Some((Some(hash),)) = row else { - // User absent or has no console password. - return Ok(false); - }; - extenddb_storage::password::verify_password(password.to_owned(), hash).await + // `None` when the user is absent or has no console password, which + // still runs one bcrypt check (see `verify_stored_password`), as a + // wrong password does. + let hash = row.and_then(|(hash,)| hash); + extenddb_storage::password::verify_stored_password(password.to_owned(), hash).await } pub(crate) async fn change_user_password_impl( @@ -346,3 +346,78 @@ impl SqliteCatalogStore { }) } } + +#[cfg(test)] +mod tests { + use extenddb_storage::management_store::ManagementStore; + use extenddb_storage::password::count_verifications; + use sqlx::sqlite::SqlitePoolOptions; + + use super::*; + + const ACCOUNT: &str = "123456789012"; + + /// bcrypt's lowest cost, for the test user's hash. It keeps the test fast + /// and tells a check against a stored hash apart from one against the + /// fixed hash, which is at the default cost. + const TEST_COST: u32 = 4; + + async fn store() -> SqliteCatalogStore { + // One connection keeps the in-memory database alive for the test. + let pool = SqlitePoolOptions::new() + .max_connections(1) + .connect("sqlite::memory:") + .await + .expect("open in-memory sqlite"); + sqlx::query("PRAGMA foreign_keys = ON") + .execute(&pool) + .await + .unwrap(); + crate::migrations::apply_for_tests(&pool).await; + SqliteCatalogStore::new(pool) + } + + #[tokio::test] + async fn an_unknown_user_or_one_without_a_password_runs_one_bcrypt_check() { + let s = store().await; + s.create_account(ACCOUNT, "acct").await.unwrap(); + let hash = bcrypt::hash("correct horse", TEST_COST).unwrap(); + s.create_user(ACCOUNT, "alice", Some(&hash)).await.unwrap(); + s.create_user(ACCOUNT, "bob", None).await.unwrap(); + + let (right, right_costs) = + count_verifications(s.verify_iam_user_password(ACCOUNT, "alice", "correct horse")) + .await; + assert!(right.unwrap()); + assert_eq!(right_costs, [TEST_COST]); + + // The cost of the one check each refused login runs: alice's hash for + // her wrong password, the fixed hash for the others. + let cases = [ + (ACCOUNT, "alice", TEST_COST, "wrong password"), + ( + ACCOUNT, + "bob", + bcrypt::DEFAULT_COST, + "user without a console password", + ), + (ACCOUNT, "nobody", bcrypt::DEFAULT_COST, "unknown user"), + ( + "111122223333", + "alice", + bcrypt::DEFAULT_COST, + "unknown account", + ), + ]; + for (account_id, user_name, cost, case) in cases { + let (verified, costs) = count_verifications(s.verify_iam_user_password( + account_id, + user_name, + "battery staple", + )) + .await; + assert!(!verified.unwrap(), "{case}: verified"); + assert_eq!(costs, [cost], "{case}: bcrypt checks and their cost"); + } + } +} diff --git a/crates/storage/Cargo.toml b/crates/storage/Cargo.toml index 3d18929b0..a024143df 100755 --- a/crates/storage/Cargo.toml +++ b/crates/storage/Cargo.toml @@ -9,7 +9,10 @@ license.workspace = true [features] # `test-util` exposes `migrations::check_released`, the checksum test each -# backend crate runs on its migration table. Production code never enables it. +# backend crate runs on its migration table, and +# `password::count_verifications`, which returns the bcrypt cost of each check +# a login ran. It also compiles the counter in `verify_password` that records +# those costs. Production code never enables it. test-util = [] # Compiles the `snapshot_conformance` check that each backend's tests run # against a real database. Production builds do not compile it. diff --git a/crates/storage/src/management_store/mod.rs b/crates/storage/src/management_store/mod.rs index a9feb55a3..29a1f7d7e 100755 --- a/crates/storage/src/management_store/mod.rs +++ b/crates/storage/src/management_store/mod.rs @@ -138,6 +138,9 @@ pub trait AdminStore: Send + Sync { /// Verify an admin password. Returns `None` if the admin does not exist, /// `Some(true)` if the password matches, `Some(false)` if it does not. + /// An unknown name returns `None` without a bcrypt check. Every login + /// path then runs the IAM user step, which runs one bcrypt check for any + /// name, so a login runs one check whether or not an admin has the name. fn verify_admin_password( &self, admin_name: &str, @@ -206,7 +209,11 @@ pub trait ManagementStore: Send + Sync { user_name: &str, ) -> BoxFuture<'_, OpResult>>; - /// Verify an IAM user's console password. + /// Verify an IAM user's console password. Returns `false` for a wrong + /// password, an unknown user and a user without a console password. + /// Implementations check through + /// [`crate::password::verify_stored_password`] in all three cases, so + /// that they run the same bcrypt work. fn verify_iam_user_password( &self, account_id: &str, diff --git a/crates/storage/src/password.rs b/crates/storage/src/password.rs index 9afcc0b86..2db24e6b1 100644 --- a/crates/storage/src/password.rs +++ b/crates/storage/src/password.rs @@ -11,7 +11,10 @@ //! //! Every backend's password verification goes through [`verify_password`], //! and the management API hashes through [`hash_password`], so the bound -//! holds whatever store a login is checked against. +//! holds whatever store a login is checked against. When a login names no +//! principal with a password, [`verify_stored_password`] checks the password +//! against a fixed hash, so the login waits for a slot and does the same +//! bcrypt work as a wrong password. //! //! A slot is held by the blocking call itself, not by the async caller. A //! caller whose future is dropped while bcrypt runs (a client that closes @@ -39,6 +42,15 @@ pub const VERIFY_WAIT: Duration = Duration::from_secs(10); /// overload apart from storage errors. pub const VERIFY_OVERLOADED: &str = "password verification is overloaded; try again later"; +/// A bcrypt hash, at the default cost, of a random value that was discarded +/// once the hash was made. [`verify_stored_password`] checks a password +/// against it when a principal has no stored hash, so that a login for a name +/// that does not exist runs the same bcrypt work as a wrong password for a +/// name that does. That check refuses the login whatever bcrypt returns: a +/// password that matched this hash would still be refused, so knowing the +/// hash does not help a caller log in. +const NO_STORED_HASH: &str = "$2b$12$L4yNgP/xgOa4sckbcBNixutZwPfrRn3iJsfX1cMGumbupNRaepaEy"; + static BCRYPT_SEMAPHORE: Semaphore = Semaphore::const_new(MAX_CONCURRENT_BCRYPT); /// bcrypt calls running right now (on the blocking pool). @@ -47,6 +59,30 @@ static RUNNING: AtomicUsize = AtomicUsize::new(0); /// The most bcrypt calls ever observed running at once in this process. static HIGH_WATER: AtomicUsize = AtomicUsize::new(0); +#[cfg(any(test, feature = "test-util"))] +tokio::task_local! { + /// The bcrypt cost of each verification run inside the current + /// [`count_verifications`] scope. + static VERIFICATIONS: std::cell::RefCell>; +} + +/// Run `future` and return its output with the bcrypt cost of each +/// verification it ran through [`verify_password`], in order. A verification +/// is recorded when bcrypt has checked the password against the hash, with +/// the cost read from the hash. A hash that bcrypt refuses with an error, +/// such as one that does not parse, records nothing. Verifications in tasks +/// that `future` spawns are not recorded, so tests that run bcrypt at the +/// same time do not change each other's records. +#[cfg(any(test, feature = "test-util"))] +pub async fn count_verifications(future: F) -> (F::Output, Vec) { + VERIFICATIONS + .scope(std::cell::RefCell::default(), async move { + let output = future.await; + (output, VERIFICATIONS.with(std::cell::RefCell::take)) + }) + .await +} + fn semaphore_closed() -> OpError { OpError::Internal("bcrypt semaphore closed".to_owned()) } @@ -100,10 +136,44 @@ pub async fn verify_password(password: String, hash: String) -> OpResult { return Err(OpError::Internal(VERIFY_OVERLOADED.to_owned())); }; let permit = acquired.map_err(|_| semaphore_closed())?; - run_bcrypt(permit, move || { - bcrypt::verify(&password, &hash).unwrap_or(false) - }) - .await + #[cfg(any(test, feature = "test-util"))] + let cost = hash + .parse::() + .ok() + .map(|parts| parts.get_cost()); + let checked = run_bcrypt(permit, move || bcrypt::verify(&password, &hash)).await?; + #[cfg(any(test, feature = "test-util"))] + if let (Some(cost), Ok(_)) = (cost, &checked) { + let _ = VERIFICATIONS.try_with(|costs| costs.borrow_mut().push(cost)); + } + Ok(checked.unwrap_or(false)) +} + +/// Verify `password` against a principal's stored bcrypt hash. `stored_hash` +/// is `None` when the principal does not exist or has no password. +/// +/// Without a stored hash, the password is checked against a fixed hash at the +/// default cost and refused whatever bcrypt returns. That check waits for a +/// bcrypt slot and runs one bcrypt verification, as a check against a stored +/// hash does. A lookup that finds no principal therefore does the same bcrypt +/// work as a wrong password. +/// +/// # Errors +/// +/// As [`verify_password`], with or without a stored hash: a check for a +/// principal that does not exist fails with the same errors, including +/// [`VERIFY_OVERLOADED`], as a check for one that does. +pub async fn verify_stored_password( + password: String, + stored_hash: Option, +) -> OpResult { + match stored_hash { + Some(hash) => verify_password(password, hash).await, + None => { + verify_password(password, NO_STORED_HASH.to_owned()).await?; + Ok(false) + } + } } /// Run one bcrypt call on the blocking pool. The permit moves into the @@ -228,6 +298,73 @@ mod tests { ); } + #[test] + fn the_fixed_hash_is_a_bcrypt_hash_at_the_default_cost() { + let parts: bcrypt::HashParts = NO_STORED_HASH.parse().unwrap(); + assert_eq!(parts.get_cost(), bcrypt::DEFAULT_COST); + // A hash bcrypt cannot decode fails before any work is done, which + // would make a check without a stored hash return at once. + assert!(bcrypt::verify("any password", NO_STORED_HASH).is_ok()); + } + + #[tokio::test] + async fn a_check_without_a_stored_hash_runs_one_verification_and_refuses() { + 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(), + Some(hash), + )) + .await; + 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; + assert!(!absent.unwrap(), "a principal with no stored hash verified"); + assert_eq!( + absent_costs, + [bcrypt::DEFAULT_COST], + "a check without a stored hash must run one verification at the default cost" + ); + } + + #[tokio::test] + async fn a_check_without_a_stored_hash_waits_for_a_bcrypt_slot() { + let _counters = COUNTERS.lock().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(), + None, + ))); + for _ in 0..100 { + tokio::task::yield_now().await; + } + // Every slot is held here, so the check cannot have run. + assert!(!check.is_finished(), "the check finished without a slot"); + assert_eq!(RUNNING.load(Ordering::SeqCst), 0); + drop(held); + let (result, costs) = check.await.unwrap(); + assert!(!result.unwrap()); + assert_eq!(costs, [bcrypt::DEFAULT_COST]); + } + + #[tokio::test] + async fn a_hash_bcrypt_refuses_records_no_verification() { + let _counters = COUNTERS.lock().await; + // The second hash has bcrypt's three parts and cost 12, but its salt + // is not bcrypt's base64, so bcrypt refuses it before hashing. + let unreadable_salt = format!("$2b$12${}", "!".repeat(53)); + assert!(unreadable_salt.parse::().is_ok()); + 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; + assert!(!verified.unwrap()); + assert!(costs.is_empty(), "a refused hash was recorded: {costs:?}"); + } + } + #[tokio::test] async fn a_malformed_hash_is_a_wrong_password_not_an_error() { let _counters = COUNTERS.lock().await; diff --git a/docs/manuals/10-security-model.md b/docs/manuals/10-security-model.md index 89da45d36..37650a7a3 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -243,6 +243,8 @@ The console login form, the management API (HTTP Basic auth) and the `/metrics` A successful login is not recorded. A storage error before or during the password check is answered `500` and is not counted against the caller. +Every login that the limits admit runs one bcrypt check, or is answered `500` after waiting 10 seconds for a slot, whatever the name. When no admin or IAM user has the name, or the IAM user has no console password, that check runs against a fixed hash, and the login is answered and counted as a wrong password. + The source address is the peer address of the TCP connection, with an IPv4-mapped IPv6 address (`::ffff:a.b.c.d`) counted as the IPv4 address. Any other IPv6 address is counted by its /64 prefix, because a /64 is the block usually assigned to one host or site and a client can send from any address in it; all addresses in one /64 share one set of limits. ExtendDB does not read `X-Forwarded-For` or any other forwarded-address header (they are caller-supplied). Callers that reach ExtendDB from one address share one set of limits. Five failures for a user from any of them refuse that user to all of them, and 20 failures in total refuse every user, until the oldest failure is 15 minutes old. Callers share an address in these cases: diff --git a/docs/manuals/12-extending-extenddb-storage.md b/docs/manuals/12-extending-extenddb-storage.md index 93db24814..5c8606efb 100755 --- a/docs/manuals/12-extending-extenddb-storage.md +++ b/docs/manuals/12-extending-extenddb-storage.md @@ -223,7 +223,7 @@ Defined in `crates/storage/src/management_store/mod.rs`. Login rate limiting: | `record_failed_login` | Record a failed login attempt with its source address | | `cleanup_old_attempts` | Garbage-collect old login attempt records | -Password verification in a backend's `AdminStore` and `ManagementStore` must go through `extenddb_storage::password::verify_password`, which bounds concurrent bcrypt work per process. +Password verification in a backend's `AdminStore` and `ManagementStore` must go through `extenddb_storage::password`, which bounds concurrent bcrypt work per process. `verify_admin_password` checks an existing admin's hash with `verify_password` and returns `None` without a bcrypt check when no admin has the name. `verify_iam_user_password` passes the user's hash to `verify_stored_password`, or `None` when the account or the user does not exist or the user has no console password; with `None`, the helper checks the password against a fixed hash and refuses it. A login checks the admin first and the IAM user only when no admin has the name, and the server runs the fixed-hash check itself for a name without `/`, so every login runs one bcrypt check whether or not the name exists. ### AuthorizationStore