missing call to `unlock()` in `get_password()`

[jkhsjdhjs]

I'm using spotifyd while storing my credentials in my KeePassXC database, which has a SecretService implementation. KeePassXC has been updated recently and now supports prompting the user to confirm access of an application to a stored secret. This prompting doesn't seem to supported by keyring-rs, as the call of keyring-rs to GetSecret now fails with an error:

method call time=1648045650.307442 sender=:1.248 -> destination=org.freedesktop.secrets serial=5 path=/org/freedesktop/secrets/collection/KeyDB; interface=org.freedesktop.Secret.Collection; member=SearchItems
   array [
      dict entry(
         string "service"
         string "spotifyd"
      )
      dict entry(
         string "username"
         string "***"
      )
   ]
method return time=1648045650.307636 sender=:1.65 -> destination=:1.248 serial=1674 reply_serial=5
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6"
   ]
method call time=1648045650.307687 sender=:1.248 -> destination=org.freedesktop.secrets serial=6 path=/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6; interface=org.freedesktop.Secret.Item; member=GetSecret
   object path "/org/freedesktop/secrets/session/7668f5ed92944dba9d4c9626a6e0f8ec"
error time=1648045650.307750 sender=:1.65 -> destination=:1.248 error_name=org.freedesktop.Secret.Error.IsLocked reply_serial=6

Here's an excerpt of a similar (but successful) message exchange by the Minecraft Launcher, which supports unlocking and prompting:

method call time=1648044309.545926 sender=:1.226 -> destination=:1.65 serial=9 path=/org/freedesktop/secrets; interface=org.freedesktop.Secret
.Service; member=SearchItems
   array [
      dict entry(
         string "tokenkey"
         string "mojangClientToken"
      )
      dict entry(
         string "xdg:schema"
         string "com.mojang.tokenstore"
      )
   ]
method return time=1648044309.546214 sender=:1.65 -> destination=:1.226 serial=1414 reply_serial=9
   array [
   ]
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/c6d5c87eec6b467788481f4cd2ae8b2b"
   ]
method call time=1648044309.546400 sender=:1.226 -> destination=:1.65 serial=10 path=/org/freedesktop/secrets; interface=org.freedesktop.Secret.Service; member=Unlock
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/c6d5c87eec6b467788481f4cd2ae8b2b"
   ]
method return time=1648044309.546534 sender=:1.65 -> destination=:1.226 serial=1415 reply_serial=10
   array [
   ]
   object path "/org/freedesktop/secrets/prompt/d9076c8584c34394aa60a72ab99ba1c2"
method call time=1648044309.547451 sender=:1.226 -> destination=:1.65 serial=17 path=/org/freedesktop/secrets/prompt/d9076c8584c34394aa60a72ab99ba1c2; interface=org.freedesktop.Secret.Prompt; member=Prompt
   string ""
method return time=1648044309.547541 sender=:1.65 -> destination=:1.226 serial=1416 reply_serial=17
signal time=1648044310.578814 sender=:1.65 -> destination=(null destination) serial=1417 path=/org/freedesktop/secrets/prompt/d9076c8584c34394aa60a72ab99ba1c2; interface=org.freedesktop.Secret.Prompt; member=Completed
   boolean false
   variant       array [
         object path "/org/freedesktop/secrets/collection/KeyDB/c6d5c87eec6b467788481f4cd2ae8b2b"
      ]
method call time=1648044310.579309 sender=:1.226 -> destination=:1.65 serial=20 path=/org/freedesktop/secrets; interface=org.freedesktop.Secret.Service; member=GetSecrets
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/c6d5c87eec6b467788481f4cd2ae8b2b"
   ]
   object path "/org/freedesktop/secrets/session/c3a1ac6acba843b492fa157419b38f71"

It can be observed, that the Minecraft Launcher first calls Unlock on the objects returned in SearchItems. Then Prompt is called on the object returned by Unlock. Only after this GetSecrets is called. keyring-rs on the other hand calls GetSecret immediately without calling Unlock and Prompt first, which is the reason for the failure.

