client: Stop checking secret length

client/src/crypto/mod.rs

@@ -56,7 +56,7 @@ mod test {
         let salt = &[0x92, 0xf4, 0xc0, 0x34, 0x0f, 0x5f, 0x36, 0xf9];
         let iteration_count = 1782;
         let password = b"test";
-        let (key, iv) = legacy_derive_key_and_iv(password, Ok(()), salt, iteration_count).unwrap();
+        let (key, iv) = legacy_derive_key_and_iv(password, salt, iteration_count).unwrap();
         assert_eq!(key.as_ref(), &expected_key[..]);
         assert_eq!(iv, &expected_iv[..]);
     }

client/src/crypto/native.rs

@@ -173,7 +173,6 @@ pub(crate) fn derive_key(
 
 pub(crate) fn legacy_derive_key_and_iv(
     secret: impl AsRef<[u8]>,
-    key_strength: Result<(), file::WeakKeyError>,
     salt: impl AsRef<[u8]>,
     iteration_count: usize,
 ) -> Result<(Key, Vec<u8>), super::Error> {
@@ -215,7 +214,8 @@ pub(crate) fn legacy_derive_key_and_iv(
     }
 
     let iv = buffer.split_off(EncAlg::key_size());
-    Ok((Key::new_with_strength(buffer, key_strength), iv))
+    // Only used to decrypt the legacy keyring, never to encrypt
+    Ok((Key::new_with_strength(buffer, Ok(())), iv))
 }
 
 /// from https://github.com/plietar/librespot/blob/master/core/src/util/mod.rs#L53

client/src/crypto/openssl.rs

@@ -195,7 +195,6 @@ pub(crate) fn derive_key(
 
 pub(crate) fn legacy_derive_key_and_iv(
     secret: impl AsRef<[u8]>,
-    key_strength: Result<(), file::WeakKeyError>,
     salt: impl AsRef<[u8]>,
     iteration_count: usize,
 ) -> Result<(Key, Vec<u8>), super::Error> {
@@ -234,5 +233,6 @@ pub(crate) fn legacy_derive_key_and_iv(
     }
 
     let iv = buffer.split_off(cipher.key_len());
-    Ok((Key::new_with_strength(buffer, key_strength), iv))
+    // Only used to decrypt the legacy keyring, never to encrypt
+    Ok((Key::new_with_strength(buffer, Ok(())), iv))
 }

client/src/file/api/legacy_keyring/encrypted.rs

@@ -11,7 +11,7 @@ use zeroize::{Zeroize, ZeroizeOnDrop};
 use super::{FILE_HEADER, FILE_HEADER_LEN};
 use crate::{
     AsAttributes, Secret, crypto,
-    file::{Error, UnlockedItem, WeakKeyError},
+    file::{Error, UnlockedItem},
 };
 
 pub const MAJOR_VERSION: u8 = 0;
@@ -29,7 +29,6 @@ impl Keyring {
     pub fn decrypt_items(self, secret: &Secret) -> Result<Vec<UnlockedItem>, Error> {
         let (key, iv) = crypto::legacy_derive_key_and_iv(
             &**secret,
-            self.key_strength(secret),
             &self.salt,
             self.iteration_count.try_into().unwrap(),
         )?;
@@ -99,10 +98,6 @@ impl Keyring {
         Ok(items)
     }
 
-    fn key_strength(&self, _secret: &[u8]) -> Result<(), WeakKeyError> {
-        Ok(())
-    }
-
     fn read_byte_array<'a>(cursor: &mut Cursor<&'a [u8]>) -> Result<Option<&'a [u8]>, Error> {
         let len = cursor.read_u32(Endian::Big)? as usize;
         if len == 0xffffffff {

client/src/file/api/mod.rs

@@ -30,8 +30,6 @@ const DEFAULT_SALT_SIZE: usize = 32;
 
 const MIN_ITERATION_COUNT: u32 = 100000;
 const MIN_SALT_SIZE: usize = 32;
-// FIXME: choose a reasonable value
-const MIN_PASSWORD_LENGTH: usize = 4;
 
 const FILE_HEADER: &[u8] = b"GnomeKeyring\n\r\0\n";
 const FILE_HEADER_LEN: usize = FILE_HEADER.len();
@@ -101,13 +99,13 @@ impl Keyring {
         })
     }
 
-    pub fn key_strength(&self, secret: &[u8]) -> Result<(), WeakKeyError> {
+    /// Check the key derivation parameters stored in the file.
+    /// The password itself is not checked, its policy is up to the caller.
+    pub fn key_strength(&self) -> Result<(), WeakKeyError> {
         if self.iteration_count < MIN_ITERATION_COUNT {
             Err(WeakKeyError::IterationCountTooLow(self.iteration_count))
         } else if self.salt.len() < MIN_SALT_SIZE {
             Err(WeakKeyError::SaltTooShort(self.salt.len()))
-        } else if secret.len() < MIN_PASSWORD_LENGTH {
-            Err(WeakKeyError::PasswordTooShort(secret.len()))
         } else {
             Ok(())
         }
@@ -300,16 +298,7 @@ impl Keyring {
     pub fn derive_key(&self, secret: &Secret) -> Result<Key, crypto::Error> {
         crypto::derive_key(
             &**secret,
-            self.key_strength(secret),
-            &self.salt,
-            self.iteration_count.try_into().unwrap(),
-        )
-    }
-
-    pub(crate) fn derive_key_unchecked(&self, secret: &Secret) -> Result<Key, crypto::Error> {
-        crypto::derive_key(
-            &**secret,
-            Ok(()),
+            self.key_strength(),
             &self.salt,
             self.iteration_count.try_into().unwrap(),
         )
@@ -481,24 +470,19 @@ mod tests {
     async fn key_strength() -> Result<(), Error> {
         let mut keyring = Keyring::new()?;
         keyring.iteration_count = 50000; // Less than MIN_ITERATION_COUNT (100000)
-        let secret = Secret::from("test-password-that-is-long-enough");
-        let result = keyring.key_strength(&secret);
+        let result = keyring.key_strength();
         assert!(matches!(
             result,
             Err(WeakKeyError::IterationCountTooLow(50000))
         ));
 
-        let keyring = Keyring::new()?;
-        let secret = Secret::from("ab");
-        let result = keyring.key_strength(&secret);
-        assert!(matches!(result, Err(WeakKeyError::PasswordTooShort(2))));
-
         let mut keyring = Keyring::new()?;
         keyring.salt = vec![1, 2, 3, 4]; // Less than MIN_SALT_SIZE (32)
-        let secret = Secret::from("test-password-that-is-long-enough");
-        let result = keyring.key_strength(&secret);
+        let result = keyring.key_strength();
         assert!(matches!(result, Err(WeakKeyError::SaltTooShort(4))));
 
+        assert!(Keyring::new()?.key_strength().is_ok());
+
         Ok(())
     }
 }

client/src/file/error.rs

@@ -184,8 +184,6 @@ pub enum WeakKeyError {
     IterationCountTooLow(u32),
     /// Avoid attack on existing files
     SaltTooShort(usize),
-    /// Just not secure enough to store password
-    PasswordTooShort(usize),
     /// Should not occur
     ///
     /// Used by [`dbus`](crate::dbus) module that does not currently
@@ -200,9 +198,6 @@ impl std::fmt::Display for WeakKeyError {
         match self {
             Self::IterationCountTooLow(count) => write!(f, "Iteration count too low: {count}"),
             Self::SaltTooShort(length) => write!(f, "Salt too short: {length}"),
-            Self::PasswordTooShort(length) => {
-                write!(f, "Password too short: {length}")
-            }
             Self::StrengthUnknown => write!(f, "Strength unknown"),
         }
     }

client/src/file/unlocked_keyring.rs

@@ -184,10 +184,7 @@ impl UnlockedKeyring {
         let legacy_keyring = api::LegacyKeyring::try_from(content)?;
         let mut keyring = api::Keyring::new()?;
 
-        let key = secret
-            .as_ref()
-            .map(|s| keyring.derive_key_unchecked(s))
-            .transpose()?;
+        let key = secret.as_ref().map(|s| keyring.derive_key(s)).transpose()?;
         let decrypted_items = legacy_keyring
             .decrypt_items(&secret.clone().unwrap_or_else(|| Secret::from(vec![])))?;
 

client/tests/file_unlocked_keyring.rs

@@ -320,22 +320,25 @@ async fn delete() -> Result<(), Error> {
 }
 
 #[tokio::test]
-async fn write_with_weak_key() -> Result<(), Error> {
+async fn write_with_short_password() -> Result<(), Error> {
     let temp_dir = tempdir()?;
-    let path = temp_dir.path().join("write_with_weak_key.keyring");
+    let path = temp_dir.path().join("write_with_short_password.keyring");
 
-    let secret = Secret::from(vec![1, 2]);
-    let keyring = UnlockedKeyring::load(&path, Some(secret)).await?;
+    // Password policy is up to the caller, e.g. keyrings migrated from
+    // gnome-keyring can have any password.
+    let secret = Secret::from(vec![1]);
+    let keyring = UnlockedKeyring::load(&path, Some(secret.clone())).await?;
     let attributes: HashMap<&str, &str> = HashMap::default();
 
-    let result = keyring
+    keyring
         .create_item("label", &attributes, "my-password", false)
-        .await;
+        .await?;
+    keyring.write().await?;
 
-    assert!(matches!(
-        result,
-        Err(Error::WeakKey(WeakKeyError::PasswordTooShort(2)))
-    ));
+    let keyring = UnlockedKeyring::load(&path, Some(secret)).await?;
+    let items = keyring.items().await?;
+    assert_eq!(items.len(), 1);
+    assert_eq!(items[0].secret(), Secret::text("my-password"));
 
     Ok(())
 }