Skip to content
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

Open
brotskydotcom opened this issue Dec 24, 2024 · 8 comments
Open

Allow choice of credential store at runtime. #232

brotskydotcom opened this issue Dec 24, 2024 · 8 comments

Comments

@brotskydotcom
Copy link
Collaborator

(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?

@brotskydotcom
Copy link
Collaborator Author

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 set_default_credential_builder. @soywod, can you say more about what flexibility you are trying to get?

@soywod
Copy link
Contributor

soywod commented Dec 25, 2024

so there would be no functional difference in choosing one or the other.

This may be partially wrong. So far the dbus crate exposes a blocking API (std) and a nonblock API (tokio). The zbus crate exposes by default an async API (async-io), another async API behind a cargo feature (tokio) and a blocking API. But you cannot use the blocking API only, so it comes with futures-* dependencies. Both do not manage the same way: one needs a third party lib (libdbus), the other spawns an async execute (although looks like you can disable it).

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:

  • Have cumulative cargo features and not exclusive ones
  • Expose components both at compile time (cargo features) and runtime
  • Not care about I/O and runtime, no need to deal with duplicate async/non async traits
  • Propose default I/O connectors, and make sure lib consumers can develop their own connector if needed

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 imap-next, and it works great. The new keyring prototype I experiment with looks promising also. The only drawback I found is that it makes the code a bit more complicated to understand, it's kind of an unusual way to code. I really invite you to read more about the pattern and real world examples I shared to make your own opinion.

@brotskydotcom
Copy link
Collaborator Author

brotskydotcom commented Dec 26, 2024

so there would be no functional difference in choosing one or the other.

This may be partially wrong. So far the dbus crate exposes a blocking API (std) and a nonblock API (tokio).

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:

  1. Abstract credential store behavior is, for all intents and purposes, synchronous. Without a deep understanding of how a credential store works, it's incredibly dangerous to make multiple calls into that store at the same time. Calls to credential stores should always be serialized by the programmer, not the store.
  2. While it's abstractly clear how to wrap an asynchronous interface around a synchronous one (allocate a separate thread for all uses of the synchronous interface, and have the async interface queue all accesses on that thread and wait for their completion), the details of how to do this effectively will vary tremendously based on the system architecture of the consumer and so it should be done by the consumer not by the crate being consumed.

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 dbus-secret-service in any new application you are building. Can you give me a real-world use case you have that requires zbus?

Very interested in your thoughts here.

@soywod
Copy link
Contributor

soywod commented Jan 1, 2025

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.

In term of pure D-Bus communication I agree. That said, the Secret Service interface is not the same depending on the crate used. dbus generates a different API from zbus. One uses a CLI to generate a Rust file, the other uses macros, and they do not have the same output.

This stack overflow answer gives a nice, concise definition of functional vs. non-functional requirements.

Could you update the link? Looks like the wrong one, I'm curious to read about it.

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:

  1. Abstract credential store behavior is, for all intents and purposes, synchronous. Without a deep understanding of how a credential store works, it's incredibly dangerous to make multiple calls into that store at the same time. Calls to credential stores should always be serialized by the programmer, not the store.

While I understand your statement that, in fine, a credential store is synchronous:

  • Keyring only exposes methods. Consumers should be responsible for not doing multiple calls at the same time. If I, as a lib consumer, have such piece of code in my app, I have serious issues with my architecture! What I mean is that you cannot be responsible for wrong implementations. A good documentation (as it is already the case) should be enough.
  • How is it "incredibly dangerous"? A secret store is "just" a database, I don't see any horrible scenario except for dead lock or call failure, but I may miss information there.
  1. While it's abstractly clear how to wrap an asynchronous interface around a synchronous one (allocate a separate thread for all uses of the synchronous interface, and have the async interface queue all accesses on that thread and wait for their completion), the details of how to do this effectively will vary tremendously based on the system architecture of the consumer and so it should be done by the consumer not by the crate being consumed.

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".

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.

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.

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 dbus-secret-service in any new application you are building. Can you give me a real-world use case you have that requires zbus?

I don't have a real-world use case. It could be someone that already depends on zbus and does not want to additionaly depend on dbus, or someone that does not want to deal with external libs (pure Rust implementation), but I don't really care. The idea of proposing a keyring library that just exposes various keychains behind feature gates is really appealing, because it's dead simple to comprehend. There is no logic / opinion like "it's only sync because …", or "this keychain cannot be used sync and async because …": it just exposes keychains. I find it a solid basement for any opinionated, higher-level crates.

@brotskydotcom
Copy link
Collaborator Author

brotskydotcom commented Jan 3, 2025

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.

My point is ... to propose a decent-enough API so that the majority of consumers could integrate easily keyring into their project.

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 secret-service crate completely. That would leave us with exactly one feature for each of the supported credential stores (Apple-native, Windows-native, Linux-native, and Secret Service) plus a bunch of (completely optional) features that control subtleties such as encryption-in-transit to the Secret Service and whether 3rd-party libraries are linked or compiled in. It's hard to imagine what would be simpler than that, especially since all of those features can be specified together providing access to all of the credential stores.

The idea of proposing a keyring library that just exposes various keychains behind feature gates is really appealing, because it's dead simple to comprehend.

I couldn't agree more. And eliminating the use of the secret-service crate would produce just that.

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 secret-service crate. In fact, the opposite is true: folks who already use zbus have filed bugs against this crate because its use of the secret-service crate brought in a conflicting version of zbus.

What I am taking away from this conversation at this point is that the only reason to keep the use of the secret-service crate in this crate is for backward compatibility, and that the next version of this crate should just omit it.

Am I missing something here?

@soywod
Copy link
Contributor

soywod commented Jan 8, 2025

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.


I believe an easy way to fix that would be to eliminate the use of the secret-service crate completely.

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 CredentialApi in fact)?

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: anything that can go wrong will go wrong 😄

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:

  • The Apple Keychain only proposes a sync API, so we could just expose functions like fn set_secret(), fn get_secret(), fn set_attributes(), fn get_attributes() etc.
  • The D-Bus based Secret Service exposes a blocking API, so we could expose same functions as the Apple Keychain
  • The D-Bus based Secret Service also exposes a nonblock, tokio-based API, so we could expose async functions instead (async fn set_secret(), async fn get_secret() etc)
  • The Z-Bus based Secret Service only expose an async API (the blocking one is just using this async API), so it would expose only async functions
  • All Secret Services share crypto stuff:
    • One is based on OpenSSL (sync), it could expose functions like fn encrypt(), fn decrypt(), fn hkdf() etc
    • One is based on Rust Crypto (sync as well), it could expose same functions as OpenSSL
    • Tomorrow, we could have a third one async, and would expose instead async functions async fn encrypt(), async fn decrypt() etc

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.

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 secret-service crate

