search

hwchen / secret-service-rs

Rust library for interfacing with Secret Service API
Apache License 2.0
72 stars 28 forks source link

Implementation of item.equal_to seems inefficient #82

Open brotskydotcom opened 2 months ago

brotskydotcom commented 2 months ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 3 weeks ago

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 commented 2 weeks ago

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.