I assume a call to unlock() and prompt() is missing here, before get_secret(): https://github.com/hwchen/keyring-rs/blob/4297618e0cf061eacedf6d7c3f164ee4074a3c5d/src/linux.rs#L50 Also it seems like prompt() isn't implemented for Item in secret-service-rs yet.

Relevant SecretService spec: https://specifications.freedesktop.org/secret-service/latest/ch08.html

[brotskydotcom]

Hi @jkhsjdhjs, platforms vary tremendously in the way they approach programmatic unlocking. All three platforms support the notion of a user unlocking secure storage from a UI session before programmatic access is attempted, and the keyring approach---in order to avoid dealing with platform-specific UI---has been to require this general unlock be performed before keyring access is attempted.

If you would like to investigate the ways various platforms support programs invoking UI-based unlock, and submit a PR that incorporates it, we would be delighted to review it. Starting with Linux only would be fine, but please try to avoid approaches that would change the API if possible.

[jkhsjdhjs]

Alright, I finally had some time to have a look at this. Sorry for the late reply @brotskydotcom. The following patch would add the functionality to this library:

diff --git a/src/linux.rs b/src/linux.rs
index 7f349c6..c64f104 100644
--- a/src/linux.rs
+++ b/src/linux.rs
@@ -47,6 +47,9 @@ pub fn get_password(map: &mut PlatformCredential) -> Result<String> {
             .search_items(map.attributes())
             .map_err(decode_error)?;
         let item = search.get(0).ok_or(ErrorCode::NoEntry)?;
+        if item.is_locked().map_err(decode_error)? {
+            item.unlock().map_err(decode_error)?;
+        }
         let bytes = item.get_secret().map_err(decode_error)?;
         // Linux keyring allows non-UTF8 values, but this library only supports adding UTF8 items
         // to the keyring, so this should only fail if we are trying to retrieve a non-UTF8

It results in the following dbus communication when an item is not locked:

method call time=1653671076.733294 sender=:1.319 -> destination=org.freedesktop.secrets serial=5 path=/org/freedesktop/secrets/collection/KeyDB; interface=org.freedesktop.Secret.Collection; member=SearchItems
   array [
      dict entry(
         string "application"
         string "rust-keyring"
      )
      dict entry(
         string "username"
         string "[email protected]"
      )
      dict entry(
         string "service"
         string "spotifyd"
      )
   ]
method return time=1653671076.733535 sender=:1.69 -> destination=:1.319 serial=172 reply_serial=5
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6"
   ]
method call time=1653671076.733918 sender=:1.319 -> destination=org.freedesktop.secrets serial=6 path=/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6; interface=org.freedesktop.DBus.Properties; member=Get
   string "org.freedesktop.Secret.Item"
   string "Locked"
method return time=1653671076.734008 sender=:1.69 -> destination=:1.319 serial=173 reply_serial=6
   variant       boolean false
method call time=1653671076.734360 sender=:1.319 -> destination=org.freedesktop.secrets serial=7 path=/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6; interface=org.freedesktop.Secret.Item; member=GetSecret
   object path "/org/freedesktop/secrets/session/39a553b925aa4670b77f14807cc1e87e"

and in this one when an item is locked:

method call time=1653671021.542487 sender=:1.317 -> destination=org.freedesktop.secrets serial=5 path=/org/freedesktop/secrets/collection/KeyDB; interface=org.freedesktop.Secret.Collection; member=SearchItems
   array [
      dict entry(
         string "service"
         string "spotifyd"
      )
      dict entry(
         string "username"
         string "[email protected]"
      )
      dict entry(
         string "application"
         string "rust-keyring"
      )
   ]
method return time=1653671021.542760 sender=:1.69 -> destination=:1.317 serial=151 reply_serial=5
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6"
   ]
method call time=1653671021.543149 sender=:1.317 -> destination=org.freedesktop.secrets serial=6 path=/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6; interface=org.freedesktop.DBus.Properties; member=Get
   string "org.freedesktop.Secret.Item"
   string "Locked"
method return time=1653671021.543231 sender=:1.69 -> destination=:1.317 serial=152 reply_serial=6
   variant       boolean true
method call time=1653671021.543608 sender=:1.317 -> destination=org.freedesktop.secrets serial=7 path=/org/freedesktop/secrets; interface=org.freedesktop.Secret.Service; member=Unlock
   array [
      object path "/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6"
   ]
method return time=1653671021.543731 sender=:1.69 -> destination=:1.317 serial=153 reply_serial=7
   array [
   ]
   object path "/org/freedesktop/secrets/prompt/84bbb993e0ee4815bdc016e474348cc2"
method call time=1653671021.544476 sender=:1.317 -> destination=org.freedesktop.secrets serial=9 path=/org/freedesktop/secrets/prompt/84bbb993e0ee4815bdc016e474348cc2; interface=org.freedesktop.Secret.Prompt; member=Prompt
   string ""
method return time=1653671021.544625 sender=:1.69 -> destination=:1.317 serial=154 reply_serial=9
signal time=1653671022.829063 sender=:1.69 -> destination=(null destination) serial=155 path=/org/freedesktop/secrets/prompt/84bbb993e0ee4815bdc016e474348cc2; interface=org.freedesktop.Secret.Prompt; member=Completed
   boolean false
   variant       array [
         object path "/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6"
      ]
method call time=1653671022.830018 sender=:1.317 -> destination=org.freedesktop.secrets serial=10 path=/org/freedesktop/secrets/collection/KeyDB/2bf27c5b9dc54eb78fcc8c97485425b6; interface=org.freedesktop.Secret.Item; member=GetSecret
   object path "/org/freedesktop/secrets/session/ac2c51fc57d641ca9243adfcb77e8336"

As can be seen, the Prompt function seems to be called implicitly after calling Unlock, this is probably handled somewhere in secret-service-rs. However, this patch has the downside of an extra method call, because using is_locked() results in an extra Get call, requesting the Locked state. That is unnecessary, because the response of the SearchItems method that is called previously to search the entries already indicates whether the items are locked or unlocked. As stated here, the first array contains unlocked items while the second one contains locked items: https://specifications.freedesktop.org/secret-service/latest/re01.html#org.freedesktop.Secret.Service.SearchItems Thus my patch from above is far from ideal.

However, the search_items() method of secret-service-rs only provides a single vector of all returned items: https://github.com/hwchen/secret-service-rs/blob/d6aaa774f0ec504ff5f26662279e07175b8ef111/src/collection.rs#L117 Thus there's no way of determining whether the returned items are locked or unlocked, at least not without an extra method call. Because of this, I believe that the API of search_items() in secret-service-rs needs to be changed, so it indicates which items are locked and which aren't.

[brotskydotcom]

@jkhsjdhjs Just wanted to let you know that I still haven't had time to look at this, but I haven't forgotten about it. I've been sort of hoping @hwchen would take it on, since he's our secret-service expert.

[jkhsjdhjs]

@brotskydotcom Thanks for letting me know! I investigated this a bit further and have to revise some previous statements:

While going through the SecretService specification I initially failed to notice that there are actually multiple interfaces: The SearchItems method is specified for the Service interface and for the Collection interface as well. Collection.SearchItems is actually specified to only return a single array of the search results, while only Service.SearchItems returns locked and unlocked items as separate arrays. Thus what I said here is wrong:

Because of this, I believe that the API of search_items() in secret-service-rs needs to be changed, so it indicates which items are locked and which aren't.

It is implemented correctly since it implements Collection.SearchItems, not Service.SearchItems.

In the dbus logs I posted in the issue description one can also see that the Minecraft Launcher uses Service.SearchItems while keyring-rs uses Collection.SearchItems. Since only the response of Service.SearchItems indicates which items are locked and which aren't I think it would be best to also use Service.SearchItems here. For this, the search_items() function here would need to be changed, as in the current state it just collects locked and unlocked items in a single vector: https://github.com/hwchen/secret-service-rs/blob/d6aaa774f0ec504ff5f26662279e07175b8ef111/src/lib.rs#L282

[brotskydotcom]

@jkhsjdhjs I don't see an issue about this request to use Collection.SearchItems in the secret-service issues list. I know they are close to a new release so this would seem to be the right time for them to consider this?