Well, at app level it can be problematic. If you remove the secret-service crate, my app release process will not be the same as I need to deal with libdbus. As you said it is probably fine, D-Bus lib should be available everywhere, but yet my actual release process would not work. This is what I meant by "pure Rust implementation": to facilitate release creation and distribution. That is also the reason why I'm seeking for flexibilityQ some people might want a pure Rust implem not to deal with external libs (Z-Bus is the only way if using Secret Service), some people might want to use libdbus etc.

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 keyring-rs?

@brotskydotcom
Copy link
Collaborator Author

brotskydotcom commented Jan 9, 2025

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 CredentialApi in fact)?

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.

What if tomorrow the Apple Keychain API becomes async?

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 dbus crate (which happened quite a while ago) didn't affect how the dbus-secret-service crate uses the dbus crate.

If you remove the secret-service crate, my app release process will not be the same as I need to deal with libdbus.

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.

This is what I meant by "pure Rust implementation": to facilitate release creation and distribution.

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 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 keyring-rs?

I believe that your core concerns are captured by the following question:

  • In addition to the current, synchronous form of the CredentialAPI trait, can keyring-rs support an AsyncCredentialAPI trait all of whose entries are asynchronous?

That's a very interesting question, and I believe it has two answers:

  1. Not directly, because Rust itself does not (yet) support traits with asynchronous entries. The reasons for this are rooted in the core design of the language, which was done without specifying a mechanism for asynchronicity. This is exactly why we have both std-async and tokio, and why the tokio infrastructure has had to rewrite large portions of the Rust standard libraries.
  2. Indirectly yes, by writing a version of the CredentialAPI that uses polling (Rust's core mechanism for asynchrony). This is exactly the mechanism that's used by the rust dbus crate, and what allows it to be used by the dbus-tokio crate.

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 tokio-keyring crate, I would be delighted to move that direction in a new major version of this crate!

@soywod
Copy link
Contributor

soywod commented Jan 9, 2025

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).

The frontier is thin between both, because the model carries the non-functional spec by having or not the async keyword its definition. But yes I understand your point.

I don't think that's true. You can simply link dbus statically (using the "vendored" feature).

You totally right, and I forgot about this feature. So the whole "pure Rust implementation" point is resolved by the vendored feature, great news.

Rust itself does not (yet) support traits with asynchronous entries

If I do not mistake, Rust v1.75 brought support of → impl Trait and async fn in traits, see their blogpost.

Indirectly yes, by writing a version of the CredentialAPI that uses polling

Do you mean:

  • to have a single, poll-based CredentialAPI for both sync and async?
  • to have a dedicated poll-based CredentialAPI for async (you end up with a CredentialAPI and an AsyncCredentialAPI)?

Then you end up with the initial Tokio wrapper I use to wrap calls to keyring-rs with tokio::task::spawn_blocking.

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 keyring-rs:

  • Remove OS-specific restrictions, so that all keyrings are accessible via feature gates only
  • Remove keyring compilation restrictions, so that any keyring can be used at runtime
  • Remove Secret Service crypto restrictions, so that any crypto can be used at runtime
  • Expose Secret Service crypto code, so that people could re-use them in their own, custom (D|Z)-bus based keyring (D-Bus tokio, Z-Bus async-std etc)
  • Remove support of the actual Z-Bus based Secret Service, to remain with only sync implems
  • Make the default credential feature optional (enabled by default?), so that people can develop their own default logic at runtime

At pimalaya/keyring-lib

  • Make use of the Sans I/O pattern
  • Link I/O connectors to the ones existing in keyring-rs (Apple Keychain std, Windows Credential std, D-Bus Secret Service std etc)
  • Add more I/O connectors by re-using types, fns and crypto codes from keyring-rs (D-Bus Secret Service tokio, Z-Bus Secret Service tokio/async-std)

This way, we end up with a nice overall keyring support for Rust:

  • keyring-rs brings a solid, generic and cross-platform keyring basement, with std support
  • pimalaya/keyring-lib extends keyring-rs by providing a Sans I/O API, and adds tokio + async-std support (not really for functionalities, but mostly for better integration as discussed earlier in our conversation)

People can still develop their own credential thanks to keyring-rs genericity and pimalaya/keyring-lib Sans I/O implementation.


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 zbus crate is mainly async? It just does not make sense to use Z-Bus for Secret Service?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants