From fbc360d1a94ef2ebf63d979bb403228a700f43c8 Mon Sep 17 00:00:00 2001 From: reivilibre Date: Thu, 11 Jul 2024 10:17:39 +0100 Subject: [PATCH] Backend work to support minimum password complexity (#2965) * config: Add minimum password complexity option * PasswordManager: add function for checking if complexity is sufficient * Enforce password complexity on registration, change and recovery * cli: Use exit code 1 for weak passwords This seems preferable to exit code 0, but ideally we should choose one and document it. * Expose minimum password complexity score over GraphQL --- Cargo.lock | 75 ++++++++++++++++++ crates/cli/src/commands/config.rs | 6 +- crates/cli/src/commands/database.rs | 6 +- crates/cli/src/commands/debug.rs | 6 +- crates/cli/src/commands/doctor.rs | 6 +- crates/cli/src/commands/manage.rs | 56 ++++++++++---- crates/cli/src/commands/mod.rs | 4 +- crates/cli/src/commands/server.rs | 6 +- crates/cli/src/commands/templates.rs | 6 +- crates/cli/src/commands/worker.rs | 6 +- crates/cli/src/main.rs | 10 +-- crates/cli/src/util.rs | 3 +- crates/config/src/sections/passwords.rs | 24 ++++++ crates/data-model/src/site_config.rs | 4 + crates/handlers/Cargo.toml | 1 + .../handlers/src/graphql/model/site_config.rs | 6 ++ crates/handlers/src/graphql/mutations/user.rs | 17 ++++- crates/handlers/src/passwords.rs | 76 +++++++++++++------ crates/handlers/src/test_utils.rs | 6 +- crates/handlers/src/views/recovery/finish.rs | 12 +++ crates/handlers/src/views/register.rs | 14 +++- docs/config.schema.json | 10 ++- frontend/schema.graphql | 6 ++ frontend/src/gql/graphql.ts | 6 ++ frontend/src/gql/schema.ts | 11 +++ 25 files changed, 317 insertions(+), 66 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 00c3136a..136cf0d0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -762,6 +762,21 @@ dependencies = [ "which", ] +[[package]] +name = "bit-set" +version = "0.5.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0700ddab506f33b20a03b13996eccd309a48e5ff77d0d95926aa0210fb4e95f1" +dependencies = [ + "bit-vec", +] + +[[package]] +name = "bit-vec" +version = "0.6.3" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "349f9b6a179ed607305526ca489b34ad0a41aed5f7980fa90eb03160b69598fb" + [[package]] name = "bitflags" version = "1.3.2" @@ -1555,6 +1570,37 @@ dependencies = [ "serde", ] +[[package]] +name = "derive_builder" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "0350b5cb0331628a5916d6c5c0b72e97393b8b6b03b47a9284f4e7f5a405ffd7" +dependencies = [ + "derive_builder_macro", +] + +[[package]] +name = "derive_builder_core" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d48cda787f839151732d396ac69e3473923d54312c070ee21e9effcaa8ca0b1d" +dependencies = [ + "darling 0.20.9", + "proc-macro2", + "quote", + "syn 2.0.68", +] + +[[package]] +name = "derive_builder_macro" +version = "0.20.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "206868b8242f27cecce124c19fd88157fbd0dd334df2587f36417bafbc85097b" +dependencies = [ + "derive_builder_core", + "syn 2.0.68", +] + [[package]] name = "dialoguer" version = "0.11.0" @@ -1773,6 +1819,17 @@ version = "0.3.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "2acce4a10f12dc2fb14a218589d4f1f62ef011b2d0cc4b3cb1bba8e94da14649" +[[package]] +name = "fancy-regex" +version = "0.13.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "531e46835a22af56d1e3b66f04844bed63158bc094a628bec1d321d9b4c44bf2" +dependencies = [ + "bit-set", + "regex-automata 0.4.7", + "regex-syntax 0.8.4", +] + [[package]] name = "fast_chemail" version = "0.9.6" @@ -3306,6 +3363,7 @@ dependencies = [ "ulid", "url", "zeroize", + "zxcvbn", ] [[package]] @@ -7428,3 +7486,20 @@ dependencies = [ "quote", "syn 2.0.68", ] + +[[package]] +name = "zxcvbn" +version = "3.0.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "579b1d84df61d9d04cd250035843fee2f86a4b4bb176f102fec20779fd0bd38b" +dependencies = [ + "derive_builder", + "fancy-regex", + "getrandom", + "itertools 0.13.0", + "lazy_static", + "regex", + "time", + "wasm-bindgen", + "web-sys", +] diff --git a/crates/cli/src/commands/config.rs b/crates/cli/src/commands/config.rs index 51508d24..0de119f7 100644 --- a/crates/cli/src/commands/config.rs +++ b/crates/cli/src/commands/config.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use anyhow::Context; use camino::Utf8PathBuf; use clap::Parser; @@ -68,7 +70,7 @@ enum Subcommand { } impl Options { - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { use Subcommand as SC; match self.subcommand { SC::Dump { output } => { @@ -139,6 +141,6 @@ impl Options { } } - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/commands/database.rs b/crates/cli/src/commands/database.rs index fc683040..9572bf1b 100644 --- a/crates/cli/src/commands/database.rs +++ b/crates/cli/src/commands/database.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use anyhow::Context; use clap::Parser; use figment::Figment; @@ -34,7 +36,7 @@ enum Subcommand { } impl Options { - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { let _span = info_span!("cli.database.migrate").entered(); let config = DatabaseConfig::extract(figment)?; let mut conn = database_connection_from_config(&config).await?; @@ -46,6 +48,6 @@ impl Options { .await .context("could not run migrations")?; - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/commands/debug.rs b/crates/cli/src/commands/debug.rs index 8404ef49..25308695 100644 --- a/crates/cli/src/commands/debug.rs +++ b/crates/cli/src/commands/debug.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use clap::Parser; use figment::Figment; use http_body_util::BodyExt; @@ -67,7 +69,7 @@ fn print_headers(parts: &hyper::http::response::Parts) { impl Options { #[tracing::instrument(skip_all)] - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { use Subcommand as SC; let http_client_factory = HttpClientFactory::new(); match self.subcommand { @@ -130,6 +132,6 @@ impl Options { } } - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/commands/doctor.rs b/crates/cli/src/commands/doctor.rs index e75bb4f4..57240acf 100644 --- a/crates/cli/src/commands/doctor.rs +++ b/crates/cli/src/commands/doctor.rs @@ -17,6 +17,8 @@ //! The code is quite repetitive for now, but we can refactor later with a //! better check abstraction +use std::process::ExitCode; + use anyhow::Context; use clap::Parser; use figment::Figment; @@ -35,7 +37,7 @@ pub(super) struct Options {} impl Options { #[allow(clippy::too_many_lines)] - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { let _span = info_span!("cli.doctor").entered(); info!("💡 Running diagnostics, make sure that both MAS and Synapse are running, and that MAS is using the same configuration files as this tool."); @@ -423,6 +425,6 @@ Error details: {e}"# ), } - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/commands/manage.rs b/crates/cli/src/commands/manage.rs index ab66a38f..e861b9f0 100644 --- a/crates/cli/src/commands/manage.rs +++ b/crates/cli/src/commands/manage.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::collections::BTreeMap; +use std::{collections::BTreeMap, process::ExitCode}; use anyhow::Context; use clap::{ArgAction, CommandFactory, Parser}; @@ -34,7 +34,7 @@ use mas_storage::{ use mas_storage_pg::{DatabaseError, PgRepository}; use rand::{RngCore, SeedableRng}; use sqlx::{types::Uuid, Acquire}; -use tracing::{info, info_span, warn}; +use tracing::{error, info, info_span, warn}; use crate::util::{database_connection_from_config, password_manager_from_config}; @@ -69,7 +69,14 @@ enum Subcommand { VerifyEmail { username: String, email: String }, /// Set a user password - SetPassword { username: String, password: String }, + SetPassword { + username: String, + password: String, + /// Don't enforce that the password provided is above the minimum + /// configured complexity. + #[clap(long)] + ignore_complexity: bool, + }, /// Issue a compatibility token IssueCompatibilityToken { @@ -158,19 +165,27 @@ enum Subcommand { /// Set the user's display name #[arg(short, long, help_heading = USER_ATTRIBUTES_HEADING)] display_name: Option, + /// Don't enforce that the password provided is above the minimum + /// configured complexity. + #[clap(long)] + ignore_password_complexity: bool, }, } impl Options { #[allow(clippy::too_many_lines)] - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { use Subcommand as SC; let clock = SystemClock::default(); // XXX: we should disallow SeedableRng::from_entropy let mut rng = rand_chacha::ChaChaRng::from_entropy(); match self.subcommand { - SC::SetPassword { username, password } => { + SC::SetPassword { + username, + password, + ignore_complexity, + } => { let _span = info_span!("cli.manage.set_password", user.username = %username).entered(); @@ -188,6 +203,11 @@ impl Options { .await? .context("User not found")?; + if !ignore_complexity && !password_manager.is_password_complex_enough(&password)? { + error!("That password is too weak."); + return Ok(ExitCode::from(1)); + } + let password = password.into_bytes().into(); let (version, hashed_password) = password_manager.hash(&mut rng, password).await?; @@ -199,7 +219,7 @@ impl Options { info!(%user.id, %user.username, "Password changed"); repo.into_inner().commit().await?; - Ok(()) + Ok(ExitCode::SUCCESS) } SC::VerifyEmail { username, email } => { @@ -236,7 +256,7 @@ impl Options { repo.into_inner().commit().await?; info!(?email, "Email marked as verified"); - Ok(()) + Ok(ExitCode::SUCCESS) } SC::IssueCompatibilityToken { @@ -284,7 +304,7 @@ impl Options { "Compatibility token issued: {}", compat_access_token.token ); - Ok(()) + Ok(ExitCode::SUCCESS) } SC::ProvisionAllUsers => { @@ -309,7 +329,7 @@ impl Options { repo.into_inner().commit().await?; - Ok(()) + Ok(ExitCode::SUCCESS) } SC::KillSessions { username, dry_run } => { @@ -427,7 +447,7 @@ impl Options { txn.commit().await?; } - Ok(()) + Ok(ExitCode::SUCCESS) } SC::LockUser { @@ -462,7 +482,7 @@ impl Options { repo.into_inner().commit().await?; - Ok(()) + Ok(ExitCode::SUCCESS) } SC::UnlockUser { username } => { @@ -483,7 +503,7 @@ impl Options { repo.user().unlock(user).await?; repo.into_inner().commit().await?; - Ok(()) + Ok(ExitCode::SUCCESS) } SC::RegisterUser { @@ -495,6 +515,7 @@ impl Options { no_admin, display_name, yes, + ignore_password_complexity, } => { let http_client_factory = HttpClientFactory::new(); let password_config = PasswordsConfig::extract(figment)?; @@ -512,6 +533,15 @@ impl Options { let txn = conn.begin().await?; let mut repo = PgRepository::from_conn(txn); + if let Some(password) = &password { + if !ignore_password_complexity + && !password_manager.is_password_complex_enough(password)? + { + error!("That password is too weak."); + return Ok(ExitCode::from(1)); + } + } + // If the username is provided, check if it's available and normalize it. let localpart = if let Some(username) = username { check_and_normalize_username(&username, &mut repo, &homeserver) @@ -722,7 +752,7 @@ impl Options { warn!("Aborted"); } - Ok(()) + Ok(ExitCode::SUCCESS) } } } diff --git a/crates/cli/src/commands/mod.rs b/crates/cli/src/commands/mod.rs index 6b81cd3c..57de4947 100644 --- a/crates/cli/src/commands/mod.rs +++ b/crates/cli/src/commands/mod.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use camino::Utf8PathBuf; use clap::Parser; use figment::{ @@ -67,7 +69,7 @@ pub struct Options { } impl Options { - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { use Subcommand as S; // We Box the futures for each subcommand so that we avoid this function being // big on the stack all the time diff --git a/crates/cli/src/commands/server.rs b/crates/cli/src/commands/server.rs index b2bcd286..f3f9f409 100644 --- a/crates/cli/src/commands/server.rs +++ b/crates/cli/src/commands/server.rs @@ -12,7 +12,7 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::{collections::BTreeSet, sync::Arc, time::Duration}; +use std::{collections::BTreeSet, process::ExitCode, sync::Arc, time::Duration}; use anyhow::Context; use clap::Parser; @@ -65,7 +65,7 @@ pub(super) struct Options { impl Options { #[allow(clippy::too_many_lines)] - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { let span = info_span!("cli.run.init").entered(); let config = AppConfig::extract(figment)?; @@ -309,6 +309,6 @@ impl Options { state.activity_tracker.shutdown().await; - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/commands/templates.rs b/crates/cli/src/commands/templates.rs index 9975965e..fcfb4255 100644 --- a/crates/cli/src/commands/templates.rs +++ b/crates/cli/src/commands/templates.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use clap::Parser; use figment::Figment; use mas_config::{ @@ -37,7 +39,7 @@ enum Subcommand { } impl Options { - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { use Subcommand as SC; match self.subcommand { SC::Check => { @@ -66,7 +68,7 @@ impl Options { templates_from_config(&template_config, &site_config, &url_builder).await?; templates.check_render(clock.now(), &mut rng)?; - Ok(()) + Ok(ExitCode::SUCCESS) } } } diff --git a/crates/cli/src/commands/worker.rs b/crates/cli/src/commands/worker.rs index 3f78b053..26234bdf 100644 --- a/crates/cli/src/commands/worker.rs +++ b/crates/cli/src/commands/worker.rs @@ -12,6 +12,8 @@ // See the License for the specific language governing permissions and // limitations under the License. +use std::process::ExitCode; + use clap::Parser; use figment::Figment; use mas_config::{AppConfig, ConfigurationSection}; @@ -32,7 +34,7 @@ use crate::util::{ pub(super) struct Options {} impl Options { - pub async fn run(self, figment: &Figment) -> anyhow::Result<()> { + pub async fn run(self, figment: &Figment) -> anyhow::Result { let span = info_span!("cli.worker.init").entered(); let config = AppConfig::extract(figment)?; @@ -82,6 +84,6 @@ impl Options { span.exit(); monitor.run().await?; - Ok(()) + Ok(ExitCode::SUCCESS) } } diff --git a/crates/cli/src/main.rs b/crates/cli/src/main.rs index ad5352b1..b619d968 100644 --- a/crates/cli/src/main.rs +++ b/crates/cli/src/main.rs @@ -14,7 +14,7 @@ #![allow(clippy::module_name_repetitions)] -use std::{io::IsTerminal, sync::Arc}; +use std::{io::IsTerminal, process::ExitCode, sync::Arc}; use anyhow::Context; use clap::Parser; @@ -35,7 +35,7 @@ mod telemetry; mod util; #[tokio::main] -async fn main() -> anyhow::Result<()> { +async fn main() -> anyhow::Result { // We're splitting the "fallible" part of main in another function to have a // chance to shutdown the telemetry exporters regardless of if there was an // error or not @@ -44,7 +44,7 @@ async fn main() -> anyhow::Result<()> { res } -async fn try_main() -> anyhow::Result<()> { +async fn try_main() -> anyhow::Result { // Load environment variables from .env files // We keep the path to log it afterwards let dotenv_path: Result, _> = dotenvy::dotenv() @@ -136,7 +136,5 @@ async fn try_main() -> anyhow::Result<()> { // And run the command tracing::trace!(?opts, "Running command"); - opts.run(&figment).await?; - - Ok(()) + opts.run(&figment).await } diff --git a/crates/cli/src/util.rs b/crates/cli/src/util.rs index 730b3915..0ca1024d 100644 --- a/crates/cli/src/util.rs +++ b/crates/cli/src/util.rs @@ -53,7 +53,7 @@ pub async fn password_manager_from_config( (version, hasher) }); - PasswordManager::new(schemes) + PasswordManager::new(config.minimum_complexity(), schemes) } pub fn mailer_from_config( @@ -173,6 +173,7 @@ pub fn site_config_from_config( account_recovery_allowed: password_config.enabled() && experimental_config.account_recovery_enabled, captcha, + minimum_password_complexity: password_config.minimum_complexity(), }) } diff --git a/crates/config/src/sections/passwords.rs b/crates/config/src/sections/passwords.rs index 6296f284..ce117763 100644 --- a/crates/config/src/sections/passwords.rs +++ b/crates/config/src/sections/passwords.rs @@ -35,6 +35,10 @@ fn default_enabled() -> bool { true } +fn default_minimum_complexity() -> u8 { + 3 +} + /// User password hashing config #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct PasswordsConfig { @@ -44,6 +48,18 @@ pub struct PasswordsConfig { #[serde(default = "default_schemes")] schemes: Vec, + + /// Score between 0 and 4 determining the minimum allowed password + /// complexity. Scores are based on the ESTIMATED number of guesses + /// needed to guess the password. + /// + /// - 0: less than 10^2 (100) + /// - 1: less than 10^4 (10'000) + /// - 2: less than 10^6 (1'000'000) + /// - 3: less than 10^8 (100'000'000) + /// - 4: any more than that + #[serde(default = "default_minimum_complexity")] + minimum_complexity: u8, } impl Default for PasswordsConfig { @@ -51,6 +67,7 @@ impl Default for PasswordsConfig { Self { enabled: default_enabled(), schemes: default_schemes(), + minimum_complexity: default_minimum_complexity(), } } } @@ -96,6 +113,13 @@ impl PasswordsConfig { self.enabled } + /// Minimum complexity of passwords, from 0 to 4, according to the zxcvbn + /// scorer. + #[must_use] + pub fn minimum_complexity(&self) -> u8 { + self.minimum_complexity + } + /// Load the password hashing schemes defined by the config /// /// # Errors diff --git a/crates/data-model/src/site_config.rs b/crates/data-model/src/site_config.rs index 17893646..d2606220 100644 --- a/crates/data-model/src/site_config.rs +++ b/crates/data-model/src/site_config.rs @@ -78,4 +78,8 @@ pub struct SiteConfig { /// Captcha configuration pub captcha: Option, + + /// Minimum password complexity, between 0 and 4. + /// This is a score from zxcvbn. + pub minimum_password_complexity: u8, } diff --git a/crates/handlers/Cargo.toml b/crates/handlers/Cargo.toml index f385feb7..04006831 100644 --- a/crates/handlers/Cargo.toml +++ b/crates/handlers/Cargo.toml @@ -91,6 +91,7 @@ mas-storage.workspace = true mas-storage-pg.workspace = true mas-templates.workspace = true oauth2-types.workspace = true +zxcvbn = "3.0.1" [dev-dependencies] insta = "1.39.0" diff --git a/crates/handlers/src/graphql/model/site_config.rs b/crates/handlers/src/graphql/model/site_config.rs index d8259614..f22b6dbf 100644 --- a/crates/handlers/src/graphql/model/site_config.rs +++ b/crates/handlers/src/graphql/model/site_config.rs @@ -46,6 +46,11 @@ pub struct SiteConfig { /// Whether passwords are enabled and users can change their own passwords. password_change_allowed: bool, + + /// Minimum password complexity, from 0 to 4, in terms of a zxcvbn score. + /// The exact scorer (including dictionaries and other data tables) + /// in use is . + minimum_password_complexity: u8, } #[ComplexObject] @@ -69,6 +74,7 @@ impl SiteConfig { display_name_change_allowed: data_model.displayname_change_allowed, password_login_enabled: data_model.password_login_enabled, password_change_allowed: data_model.password_change_allowed, + minimum_password_complexity: data_model.minimum_password_complexity, } } } diff --git a/crates/handlers/src/graphql/mutations/user.rs b/crates/handlers/src/graphql/mutations/user.rs index 86bc3577..7568c2ee 100644 --- a/crates/handlers/src/graphql/mutations/user.rs +++ b/crates/handlers/src/graphql/mutations/user.rs @@ -480,6 +480,20 @@ impl UserMutations { }); } + let password_manager = state.password_manager(); + + if !password_manager.is_enabled() { + return Ok(SetPasswordPayload { + status: SetPasswordStatus::PasswordChangesDisabled, + }); + } + + if !password_manager.is_password_complex_enough(&input.new_password)? { + return Ok(SetPasswordPayload { + status: SetPasswordStatus::InvalidNewPassword, + }); + } + let mut repo = state.repository().await?; let Some(user) = repo.user().lookup(user_id).await? else { return Ok(SetPasswordPayload { @@ -487,13 +501,12 @@ impl UserMutations { }); }; - let password_manager = state.password_manager(); if !requester.is_admin() { // If the user isn't an admin, we: // - check that password changes are enabled // - check that they know their current password - if !state.site_config().password_change_allowed || !password_manager.is_enabled() { + if !state.site_config().password_change_allowed { return Ok(SetPasswordPayload { status: SetPasswordStatus::PasswordChangesDisabled, }); diff --git a/crates/handlers/src/passwords.rs b/crates/handlers/src/passwords.rs index af3caf2b..6095ffcc 100644 --- a/crates/handlers/src/passwords.rs +++ b/crates/handlers/src/passwords.rs @@ -21,6 +21,7 @@ use pbkdf2::Pbkdf2; use rand::{CryptoRng, Rng, RngCore, SeedableRng}; use thiserror::Error; use zeroize::Zeroizing; +use zxcvbn::zxcvbn; pub type SchemeVersion = u16; @@ -34,6 +35,9 @@ pub struct PasswordManager { } struct InnerPasswordManager { + /// Minimum complexity score of new passwords (between 0 and 4) as evaluated + /// by zxcvbn. + minimum_complexity: u8, current_hasher: Hasher, current_version: SchemeVersion, @@ -42,7 +46,8 @@ struct InnerPasswordManager { } impl PasswordManager { - /// Creates a new [`PasswordManager`] from an iterator. The first item in + /// Creates a new [`PasswordManager`] from an iterator and a minimum allowed + /// complexity score between 0 and 4. The first item in /// the iterator will be the default hashing scheme. /// /// # Example @@ -50,7 +55,7 @@ impl PasswordManager { /// ```rust /// pub use mas_handlers::passwords::{PasswordManager, Hasher}; /// - /// PasswordManager::new([ + /// PasswordManager::new(3, [ /// (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), /// (2, Hasher::argon2id(None)), /// (1, Hasher::bcrypt(Some(10), None)), @@ -61,6 +66,7 @@ impl PasswordManager { /// /// Returns an error if the iterator was empty pub fn new>( + minimum_complexity: u8, iter: I, ) -> Result { let mut iter = iter.into_iter(); @@ -75,6 +81,7 @@ impl PasswordManager { Ok(Self { inner: Some(Arc::new(InnerPasswordManager { + minimum_complexity, current_hasher, current_version, other_hashers, @@ -103,6 +110,18 @@ impl PasswordManager { self.inner.clone().ok_or(PasswordManagerDisabledError) } + /// Returns true if and only if the given password satisfies the minimum + /// complexity requirements. + /// + /// # Errors + /// + /// Returns an error if the password manager is disabled + pub fn is_password_complex_enough(&self, password: &str) -> Result { + let inner = self.get_inner()?; + let score = zxcvbn(password, &[]); + Ok(u8::from(score.score()) >= inner.minimum_complexity) + } + /// Hash a password with the default hashing scheme. /// Returns the version of the hashing scheme used and the hashed password. /// @@ -461,13 +480,16 @@ mod tests { let password = Zeroizing::new(b"hunter2".to_vec()); let wrong_password = Zeroizing::new(b"wrong-password".to_vec()); - let manager = PasswordManager::new([ - // Start with one hashing scheme: the one used by synapse, bcrypt + pepper - ( - 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), - ), - ]) + let manager = PasswordManager::new( + 0, + [ + // Start with one hashing scheme: the one used by synapse, bcrypt + pepper + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), + ], + ) .unwrap(); let (version, hash) = manager @@ -510,13 +532,16 @@ mod tests { .await .expect_err("Verification should have failed"); - let manager = PasswordManager::new([ - (2, Hasher::argon2id(None)), - ( - 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), - ), - ]) + let manager = PasswordManager::new( + 0, + [ + (2, Hasher::argon2id(None)), + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), + ], + ) .unwrap(); // Verifying still works @@ -563,14 +588,17 @@ mod tests { .await .expect_err("Verification should have failed"); - let manager = PasswordManager::new([ - (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), - (2, Hasher::argon2id(None)), - ( - 1, - Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), - ), - ]) + let manager = PasswordManager::new( + 0, + [ + (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), + (2, Hasher::argon2id(None)), + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), + ], + ) .unwrap(); // Verifying still works diff --git a/crates/handlers/src/test_utils.rs b/crates/handlers/src/test_utils.rs index cdead2bb..f2870870 100644 --- a/crates/handlers/src/test_utils.rs +++ b/crates/handlers/src/test_utils.rs @@ -136,6 +136,7 @@ pub fn test_site_config() -> SiteConfig { password_change_allowed: true, account_recovery_allowed: true, captcha: None, + minimum_password_complexity: 1, } } @@ -178,7 +179,10 @@ impl TestState { let metadata_cache = MetadataCache::new(); let password_manager = if site_config.password_login_enabled { - PasswordManager::new([(1, Hasher::argon2id(None))])? + PasswordManager::new( + site_config.minimum_password_complexity, + [(1, Hasher::argon2id(None))], + )? } else { PasswordManager::disabled() }; diff --git a/crates/handlers/src/views/recovery/finish.rs b/crates/handlers/src/views/recovery/finish.rs index e5dcbe38..0086100a 100644 --- a/crates/handlers/src/views/recovery/finish.rs +++ b/crates/handlers/src/views/recovery/finish.rs @@ -226,6 +226,18 @@ pub(crate) async fn post( ); } + if !password_manager.is_password_complex_enough(&form.new_password)? { + // TODO This error should be localised, + // but actually this entire form should be pushed down into the + // React frontend so user gets real-time feedback. + form_state = form_state.with_error_on_field( + RecoveryFinishFormField::NewPassword, + FieldError::Policy { + message: "Password is too weak".to_owned(), + }, + ); + } + let res = policy.evaluate_password(&form.new_password).await?; if !res.valid() { diff --git a/crates/handlers/src/views/register.rs b/crates/handlers/src/views/register.rs index 67cb9425..7049f6fd 100644 --- a/crates/handlers/src/views/register.rs +++ b/crates/handlers/src/views/register.rs @@ -198,6 +198,16 @@ pub(crate) async fn post( ); } + if !password_manager.is_password_complex_enough(&form.password)? { + // TODO localise this error + state.add_error_on_field( + RegisterFormField::Password, + FieldError::Policy { + message: "Password is too weak".to_owned(), + }, + ); + } + // If the site has terms of service, the user must accept them if site_config.tos_uri.is_some() && form.accept_terms != "on" { state.add_error_on_field(RegisterFormField::AcceptTerms, FieldError::Required); @@ -402,8 +412,8 @@ mod tests { "csrf": csrf_token, "username": "john", "email": "john@example.com", - "password": "hunter2", - "password_confirm": "hunter2", + "password": "correcthorsebatterystaple", + "password_confirm": "correcthorsebatterystaple", "accept_terms": "on", }), ); diff --git a/docs/config.schema.json b/docs/config.schema.json index 7cc7df8a..46024fb9 100644 --- a/docs/config.schema.json +++ b/docs/config.schema.json @@ -144,7 +144,8 @@ "version": 1, "algorithm": "argon2id" } - ] + ], + "minimum_complexity": 3 }, "allOf": [ { @@ -1507,6 +1508,13 @@ "items": { "$ref": "#/definitions/HashingScheme" } + }, + "minimum_complexity": { + "description": "Score between 0 and 4 determining the minimum allowed password complexity. Scores are based on the ESTIMATED number of guesses needed to guess the password.\n\n- 0: less than 10^2 (100) - 1: less than 10^4 (10'000) - 2: less than 10^6 (1'000'000) - 3: less than 10^8 (100'000'000) - 4: any more than that", + "default": 3, + "type": "integer", + "format": "uint8", + "minimum": 0.0 } } }, diff --git a/frontend/schema.graphql b/frontend/schema.graphql index c546e521..a5eab5b3 100644 --- a/frontend/schema.graphql +++ b/frontend/schema.graphql @@ -1388,6 +1388,12 @@ type SiteConfig implements Node { """ passwordChangeAllowed: Boolean! """ + Minimum password complexity, from 0 to 4, in terms of a zxcvbn score. + The exact scorer (including dictionaries and other data tables) + in use is . + """ + minimumPasswordComplexity: Int! + """ The ID of the site configuration. """ id: ID! diff --git a/frontend/src/gql/graphql.ts b/frontend/src/gql/graphql.ts index 66d08810..dea24262 100644 --- a/frontend/src/gql/graphql.ts +++ b/frontend/src/gql/graphql.ts @@ -1022,6 +1022,12 @@ export type SiteConfig = Node & { id: Scalars['ID']['output']; /** Imprint to show in the footer. */ imprint?: Maybe; + /** + * Minimum password complexity, from 0 to 4, in terms of a zxcvbn score. + * The exact scorer (including dictionaries and other data tables) + * in use is . + */ + minimumPasswordComplexity: Scalars['Int']['output']; /** Whether passwords are enabled and users can change their own passwords. */ passwordChangeAllowed: Scalars['Boolean']['output']; /** Whether passwords are enabled for login. */ diff --git a/frontend/src/gql/schema.ts b/frontend/src/gql/schema.ts index 55b7dc9c..20fd4d78 100644 --- a/frontend/src/gql/schema.ts +++ b/frontend/src/gql/schema.ts @@ -2554,6 +2554,17 @@ export default { }, "args": [] }, + { + "name": "minimumPasswordComplexity", + "type": { + "kind": "NON_NULL", + "ofType": { + "kind": "SCALAR", + "name": "Any" + } + }, + "args": [] + }, { "name": "passwordChangeAllowed", "type": {