I'm going to mark this issue as waiting for the secret-service enhancements.

[brotskydotcom]

Hi @jkhsjdhjs I'm working on a new major release of keyring-rs that uses the (upcoming) 3.0 release of secret-service and I plan to take care of this issue in that release. But I've noticed a discrepancy in API and I wanted to get your opinion on my plan.

1. There is a semantic difference between "secret-service-level search" and "collection-level search" in secret service; as can be seen in the spec: service-level search goes through all collections and returns separate locked and unlocked lists; collection-level search goes through just one collection and returns both locked and unlocked items together.
2. Keyring-rs currently restricts its search to a single collection (specified in the target used in entry creation), so using service-level search would change its semantics.

My current plan is to leave the collection-specific semantics of keyring intact and go with your first proposed fix above (checking if each item is locked before returning it). I am willing to add a way of invoking service-level search in secret-service (because the new release uses traits, I can make credential-store-specific functionality available to clients), but for now I believe maintaining backward-compatibility of search semantics for older clients is worth the overhead of the extra is_locked call.

I would appreciate your take on this.

[jkhsjdhjs]

Sorry for the late reply, I didn't have much time in the past few days and also thought about this a bit.

First of all, I agree with you: semantics are important. Since the name of the collection is part of the SsCredential struct, it should be respected wherever possible.

Your current plan is to keep the current collection-specific semantics as is, but I argue that the current collection-specific semantics aren't correct, and since the next release will be a major release, that would be an opportunity to change these as well.

1. A credential isn't inherently bound to a collection. It may not be contained in any collection, and it may also be contained in several. With the current implementation, an SsCredential is only stored in a collection if set_password() is called.

2. Collection names are just aliases and not necessarily static. Thus, it doesn't really make sense to bind a credential to the name of a collection. SecretService allows any application to create aliases for collections, so even the name "default" may point to different collections each time [1].

3. The current semantically-correct thing to do is to only search the specified collection in get_password. However, the spec recommends to use the service-level search when searching for entries:

   In order to search for items, use the SearchItems() method of the Service interface. [2]

   I assume that most other implementations also do it that way, an example is given in the linked comment, where one can observe the minecraft launcher using the service-level search: https://github.com/hwchen/keyring-rs/issues/84#issue-1178278107. As point 2 states, collection aliases aren't static, so a stored credential may be contained in a collection with a different alias the next time it is requested. This would break the collection-level search. Also, the user may split their credentials into several collections, only one of which would be searched when using the collection-level search.

I propose to remove the collection attribute from the SsCredential struct and instead specify the collection alias when calling set_password and delete_password as function parameters. Of course this would break the API, and clients would have to adjust a bit, but since you're currently preparing a new major release anyways, I think this would be the right time to consider this.

Keep in mind that I'm not a SecretService expert, this is just the way I interpret the spec and the semantic discrepancies I noticed between spec and implementation. I would appreciate your opinion on this, @brotskydotcom! Also, if anyone else is reading this, feel free to join the discussion!

[1] https://specifications.freedesktop.org/secret-service/latest/ch04.html [2] https://specifications.freedesktop.org/secret-service/latest/ch05.html

[brotskydotcom]

Thanks so much @jkhsjdhjs for this thoughtful (and persuasive) analysis. I have a number of thoughts, so bear with me while I use this response to think them through. I think I'm likely to arrive at a place where we are both happy.

