From f5b34b5b18ff0365362afe30a2692438f8184d0d Mon Sep 17 00:00:00 2001 From: Quentin Gliech Date: Thu, 21 Mar 2024 16:54:00 +0100 Subject: [PATCH] Flatten the passwords config section --- crates/cli/src/util.rs | 4 +- crates/config/src/sections/passwords.rs | 99 +++++++++++++++---------- crates/handlers/src/passwords.rs | 26 +++++-- docs/config.schema.json | 98 +++++++++++------------- 4 files changed, 123 insertions(+), 104 deletions(-) diff --git a/crates/cli/src/util.rs b/crates/cli/src/util.rs index a8c47603..e92589d7 100644 --- a/crates/cli/src/util.rs +++ b/crates/cli/src/util.rs @@ -41,11 +41,11 @@ pub async fn password_manager_from_config( .load() .await? .into_iter() - .map(|(version, algorithm, secret)| { + .map(|(version, algorithm, cost, secret)| { use mas_handlers::passwords::Hasher; let hasher = match algorithm { mas_config::PasswordAlgorithm::Pbkdf2 => Hasher::pbkdf2(secret), - mas_config::PasswordAlgorithm::Bcrypt { cost } => Hasher::bcrypt(cost, secret), + mas_config::PasswordAlgorithm::Bcrypt => Hasher::bcrypt(cost, secret), mas_config::PasswordAlgorithm::Argon2id => Hasher::argon2id(secret), }; diff --git a/crates/config/src/sections/passwords.rs b/crates/config/src/sections/passwords.rs index 9835a8da..1915f982 100644 --- a/crates/config/src/sections/passwords.rs +++ b/crates/config/src/sections/passwords.rs @@ -12,8 +12,6 @@ // See the License for the specific language governing permissions and // limitations under the License. -use std::borrow::Cow; - use anyhow::bail; use async_trait::async_trait; use camino::Utf8PathBuf; @@ -27,7 +25,9 @@ fn default_schemes() -> Vec { vec![HashingScheme { version: 1, algorithm: Algorithm::Argon2id, + cost: None, secret: None, + secret_file: None, }] } @@ -66,6 +66,36 @@ impl ConfigurationSection for PasswordsConfig { Ok(Self::default()) } + fn validate(&self, figment: &figment::Figment) -> Result<(), figment::Error> { + let annotate = |mut error: figment::Error| { + error.metadata = figment.find_metadata(Self::PATH.unwrap()).cloned(); + error.profile = Some(figment::Profile::Default); + error.path = vec![Self::PATH.unwrap().to_owned()]; + Err(error) + }; + + if !self.enabled { + // Skip validation if password-based authentication is disabled + return Ok(()); + } + + if self.schemes.is_empty() { + return annotate(figment::Error::from( + "Requires at least one password scheme in the config".to_owned(), + )); + } + + for scheme in &self.schemes { + if scheme.secret.is_some() && scheme.secret_file.is_some() { + return annotate(figment::Error::from( + "Cannot specify both `secret` and `secret_file`".to_owned(), + )); + } + } + + Ok(()) + } + fn test() -> Self { Self::default() } @@ -84,7 +114,9 @@ impl PasswordsConfig { /// /// Returns an error if the config is invalid, or if the secret file could /// not be read. - pub async fn load(&self) -> Result>)>, anyhow::Error> { + pub async fn load( + &self, + ) -> Result, Option>)>, anyhow::Error> { let mut schemes: Vec<&HashingScheme> = self.schemes.iter().collect(); schemes.sort_unstable_by_key(|a| a.version); schemes.dedup_by_key(|a| a.version); @@ -102,64 +134,53 @@ impl PasswordsConfig { let mut mapped_result = Vec::with_capacity(schemes.len()); for scheme in schemes { - let secret = if let Some(secret_or_file) = &scheme.secret { - Some(secret_or_file.load().await?.into_owned()) - } else { - None + let secret = match (&scheme.secret, &scheme.secret_file) { + (Some(secret), None) => Some(secret.clone().into_bytes()), + (None, Some(secret_file)) => { + let secret = tokio::fs::read(secret_file).await?; + Some(secret) + } + (Some(_), Some(_)) => bail!("Cannot specify both `secret` and `secret_file`"), + (None, None) => None, }; - mapped_result.push((scheme.version, scheme.algorithm, secret)); + mapped_result.push((scheme.version, scheme.algorithm, scheme.cost, secret)); } Ok(mapped_result) } } -#[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "snake_case")] -pub enum SecretOrFile { - Secret(String), - #[schemars(with = "String")] - SecretFile(Utf8PathBuf), -} - -impl SecretOrFile { - async fn load(&self) -> Result, std::io::Error> { - match self { - Self::Secret(secret) => Ok(Cow::Borrowed(secret.as_bytes())), - Self::SecretFile(path) => { - let secret = tokio::fs::read(path).await?; - Ok(Cow::Owned(secret)) - } - } - } -} - #[derive(Debug, Clone, Serialize, Deserialize, JsonSchema)] pub struct HashingScheme { version: u16, - #[serde(flatten)] algorithm: Algorithm, - #[serde(flatten)] - secret: Option, + /// Cost for the bcrypt algorithm + #[serde(skip_serializing_if = "Option::is_none")] + #[schemars(default = "default_bcrypt_cost")] + cost: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + secret: Option, + + #[serde(skip_serializing_if = "Option::is_none")] + #[schemars(with = "Option")] + secret_file: Option, } -fn default_bcrypt_cost() -> u32 { - 12 +#[allow(clippy::unnecessary_wraps)] +fn default_bcrypt_cost() -> Option { + Some(12) } /// A hashing algorithm #[derive(Debug, Clone, Copy, Serialize, Deserialize, JsonSchema)] -#[serde(rename_all = "lowercase", tag = "algorithm")] +#[serde(rename_all = "lowercase")] pub enum Algorithm { /// bcrypt - Bcrypt { - /// Hashing cost - #[serde(default = "default_bcrypt_cost")] - cost: u32, - }, + Bcrypt, /// argon2id Argon2id, diff --git a/crates/handlers/src/passwords.rs b/crates/handlers/src/passwords.rs index 9e29d9bb..9ab63f1a 100644 --- a/crates/handlers/src/passwords.rs +++ b/crates/handlers/src/passwords.rs @@ -53,7 +53,7 @@ impl PasswordManager { /// PasswordManager::new([ /// (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), /// (2, Hasher::argon2id(None)), - /// (1, Hasher::bcrypt(10, None)), + /// (1, Hasher::bcrypt(Some(10), None)), /// ]).unwrap(); /// ``` /// @@ -216,7 +216,7 @@ pub struct Hasher { impl Hasher { /// Creates a new hashing scheme based on the bcrypt algorithm #[must_use] - pub const fn bcrypt(cost: u32, pepper: Option>) -> Self { + pub const fn bcrypt(cost: Option, pepper: Option>) -> Self { let algorithm = Algorithm::Bcrypt { cost }; Self { algorithm, pepper } } @@ -252,7 +252,7 @@ impl Hasher { #[derive(Debug, Clone, Copy)] enum Algorithm { - Bcrypt { cost: u32 }, + Bcrypt { cost: Option }, Argon2id, Pbkdf2, } @@ -273,7 +273,7 @@ impl Algorithm { let salt = rng.gen(); - let hashed = bcrypt::hash_with_salt(password, cost, salt)?; + let hashed = bcrypt::hash_with_salt(password, cost.unwrap_or(12), salt)?; Ok(hashed.format_for_version(bcrypt::Version::TwoB)) } @@ -369,7 +369,7 @@ mod tests { let pepper = b"a-secret-pepper"; let pepper2 = b"the-wrong-pepper"; - let alg = Algorithm::Bcrypt { cost: 10 }; + let alg = Algorithm::Bcrypt { cost: Some(10) }; // Hash with a pepper let hash = alg .hash_blocking(&mut rng, password, Some(pepper)) @@ -454,6 +454,7 @@ mod tests { assert!(alg.verify_blocking(&hash, password, Some(pepper)).is_err()); } + #[allow(clippy::too_many_lines)] #[tokio::test] async fn hash_verify_and_upgrade() { // Tests the whole password manager, by hashing a password and upgrading it @@ -465,7 +466,10 @@ mod tests { let manager = PasswordManager::new([ // Start with one hashing scheme: the one used by synapse, bcrypt + pepper - (1, Hasher::bcrypt(10, Some(b"a-secret-pepper".to_vec()))), + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), ]) .unwrap(); @@ -511,7 +515,10 @@ mod tests { let manager = PasswordManager::new([ (2, Hasher::argon2id(None)), - (1, Hasher::bcrypt(10, Some(b"a-secret-pepper".to_vec()))), + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), ]) .unwrap(); @@ -562,7 +569,10 @@ mod tests { let manager = PasswordManager::new([ (3, Hasher::argon2id(Some(b"a-secret-pepper".to_vec()))), (2, Hasher::argon2id(None)), - (1, Hasher::bcrypt(10, Some(b"a-secret-pepper".to_vec()))), + ( + 1, + Hasher::bcrypt(Some(10), Some(b"a-secret-pepper".to_vec())), + ), ]) .unwrap(); diff --git a/docs/config.schema.json b/docs/config.schema.json index 7e4f3b42..4ce59eb7 100644 --- a/docs/config.schema.json +++ b/docs/config.schema.json @@ -1532,63 +1532,9 @@ } }, "HashingScheme": { - "description": "A hashing algorithm", "type": "object", - "oneOf": [ - { - "description": "bcrypt", - "type": "object", - "required": [ - "algorithm" - ], - "properties": { - "algorithm": { - "type": "string", - "enum": [ - "bcrypt" - ] - }, - "cost": { - "description": "Hashing cost", - "default": 12, - "type": "integer", - "format": "uint32", - "minimum": 0.0 - } - } - }, - { - "description": "argon2id", - "type": "object", - "required": [ - "algorithm" - ], - "properties": { - "algorithm": { - "type": "string", - "enum": [ - "argon2id" - ] - } - } - }, - { - "description": "PBKDF2", - "type": "object", - "required": [ - "algorithm" - ], - "properties": { - "algorithm": { - "type": "string", - "enum": [ - "pbkdf2" - ] - } - } - } - ], "required": [ + "algorithm", "version" ], "properties": { @@ -1596,9 +1542,51 @@ "type": "integer", "format": "uint16", "minimum": 0.0 + }, + "algorithm": { + "$ref": "#/definitions/Algorithm" + }, + "cost": { + "description": "Cost for the bcrypt algorithm", + "default": 12, + "type": "integer", + "format": "uint32", + "minimum": 0.0 + }, + "secret": { + "type": "string" + }, + "secret_file": { + "type": "string" } } }, + "Algorithm": { + "description": "A hashing algorithm", + "oneOf": [ + { + "description": "bcrypt", + "type": "string", + "enum": [ + "bcrypt" + ] + }, + { + "description": "argon2id", + "type": "string", + "enum": [ + "argon2id" + ] + }, + { + "description": "PBKDF2", + "type": "string", + "enum": [ + "pbkdf2" + ] + } + ] + }, "MatrixConfig": { "description": "Configuration related to the Matrix homeserver", "type": "object",