Implementation of item.equal_to seems inefficient

[brotskydotcom]

I'm adapting the current secret-service code to also work synchronously with the dbus library, and I came across something I don't understand. The implementation of item.equal_to checks both that the path objects of the two items are the same and that the attributes of the two items are the same. But if the path objects are the same, how could their attributes possibly be different? It seems that this implementation is doing two unnecessary round-trips through the bus.

I think this method is actually supposed to be checking whether the underlying credential for two different items are the same or not. In that case, just checking the path values should be enough. As far as I know, there aren't multiple paths for the same object in the store.

[complexspaces]

Hey there, thanks for the question. Its great to also hear someone is working on the newer dbus integration 🎉.

I don't 100% know why this was written this way since it was before my time maintaining the crate. One thing that comes to mind is if it would be possible for a secret service provider backend to be changed in the duration a process holds onto a secret_service::Item object. The specification says this, but its kind of vague:

The object path of an item or collection should not change for its lifetime, under normal circumstances.

[brotskydotcom]

I think it's quite possible that @hwchen intended equal_to to return true if the attributes were the same even if the paths were different. Unfortunately he didn't make it an or, he made it an and. And if the paths are the same there's no way the attributes (which are both fetched fresh in the implementation) can be different. Walther, if you want to weigh in here, that would be great.

I've completed and published the dbus-secret-service crate if you are interested. I was looking for a way to make this implementation be the "blocking" implementation in this crate, but I don't see any clean way to do that. It would require conditionalizing the dependencies in a way that requires major surgery, and would change the API in a SerVer-incompatible way. If you are interested in merging the two crates at your next major release, let me know.

[complexspaces]

And if the paths are the same there's no way the attributes (which are both fetched fresh in the implementation) can be different.

Good callout, I see what you mean now. Even if the provider changed at runtime you'd never notice it because the attributes.... In that case I don't see a problem with simplifying this to just check the path since it makes no sense for a single provider to return different attributes for the same path.

and would change the API in a SerVer-incompatible way. If you are interested in merging the two crates at your next major release, let me know.

I don't want to ask you for free work, but if you're willing could you list out a few of the breaking changes you think would be needed since you seem to have at least taken a cursory look? At a glance I see that session encryption is an optional feature in your library.

[brotskydotcom]

The biggest issues I ran into are the following:

1. Sync vs. Async interfaces. Depending on whether you use dbus or zbus, the top-level (externally public) interface needs to be one or the other. The simplest way I thought of doing that was to have two different versions of the secret_service constructor, returning two different types of objects, one of which had an async interface and one of which had a sync interface.
2. Code conditionalization. Each of the implementations depends on their respective dbus or zbus interfaces, so the feature choice of sync or async will have to completely control which code set gets loaded.

Because of the sync/async issue, it's not at all clear to me why we would want to combine the crates. Really they are always going to be completely separate implementations, and they don't actually share any code, so putting them together in one crate doesn't seem like it buys us anything. That's why I ended up just making a separate crate. As long as we cooperate to keep the blocking interface of this crate and the only interface of the dbus crate in sync, I think we're fine.

[soywod]

I've been using secret-service for a while now, and it worked perfectly so far. But my needs changed, and I faced the same issues / concerns as yours.

The solution I am experimenting at the moment is the Sans I/O approach, using iterable and composable state machines that produce I/O requests. Basically, I/O are executed at the highest level, by I/O connectors: it gives so much flexibility (but it also increases the complexity).

So far I was able to make it work with Apple Keychain and Windows Credentials (std only), D-Bus Secret Service (std and tokio) and Z-Bus Secret Service (std, async-std and tokio). Secret Service crypto providers are also available (OpenSSL and Rust Crypto, std only):

https://github.com/pimalaya/keyring

It looks "overkilled" at the keyring scope, but it makes a lot of sense at higher level.

Here some example to illustrate the experiment:

// Let's say you want to read a secret from a keyring entry
// based on a blocking Secret Service (D-Bus). Here the I/O connector:
let mut dbus = SecretServiceDbusStdIoConnector::new(&service);

// You can use the ReadEntryFlow state machine this way:
let mut flow = ReadEntryFlow::new(&key, encryption);

while let Some(io) = flow.next() {
    match io {
        Io::Read => {
	    // This is where the actual I/O is executed
            dbus.read(&mut flow).unwrap();
        }
        _ => continue,
    }
}

It can be easily composed. Here an example with the Secret Service crypto, where you introduce 2 new I/O encrypt and decrypt:

// This time we use the nonblock Secret Service, based on Z-Bus:
let mut dbus = SecretServiceZbusTokioIoConnector::new(&service).await;

// We also introduce another I/O connector for crypto, based on OpenSSL (std)
let mut crypto = OpensslStdIoConnector::new(dbus.session()).unwrap();

// Same state machine
let mut flow = ReadEntryFlow::new(&key, encryption);

while let Some(io) = flow.next() {
    match io {
        Io::Entry(EntryIo::Read) => {
            dbus.read(&mut flow).await.unwrap();
        }
        Io::Crypto(CryptoIo::Decrypt) => {
            crypto.decrypt(&mut flow).unwrap();
        }
        _ => continue,
    }
}

Curious to know your opinion.

[brotskydotcom]

Hi @soywod, it's not clear whether you're addressing the "your opinion" question to me or to @complexspaces. You posted this in the secret-service repo, not the keyring repo, but it seems you are talking about a way of implementing keyring differently? Is there an issue you feel this approach addresses?

[soywod]

I found your questions in #82 (comment) very interesting, I am also asking myself the same ones for my projects. See my comment as an attempt to reply to them. I'm still experimenting with the idea, I did not feel ready to open a dedicated issue for that purpose.

That said, if you feel like, then I can definitely transfer it. As I said I really enjoyed keyring-rs (and subcrates like secret-service, dbus-secret-service) so far, but I just feel the need to upgrade to sth more generic. Having mutually exclusive features is not advised, and my projects feel the limitations. I need my lib consumers to be able to use any variant of keyring or secret-service at compile time using carge features and at runtime (user config file), and not being restricted to OS.

The actual state of my experiment gathers keyring-rs stuff like Apple Keychain and Windows Credentials, secret-service stuff like async Z-Bus secret service and dbus-secret-service like sync/async D-Bus secret service. Thanks to the genericity of the experiment, I was able to put code in common between interfaces. It can definitely be improved tho.

[brotskydotcom]

OK, I get it now. You are asking that clients be able to choose their keystore at runtime, not at compile time. That's certainly a valid feature request: see hwchen/keyring-rs#232.

[soywod]

To be more precise,: keystore could be selected at compile time AND runtime. This is the most flexible in my opinion: someone may need just the Apple Keychain for a MacOS app, someone may need all keychains available at runtime etc.