• The only point you make that I disagree with is that "the spec recommends to use the service-level search when searching for entries". While I agree that the spec makes that recommendation, the spec immediately follows that recommendation with this sentence "The SearchItems() method of the Collection interface is similar, except for it only searches a single collection", which is both an equally strong endorsement of collection-level search and a lie. (As you and I both know, the SearchItems method of the Collection interface differs critically from service-level search in that it doesn't distinguish locked from unlocked items.)
• The points you make about aliases being changeable are true but don't seem to me to really bear on this discussion. Obviously if multiple clients use the same collection then they will need to agree about what to call that collection. But I think it's reasonable for them to expect that if they use different collection names from each other to store an item then they can use the same attributes on that item without fear of ambiguity when they search.
• The two points you make that influence me most strongly are these:
  • Collections are simply holders of items, and the same item can be in no collection or in many collections. From the secret-service point of view, the collection is thus not a required "search key" for an item (in the way, for example, the required "domain" field is in the macOS/iOS keychain interface).
  • Major versions are the right times to make appropriate semantic changes in a crate's interface. Thus if we are going to make an interface change to more closely mirror secret-service semantics around search, this is the time to make that change.

Where this all gets me is here:

1. The optional target field of an entry has intentionally vague semantics at the crate interface level: it's explicitly left to each of the credential stores to decide how to handle that field. What the crate-level interface does imply, however, is that two entries with the same service and username values can use different target values to avoid collision in the credential store.
2. The v2 meaning of target for secret-service was the collection used both for creation and for search, a convention that was motivated by the macOS/iOS meaning of target. But that meaning is arguably incorrect, as you pointed out. While that meaning of target definitely makes sense in the creation scenario, and it gives clients a useful way of specifying a specific collection they already know exists, it doesn't make a lot of sense in the search scenario, because secret-service offers collection-independent search (and that search may in fact be more performant, depending on whether there is a global index or cache maintained by the service and whether the items being searched for are locked).
3. Given (1) and (2), the only issue I have with switching to service-level rather than collection-level search is the collision it could introduce for existing credentials that differ only in their target attribute. (For new credentials I am happy to add a target attribute which can be used to disambiguate.) I can see at least a couple of ways of dealing with this, the second of which is the most backward compatible but introduces performance overhead in collision cases:
   • introduce a new error which indicates the existence of a collision and returns multiple entries;
   • in the presence of collision, use collection-level search to disambiguate.
4. Now that credential stores are full-fledged objects with constructors, I can make it easy for clients to construct a secret-service store with a specific ambiguity-resolution strategy (or even a different target-interpretation strategy). So clients who really want the legacy semantics on search can just opt into it by using a parameter when they construct their store. Alternatively, I could leave the default as v2 collection-level search (with unlocking of found items) and let new clients use a parameter to opt into service-level search.

I would very much appreciate thoughts on all this!

[brotskydotcom]

Changing the title and reopening this issue as it has migrated from a bug report to a new-version design question.

[jkhsjdhjs]

Thanks for the quick reply, and again sorry for the delay! Also thanks for the explanation, I actually never had a look at the keyring-rs interface, I only viewed the SecretService specific files so far.

I can see the problem with switching to service-level search, and I also think that using a target attribute in the future is reasonable. For already-existing entries this could lead to collisions, as you correctly pointed out.

introduce a new error which indicates the existence of a collision and returns multiple entries;

I'm not in favor of this first alternative, since a new error type would be required for this behavior and application developers would thus have to adjust their code to handle this error correctly.

in the presence of collision, use collection-level search to disambiguate.

However, I like this alternative! It has the advantage that the applications using keyring-rs don't need to be changed in any way. The performance overhead wouldn't be a problem IMO, since dbus is quite fast and I'm pretty sure the fallback to collection-level search wouldn't be noticeable if the user is simply searching for single entries.

I don't quite understand your fourth point though. When falling back to collection-level search, this is the same behavior as currently implemented. So if there are still ambiguities when using collection-level search, these ambiguities would be present currently as well. New entries would be created with the new target attribute, which disambiguates them. So unless you plan to change the existing ambiguity behavior aside from this issue as well, I don't see the need for an ambiguity resolution strategy.

One question I'm currently asking myself regarding the collection-level search fallback however is: If an entry is fetched via the collection-level search fallback and a new password for that entry is set, would that result in the creation of a new entry (because of the new target attribute that would be added) or would it update the existing entry?

[brotskydotcom]

Thanks again for a very useful reply; this conversation is really helping me sort out my thoughts. A few clarifications:

• The addition of the error on colliding entries is required regardless, both for the Mac keychain and the Linux secret-service, because in both services (as far as I know) attributes you don't specify as part of the search may be present in the entry. Thus (for example), if someone were to use keychain-rs to add an entry with service="foo" and user="bar", and some 3rd party app added a credential with service="foo" and user="bar" and client="baz", the keychain-user would actually get back both those entries in a search. (I haven't tested this on Linux, but I know it happens on Mac.)

Now let's talk about the "target" attribute:

• If I add a "target" attribute to newly-created entries that are given explicit targets, then for backward compatibility I need to do one of the following:
  • search for entries without specifying the target, so that both new-style and old-style entries are found (per the search behavior described above).
  • search for entries with the target (which will only find new-style credentials), and then if that fails do a second search without the target (to find old-style credentials).
• Regardless of which of these I do, I have to do two things in order to avoid creating both old-style and new-style credentials for the same entry:
  • whenever I find an old-style credential, I need to replace it with a new-style credential.
  • I have to do search-before-set, in case there's an old-style credential.

This is a lot of complexity and performance impact, none of which is needed if I don't add the target attribute. If I don't add the target attribute, then I have to something to deal with with target ambiguity, such as:

1. Start with collection-level search. This means each lookup incurs the additional cost of the "is it unlocked?" call, but guarantees the client always gets the target they searched for.
2. Start with service-level search, falling back to collection-level search in the case of ambiguity. This is cheaper, but gives the client no guarantee that the credential they find has the same target as the one they looked for.
3. A variant of (2) that puts the target into the description, so new-style credentials are guaranteed to have the right target. (This variant depends on being able to update the description as part of updating the password in an existing credential.)

I'm planning to do (1) by default, since that's by far the simplest and the most backward-compatible. But it's my belief that (1) and (2/3) can have very different performance impacts on users who don't care about target ambiguity. That's why I talked about allowing users to choose their ambiguity strategy, so they could choose between 1 and 2/3. Now that credential stores are represented by credential builders, it's easy to add options to the secret-service builder constructor to choose between these strategies, and for a user who wants a different strategy to make it his default.

As always, I would greatly value your feedback.

[jkhsjdhjs]

Thanks for the clarification! ~~I'm still not sure if the new error type is really required for linux. AFAICT keyring-rs always stores the application and the service attribute, where application is stored as rust-keyring and service as the name of the application using keyring-rs, so for example spotifyd. So when searching, these attributes should be (and I believe currently are) included in the search.~~ But now that I think about it, I guess if a third party app creates these attributes as well, the application searching for an entry should me made aware of this. So yeah, I agree, the error is reasonable.

Hmm, the solutions you propose make sense. If keyring-rs handles the ambiguity it makes sense to use resolution strategies. But now I'm thinking, if the new error type is required anyways, it may be simpler to just append the ambiguous entries to the error and let the user decide which one they want to use.

What about the following:

Search on Service interface with target attribute
  Duplicates: Return Error containing all duplicate entries
  No results: Search on Collection interface without target attribute (using target as collection alias)
    Return response and mark entry as legacy internally
  Exactly 1 result: return

Entries marked as legacy internally would have to be extended by the target attribute once set_password() is called. On the next get_password() call it would then be found by the service-level search.

[brotskydotcom]

OK, sorry for the delay in replying, but I've been doing a lot of testing and a lot of thinking and I think I have a fairly elegant solution. We introduce one option in the secret-service credential store builder: a boolean search_all which means to search for credentials in all collections (i.e., use service-level search). When you turn on search_all in the builder, it causes all the credentials to include a target attribute (as well as using the target for the collection name in which the credential is created), and it always searches for credentials at service level including the target attribute. This means:

• if search_all is off (the default), then we are completely compatible with v1 credentials and we have to do the explicit check for a locked credential on get and delete.
• if search_all is on, then we will never see any v1 or search_all=off credentials at all (because none have a target attribute).

Note that v1 and search_all=off will find credentials built with search_all=on (because they ignore the target attribute). In addition, because of the way the secret service works, doing a v1 or search_all=off set-password call will remove the target attribute from an existing credential with the same service and user.

Thus, to write a v2 client that always uses service-level search but is compatible with non-service-level search clients, the service-level search client should always delete any existing non-target entry with the same user and service before setting a password. This is very easy to do because you can use two credential stores side-by-side in the same v2 client.

