-
-
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 |
Would you be interested in a PR containing the changes I mentioned in my previous response? |
Sorry to take so long to reply! Thanks for summarizing the conversation so well! Here are my thoughts about your proposed keyring-rs changes, and yes I would love a PR with those that can be made just in keyring-rs (you'd have to alter underlying crates for some of these). I believe this PR would have to be the basis of a new version, because it would break backwards-compatibility.
AFAIK, this can't be done, because the credential store implementations depend on crates which aren't available on all operating systems. The only reason for the OS-specific restrictions are to allow compilation.
Yes, this would leave us with only the dbus-secret-service dependency.
I'm not sure what restrictions you are referring to, as the way the dbus-secret-service crate works already allows use of crypto to be selected at runtime. Perhaps you mean trying to compile with both the "rust-crypto" and "openssl-crypto" features, and allowing choice of which crypto to use at runtime? First of all, that would be a change in the dbus-secret-service crate, not in the keyring-rs crate. Second, it seems pointless to me. The only reason the dbus-secret-service crate allows openssl crypto is for compatibility with the secret-service crate, which is no longer being used. There are no functional or footprint or compatibility advantages to exposing the use of openssl in keyring-rs, so we should just standardize on the use of rust crypto.
Aside from the fact that this would not be a change in keyring-rs, I don't quite understand what you mean. There is only one type of encryption supported by the Secret Service itself, and whether to use it or not is already selectable at runtime (once we restrict ourselves to the dbus-secret-service dependency).
You keep coming back to this issue, and I don't understand why. AFAICT, there is absolutely no harm in in having the crate always select a default credential store, and it saves naive clients from the need to call
You keep talking about "Secret Service methods," but the Secret Service is just one available credential store among many. What I have said consistently is that, since the keyring-rs interface is synchronous, having its implementation incorporate an async runtime is unfriendly to clients. That's why I want to drop use of the "secret-service" crate: because its implementation requires an async runtime. Hope this helps clarify my position. |
Yes it can be done, you just shift to the lib consumer the responsibility to choose the right feature for the right need / OS. I believe it can reduce noise (in-code compilation conditions) and make the API simpler to comprehend and use. One feature = one store. It should not be the responsibility of keyring-rs to impose restrictions for users. Plus, OS-specific compilation conditions prevent developers to interact with code that does not run on his host platform. Now, if a user tries to build an app using the Apple Keychain on a Linux it's his problem, the compiler will gently reminds him that the security framework is not available. Let's wait for the PR, it will be easier to explain and to discuss about it.
Great.
It makes sense, let's just keep Rust crypto.
You can discard this point since we only keep Rust crypto.
This is because you still see keyring-rs as a higher tool than it should be. For sure it does not harm, but it adds an extra opinionated layer. I know that for my personal use of keyring-rs I don't care about the default keystore. I want to do my own logic (at app level) of which keystore is available on which OS, I don't want keyring-rs to make this choice for me. That said, we know that a default credential would be useful for simple use (which will be the most common case), hence my proposition of feature-gating it.
To be consistent. The fact to have a different behaviour depending on the number of available stores on a specific OS is already an opinion. I really see keyring-rs as a simple library exposing feature-gated keystores, with a common sync interface. And that's it! One feature enables one keystore, nothing more. Which OS, which default, which version etc is already an opinionated layer that adds noises to the core lib. I believe those layers would fit better in another crate. For example a crate that exposes a default keystore based on the host machine.
For my own needs, I could easily create my I/O-free lib at the top of keyring, as the abstraction is really minimal. I would have control over restrictions like OS, default etc.
You can also drop this point as discussed earlier.
Yes definitely, I have enough information to propose a PR in the nearest weeks. It will be easier to continue discussion there. Thank you for your time! |
(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: