Flatten the passwords config section

Author: sandhose

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),
             };
 

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<HashingScheme> {
     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<Vec<(u16, Algorithm, Option<Vec<u8>>)>, anyhow::Error> {
+    pub async fn load(
+        &self,
+    ) -> Result<Vec<(u16, Algorithm, Option<u32>, Option<Vec<u8>>)>, 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<Cow<'_, [u8]>, 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<SecretOrFile>,
+    /// Cost for the bcrypt algorithm
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[schemars(default = "default_bcrypt_cost")]
+    cost: Option<u32>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    secret: Option<String>,
+
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[schemars(with = "Option<String>")]
+    secret_file: Option<Utf8PathBuf>,
 }
 
-fn default_bcrypt_cost() -> u32 {
-    12
+#[allow(clippy::unnecessary_wraps)]
+fn default_bcrypt_cost() -> Option<u32> {
+    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,

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<Vec<u8>>) -> Self {
+    pub const fn bcrypt(cost: Option<u32>, pepper: Option<Vec<u8>>) -> 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<u32> },
     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();
 

docs/config.schema.json

@@ -1532,73 +1532,61 @@
       }
     },
     "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": {
         "version": {
           "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",