From 235d0e482153ed4b788433664fe756f98ed2e11c Mon Sep 17 00:00:00 2001 From: "Can H. Tartanoglu" Date: Mon, 24 Aug 2026 21:03:22 +0200 Subject: [PATCH 1/4] client: support already-derived file keys --- client/src/crypto/native.rs | 4 + client/src/crypto/openssl.rs | 4 + client/src/file/error.rs | 11 ++ client/src/file/locked_keyring.rs | 111 +++++++----- client/src/file/unlocked_keyring.rs | 51 ++++++ client/src/key.rs | 25 +++ client/src/lib.rs | 4 - client/tests/file_unlocked_keyring.rs | 242 +++++++++++++++++++++++++- 8 files changed, 403 insertions(+), 49 deletions(-) diff --git a/client/src/crypto/native.rs b/client/src/crypto/native.rs index 76cfa7959..872d74484 100644 --- a/client/src/crypto/native.rs +++ b/client/src/crypto/native.rs @@ -73,6 +73,10 @@ pub(crate) fn iv_len() -> usize { DecAlg::iv_size() } +pub(crate) fn key_len() -> usize { + DecAlg::key_size() +} + pub(crate) fn generate_private_key() -> Result>, super::Error> { let mut key = vec![0u8; EncAlg::key_size()]; getrandom::fill(&mut key)?; diff --git a/client/src/crypto/openssl.rs b/client/src/crypto/openssl.rs index fcd18e46f..d2d350e06 100644 --- a/client/src/crypto/openssl.rs +++ b/client/src/crypto/openssl.rs @@ -81,6 +81,10 @@ pub(crate) fn iv_len() -> usize { cipher.iv_len().unwrap() } +pub(crate) fn key_len() -> usize { + Cipher::from_nid(ENC_ALG).unwrap().key_len() +} + pub(crate) fn generate_private_key() -> Result>, super::Error> { let cipher = Cipher::from_nid(ENC_ALG).unwrap(); let mut buf = Zeroizing::new(vec![0; cipher.key_len()]); diff --git a/client/src/file/error.rs b/client/src/file/error.rs index 1adeff4d5..2dd737905 100644 --- a/client/src/file/error.rs +++ b/client/src/file/error.rs @@ -15,6 +15,10 @@ pub enum Error { SaltSizeMismatch(usize, u32), /// Key for some reason too weak to trust it for writing WeakKey(WeakKeyError), + /// A file encryption key has an unexpected length. + InvalidKeyLength { expected: usize, actual: usize }, + /// A legacy keyring cannot be migrated without its source secret. + LegacyMigrationRequiresSecret, /// Input/Output. Io(std::io::Error), /// Unexpected MAC digest value. @@ -114,6 +118,13 @@ impl std::fmt::Display for Error { "Salt size is not as expected. Array: {arr}, Explicit: {explicit}" ), Self::WeakKey(err) => write!(f, "{err}"), + Self::InvalidKeyLength { expected, actual } => write!( + f, + "Invalid file key length: expected {expected} bytes, got {actual}", + ), + Self::LegacyMigrationRequiresSecret => { + write!(f, "Migrating a legacy keyring requires its source secret") + } Self::Io(e) => write!(f, "IO error {e}"), Self::MacError => write!(f, "Mac digest is not equal to the expected value"), Self::ChecksumMismatch => write!(f, "Incorrect secret or corrupted keyring data"), diff --git a/client/src/file/locked_keyring.rs b/client/src/file/locked_keyring.rs index e6bfbd639..ba4ddf334 100644 --- a/client/src/file/locked_keyring.rs +++ b/client/src/file/locked_keyring.rs @@ -19,7 +19,7 @@ use tokio::{ }; use super::{Error, LockedItem, UnlockedKeyring, api}; -use crate::Secret; +use crate::{Key, Secret}; /// A locked keyring that requires a secret to unlock. #[derive(Debug)] @@ -44,6 +44,17 @@ impl LockedKeyring { Ok(keyring.validate_secret(secret)?) } + /// Validate that an already-derived key can decrypt this keyring. + /// + /// Empty keyrings return `true` because they contain no item with which to + /// authenticate the key. Callers that persist keys for empty keyrings must + /// bind them to the exact keyring file separately. + pub async fn validate_key(&self, key: &Key) -> Result { + key.validate_file_key()?; + let keyring = self.keyring.read().await; + Ok(keyring.items.is_empty() || keyring.items.iter().any(|item| item.is_valid(Some(key)))) + } + pub async fn validate_unencrypted(&self) -> Result { let keyring = self.keyring.read().await; Ok(keyring.validate_unencrypted()) @@ -78,6 +89,24 @@ impl LockedKeyring { self.unlock_inner(secret, true).await } + /// Unlocks a keyring with an already-derived key and validates it. + /// + /// An exact-length [`Key::new`] value is treated as direct key material and + /// may be used for subsequent writes. The caller is responsible for + /// supplying a key with sufficient entropy. + /// + /// Empty keyrings cannot authenticate the key and therefore accept any key + /// of the required length, matching [`Self::validate_key`]. + pub async fn unlock_with_key(self, key: Key) -> Result { + let key = key.into_file_key()?; + { + let inner_keyring = self.keyring.read().await; + Self::validate_items(&inner_keyring, &key)?; + } + + Ok(self.into_unlocked(Some(Arc::new(key)), None)) + } + /// Unlocks a keyring without validating it /// /// # Safety @@ -100,52 +129,52 @@ impl LockedKeyring { let inner_keyring = self.keyring.read().await; let key = inner_keyring.derive_key(&secret)?; - - let mut n_broken_items = 0; - let mut n_valid_items = 0; - for encrypted_item in &inner_keyring.items { - if encrypted_item.is_valid(Some(&key)) { - n_valid_items += 1; - } else { - n_broken_items += 1; - } - } - - drop(inner_keyring); - - if n_valid_items == 0 && n_broken_items != 0 { - #[cfg(feature = "tracing")] - tracing::error!("Keyring cannot be decrypted. Invalid secret."); - return Err(Error::IncorrectSecret); - } else if n_broken_items > n_valid_items { - #[cfg(feature = "tracing")] - { - tracing::warn!( - "The file contains {n_broken_items} broken items and {n_valid_items} valid ones." - ); - tracing::info!( - "Please switch to `UnlockedKeyring::load_unchecked` to load the keyring without the secret validation. - `Keyring::delete_broken_items` can be used to remove them or alternatively with `oo7-cli --repair`." - ); - } - return Err(Error::PartiallyCorruptedKeyring { - valid_items: n_valid_items, - broken_items: n_broken_items, - }); - } + Self::validate_items(&inner_keyring, &key)?; Some(Arc::new(key)) } else { None }; - Ok(UnlockedKeyring { + Ok(self.into_unlocked(key, Some(Arc::new(secret)))) + } + + fn validate_items(keyring: &api::Keyring, key: &Key) -> Result<(), Error> { + let (n_valid_items, n_broken_items) = + keyring.items.iter().fold((0, 0), |(valid, broken), item| { + if item.is_valid(Some(key)) { + (valid + 1, broken) + } else { + (valid, broken + 1) + } + }); + + if n_valid_items == 0 && n_broken_items != 0 { + #[cfg(feature = "tracing")] + tracing::error!("Keyring cannot be decrypted. Invalid key material."); + Err(Error::IncorrectSecret) + } else if n_broken_items > n_valid_items { + #[cfg(feature = "tracing")] + tracing::warn!( + "The file contains {n_broken_items} broken items and {n_valid_items} valid ones." + ); + Err(Error::PartiallyCorruptedKeyring { + valid_items: n_valid_items, + broken_items: n_broken_items, + }) + } else { + Ok(()) + } + } + + fn into_unlocked(self, key: Option>, secret: Option>) -> UnlockedKeyring { + UnlockedKeyring { keyring: self.keyring, path: self.path, mtime: self.mtime, key: Mutex::new(key), - secret: Mutex::new(Some(Arc::new(secret))), - }) + secret: Mutex::new(secret), + } } /// Unlocks a keyring without a secret, for unencrypted keyrings. @@ -162,13 +191,7 @@ impl LockedKeyring { } drop(inner_keyring); - Ok(UnlockedKeyring { - keyring: self.keyring, - path: self.path, - mtime: self.mtime, - key: Mutex::new(None), - secret: Mutex::new(None), - }) + Ok(self.into_unlocked(None, None)) } /// Load a keyring from a file path. diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index c309068dc..0f467134f 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/client/src/file/unlocked_keyring.rs @@ -50,6 +50,12 @@ impl UnlockedKeyring { Self::load_inner(path, secret, true).await } + /// Load and unlock a keyring with an already-derived key. + #[cfg_attr(feature = "tracing", tracing::instrument(skip(key), fields(path = ?path.as_ref())))] + pub async fn load_with_key(path: impl AsRef, key: Key) -> Result { + LockedKeyring::load(path).await?.unlock_with_key(key).await + } + /// Load from a keyring file without validating the secret. /// /// # Arguments @@ -227,6 +233,17 @@ impl UnlockedKeyring { } } + async fn open_with_key_paths( + v1_path: PathBuf, + v0_path: PathBuf, + key: Key, + ) -> Result { + if !v1_path.exists() && v0_path.exists() { + return Err(Error::LegacyMigrationRequiresSecret); + } + Self::load_with_key(v1_path, key).await + } + /// Open a keyring with given name from the default directory. /// /// This function will automatically migrate the keyring to the @@ -243,6 +260,18 @@ impl UnlockedKeyring { Self::open_with_paths(v1_path, v0_path, secret).await } + /// Open a named current-format keyring with an already-derived key. + /// + /// Unlike [`Self::open`], this cannot migrate a legacy keyring because the + /// source secret required by the legacy key derivation is unavailable. It + /// returns [`Error::LegacyMigrationRequiresSecret`] instead. + #[cfg_attr(feature = "tracing", tracing::instrument(skip(key)))] + pub async fn open_with_key(name: &str, key: Key) -> Result { + let v1_path = api::Keyring::path(name, api::MAJOR_VERSION)?; + let v0_path = api::Keyring::path(name, api::LEGACY_MAJOR_VERSION)?; + Self::open_with_key_paths(v1_path, v0_path, key).await + } + /// Open or create a keyring at a specific data directory. /// /// This is useful for tests and cases where you want explicit control over @@ -289,6 +318,21 @@ impl UnlockedKeyring { Self::open_with_paths(v1_path, v0_path, secret).await } + /// Open a named current-format keyring at a specific data directory with + /// an already-derived key. + /// + /// This does not migrate legacy keyrings; see [`Self::open_with_key`]. + #[cfg_attr(feature = "tracing", tracing::instrument(skip(key), fields(data_dir = ?data_dir.as_ref())))] + pub async fn open_at_with_key( + data_dir: impl AsRef, + name: &str, + key: Key, + ) -> Result { + let v1_path = api::Keyring::path_at(&data_dir, name, api::MAJOR_VERSION); + let v0_path = api::Keyring::path_at(&data_dir, name, api::LEGACY_MAJOR_VERSION); + Self::open_with_key_paths(v1_path, v0_path, key).await + } + /// Lock the keyring. pub fn lock(self) -> LockedKeyring { LockedKeyring { @@ -586,6 +630,13 @@ impl UnlockedKeyring { /// Returns `None` when no secret is set (unencrypted keyring). #[cfg_attr(feature = "tracing", tracing::instrument(skip(self)))] async fn derive_key(&self) -> Result>, crate::crypto::Error> { + { + let key_lock = self.key.lock().await; + if key_lock.is_some() { + return Ok(key_lock.clone()); + } + } + let keyring = Arc::clone(&self.keyring); let secret_lock = self.secret.lock().await; let secret = match secret_lock.as_ref() { diff --git a/client/src/key.rs b/client/src/key.rs index 61e03e9b6..5ec770a32 100644 --- a/client/src/key.rs +++ b/client/src/key.rs @@ -34,6 +34,11 @@ impl AsMut<[u8]> for Key { } impl Key { + /// Construct a key from bytes. + /// + /// The key's source strength is unknown. File-keyring APIs accept an + /// exact-length value as direct key material, so callers are responsible + /// for supplying sufficient entropy. pub const fn new(key: Vec) -> Self { Self::new_with_strength(key, Err(file::WeakKeyError::StrengthUnknown)) } @@ -49,6 +54,26 @@ impl Key { Self { key, strength } } + pub(crate) fn validate_file_key(&self) -> Result<(), file::Error> { + let expected = crypto::key_len(); + if self.key.len() == expected { + Ok(()) + } else { + Err(file::Error::InvalidKeyLength { + expected, + actual: self.key.len(), + }) + } + } + + pub(crate) fn into_file_key(mut self) -> Result { + self.validate_file_key()?; + if matches!(self.strength, Err(file::WeakKeyError::StrengthUnknown)) { + self.strength = Ok(()); + } + Ok(self) + } + pub fn generate_private_key() -> Result { Ok(Self::new(crypto::generate_private_key()?.to_vec())) } diff --git a/client/src/lib.rs b/client/src/lib.rs index 197266dd4..4cc467a4b 100644 --- a/client/src/lib.rs +++ b/client/src/lib.rs @@ -22,11 +22,7 @@ mod key; mod mac; mod migration; -#[cfg(feature = "unstable")] -#[cfg_attr(docsrs, doc(cfg(feature = "unstable")))] pub use key::Key; -#[cfg(not(feature = "unstable"))] -pub(crate) use key::Key; pub use mac::Mac; #[cfg(not(feature = "unstable"))] diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 91b2f951a..1ab474d73 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -2,7 +2,7 @@ use std::{collections::HashMap, path::PathBuf, sync::Arc}; #[cfg(feature = "async-std")] use async_std::fs; -use oo7::{Secret, XDG_SCHEMA_ATTRIBUTE, file::*}; +use oo7::{Key, Secret, XDG_SCHEMA_ATTRIBUTE, file::*}; use tempfile::tempdir; #[cfg(feature = "tokio")] use tokio::fs; @@ -11,6 +11,246 @@ fn strong_key() -> Secret { Secret::from([1, 2].into_iter().cycle().take(64).collect::>()) } +#[tokio::test] +async fn validate_and_unlock_with_key() -> Result<(), Error> { + let temp_dir = tempdir()?; + let path = temp_dir.path().join("key-unlock.keyring"); + let keyring = UnlockedKeyring::load(&path, Some(strong_key())).await?; + keyring + .create_item("Item", &[("account", "alice")], "secret", false) + .await?; + let key = keyring.key().await?.unwrap(); + let key = key.as_ref().as_ref().to_vec(); + drop(keyring); + + let locked = LockedKeyring::load(&path).await?; + assert!(!locked.validate_key(&Key::new(vec![9; 16])).await?); + assert!(locked.validate_key(&Key::new(key.clone())).await?); + + let locked_with_wrong_key = LockedKeyring::load(&path).await?; + assert!(matches!( + locked_with_wrong_key + .unlock_with_key(Key::new(vec![9; 16])) + .await, + Err(Error::IncorrectSecret) + )); + + let keyring = locked.unlock_with_key(Key::new(key.clone())).await?; + let cached_key = keyring.key().await?.unwrap(); + assert_eq!(cached_key.as_ref().as_ref(), key); + let item = keyring.lookup_item(&[("account", "alice")]).await?.unwrap(); + assert_eq!(item.secret(), Secret::text("secret")); + + Ok(()) +} + +#[tokio::test] +async fn malformed_file_keys_are_rejected() -> Result<(), Error> { + let temp_dir = tempdir()?; + let path = temp_dir.path().join("invalid-key-length.keyring"); + let keyring = UnlockedKeyring::load(&path, Some(strong_key())).await?; + keyring + .create_item("Item", &[("account", "alice")], "secret", false) + .await?; + drop(keyring); + + for actual in [0, 15, 17, 32] { + let locked = LockedKeyring::load(&path).await?; + assert!(matches!( + locked.validate_key(&Key::new(vec![0; actual])).await, + Err(Error::InvalidKeyLength { + expected: 16, + actual: error_actual, + }) if error_actual == actual + )); + + let locked = LockedKeyring::load(&path).await?; + assert!(matches!( + locked.unlock_with_key(Key::new(vec![0; actual])).await, + Err(Error::InvalidKeyLength { + expected: 16, + actual: error_actual, + }) if error_actual == actual + )); + } + + Ok(()) +} + +#[tokio::test] +async fn key_only_lifecycle_and_password_change() -> Result<(), Error> { + let temp_dir = tempdir()?; + let path = temp_dir.path().join("key-lifecycle.keyring"); + let original_secret = strong_key(); + let keyring = UnlockedKeyring::load(&path, Some(original_secret.clone())).await?; + keyring + .create_item("Original", &[("id", "original")], "before", false) + .await?; + let key = keyring.key().await?.unwrap(); + let key = key.as_ref().as_ref().to_vec(); + drop(keyring); + + let keyring = UnlockedKeyring::load_with_key(&path, Key::new(key.clone())).await?; + let cached_key = keyring.key().await?.unwrap(); + assert_eq!(cached_key.as_ref().as_ref(), key); + let mut original = keyring.lookup_item(&[("id", "original")]).await?.unwrap(); + original.set_secret("after"); + keyring.replace_item_index(0, &original).await?; + keyring + .create_item("Added", &[("id", "added")], "created", false) + .await?; + drop(keyring); + + let locked = LockedKeyring::load(&path).await?; + assert!(locked.validate_key(&Key::new(key.clone())).await?); + let keyring = UnlockedKeyring::load_with_key(&path, Key::new(key)).await?; + assert_eq!( + keyring + .lookup_item(&[("id", "original")]) + .await? + .unwrap() + .secret(), + Secret::text("after") + ); + assert_eq!( + keyring + .lookup_item(&[("id", "added")]) + .await? + .unwrap() + .secret(), + Secret::text("created") + ); + + let replacement_secret = Secret::blob(vec![3; 64]); + keyring.change_secret(replacement_secret.clone()).await?; + drop(keyring); + + assert!( + UnlockedKeyring::load(&path, Some(original_secret)) + .await + .is_err() + ); + let keyring = UnlockedKeyring::load(&path, Some(replacement_secret)).await?; + assert_eq!(keyring.n_items().await, 2); + + Ok(()) +} + +#[tokio::test] +async fn empty_keyring_cannot_authenticate_key() -> Result<(), Error> { + let temp_dir = tempdir()?; + let path = temp_dir.path().join("empty-key.keyring"); + let keyring = UnlockedKeyring::load(&path, Some(strong_key())).await?; + assert!(keyring.key().await?.is_some()); + keyring.write().await?; + drop(keyring); + + let locked = LockedKeyring::load(&path).await?; + assert!(locked.validate_key(&Key::new(vec![9; 16])).await?); + let keyring = locked.unlock_with_key(Key::new(vec![9; 16])).await?; + keyring + .create_item("Rebound", &[("id", "rebound")], "secret", false) + .await?; + drop(keyring); + + assert!( + UnlockedKeyring::load(&path, Some(strong_key())) + .await + .is_err() + ); + let keyring = UnlockedKeyring::load_with_key(&path, Key::new(vec![9; 16])).await?; + assert_eq!(keyring.n_items().await, 1); + + Ok(()) +} + +#[tokio::test] +async fn unencrypted_keyring_requires_no_key() -> Result<(), Error> { + let temp_dir = tempdir()?; + let path = temp_dir.path().join("unencrypted-key.keyring"); + let keyring = UnlockedKeyring::load(&path, None).await?; + keyring + .create_item("Plain", &[("id", "plain")], "secret", false) + .await?; + drop(keyring); + + let locked = LockedKeyring::load(&path).await?; + assert!(!locked.validate_key(&Key::new(vec![1; 16])).await?); + assert!(matches!( + locked.unlock_with_key(Key::new(vec![1; 16])).await, + Err(Error::IncorrectSecret) + )); + + let keyring = LockedKeyring::load(&path) + .await? + .unlock_unencrypted() + .await?; + assert!(keyring.key().await?.is_none()); + assert_eq!(keyring.n_items().await, 1); + + Ok(()) +} + +#[tokio::test] +async fn open_at_with_key_uses_named_current_keyring() -> Result<(), Error> { + let data_dir = tempdir()?; + let keyring = UnlockedKeyring::open_at(data_dir.path(), "named", Some(strong_key())).await?; + keyring + .create_item("Named", &[("id", "named")], "secret", false) + .await?; + let key = keyring.key().await?.unwrap(); + let key = key.as_ref().as_ref().to_vec(); + drop(keyring); + + let keyring = + UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(key)).await?; + assert_eq!( + keyring + .lookup_item(&[("id", "named")]) + .await? + .unwrap() + .secret(), + Secret::text("secret") + ); + + let direct_key = vec![4; 16]; + let direct = UnlockedKeyring::open_at_with_key( + data_dir.path(), + "new-with-key", + Key::new(direct_key.clone()), + ) + .await?; + direct + .create_item("Direct", &[("id", "direct")], "secret", false) + .await?; + drop(direct); + let direct = + UnlockedKeyring::open_at_with_key(data_dir.path(), "new-with-key", Key::new(direct_key)) + .await?; + assert_eq!(direct.n_items().await, 1); + + Ok(()) +} + +#[tokio::test] +async fn open_at_with_key_does_not_shadow_legacy_keyring() -> Result<(), Error> { + let data_dir = tempdir()?; + let keyrings_dir = data_dir.path().join("keyrings"); + fs::create_dir_all(&keyrings_dir).await?; + let fixture = PathBuf::from(env!("CARGO_MANIFEST_DIR")) + .join("fixtures") + .join("default.keyring"); + fs::copy(fixture, keyrings_dir.join("named.keyring")).await?; + + assert!(matches!( + UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(vec![1; 16])).await, + Err(Error::LegacyMigrationRequiresSecret) + )); + assert!(!keyrings_dir.join("v1").join("named.keyring").exists()); + + Ok(()) +} + #[tokio::test] async fn repeated_write() -> Result<(), Error> { let temp_dir = tempdir()?; From 0b8b730b3c731043209a48a3dfb015e98310180c Mon Sep 17 00:00:00 2001 From: "Can H. Tartanoglu" Date: Tue, 25 Aug 2026 01:49:23 +0200 Subject: [PATCH 2/4] client: clarify derived-key validation --- client/src/file/locked_keyring.rs | 57 +++++++++++++++++++++------ client/tests/file_unlocked_keyring.rs | 12 ++++++ 2 files changed, 58 insertions(+), 11 deletions(-) diff --git a/client/src/file/locked_keyring.rs b/client/src/file/locked_keyring.rs index ba4ddf334..2fbfaa554 100644 --- a/client/src/file/locked_keyring.rs +++ b/client/src/file/locked_keyring.rs @@ -44,11 +44,15 @@ impl LockedKeyring { Ok(keyring.validate_secret(secret)?) } - /// Validate that an already-derived key can decrypt this keyring. + /// Validate that an already-derived key can decrypt at least one item in + /// this keyring. /// /// Empty keyrings return `true` because they contain no item with which to /// authenticate the key. Callers that persist keys for empty keyrings must /// bind them to the exact keyring file separately. + /// + /// A partially corrupted keyring may return `true` here but still fail + /// [`Self::unlock_with_key`] when broken items outnumber valid items. pub async fn validate_key(&self, key: &Key) -> Result { key.validate_file_key()?; let keyring = self.keyring.read().await; @@ -99,10 +103,13 @@ impl LockedKeyring { /// of the required length, matching [`Self::validate_key`]. pub async fn unlock_with_key(self, key: Key) -> Result { let key = key.into_file_key()?; - { + let validation = { let inner_keyring = self.keyring.read().await; - Self::validate_items(&inner_keyring, &key)?; - } + Self::validate_items(&inner_keyring, &key) + }; + #[cfg(feature = "tracing")] + Self::log_validation_error(&validation, false); + validation?; Ok(self.into_unlocked(Some(Arc::new(key)), None)) } @@ -129,7 +136,10 @@ impl LockedKeyring { let inner_keyring = self.keyring.read().await; let key = inner_keyring.derive_key(&secret)?; - Self::validate_items(&inner_keyring, &key)?; + let validation = Self::validate_items(&inner_keyring, &key); + #[cfg(feature = "tracing")] + Self::log_validation_error(&validation, true); + validation?; Some(Arc::new(key)) } else { @@ -150,14 +160,8 @@ impl LockedKeyring { }); if n_valid_items == 0 && n_broken_items != 0 { - #[cfg(feature = "tracing")] - tracing::error!("Keyring cannot be decrypted. Invalid key material."); Err(Error::IncorrectSecret) } else if n_broken_items > n_valid_items { - #[cfg(feature = "tracing")] - tracing::warn!( - "The file contains {n_broken_items} broken items and {n_valid_items} valid ones." - ); Err(Error::PartiallyCorruptedKeyring { valid_items: n_valid_items, broken_items: n_broken_items, @@ -167,6 +171,37 @@ impl LockedKeyring { } } + #[cfg(feature = "tracing")] + fn log_validation_error(validation: &Result<(), Error>, source_secret: bool) { + match validation { + Err(Error::IncorrectSecret) if source_secret => { + tracing::error!("Keyring cannot be decrypted. Invalid secret."); + } + Err(Error::IncorrectSecret) => { + tracing::error!("Keyring cannot be decrypted. Invalid key material."); + } + Err(Error::PartiallyCorruptedKeyring { + valid_items, + broken_items, + }) => { + tracing::warn!( + "The file contains {broken_items} broken items and {valid_items} valid ones." + ); + if source_secret { + tracing::info!( + "Please switch to `UnlockedKeyring::load_unchecked` to load the keyring without the secret validation. + `Keyring::delete_broken_items` can be used to remove them or alternatively with `oo7-cli --repair`." + ); + } else { + tracing::info!( + "Recover the keyring with its source secret; key-based unlock does not bypass validation." + ); + } + } + _ => {} + } + } + fn into_unlocked(self, key: Option>, secret: Option>) -> UnlockedKeyring { UnlockedKeyring { keyring: self.keyring, diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 1ab474d73..691c1aa74 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -1163,6 +1163,8 @@ async fn partially_corrupted_keyring_error() -> Result<(), Error> { keyring .create_item("valid2", &[("attr", "value2")], "password2", false) .await?; + let key = keyring.key().await?.unwrap(); + let key = key.as_ref().as_ref().to_vec(); drop(keyring); // Load_unchecked with wrong password and add 3 broken items (more than valid) @@ -1179,6 +1181,16 @@ async fn partially_corrupted_keyring_error() -> Result<(), Error> { .await?; drop(keyring); + let locked = LockedKeyring::load(&keyring_path).await?; + assert!(locked.validate_key(&Key::new(key.clone())).await?); + assert!(matches!( + locked.unlock_with_key(Key::new(key)).await, + Err(Error::PartiallyCorruptedKeyring { + valid_items: 2, + broken_items: 3, + }) + )); + let result = UnlockedKeyring::load(&keyring_path, Some(correct_secret)).await; assert!(result.is_err()); match result.unwrap_err() { From 893f3f38942d1e4f7114ca3e6854d11f9050e7ea Mon Sep 17 00:00:00 2001 From: "Can H. Tartanoglu" Date: Tue, 25 Aug 2026 08:45:10 +0200 Subject: [PATCH 3/4] client: document public key surface --- client/src/key.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/client/src/key.rs b/client/src/key.rs index 5ec770a32..437e81b19 100644 --- a/client/src/key.rs +++ b/client/src/key.rs @@ -3,7 +3,11 @@ use zeroize::{Zeroize, ZeroizeOnDrop}; use crate::{crypto, file}; -/// A key. +/// Cryptographic key material. +/// +/// File-keyring APIs accept already-derived values constructed with +/// [`Self::new`]. The same type also exposes the low-level key-exchange helpers +/// and zvariant conversions used by the D-Bus backend. #[derive(Zeroize, ZeroizeOnDrop)] pub struct Key { key: Vec, From fa4b0dcee17f9f9b6013dc0f086cb1f461a0b628 Mon Sep 17 00:00:00 2001 From: "Can H. Tartanoglu" Date: Tue, 25 Aug 2026 11:25:31 +0200 Subject: [PATCH 4/4] client: address file-key review feedback --- client/src/file/api/mod.rs | 26 ++++++++++++++++ client/src/file/error.rs | 5 --- client/src/file/locked_keyring.rs | 28 ++--------------- client/src/file/unlocked_keyring.rs | 23 ++------------ client/src/key.rs | 4 +-- client/tests/file_unlocked_keyring.rs | 44 ++++++++++++++++++++++++--- 6 files changed, 72 insertions(+), 58 deletions(-) diff --git a/client/src/file/api/mod.rs b/client/src/file/api/mod.rs index aedace5a5..165aedf39 100644 --- a/client/src/file/api/mod.rs +++ b/client/src/file/api/mod.rs @@ -333,6 +333,32 @@ impl Keyring { Ok(self.items.iter().any(|item| item.is_valid(Some(&key)))) } + pub(super) fn validate_key(&self, key: &Key) -> bool { + self.items.is_empty() || self.items.iter().any(|item| item.is_valid(Some(key))) + } + + pub(super) fn validate_items(&self, key: &Key) -> Result<(), Error> { + let (valid_items, broken_items) = + self.items.iter().fold((0, 0), |(valid, broken), item| { + if item.is_valid(Some(key)) { + (valid + 1, broken) + } else { + (valid, broken + 1) + } + }); + + if valid_items == 0 && broken_items != 0 { + Err(Error::IncorrectSecret) + } else if broken_items > valid_items { + Err(Error::PartiallyCorruptedKeyring { + valid_items, + broken_items, + }) + } else { + Ok(()) + } + } + pub fn validate_unencrypted(&self) -> bool { self.items.iter().all(|item| item.is_valid(None)) } diff --git a/client/src/file/error.rs b/client/src/file/error.rs index 2dd737905..0d5b24d1d 100644 --- a/client/src/file/error.rs +++ b/client/src/file/error.rs @@ -17,8 +17,6 @@ pub enum Error { WeakKey(WeakKeyError), /// A file encryption key has an unexpected length. InvalidKeyLength { expected: usize, actual: usize }, - /// A legacy keyring cannot be migrated without its source secret. - LegacyMigrationRequiresSecret, /// Input/Output. Io(std::io::Error), /// Unexpected MAC digest value. @@ -122,9 +120,6 @@ impl std::fmt::Display for Error { f, "Invalid file key length: expected {expected} bytes, got {actual}", ), - Self::LegacyMigrationRequiresSecret => { - write!(f, "Migrating a legacy keyring requires its source secret") - } Self::Io(e) => write!(f, "IO error {e}"), Self::MacError => write!(f, "Mac digest is not equal to the expected value"), Self::ChecksumMismatch => write!(f, "Incorrect secret or corrupted keyring data"), diff --git a/client/src/file/locked_keyring.rs b/client/src/file/locked_keyring.rs index 2fbfaa554..8e221f81c 100644 --- a/client/src/file/locked_keyring.rs +++ b/client/src/file/locked_keyring.rs @@ -56,7 +56,7 @@ impl LockedKeyring { pub async fn validate_key(&self, key: &Key) -> Result { key.validate_file_key()?; let keyring = self.keyring.read().await; - Ok(keyring.items.is_empty() || keyring.items.iter().any(|item| item.is_valid(Some(key)))) + Ok(keyring.validate_key(key)) } pub async fn validate_unencrypted(&self) -> Result { @@ -105,7 +105,7 @@ impl LockedKeyring { let key = key.into_file_key()?; let validation = { let inner_keyring = self.keyring.read().await; - Self::validate_items(&inner_keyring, &key) + inner_keyring.validate_items(&key) }; #[cfg(feature = "tracing")] Self::log_validation_error(&validation, false); @@ -136,7 +136,7 @@ impl LockedKeyring { let inner_keyring = self.keyring.read().await; let key = inner_keyring.derive_key(&secret)?; - let validation = Self::validate_items(&inner_keyring, &key); + let validation = inner_keyring.validate_items(&key); #[cfg(feature = "tracing")] Self::log_validation_error(&validation, true); validation?; @@ -149,28 +149,6 @@ impl LockedKeyring { Ok(self.into_unlocked(key, Some(Arc::new(secret)))) } - fn validate_items(keyring: &api::Keyring, key: &Key) -> Result<(), Error> { - let (n_valid_items, n_broken_items) = - keyring.items.iter().fold((0, 0), |(valid, broken), item| { - if item.is_valid(Some(key)) { - (valid + 1, broken) - } else { - (valid, broken + 1) - } - }); - - if n_valid_items == 0 && n_broken_items != 0 { - Err(Error::IncorrectSecret) - } else if n_broken_items > n_valid_items { - Err(Error::PartiallyCorruptedKeyring { - valid_items: n_valid_items, - broken_items: n_broken_items, - }) - } else { - Ok(()) - } - } - #[cfg(feature = "tracing")] fn log_validation_error(validation: &Result<(), Error>, source_secret: bool) { match validation { diff --git a/client/src/file/unlocked_keyring.rs b/client/src/file/unlocked_keyring.rs index 0f467134f..84f8d55e6 100644 --- a/client/src/file/unlocked_keyring.rs +++ b/client/src/file/unlocked_keyring.rs @@ -233,17 +233,6 @@ impl UnlockedKeyring { } } - async fn open_with_key_paths( - v1_path: PathBuf, - v0_path: PathBuf, - key: Key, - ) -> Result { - if !v1_path.exists() && v0_path.exists() { - return Err(Error::LegacyMigrationRequiresSecret); - } - Self::load_with_key(v1_path, key).await - } - /// Open a keyring with given name from the default directory. /// /// This function will automatically migrate the keyring to the @@ -261,15 +250,10 @@ impl UnlockedKeyring { } /// Open a named current-format keyring with an already-derived key. - /// - /// Unlike [`Self::open`], this cannot migrate a legacy keyring because the - /// source secret required by the legacy key derivation is unavailable. It - /// returns [`Error::LegacyMigrationRequiresSecret`] instead. #[cfg_attr(feature = "tracing", tracing::instrument(skip(key)))] pub async fn open_with_key(name: &str, key: Key) -> Result { let v1_path = api::Keyring::path(name, api::MAJOR_VERSION)?; - let v0_path = api::Keyring::path(name, api::LEGACY_MAJOR_VERSION)?; - Self::open_with_key_paths(v1_path, v0_path, key).await + Self::load_with_key(v1_path, key).await } /// Open or create a keyring at a specific data directory. @@ -320,8 +304,6 @@ impl UnlockedKeyring { /// Open a named current-format keyring at a specific data directory with /// an already-derived key. - /// - /// This does not migrate legacy keyrings; see [`Self::open_with_key`]. #[cfg_attr(feature = "tracing", tracing::instrument(skip(key), fields(data_dir = ?data_dir.as_ref())))] pub async fn open_at_with_key( data_dir: impl AsRef, @@ -329,8 +311,7 @@ impl UnlockedKeyring { key: Key, ) -> Result { let v1_path = api::Keyring::path_at(&data_dir, name, api::MAJOR_VERSION); - let v0_path = api::Keyring::path_at(&data_dir, name, api::LEGACY_MAJOR_VERSION); - Self::open_with_key_paths(v1_path, v0_path, key).await + Self::load_with_key(v1_path, key).await } /// Lock the keyring. diff --git a/client/src/key.rs b/client/src/key.rs index 437e81b19..27ba9cc87 100644 --- a/client/src/key.rs +++ b/client/src/key.rs @@ -6,8 +6,8 @@ use crate::{crypto, file}; /// Cryptographic key material. /// /// File-keyring APIs accept already-derived values constructed with -/// [`Self::new`]. The same type also exposes the low-level key-exchange helpers -/// and zvariant conversions used by the D-Bus backend. +/// [`Self::new`]. Key bytes are redacted from [`Debug`](std::fmt::Debug) +/// output. #[derive(Zeroize, ZeroizeOnDrop)] pub struct Key { key: Vec, diff --git a/client/tests/file_unlocked_keyring.rs b/client/tests/file_unlocked_keyring.rs index 691c1aa74..b383eb70e 100644 --- a/client/tests/file_unlocked_keyring.rs +++ b/client/tests/file_unlocked_keyring.rs @@ -233,20 +233,54 @@ async fn open_at_with_key_uses_named_current_keyring() -> Result<(), Error> { } #[tokio::test] -async fn open_at_with_key_does_not_shadow_legacy_keyring() -> Result<(), Error> { +async fn open_at_with_key_ignores_legacy_keyring() -> Result<(), Error> { let data_dir = tempdir()?; let keyrings_dir = data_dir.path().join("keyrings"); fs::create_dir_all(&keyrings_dir).await?; let fixture = PathBuf::from(env!("CARGO_MANIFEST_DIR")) .join("fixtures") .join("default.keyring"); - fs::copy(fixture, keyrings_dir.join("named.keyring")).await?; + let v0_path = keyrings_dir.join("named.keyring"); + let v1_path = keyrings_dir.join("v1").join("named.keyring"); + fs::copy(fixture, &v0_path).await?; + let v0_bytes = fs::read(&v0_path).await?; + let v0_metadata = fs::metadata(&v0_path).await?; + assert!(!v1_path.exists()); + + let key = vec![1; 16]; + let keyring = + UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(key.clone())).await?; + assert_eq!(keyring.n_items().await, 0); + assert!(!v1_path.exists()); + assert_eq!(fs::read(&v0_path).await?, v0_bytes); + assert_eq!(fs::metadata(&v0_path).await?.len(), v0_metadata.len()); + assert_eq!( + fs::metadata(&v0_path).await?.modified()?, + v0_metadata.modified()? + ); + + keyring + .create_item("Direct", &[("id", "direct")], "secret", false) + .await?; + drop(keyring); + assert!(v1_path.exists()); + assert_eq!(fs::read(&v0_path).await?, v0_bytes); + let keyring = + UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(key)).await?; + assert_eq!( + keyring + .lookup_item(&[("id", "direct")]) + .await? + .unwrap() + .secret(), + Secret::text("secret") + ); + drop(keyring); assert!(matches!( - UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(vec![1; 16])).await, - Err(Error::LegacyMigrationRequiresSecret) + UnlockedKeyring::open_at_with_key(data_dir.path(), "named", Key::new(vec![2; 16])).await, + Err(Error::IncorrectSecret) )); - assert!(!keyrings_dir.join("v1").join("named.keyring").exists()); Ok(()) }