-
-
Notifications
You must be signed in to change notification settings - Fork 53
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Allow choice of credential store at runtime. #232
Comments
I'm not sure this requires any enhancement. The only two credential stores you can't use at the same time are the synchronous and asynchronous versions of the secret service, and those are the same credential store, so there would be no functional difference in choosing one or the other. In all other cases, you can build in all the credential stores and then choose between them at runtime by using |
This may be partially wrong. So far the My point is: some people may need the blocking D-Bus API, some the Z-Bus tokio API, some both. If you expose everything, people can compose the way they want. This idea brings us to the last point (and joins your questions): as a lib you don't want to rewrite from scratch every implementations you have for std, tokio, async-std etc. Code needs to be put in common. To summarize: ideally, a reusable and composable lib should:
The Sans I/O pattern looks the most promising to me, and seem to fit all the cases. I partially contibuted to a Sans I/O project called |
This is not a functional difference. Whether an interface is sync or async is a "non-functional" distinction: it does not affect the specified input/output behavior of the interface, only the form of the computation that produces those outputs from those inputs. This stack overflow answer gives a nice, concise definition of functional vs. non-functional requirements. Putting this another way: there is no way for a user of an application which uses keyring to know whether, under the covers, that application's developer is using a sync or async credential store. Only the developer knows this. It sounds like what you really want is a crate that exposes both synchronous and asynchronous interfaces. I don't see the point in this, for two reasons:
You have mentioned how much more complex the architecture is when you try to attract whether you are using an async or sync component underneath the API surface. In my experience, it also tends to be inefficient, because the generalized architecture can't be optimized based on the client architecture. The only reason I have even left zbus support in keyring at all is for backwards compatibility. I don't know of any systems that use dbus that don't have a dbus library, so there's really no reason not to use Very interested in your thoughts here. |
In term of pure D-Bus communication I agree. That said, the Secret Service interface is not the same depending on the crate used.
Could you update the link? Looks like the wrong one, I'm curious to read about it.
While I understand your statement that, in fine, a credential store is synchronous:
Lib consumers do not really care about your statement. All they see (me including) is: does this keyring lib support my environment? Can I integrate it easily? You may be right from a technical point of view, but from the UX point of view it is definitely a blocker. People value as much integration (even more) than perfs. If you focus on perfs or correctness of implementation, you can just take the sync interface and do your own connector. So I would not be as strict as "it should be done by the consumer": I would rather say: "it could be done by the consumer in case the default, non-optimized and decent-enough I/O implementation proposed by the crate does not meet the needs".
I believe we agree on the same thing, we just misunderstood each other. My point is not to proposed an optimized I/O implementation, but to propose a decent-enough API so that the majority of consumers could integrate easily keyring into their project. Optimization is already an edge case, yet concerned users should have the tools to do it.
I don't have a real-world use case. It could be someone that already depends on |
Thanks for your thoughtful comments! Here are my thoughts in return: First, sorry for the incorrect link about functional vs. non-functional requirements I posted earlier. The correct one is this StackOverflow answer. The references from there are also useful.
Don't we already have that? I don't hear you proposing any changes to the keyring API to make it easier to integrate. What I do hear is you proposing changes in the way one specifies features that control the available credential stores. I completely agree with you that the current feature set is way more complicated than I would like. I believe an easy way to fix that would be to eliminate the use of the
I couldn't agree more. And eliminating the use of the When I asked you for a use case that justifies retaining asynchronous access to the Secret Service, neither of the two you suggested (already using zbus, want a pure Rust implementation) would be problematic if we eliminated the What I am taking away from this conversation at this point is that the only reason to keep the use of the Am I missing something here? |
Thank you for the Stack Overflow answer, really interesting! So we agree on the following statement: all keystores should be accessible via cargo features, without OS restriction nor compilation condition.
You want to get rid of the only async keystore in order to remain with only sync ones, right? This way, you only have 1 unified API around sync keystores (the actual trait If so then, while it makes sense and it would probably work well, I still find the approach not "flexible" enough. What if tomorrow the Apple Keychain API becomes async? All the structure falls appart. It will probably not happen, but it is just to illustrate the fact that by sticking to a non-functional I/O model you always reach the limits sooner or later. Think Murphy's law: My initial proposition was to expose all keystores behind feature gates, so users can compose them the way they want/need. The same logic can be applied to keystore implementations: instead of having a shared, unified API (which forces you to choose an I/O model) we could just expose tools to operate on that keystore. Users could just compose them in the I/O model of their choice. For example:
Based on that, users could take the functions they need to compose their own I/O connector. As an example, see this std connector: it's just "plumbery work", building a giant std I/O connector with all std-compliant implementations.
Well, at app level it can be problematic. If you remove the That said, app concerns are not the same as yours, so I could definitely understand that sticking to sync API is more appealing to you. If you do so then I will still need to develop a wrapper. The question is how much of this wrapper can be part of |
Not quite. You are confusing the "functional specification" of the credential store (which is modeled in the Credential API) with the "non-functional specification" of how the crate accesses the credential store (which may be via synchronous or asynchronous implementations of the Credential API). I want to eliminate this crate's use of asynchronous mechanisms to access the underlying credential stores, because it's an unnecessary complication that I believe serves no purpose either functionally or non-functionally. Putting this another way, I claim that how this crate accesses the underlying credential stores doesn't affect client applications either functionally or non-functionally.
Do you mean what if it stopped being accessible via synchronous APIs? That would break the entire existing MacOS world, which is why it's not going to happen. The addition of async APIs to the Apple Keychain wouldn't affect this crate at all, in exactly the same way that the addition of async APIs to the
I don't think that's true. You can simply link dbus statically (using the "vendored" feature). That will produce an app that works independent of the presence of libdbus on the target machine.
If you use the "vendored" feature, you are not relying on any non-Rust libraries at runtime. I'm not sure what else you would mean by a "pure Rust implementation."
I believe that your core concerns are captured by the following question:
That's a very interesting question, and I believe it has two answers:
It's answer #2 that I believe is the correct answer to your question about "how much of this wrapper can be part of keyring-rs". If you are interested in contributing a polling-based equivalent of the CredentialAPI, which would (for example) enable the creation of a |
The frontier is thin between both, because the model carries the non-functional spec by having or not the
You totally right, and I forgot about this feature. So the whole "pure Rust implementation" point is resolved by the vendored feature, great news.
If I do not mistake, Rust
Do you mean:
Then you end up with the initial Tokio wrapper I use to wrap calls to At this point I don't see the benefits of exposing an AsyncCredentialAPI. Sticking to one single blocking CredentialAPI looks like the way to go. To summarize the current discussion, here the changes we could do: At
At
This way, we end up with a nice overall keyring support for Rust:
People can still develop their own credential thanks to Just a side question to understand better your opinion: since you believe that access to Secret Service methods should be sync, can I safely assume that you do not value a Z-Bus based Secret Service since the |
(Requested by @soywod)
The current usage pattern for this crate is that the developer chooses the credential store to use (for each platform) at compile time, and the users of the developer's application have no choice in the matter. Because it's not possible, currently, to build this crate with all possible credential stores on all platforms, there is no way for a developer to let his application users choose their desired credential store at runtime.
Perhaps, in the next major version of keyring, this could change?
The text was updated successfully, but these errors were encountered: