diff --git a/Cargo.lock b/Cargo.lock index b836d650e..29d03fa3c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1806,6 +1806,7 @@ dependencies = [ "extenddb-core", "extenddb-engine", "extenddb-storage", + "extenddb-storage-sqlite", "futures", "hyper", "libc", @@ -1815,6 +1816,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.10.9", + "sqlx", "syslog-tracing", "tempfile", "time", diff --git a/crates/server/Cargo.toml b/crates/server/Cargo.toml index e674e5c58..812c1098b 100755 --- a/crates/server/Cargo.toml +++ b/crates/server/Cargo.toml @@ -52,5 +52,10 @@ 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"] } tempfile = "3" tokio = { workspace = true, features = ["test-util"] } diff --git a/crates/server/src/console/pages/auth_pages.rs b/crates/server/src/console/pages/auth_pages.rs index 7021544d6..a2d82b7f8 100755 --- a/crates/server/src/console/pages/auth_pages.rs +++ b/crates/server/src/console/pages/auth_pages.rs @@ -220,7 +220,7 @@ async fn try_admin_login( let Some(hash) = hash else { return Ok(AdminLoginResult::NotFound); }; - Ok(match verify_login(password, hash).await? { + Ok(match verify_login(password, Some(hash)).await? { Some(stamp) => { AdminLoginResult::Authenticated(CallerIdentity::Admin(username.to_owned()), stamp) } @@ -228,34 +228,42 @@ 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, a user without a console +/// password 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. When no stored hash is found, the password is checked against a +/// fixed hash, so that the login 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 PasswordHashStore, ) -> Result, ()> { - let Some((acct_id, uname)) = username.split_once('/') else { - return Ok(None); - }; - // `None` when the user does not exist or has no console password. - let hash = store - .get_user_password_hash(acct_id, uname) - .await - .map_err(|e| { - log_password_check_error("Console login: DB error during IAM user auth", &e) - })?; - let Some(hash) = hash else { - return Ok(None); - }; - let Some(stamp) = verify_login(password, hash).await? else { - return Ok(None); - }; - let identity = CallerIdentity::IamUser { - account_id: acct_id.to_owned(), - user_name: uname.to_owned(), + let user = username.split_once('/'); + // `None` when the name has no `/`, or when the user does not exist or has + // no console password. + let hash = match user { + Some((acct_id, uname)) => { + store + .get_user_password_hash(acct_id, uname) + .await + .map_err(|e| { + log_password_check_error("Console login: DB error during IAM user auth", &e) + })? + } + None => None, }; - Ok(Some((identity, stamp))) + let stamp = verify_login(password, hash).await?; + Ok(match (user, stamp) { + (Some((acct_id, uname)), Some(stamp)) => { + let identity = CallerIdentity::IamUser { + account_id: acct_id.to_owned(), + user_name: uname.to_owned(), + }; + Some((identity, stamp)) + } + _ => None, + }) } /// Verify a login password against the hash read from the catalog. On a @@ -264,17 +272,23 @@ async fn try_iam_login( /// lands during the login either fails the login or ends the new session at /// its first request. /// +/// `hash` is `None` when the login names no principal with a password. The +/// password is then checked against a fixed hash and refused, so the login +/// runs the same bcrypt work as a wrong password (see +/// [`extenddb_storage::password::verify_stored_password`]). +/// /// All console login verification goes through this function, and through /// the bcrypt slots of [`extenddb_storage::password::verify_password`]. -/// `Ok(None)` is a wrong password. `Err(())` is a verification that did not -/// run (no bcrypt slot within the wait, or a failed blocking task); the -/// caller answers 500 and records nothing. -async fn verify_login(password: &str, hash: String) -> Result, ()> { - let stamp = CredentialStamp::of(&hash); - let matched = extenddb_storage::password::verify_password(password.to_owned(), hash) +/// `Ok(None)` is a wrong password or a login without a stored hash. +/// `Err(())` is a verification that did not run (no bcrypt slot within the +/// wait, or a failed blocking task); the caller answers 500 and records +/// nothing. +async fn verify_login(password: &str, hash: Option) -> Result, ()> { + let stamp = hash.as_deref().map(CredentialStamp::of); + let matched = extenddb_storage::password::verify_stored_password(password.to_owned(), hash) .await .map_err(|e| log_password_check_error("Console login: password verification failed", &e))?; - Ok(matched.then_some(stamp)) + Ok(stamp.filter(|_| matched)) } #[cfg(test)] @@ -396,6 +410,33 @@ 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 login_result = checked.unwrap_or_else(|()| panic!("{}: storage error", login.case)); + assert_eq!( + Outcome::of(login_result.as_ref().map(|(identity, _)| identity)), + login.outcome, + "{}", + login.case + ); + } + } } #[cfg(test)] 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 ee55cab6d..0db1e676d 100644 --- a/crates/storage-cassandra/src/management_store/users.rs +++ b/crates/storage-cassandra/src/management_store/users.rs @@ -392,17 +392,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 e2574fc83..06a66fa21 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, @@ -229,7 +232,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 e8b663d28..dbbca5e90 100755 --- a/docs/manuals/10-security-model.md +++ b/docs/manuals/10-security-model.md @@ -245,6 +245,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 2c8af5c9d..6c54f3344 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, so every login runs one bcrypt check whether or not the name exists. The management API checks through these two methods and runs the fixed-hash check itself for a name without `/`. The console reads the stored hashes through `PasswordHashStore` and passes them to `verify_stored_password` itself, with `None` for a name without `/` and when `get_user_password_hash` returns `None`. ### AuthorizationStore