Also note that, in order to release this version now, I had to revert back to using v2 of the secret service, because v3 has not yet been released. This means that keyring v2 clients that do service-level search are going to pay the same "ensure-unlocked" penalty on every lookup that collection-level search clients do, because secret-service v2 doesn't have your enhancement to return the separate locked and unlocked collections. But as soon as secret-service v3 gets released I will do a dot release of keyring v2 that will take advantage of it.

I'm putting out beta.1 of v2 very soon. Please give it a try and let me know what you think!

[jkhsjdhjs]

What I like about this solution is that it's fully backwards compatible and that it allows for the credentials to be migrated. But if I understood it correctly, the applications using this library would have to handle the migration themselves, so they would have to instatiate two separate credential stores, use one store to retrieve old credentials and the other to store credentials in the new fashion. While this gives the application full flexibility in what they want to do, I'm concerned whether the applications want that flexibility. Since search_all is set to false by default, I fear that developers won't even notice a change and thus don't adjust anything when upgrading keyring-rs to v2. That might be good when caring about backwards-compatibility, but it also causes credentials to never be upgraded to the new style. And even worse, applications that are just starting to use this library will probably also use collection-level search, since it's the default.

So if we're doing it that way, I think this change should be the first mention in the changelog, it should be documented properly and maybe there should even be a deprecation warning when using search_all=false, because developers really should be aware of this. Aside that, shouldn't a set-password call with search_all=false also add the target attribute? This way the credentials would be made compatible with search_all=true, and maybe search_all=false could eventually be removed in v3 or v4.

Again, this is all under the premise that I understood it correctly. Please let me know what you think!

[brotskydotcom]

Thanks for the feedback, I really appreciate it. Let me respond in inverse order:

• The problem with adding a target attribute in set-password even with search-all=false is that it means set-password has to do a search for the legacy credential before adding the new one. Otherwise you end up with two credentials. There's really no way around this: the target attribute is really only there for service-level search, so if you're not using service-level search then you shouldn't add it.
• I like the idea of forcing developers to say "yes maintain legacy compatibility" explicitly so that people who come to the crate for the first time do the right thing by default. So I think it makes sense to do several things:
  • make search-all=true the default
  • provide a feature that makes search-all=false the default, but don't include it as a default feature.
  • document how legacy users can either set that feature when upgrading to 2.0 or they can do explicit upgrading.
  • write an example that does the dance to upgrade older credentials (so developers can see how easy it is).

How does that sound?

[brotskydotcom]

So I have figured out a much, much simpler approach to all this:

1. We always use service-level search. So no options are needed to turn it on or off.
2. We always add a target attribute when creating new items.
3. We always just search for the service and user. So we find both v1-style entries (with no target attribute) and v2-style entries (with the target attribute). We filter the found results so they are either v1-style entries (which are assumed to have a matching target) or v2-style entries with a matching target.
4. If we ever get multiple hits on a search, we return an ambiguous error.
5. set-password works by doing get-password and then, if it finds a unique item, setting the password on that item. If there is no matching item, it creates one in a collection labeled by the target (creating that collection if necessary). Note that the default target is the only collection found by alias; all other collections are found by label.

This seems to provide the best of all worlds: v1 and v2 are now completely compatible, and we can create collections if the client uses a non-default target. (Note that v1 never created collections, and it always used the target name as an alias, so in effect it was completely restricted to using the default collection.)

If (due to 3rd party items) an ambiguity is found, there are platform-specific entries for getting all the passwords or deleting all the matching items.

[jkhsjdhjs]

That sounds like an excellent approach to me!

set-password works by doing get-password and then, if it finds a unique item, setting the password on that item.

Maybe it should also add the target attribute to that item, if it doesn't has one yet?

[brotskydotcom]

Maybe it should also add the target attribute to that item, if it doesn't has one yet?

I don't think there's any way to do that short of deleting and re-creating the item. (Do you know of one?) That would be two more calls to the Dbus and it might actually have the effect of changing the collection the item is in (if the original item was created in the default collection but the new item has a different target). If a 3rd party is creating items I'd rather have keyring read and set them without altering them.