Merge pull request #205 from brotskydotcom/issue-204

Always include the target when searching for credentials.

Author: brotskydotcom

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.1.0"
+version = "3.2.0"
 rust-version = "1.75"
 edition = "2021"
 exclude = [".github/"]

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.
+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).
 
 The MSRV has been moved to 1.75, and all direct dependencies are at their latest stable versions.
 

examples/cli.rs

@@ -60,18 +60,44 @@ 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<Entry> {
+        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> {
+        Entry::new(service, user)
+    }
+}
+
 #[derive(Debug, Parser)]
 #[clap(author = "github.com/hwchen/keyring-rs")]
 /// Keyring CLI: A command-line interface to platform secure storage
 pub struct Cli {
-    #[clap(short, long, action)]
-    /// Write debugging info to stderr, including retrieved passwords
-    /// and secrets. If an operation fails, error information is provided.
+    #[clap(short, long, action, verbatim_doc_comment)]
+    /// Write debugging info to stderr, including retrieved passwords and secrets.
+    /// If an operation fails, detailed error information is provided.
     pub verbose: bool,
 
     #[clap(short, long, value_parser)]
-    /// The (optional) target for the entry.  If none is provided,
-    /// the entry will be created from the service and user only.
+    /// The (optional) target for the entry.
     pub target: Option<String>,
 
     #[clap(short, long, value_parser, default_value = "keyring-cli")]
@@ -82,6 +108,12 @@ 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,
 }
@@ -120,7 +152,13 @@ impl Cli {
     }
 
     fn entry_for(&self) -> Result<Entry> {
-        if let Some(target) = &self.target {
+        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 {
             Entry::new_with_target(target, &self.service, &self.user)
         } else {
             Entry::new(&self.service, &self.user)

src/secret_service.rs

@@ -28,19 +28,26 @@ 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.
 
-## Async runtime required
-
-While this crate uses the secret-service via its blocking API,
-the secret-service crate is built on zbus which always talks to the dbus via async calls.
-Thus, using the secret-service implies using an async runtime under the covers.
-If you are already using an async runtime,
-you can use keyring features to make sure that secret-service
-uses a compatible runtime. But be careful: if you make keyring calls
-on the main thread in this situation, you will likely crash because
-you will block the main thread (see\
+## 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,
+and specifying `tokio` as your runtime, be careful:
+if you make keyring calls on the main thread, you will likely deadlock (see\
 [this issue on GitHub](https://github.com/hwchen/keyring-rs/issues/132)
-for details).  You will need to spawn a separate thread on which
-you make your keyring calls so the main thread doesn't block.
+for details).  You need to spawn a separate thread on which
+you make your keyring calls to avoid this.
 
 ## Headless usage
 
@@ -233,6 +240,26 @@ impl SsCredential {
         })
     }
 
+    /// Create a credential that has *no* target and the given service and user.
+    ///
+    /// This emulates what keyring v1 did, and can be very handy when you need to
+    /// access an old v1 credential that's in your secret service default collection.
+    pub fn new_with_no_target(service: &str, user: &str) -> Result<Self> {
+        let attributes = HashMap::from([
+            ("service".to_string(), service.to_string()),
+            ("username".to_string(), user.to_string()),
+            ("application".to_string(), "rust-keyring".to_string()),
+        ]);
+        Ok(Self {
+            attributes,
+            label: format!(
+                "keyring-rs v{} for no target, service '{service}', user '{user}'",
+                env!("CARGO_PKG_VERSION"),
+            ),
+            target: None,
+        })
+    }
+
     /// Create a credential from an underlying item.
     ///
     /// The created credential will have all the attributes and label
@@ -259,15 +286,15 @@ impl SsCredential {
     /// (This is useful if [get_password](SsCredential::get_password)
     /// returns an [Ambiguous](ErrorCode::Ambiguous) error.)
     pub fn get_all_passwords(&self) -> Result<Vec<String>> {
-        self.map_matching_items(get_item_password, true)
+        self.map_matching_items(get_item_password, false)
     }
 
     /// If there are multiple matching items for this credential, delete all of them.
     ///
     /// (This is useful if [delete_credential](SsCredential::delete_credential)
     /// returns an [Ambiguous](ErrorCode::Ambiguous) error.)
     pub fn delete_all_passwords(&self) -> Result<()> {
-        self.map_matching_items(delete_item, true)?;
+        self.map_matching_items(delete_item, false)?;
         Ok(())
     }
 
@@ -293,27 +320,24 @@ impl SsCredential {
         let ss = SecretService::connect(session_type).map_err(platform_failure)?;
         let attributes: HashMap<&str, &str> = self.search_attributes().into_iter().collect();
         let search = ss.search_items(attributes).map_err(decode_error)?;
-        let target = self.target.as_ref().ok_or_else(empty_target)?;
-        let unlocked = matching_target_items(&search.unlocked, target)?;
-        let locked = matching_target_items(&search.locked, target)?;
         if require_unique {
-            let count = locked.len() + unlocked.len();
+            let count = search.locked.len() + search.unlocked.len();
             if count == 0 {
                 return Err(ErrorCode::NoEntry);
             } else if count > 1 {
                 let mut creds: Vec<Box<Credential>> = vec![];
-                for item in locked.into_iter().chain(unlocked.into_iter()) {
+                for item in search.locked.iter().chain(search.unlocked.iter()) {
                     let cred = Self::new_from_item(item)?;
                     creds.push(Box::new(cred))
                 }
                 return Err(ErrorCode::Ambiguous(creds));
             }
         }
         let mut results: Vec<T> = vec![];
-        for item in unlocked.into_iter() {
+        for item in search.unlocked.iter() {
             results.push(f(item)?);
         }
-        for item in locked.into_iter() {
+        for item in search.locked.iter() {
             item.unlock().map_err(decode_error)?;
             results.push(f(item)?);
         }
@@ -335,6 +359,9 @@ impl SsCredential {
     /// but this just selects the ones we search on
     fn search_attributes(&self) -> HashMap<&str, &str> {
         let mut result: HashMap<&str, &str> = HashMap::new();
+        if self.target.is_some() {
+            result.insert("target", self.attributes["target"].as_str());
+        }
         result.insert("service", self.attributes["service"].as_str());
         result.insert("username", self.attributes["username"].as_str());
         result
@@ -429,25 +456,6 @@ pub fn delete_item(item: &Item) -> Result<()> {
     item.delete().map_err(decode_error)
 }
 
-/// Given a slice of items, filter out the ones that have an explicit target
-/// attribute that doesn't match the given target.
-///
-/// References to the matching items are returned in a new vector.
-pub fn matching_target_items<'a>(
-    source: &'a [Item<'a>],
-    target: &str,
-) -> Result<Vec<&'a Item<'a>>> {
-    let mut result: Vec<&'a Item<'a>> = vec![];
-    for i in source.iter() {
-        match i.get_attributes().map_err(decode_error)?.get("target") {
-            None => result.push(i),
-            Some(item_target) if target.eq(item_target) => result.push(i),
-            _ => {}
-        }
-    }
-    Ok(result)
-}
-
 //
 // Error utilities
 //