diff --git a/crates/handlers/src/admin/v1/users/set_password.rs b/crates/handlers/src/admin/v1/users/set_password.rs index fbcc48588..633d72e67 100644 --- a/crates/handlers/src/admin/v1/users/set_password.rs +++ b/crates/handlers/src/admin/v1/users/set_password.rs @@ -145,7 +145,7 @@ mod tests { use zeroize::Zeroizing; use crate::{ - passwords::PasswordManager, + passwords::{PasswordManager, PasswordVerificationResult}, test_utils::{RequestBuilderExt, ResponseExt, TestState, setup}, }; @@ -185,7 +185,7 @@ mod tests { let mut repo = state.repository().await.unwrap(); let user_password = repo.user_password().active(&user).await.unwrap().unwrap(); let password = Zeroizing::new(String::from("this is a good enough password")); - state + let res = state .password_manager .verify( user_password.version, @@ -194,6 +194,7 @@ mod tests { ) .await .unwrap(); + assert_eq!(res, PasswordVerificationResult::Success(())); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] @@ -244,7 +245,7 @@ mod tests { let mut repo = state.repository().await.unwrap(); let user_password = repo.user_password().active(&user).await.unwrap().unwrap(); let password = Zeroizing::new("password".to_owned()); - state + let res = state .password_manager .verify( user_password.version, @@ -253,6 +254,7 @@ mod tests { ) .await .unwrap(); + assert_eq!(res, PasswordVerificationResult::Success(())); } #[sqlx::test(migrator = "mas_storage_pg::MIGRATOR")] diff --git a/crates/handlers/src/compat/login.rs b/crates/handlers/src/compat/login.rs index 251030c0d..22a74a405 100644 --- a/crates/handlers/src/compat/login.rs +++ b/crates/handlers/src/compat/login.rs @@ -32,7 +32,8 @@ use zeroize::Zeroizing; use super::{MatrixError, MatrixJsonBody}; use crate::{ BoundActivityTracker, Limiter, METER, RequesterFingerprint, impl_from_error_for_route, - passwords::PasswordManager, rate_limit::PasswordCheckLimitedError, + passwords::{PasswordManager, PasswordVerificationResult}, + rate_limit::PasswordCheckLimitedError, }; static LOGIN_COUNTER: LazyLock> = LazyLock::new(|| { @@ -193,7 +194,7 @@ pub enum RouteError { NoPassword, #[error("password verification failed")] - PasswordVerificationFailed(#[source] anyhow::Error), + PasswordMismatch, #[error("request rate limited")] RateLimited(#[from] PasswordCheckLimitedError), @@ -210,6 +211,12 @@ pub enum RouteError { impl_from_error_for_route!(mas_storage::RepositoryError); +impl From for RouteError { + fn from(err: anyhow::Error) -> Self { + Self::Internal(err.into()) + } +} + impl IntoResponse for RouteError { fn into_response(self) -> axum::response::Response { let sentry_event_id = @@ -241,13 +248,11 @@ impl IntoResponse for RouteError { error: "Missing property 'identifier", status: StatusCode::BAD_REQUEST, }, - Self::UserNotFound | Self::NoPassword | Self::PasswordVerificationFailed(_) => { - MatrixError { - errcode: "M_FORBIDDEN", - error: "Invalid username/password", - status: StatusCode::FORBIDDEN, - } - } + Self::UserNotFound | Self::NoPassword | Self::PasswordMismatch => MatrixError { + errcode: "M_FORBIDDEN", + error: "Invalid username/password", + status: StatusCode::FORBIDDEN, + }, Self::LoginTookTooLong => MatrixError { errcode: "M_FORBIDDEN", error: "Login token expired", @@ -576,28 +581,32 @@ async fn user_password_login( // Verify the password let password = Zeroizing::new(password); - let new_password_hash = password_manager + match password_manager .verify_and_upgrade( &mut rng, user_password.version, password, user_password.hashed_password.clone(), ) - .await - .map_err(RouteError::PasswordVerificationFailed)?; - - if let Some((version, hashed_password)) = new_password_hash { - // Save the upgraded password if needed - repo.user_password() - .add( - &mut rng, - clock, - &user, - version, - hashed_password, - Some(&user_password), - ) - .await?; + .await? + { + PasswordVerificationResult::Success(Some((version, hashed_password))) => { + // Save the upgraded password if needed + repo.user_password() + .add( + &mut rng, + clock, + &user, + version, + hashed_password, + Some(&user_password), + ) + .await?; + } + PasswordVerificationResult::Success(None) => {} + PasswordVerificationResult::Failure => { + return Err(RouteError::PasswordMismatch); + } } // We're about to create a device, let's explicitly acquire a lock, so that diff --git a/crates/handlers/src/passwords.rs b/crates/handlers/src/passwords.rs index 0a9e41807..a6f4e8823 100644 --- a/crates/handlers/src/passwords.rs +++ b/crates/handlers/src/passwords.rs @@ -9,7 +9,7 @@ use std::{collections::HashMap, sync::Arc}; use anyhow::Context; use argon2::{Argon2, PasswordHash, PasswordHasher, PasswordVerifier, password_hash::SaltString}; use futures_util::future::OptionFuture; -use pbkdf2::Pbkdf2; +use pbkdf2::{Pbkdf2, password_hash}; use rand::{CryptoRng, RngCore, SeedableRng, distributions::Standard, prelude::Distribution}; use thiserror::Error; use zeroize::Zeroizing; @@ -17,6 +17,50 @@ use zxcvbn::zxcvbn; pub type SchemeVersion = u16; +/// The result of a password verification, which is `true` if the password +/// matches the hashed password, and `false` otherwise. +/// +/// In the success case it can also contain additional data, such as the new +/// hashing scheme and the new hashed password. +#[must_use] +#[derive(Debug, PartialEq, Eq, Clone)] +pub enum PasswordVerificationResult { + /// The password matches the stored password hash + Success(T), + /// The password does not match the stored password hash + Failure, +} + +impl PasswordVerificationResult<()> { + fn success() -> Self { + Self::Success(()) + } + + fn failure() -> Self { + Self::Failure + } +} + +impl PasswordVerificationResult { + /// Converts the result into a new result with the given data. + fn with_data(self, data: N) -> PasswordVerificationResult { + match self { + Self::Success(_) => PasswordVerificationResult::Success(data), + Self::Failure => PasswordVerificationResult::Failure, + } + } +} + +impl From for PasswordVerificationResult<()> { + fn from(value: bool) -> Self { + if value { + Self::success() + } else { + Self::failure() + } + } +} + #[derive(Debug, Error)] #[error("Password manager is disabled")] pub struct PasswordManagerDisabledError; @@ -149,11 +193,11 @@ impl PasswordManager { scheme: SchemeVersion, password: Zeroizing, hashed_password: String, - ) -> Result<(), anyhow::Error> { + ) -> Result { let inner = self.get_inner()?; let span = tracing::Span::current(); - tokio::task::spawn_blocking(move || { + let result = tokio::task::spawn_blocking(move || { span.in_scope(move || { let hasher = if scheme == inner.current_version { &inner.current_hasher @@ -169,7 +213,7 @@ impl PasswordManager { }) .await??; - Ok(()) + Ok(result) } /// Verify a password hash for the given hashing scheme, and upgrade it on @@ -186,7 +230,7 @@ impl PasswordManager { scheme: SchemeVersion, password: Zeroizing, hashed_password: String, - ) -> Result, anyhow::Error> { + ) -> Result>, anyhow::Error> { let inner = self.get_inner()?; // If the current scheme isn't the default one, we also hash with the default @@ -198,11 +242,11 @@ impl PasswordManager { let verify_fut = self.verify(scheme, password, hashed_password); let (new_hash_res, verify_res) = tokio::join!(new_hash_fut, verify_fut); - verify_res?; + let password_result = verify_res?; let new_hash = new_hash_res.transpose()?; - Ok(new_hash) + Ok(password_result.with_data(new_hash)) } } @@ -276,7 +320,7 @@ impl Hasher { &self, hashed_password: &str, password: Zeroizing, - ) -> Result<(), anyhow::Error> { + ) -> Result { let password = self.normalize_password(password); self.algorithm @@ -345,8 +389,8 @@ impl Algorithm { hashed_password: &str, password: &[u8], pepper: Option<&[u8]>, - ) -> Result<(), anyhow::Error> { - match self { + ) -> Result { + let result = match self { Algorithm::Bcrypt { .. } => { let mut password = Zeroizing::new(password.to_vec()); if let Some(pepper) = pepper { @@ -354,7 +398,7 @@ impl Algorithm { } let result = bcrypt::verify(password, hashed_password)?; - anyhow::ensure!(result, "wrong password"); + PasswordVerificationResult::from(result) } Algorithm::Argon2id => { @@ -370,7 +414,11 @@ impl Algorithm { let hashed_password = PasswordHash::new(hashed_password)?; - phf.verify_password(password.as_ref(), &hashed_password)?; + match phf.verify_password(password.as_ref(), &hashed_password) { + Ok(()) => PasswordVerificationResult::success(), + Err(password_hash::Error::Password) => PasswordVerificationResult::failure(), + Err(e) => Err(e)?, + } } Algorithm::Pbkdf2 => { @@ -381,11 +429,15 @@ impl Algorithm { let hashed_password = PasswordHash::new(hashed_password)?; - Pbkdf2.verify_password(password.as_ref(), &hashed_password)?; + match Pbkdf2.verify_password(password.as_ref(), &hashed_password) { + Ok(()) => PasswordVerificationResult::success(), + Err(password_hash::Error::Password) => PasswordVerificationResult::failure(), + Err(e) => Err(e)?, + } } - } + }; - Ok(()) + Ok(result) } } @@ -410,10 +462,26 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_ok()); - assert!(alg.verify_blocking(&hash, password2, Some(pepper)).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper2)).is_err()); - assert!(alg.verify_blocking(&hash, password, None).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper2)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); // Hash without pepper let hash = alg @@ -421,9 +489,21 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, None).is_ok()); - assert!(alg.verify_blocking(&hash, password2, None).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); } #[test] @@ -441,10 +521,26 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_ok()); - assert!(alg.verify_blocking(&hash, password2, Some(pepper)).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper2)).is_err()); - assert!(alg.verify_blocking(&hash, password, None).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper2)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); // Hash without pepper let hash = alg @@ -452,9 +548,21 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, None).is_ok()); - assert!(alg.verify_blocking(&hash, password2, None).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); } #[test] @@ -473,10 +581,26 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_ok()); - assert!(alg.verify_blocking(&hash, password2, Some(pepper)).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper2)).is_err()); - assert!(alg.verify_blocking(&hash, password, None).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper2)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); // Hash without pepper let hash = alg @@ -484,9 +608,21 @@ mod tests { .expect("Couldn't hash password"); insta::assert_snapshot!(hash); - assert!(alg.verify_blocking(&hash, password, None).is_ok()); - assert!(alg.verify_blocking(&hash, password2, None).is_err()); - assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_err()); + assert_eq!( + alg.verify_blocking(&hash, password, None) + .expect("Verification failed"), + PasswordVerificationResult::Success(()) + ); + assert_eq!( + alg.verify_blocking(&hash, password2, None) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); + assert_eq!( + alg.verify_blocking(&hash, password, Some(pepper)) + .expect("Verification failed"), + PasswordVerificationResult::Failure + ); } #[allow(clippy::too_many_lines)] @@ -520,16 +656,18 @@ mod tests { insta::assert_snapshot!(hash); // Just verifying works - manager + let res = manager .verify(version, password.clone(), hash.clone()) .await .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Success(())); // And doesn't work with the wrong password - manager + let res = manager .verify(version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); // Verifying with the wrong version doesn't work manager @@ -543,13 +681,14 @@ mod tests { .await .expect("Failed to verify"); - assert!(res.is_none()); + assert_eq!(res, PasswordVerificationResult::Success(None)); // Upgrading still verify that the password matches - manager + let res = manager .verify_and_upgrade(&mut rng, version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); let manager = PasswordManager::new( 0, @@ -564,16 +703,18 @@ mod tests { .unwrap(); // Verifying still works - manager + let res = manager .verify(version, password.clone(), hash.clone()) .await .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Success(())); // And doesn't work with the wrong password - manager + let res = manager .verify(version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); // Upgrading does re-hash let res = manager @@ -581,9 +722,9 @@ mod tests { .await .expect("Failed to verify"); - assert!(res.is_some()); - let (version, hash) = res.unwrap(); - + let PasswordVerificationResult::Success(Some((version, hash))) = res else { + panic!("Expected a successful upgrade"); + }; assert_eq!(version, 2); insta::assert_snapshot!(hash); @@ -593,19 +734,21 @@ mod tests { .await .expect("Failed to verify"); - assert!(res.is_none()); + assert_eq!(res, PasswordVerificationResult::Success(None)); // Upgrading still verify that the password matches - manager + let res = manager .verify_and_upgrade(&mut rng, version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); // Upgrading still verify that the password matches - manager + let res = manager .verify_and_upgrade(&mut rng, version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); let manager = PasswordManager::new( 0, @@ -624,16 +767,18 @@ mod tests { .unwrap(); // Verifying still works - manager + let res = manager .verify(version, password.clone(), hash.clone()) .await .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Success(())); // And doesn't work with the wrong password - manager + let res = manager .verify(version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); // Upgrading does re-hash let res = manager @@ -641,8 +786,9 @@ mod tests { .await .expect("Failed to verify"); - assert!(res.is_some()); - let (version, hash) = res.unwrap(); + let PasswordVerificationResult::Success(Some((version, hash))) = res else { + panic!("Expected a successful upgrade"); + }; assert_eq!(version, 3); insta::assert_snapshot!(hash); @@ -653,12 +799,13 @@ mod tests { .await .expect("Failed to verify"); - assert!(res.is_none()); + assert_eq!(res, PasswordVerificationResult::Success(None)); // Upgrading still verify that the password matches - manager + let res = manager .verify_and_upgrade(&mut rng, version, wrong_password.clone(), hash.clone()) .await - .expect_err("Verification should have failed"); + .expect("Failed to verify"); + assert_eq!(res, PasswordVerificationResult::Failure); } } diff --git a/crates/handlers/src/views/login.rs b/crates/handlers/src/views/login.rs index f684f32ad..6785e3dcc 100644 --- a/crates/handlers/src/views/login.rs +++ b/crates/handlers/src/views/login.rs @@ -38,7 +38,7 @@ use zeroize::Zeroizing; use super::shared::OptionalPostAuthAction; use crate::{ BoundActivityTracker, Limiter, METER, PreferredLanguage, RequesterFingerprint, SiteConfig, - passwords::PasswordManager, + passwords::{PasswordManager, PasswordVerificationResult}, session::{SessionOrFallback, load_session_or_fallback}, }; @@ -166,6 +166,7 @@ pub(crate) async fn post( } if !form_state.is_valid() { + tracing::warn!("Invalid login form: {form_state:?}"); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); return render( locale, @@ -189,6 +190,7 @@ pub(crate) async fn post( // First, lookup the user let Some(user) = get_user_by_email_or_by_username(site_config, &mut repo, username).await? else { + tracing::warn!(username, "User not found"); let form_state = form_state.with_error_on_form(FormError::InvalidCredentials); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); return render( @@ -207,7 +209,7 @@ pub(crate) async fn post( // Check the rate limit if let Err(e) = limiter.check_password(requester, &user) { - tracing::warn!(error = &e as &dyn std::error::Error); + tracing::warn!(error = &e as &dyn std::error::Error, "ratelimit exceeded"); let form_state = form_state.with_error_on_form(FormError::RateLimitExceeded); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); return render( @@ -228,6 +230,7 @@ pub(crate) async fn post( let Some(user_password) = repo.user_password().active(&user).await? else { // There is no password for this user, but we don't want to disclose that. Show // a generic 'invalid credentials' error instead + tracing::warn!(username, "No password for user"); let form_state = form_state.with_error_on_form(FormError::InvalidCredentials); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); return render( @@ -256,7 +259,7 @@ pub(crate) async fn post( ) .await { - Ok(Some((version, new_password_hash))) => { + Ok(PasswordVerificationResult::Success(Some((version, new_password_hash)))) => { // Save the upgraded password repo.user_password() .add( @@ -269,10 +272,11 @@ pub(crate) async fn post( ) .await? } - Ok(None) => user_password, - Err(_) => { + Ok(PasswordVerificationResult::Success(None)) => user_password, + Ok(PasswordVerificationResult::Failure) => { + tracing::warn!(username, "Failed to verify/upgrade password for user"); let form_state = form_state.with_error_on_form(FormError::InvalidCredentials); - PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); + PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "mismatch")]); return render( locale, cookie_jar, @@ -286,11 +290,13 @@ pub(crate) async fn post( ) .await; } + Err(err) => return Err(InternalError::from_anyhow(err)), }; // Now that we have checked the user password, we now want to show an error if // the user is locked or deactivated if user.deactivated_at.is_some() { + tracing::warn!(username, "User is deactivated"); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); let ctx = AccountInactiveContext::new(user) @@ -301,6 +307,7 @@ pub(crate) async fn post( } if user.locked_at.is_some() { + tracing::warn!(username, "User is locked"); PASSWORD_LOGIN_COUNTER.add(1, &[KeyValue::new(RESULT, "error")]); let (csrf_token, cookie_jar) = cookie_jar.csrf_token(&clock, &mut rng); let ctx = AccountInactiveContext::new(user)