From c207407e29bb3b37a6510c43f1c43b5e1ec04e89 Mon Sep 17 00:00:00 2001 From: Daniel Brotsky Date: Sat, 31 Aug 2024 14:05:44 -0700 Subject: [PATCH 1/5] Re-enable access to secret-servce items with no `target` attribute. This fixes hwchen/keyring-rs#207. --- examples/cli.rs | 41 +-------- src/secret_service.rs | 202 +++++++++++++++++++++++------------------- 2 files changed, 112 insertions(+), 131 deletions(-) diff --git a/examples/cli.rs b/examples/cli.rs index 3a4f295..36faa1e 100644 --- a/examples/cli.rs +++ b/examples/cli.rs @@ -60,33 +60,6 @@ fn main() { } } -#[cfg(all( - any(target_os = "linux", target_os = "freebsd", target_os = "openbsd"), - any(feature = "sync-secret-service", feature = "async-secret-service") -))] -mod v1 { - use keyring::{secret_service::SsCredential, Entry, Result}; - - /// Create a v1-like entry (one with no target attribute) - pub fn new_entry(service: &str, user: &str) -> Result { - let cred = SsCredential::new_with_no_target(service, user)?; - Ok(Entry::new_with_credential(Box::new(cred))) - } -} -#[cfg(not(all( - any(target_os = "linux", target_os = "freebsd", target_os = "openbsd"), - any(feature = "sync-secret-service", feature = "async-secret-service") -)))] -mod v1 { - use keyring::Entry; - - /// For everything but the secret service, v1 entries are the same as - /// regular entries with the default target. - pub fn new_entry(service: &str, user: &str) -> keyring::Result { - Entry::new(service, user) - } -} - #[derive(Debug, Parser)] #[clap(author = "github.com/hwchen/keyring-rs")] /// Keyring CLI: A command-line interface to platform secure storage @@ -108,12 +81,6 @@ pub struct Cli { /// The user for the entry. pub user: String, - #[clap(long, action, verbatim_doc_comment)] - /// Whether to look for v1 entries (that have no target). - /// N.B.: v1 entries can only be read or deleted, not set. - /// This may also find v2/v3 entries that have a target. - pub v1: bool, - #[clap(subcommand)] pub command: Command, } @@ -152,13 +119,7 @@ impl Cli { } fn entry_for(&self) -> Result { - if self.v1 { - if self.target.is_some() { - eprintln!("usage error: You cannot specify both --target and --v1"); - std::process::exit(1) - } - v1::new_entry(&self.service, &self.user) - } else if let Some(target) = &self.target { + if let Some(target) = &self.target { Entry::new_with_target(target, &self.service, &self.user) } else { Entry::new(&self.service, &self.user) diff --git a/src/secret_service.rs b/src/secret_service.rs index 4ded57a..6b08cf6 100644 --- a/src/secret_service.rs +++ b/src/secret_service.rs @@ -28,18 +28,6 @@ This provides better compatibility with 3rd party clients that may already have created items that match the entry, and reduces the chance of ambiguity in later searches. -## keyring v1 incompatibility - -In order to fix -[this bug](https://github.com/hwchen/keyring-rs/issues/204) -efficiently, this implementation can no longer access -credentials that have no `target` attribute. Since keyring v1 -didn't set this attribute, any old credentials left from v1 -will have to be upgraded to a v3-compatible format -using platform-specific code. You can use the new secret-service-specific -entry creation call [new_with_no_target] to -create an [Entry] that will retrieve a v1 password and/or delete it. - ## Tokio runtime caution If you are using the `async-secret-service` with this crate, @@ -224,6 +212,7 @@ impl SsCredential { return Err(empty_target()); } let target = target.unwrap_or("default"); + let attributes = HashMap::from([ ("service".to_string(), service.to_string()), ("username".to_string(), user.to_string()), @@ -318,10 +307,13 @@ impl SsCredential { #[cfg(not(any(feature = "crypto-rust", feature = "crypto-openssl")))] let session_type = EncryptionType::Plain; let ss = SecretService::connect(session_type).map_err(platform_failure)?; - let attributes: HashMap<&str, &str> = self.search_attributes().into_iter().collect(); + let attributes: HashMap<&str, &str> = self.search_attributes(false).into_iter().collect(); let search = ss.search_items(attributes).map_err(decode_error)?; + let count = search.locked.len() + search.unlocked.len(); + if count == 0 && matches!(self.target.as_ref(), Some(t) if t == "default") { + return self.map_matching_legacy_items(ss, f, require_unique); + } if require_unique { - let count = search.locked.len() + search.unlocked.len(); if count == 0 { return Err(ErrorCode::NoEntry); } else if count > 1 { @@ -344,6 +336,61 @@ impl SsCredential { Ok(results) } + /// Map a function over items that older versions of keyring + /// would have matched against this credential. + /// + /// Keyring v1 created secret service items that had no target attribute, and it was + /// only able to create items in the default collection. Keyring v2, and Keyring v3.1, + /// in order to be able to find items set by keyring v1, would first look for items + /// everywhere independent of target attribute, and then filter those found by the value + /// of the target attribute. But this matching behavior overgeneralized when the keyring + /// was locked at the time of the search (see + /// [issue #204](https://github.com/hwchen/keyring-rs/issues/204) for details). + /// + /// As of keyring v3.2, the service-wide search behavior was changed to require a + /// matching target on items. But, as pointed out in + /// [issue #207](https://github.com/hwchen/keyring-rs/issues/207), + /// this meant that items set by keyring v1 (or by 3rd party tools that didn't set + /// the target attribute) would not be found, even if they were in the default + /// collection. + /// + /// So with keyring v3.2.1, if the service-wide search fails to find any matching + /// credential, and the credential being searched for has the default target (or + /// no target), we fall back and search the default collection for a v1-style credential. + /// That preserves the legacy behavior at the cost of a second round-trip through + /// the secret service for the collection search. + pub fn map_matching_legacy_items( + &self, + ss: SecretService, + f: F, + require_unique: bool, + ) -> Result> + where + F: Fn(&Item) -> Result, + T: Sized, + { + let collection = ss.get_default_collection().map_err(decode_error)?; + let attributes = self.search_attributes(true); + let search = collection.search_items(attributes).map_err(decode_error)?; + if require_unique { + if search.len() == 0 && require_unique { + return Err(ErrorCode::NoEntry); + } else if search.len() > 1 { + let mut creds: Vec> = vec![]; + for item in search.iter() { + let cred = Self::new_from_item(item)?; + creds.push(Box::new(cred)) + } + return Err(ErrorCode::Ambiguous(creds)); + } + } + let mut results: Vec = vec![]; + for item in search.iter() { + results.push(f(item)?); + } + Ok(results) + } + /// Using strings in the credential map makes managing the lifetime /// of the credential much easier. But since the secret service expects /// a map from &str to &str, we have this utility to transform the @@ -357,9 +404,9 @@ impl SsCredential { /// Similar to [all_attributes](SsCredential::all_attributes), /// but this just selects the ones we search on - fn search_attributes(&self) -> HashMap<&str, &str> { + fn search_attributes(&self, omit_target: bool) -> HashMap<&str, &str> { let mut result: HashMap<&str, &str> = HashMap::new(); - if self.target.is_some() { + if self.target.is_some() && !omit_target { result.insert("target", self.attributes["target"].as_str()); } result.insert("service", self.attributes["service"].as_str()); @@ -423,8 +470,7 @@ pub fn get_collection<'a>(ss: &'a SecretService, name: &str) -> Result(ss: &'a SecretService, name: &str) -> Result> { let collection = if name.eq("default") { ss.get_default_collection().map_err(decode_error)? @@ -578,30 +624,6 @@ mod tests { assert!(matches!(entry.get_password(), Err(Error::NoEntry))); } - fn probe_collection(name: &str) -> bool { - #[cfg(not(feature = "async-secret-service"))] - use dbus_secret_service::{EncryptionType, SecretService}; - #[cfg(feature = "async-secret-service")] - use secret_service::{blocking::SecretService, EncryptionType}; - - let ss = - SecretService::connect(EncryptionType::Plain).expect("Can't connect to secret service"); - let result = super::get_collection(&ss, name).is_ok(); - result - } - - fn delete_collection(name: &str) { - #[cfg(not(feature = "async-secret-service"))] - use dbus_secret_service::{EncryptionType, SecretService}; - #[cfg(feature = "async-secret-service")] - use secret_service::{blocking::SecretService, EncryptionType}; - - let ss = - SecretService::connect(EncryptionType::Plain).expect("Can't connect to secret service"); - let collection = super::get_collection(&ss, name).expect("Can't find collection to delete"); - collection.delete().expect("Can't delete collection"); - } - #[test] #[ignore = "can't be run headless, because it needs to prompt"] fn test_create_new_target_collection() { @@ -677,55 +699,53 @@ mod tests { } #[test] - fn test_existing_target_works_as_expected() { - let name1 = "test_keyring1"; - let name2 = "test_keyring2"; - if !probe_collection(name1) || !probe_collection(name2) { - println!("Skipping target test since needed collections don't exist or are locked"); - return; - } - let credential1 = SsCredential::new_with_target(Some(name1), name1, name1) - .expect("Can't create credential1 with new collection"); - let entry1 = Entry::new_with_credential(Box::new(credential1)); - let credential2 = SsCredential::new_with_target(Some(name2), name1, name1) - .expect("Can't create credential2 with new collection"); - let entry2 = Entry::new_with_credential(Box::new(credential2)); - let entry3 = Entry::new(name1, name1).expect("Can't create entry in default collection"); - let password1 = "password for collection 1"; - let password2 = "password for collection 2"; - let password3 = "password for default collection"; - entry1 - .set_password(password1) - .expect("Can't set password for collection 1"); - entry2 - .set_password(password2) - .expect("Can't set password for collection 2"); - entry3 - .set_password(password3) - .expect("Can't set password for default collection"); - let actual1 = entry1 - .get_password() - .expect("Can't get password for collection 1"); - assert_eq!(actual1, password1); - let actual2 = entry2 - .get_password() - .expect("Can't get password for collection 2"); - assert_eq!(actual2, password2); - let actual3 = entry3 + fn test_legacy_entry() { + let name = generate_random_string(); + let pw = "test password"; + let v3_entry = Entry::new(&name, &name).expect("Can't create v3 entry"); + let _ = v3_entry.get_password().expect_err("Found v3 entry"); + create_v1_entry(&name, pw); + let password = v3_entry.get_password().expect("Can't find v1 entry"); + assert_eq!(password, pw); + v3_entry.delete_credential().expect("Can't delete v1 entry"); + let _ = v3_entry .get_password() - .expect("Can't get password for default collection"); - assert_eq!(actual3, password3); - entry1 - .delete_credential() - .expect("Couldn't delete password for collection 1"); - assert!(matches!(entry1.get_password(), Err(Error::NoEntry))); - entry2 - .delete_credential() - .expect("Couldn't delete password for collection 2"); - assert!(matches!(entry2.get_password(), Err(Error::NoEntry))); - entry3 - .delete_credential() - .expect("Couldn't delete password for default collection"); - assert!(matches!(entry3.get_password(), Err(Error::NoEntry))); + .expect_err("Got password for v1 entry after delete"); + } + + fn delete_collection(name: &str) { + #[cfg(not(feature = "async-secret-service"))] + use dbus_secret_service::{EncryptionType, SecretService}; + #[cfg(feature = "async-secret-service")] + use secret_service::{blocking::SecretService, EncryptionType}; + + let ss = + SecretService::connect(EncryptionType::Plain).expect("Can't connect to secret service"); + let collection = super::get_collection(&ss, name).expect("Can't find collection to delete"); + collection.delete().expect("Can't delete collection"); + } + + fn create_v1_entry(name: &str, password: &str) { + #[cfg(not(feature = "async-secret-service"))] + use dbus_secret_service::{EncryptionType, SecretService}; + #[cfg(feature = "async-secret-service")] + use secret_service::{blocking::SecretService, EncryptionType}; + + let cred = SsCredential::new_with_no_target(name, name) + .expect("Can't create credential with no target"); + let ss = + SecretService::connect(EncryptionType::Plain).expect("Can't connect to secret service"); + let collection = ss + .get_default_collection() + .expect("Can't get default collection"); + collection + .create_item( + cred.label.as_str(), + cred.all_attributes(), + password.as_bytes(), + true, // replace + "text/plain", + ) + .expect("Can't create item with no target in default collection"); } } From 42f1ea3e9867c348443dc4cfbd2b4d554d113d19 Mon Sep 17 00:00:00 2001 From: Daniel Brotsky Date: Sat, 31 Aug 2024 14:27:56 -0700 Subject: [PATCH 2/5] Update the README. Also clarify the secret-service docs so that they clarify how access to v1 data works. --- README.md | 2 +- src/secret_service.rs | 27 ++++++++++++++++----------- 2 files changed, 17 insertions(+), 12 deletions(-) diff --git a/README.md b/README.md index 9827ecb..73381f4 100644 --- a/README.md +++ b/README.md @@ -91,7 +91,7 @@ The main API change between v2 and v3 is the addition of support for non-string Another API change between v2 and v3 is that the notion of a default feature set has gone away: you must now specify explicitly which crate-supported keystores you want included (other than the `mock` keystore, which is always present). So all keyring client developers will need to update their `Cargo.toml` file to use the new features correctly. -All v2 data is fully forward-compatible with v3 data; there have been no changes at all in that respect. _However_, unlike v2, the v3 implementation of the secret service credential store will _not_ read credentials that were written by the v1 keyring. (For details about why this decision was made, see [this issue](https://github.com/hwchen/keyring-rs/issues/204)). Keyring clients who use the secret service and are still using old v1 credentials should replace those credentials with v2/v3 credentials. The CLI has been extended to allow reading and deleting v1 credentials (and thus provides sample code for how to do this). +All v2 data is fully forward-compatible with v3 data; there have been no changes at all in that respect. The MSRV has been moved to 1.75, and all direct dependencies are at their latest stable versions. diff --git a/src/secret_service.rs b/src/secret_service.rs index 6b08cf6..587b40c 100644 --- a/src/secret_service.rs +++ b/src/secret_service.rs @@ -12,20 +12,25 @@ implementation uses the following attributes: - `application` (optional & always set to `rust-keyring`) Existing items are always searched for at the service level, which -means all collections are searched. Only the required attributes are used in searches, -so 3rd party clients (such as v1 of this crate) that use matching service and user fields -will have their items found by our searches. (Items we write with -the same service and user attributes but different target attributes will also come -back in searches, but they are filtered out of the results automatically.) - -New items are always created with all four attributes, and if they have -a non-default target then they are created in a collection whose label matches -the target (creating it if necessary). +means all collections are searched. The search attributes used are +`target` (set from the entry target), `service` (set from the entry +service), and `username` (set from the entry user). Because earlier +versions of this crate did not set the `target` attribute on credentials +that were stored in the default collection, a fallback search is done +for items in the default collection with no `target` attribute *if +the original search for all three attributes returns no matches*. + +New items are always created with all three search attributes, and +they are given a label that identifies the crate and version and +attributes used in the entry. If a target other than `default` is +specified for the entry, then a collection labeled with that target +will be created (if necessary) to hold the new item. Setting the password on an entry will always update the password on an existing item in preference to creating a new item. -This provides better compatibility with 3rd party clients that may already -have created items that match the entry, and reduces the chance +This provides better compatibility with 3rd party clients, as well as earlier +versions of this crate, that may already +have created items that match the entry, and thus reduces the chance of ambiguity in later searches. ## Tokio runtime caution From a1ba734574f2922d91c5113e1b74f5e45df235f8 Mon Sep 17 00:00:00 2001 From: Daniel Brotsky Date: Sat, 31 Aug 2024 15:30:42 -0700 Subject: [PATCH 3/5] Update docs. Allow rustdoc to enable both linux-native and secret-service, so both are documented on Linux. Set rustdoc features to cover all keystores. --- Cargo.toml | 3 +++ README.md | 2 +- src/lib.rs | 20 +++++++++++++++----- 3 files changed, 19 insertions(+), 6 deletions(-) diff --git a/Cargo.toml b/Cargo.toml index 09584fc..d32f1c9 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -67,3 +67,6 @@ rpassword = "7" rand = "0.8" doc-comment = "0.3" whoami = "1" + +[package.metadata.docs.rs] +features = ["apple-native", "windows-native", "sync-secret-service", "crypto-rust"] diff --git a/README.md b/README.md index 73381f4..7df94c4 100644 --- a/README.md +++ b/README.md @@ -61,7 +61,7 @@ This crate provides built-in implementations of the following platform-specific * _macOS_, _iOS_: The local keychain. * _Windows_: The Windows Credential Manager. -To enable the stores you want, you use features: there is one feature for each possibly-included credential store. If you specify a feature (e.g., `dbus-secret-service`) _and_ your target platform (e.g., `freebsd`) supports that credential store, it will be included as the default credential store in that build. That way you can have a build command that specifies a single credential store for each of your target platforms, and use that same build command for all targets. +To enable the stores you want, you use features: there is one feature for each possibly-included credential store. If you specify a feature (e.g., `dbus-secret-service`) _and_ your target platform (e.g., `freebsd`) supports that credential store, it will be included as the default credential store in that build. That way you can have a build command that specifies a single credential store for each of your target platforms, and use that same build command for all targets. (You cannot enable more than one keystore for a given platform, except when producing docs.) If you don't enable any credential stores that are supported on a specific target, the _mock_ keystore will be the default on that target. If you enable multiple credential stores for a specific target, you will get a compile error. See the [developer docs](https://docs.rs/keyring/) for details of which features control the inclusion of which credential stores (and which platforms each credential store targets). diff --git a/src/lib.rs b/src/lib.rs index feaa6ac..df9db80 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -168,10 +168,13 @@ pub mod mock; // // no duplicate keystores on any platform // -#[cfg(any( - all(feature = "linux-native", feature = "sync-secret-service"), - all(feature = "linux-native", feature = "async-secret-service"), - all(feature = "sync-secret-service", feature = "async-secret-service") +#[cfg(all( + not(doc), + any( + all(feature = "linux-native", feature = "sync-secret-service"), + all(feature = "linux-native", feature = "async-secret-service"), + all(feature = "sync-secret-service", feature = "async-secret-service") + ) ))] compile_error!("You can enable at most one keystore per target architecture"); @@ -181,7 +184,14 @@ compile_error!("You can enable at most one keystore per target architecture"); #[cfg(all(target_os = "linux", feature = "linux-native"))] pub mod keyutils; -#[cfg(all(target_os = "linux", feature = "linux-native"))] +#[cfg(all( + target_os = "linux", + feature = "linux-native", + not(all( + doc, + any(feature = "sync-secret-service", feature = "async-secret-service") + )) +))] pub use keyutils as default; #[cfg(all( From ae8396a3c0ab0c5cdb0b0ce6b9020de7db3d57c9 Mon Sep 17 00:00:00 2001 From: Daniel Brotsky Date: Sat, 31 Aug 2024 15:40:05 -0700 Subject: [PATCH 4/5] Update version to 3.2.1. --- Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Cargo.toml b/Cargo.toml index d32f1c9..662821c 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -6,7 +6,7 @@ keywords = ["password", "credential", "keychain", "keyring", "cross-platform"] license = "MIT OR Apache-2.0" name = "keyring" repository = "https://github.com/hwchen/keyring-rs.git" -version = "3.2.0" +version = "3.2.1" rust-version = "1.75" edition = "2021" exclude = [".github/"] From a583eb3edb66550c3431771e2c8362c7c84ecdff Mon Sep 17 00:00:00 2001 From: Daniel Brotsky Date: Sat, 31 Aug 2024 15:52:43 -0700 Subject: [PATCH 5/5] Fix disallowed move in async secret service. --- src/secret_service.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/secret_service.rs b/src/secret_service.rs index 587b40c..31b4153 100644 --- a/src/secret_service.rs +++ b/src/secret_service.rs @@ -316,7 +316,7 @@ impl SsCredential { let search = ss.search_items(attributes).map_err(decode_error)?; let count = search.locked.len() + search.unlocked.len(); if count == 0 && matches!(self.target.as_ref(), Some(t) if t == "default") { - return self.map_matching_legacy_items(ss, f, require_unique); + return self.map_matching_legacy_items(&ss, f, require_unique); } if require_unique { if count == 0 { @@ -366,7 +366,7 @@ impl SsCredential { /// the secret service for the collection search. pub fn map_matching_legacy_items( &self, - ss: SecretService, + ss: &SecretService, f: F, require_unique: bool, ) -> Result>