diff --git a/client/src/crypto/native.rs b/client/src/crypto/native.rs index 76cfa795..872d7448 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 fcd18e46..d2d350e0 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/api/mod.rs b/client/src/file/api/mod.rs index aedace5a..165aedf3 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 1adeff4d..0d5b24d1 100644 --- a/client/src/file/error.rs +++ b/client/src/file/error.rs @@ -15,6 +15,8 @@ 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 }, /// Input/Output. Io(std::io::Error), /// Unexpected MAC digest value. @@ -114,6 +116,10 @@ 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::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 e6bfbd63..8e221f81 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,21 @@ impl LockedKeyring { Ok(keyring.validate_secret(secret)?) } + /// 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; + Ok(keyring.validate_key(key)) + } + pub async fn validate_unencrypted(&self) -> Result { let keyring = self.keyring.read().await; Ok(keyring.validate_unencrypted()) @@ -78,6 +93,27 @@ 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 validation = { + let inner_keyring = self.keyring.read().await; + inner_keyring.validate_items(&key) + }; + #[cfg(feature = "tracing")] + Self::log_validation_error(&validation, false); + validation?; + + Ok(self.into_unlocked(Some(Arc::new(key)), None)) + } + /// Unlocks a keyring without validating it /// /// # Safety @@ -100,52 +136,58 @@ impl LockedKeyring { let inner_keyring = self.keyring.read().await; let key = inner_keyring.derive_key(&secret)?; + let validation = inner_keyring.validate_items(&key); + #[cfg(feature = "tracing")] + Self::log_validation_error(&validation, true); + validation?; - 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; - } - } + Some(Arc::new(key)) + } else { + None + }; - drop(inner_keyring); + Ok(self.into_unlocked(key, Some(Arc::new(secret)))) + } - if n_valid_items == 0 && n_broken_items != 0 { - #[cfg(feature = "tracing")] + #[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."); - 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." - ); + } + 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." + ); } - return Err(Error::PartiallyCorruptedKeyring { - valid_items: n_valid_items, - broken_items: n_broken_items, - }); } + _ => {} + } + } - Some(Arc::new(key)) - } else { - None - }; - - Ok(UnlockedKeyring { + 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 +204,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 c309068d..84f8d55e 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 @@ -243,6 +249,13 @@ impl UnlockedKeyring { Self::open_with_paths(v1_path, v0_path, secret).await } + /// Open a named current-format keyring with an already-derived key. + #[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)?; + Self::load_with_key(v1_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 +302,18 @@ 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. + #[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); + Self::load_with_key(v1_path, key).await + } + /// Lock the keyring. pub fn lock(self) -> LockedKeyring { LockedKeyring { @@ -586,6 +611,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 61e03e9b..27ba9cc8 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`]. Key bytes are redacted from [`Debug`](std::fmt::Debug) +/// output. #[derive(Zeroize, ZeroizeOnDrop)] pub struct Key { key: Vec, @@ -34,6 +38,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 +58,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 197266dd..4cc467a4 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 91b2f951..b383eb70 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,280 @@ 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_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"); + 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![2; 16])).await, + Err(Error::IncorrectSecret) + )); + + Ok(()) +} + #[tokio::test] async fn repeated_write() -> Result<(), Error> { let temp_dir = tempdir()?; @@ -923,6 +1197,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) @@ -939,6 +1215,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() {