Version 2: plug-in credential providers, and mocking

[brotskydotcom]

OK, it's time for a v2 that addresses the issues raised in #94 and #95 (and provides the ability for clients to address the issues in #76 and #84). Here are the enhancements needed:

1. The ability for 3rd parties to provide new credential storage mechanisms, and for clients to choose those as their default mechanism.
2. Support for a "mock" credential store that allows controlling both return and error values. Preferably the mock store should also be an example of how you plug in a new storage mechanism.

These enhancements should not complicate the existing (very simple) Entry model, so that existing clients don't have to revise their code.

While we're at it, we should clean up the API so that creating entries can fail. This will be a breaking change, but one that existing clients can adapt to simply by adding expect calls to their existing entry creations.

[brotskydotcom]

Here are some design notes:

1. Platform credential stores now work by supporting two traits: the CredentialAPI (for operating on entries) and the CredentialBuilderAPI (for creating new entries).
2. The platform-independent Error class has been extended to handle parameter errors other than just "too long".
3. All the new traits are Sync and Send so that the interface is both thread-compatible and thread-safe.
4. The default platform providers have been rebuilt as pluggable credential stores.
5. Entries use default platform providers unless a different default provider is requested (and provided) by the client.
6. The Entry::new_with_credential API allows specifying a 3rd part credential for the entry.

Docs are still in process.

[landhb]

One note I noticed on the interface. Instead of having a mutable global that requires users to call:

// set the proper backend
keyring::set_default_credential_builder(Box::new(LinuxNativeCredentialBuilder));

// start an entry
let entry = Entry::new("myservice", "keyid-2").unwrap();

Could we instead add another Entry method, which users could use like:

Entry::new_with_builder(&LinuxNativeCredentialBuilder, "myservice" , "keyid-2")?;

The implementation could be:

/// Create an entry for the given target, service, and username.
/// The default credential builder is used.
pub fn new_with_builder(builder: &CredentialBuilder, service: &str, user: &str) -> Result<Entry> {
    let credential = builder.build(None, service, user)?;
    Ok(Entry { inner: credential })
}

That then removes the need for the mutable global and makes the call thread safe.

[landhb]

For the mock usage, users could do something like:

#[cfg(test)]
let entry = Entry::new_with_builder(&MockBuilder, "myservice" , "keyid-2")?;

#[cfg(not(test))]
let entry = Entry::new("myservice" , "keyid-2")?;

[brotskydotcom]

Hi @landhb it's not clear to me what advantage new_with_builder has over the existing new_with_credential. If someone has their hands on a desired credential builder, then they can just call it directly and pass the result to new_with_credential rather than having the crate make that call for them. (Another advantage of new_with_credential is that it allows the client to control what arguments are given to the credential builder, for example to build credentials that use different naming conventions.)

As I tried to make clear in the docs, set_default_credential_builder is for use by clients who are "bringing their own" credential builder. The client is expected to make the call early in their initialization sequence before there is any competition from other threads. While I could do a more robust job of synchronizing the set_default_credential_builder call against Entry::new calls, I really don't want to, because that might suggest it was intended to be used in the middle of a session.

Please let me know if this doesn't address your concern properly, or if you think the documentation needs to be improved.

[Sytten]

I like the new trait based approach, but dealing with the new async interface for secret service will be challenging. There is not currently a good answer except spawning a tokio runtime for the blocking call I am afraid (https://tokio.rs/tokio/topics/bridging). Meaning you also need to support different features for each too :/

[landhb]

@brotskydotcom new_with_credential definitely fits that need as well. My thought process was if the library user can use the Entry API with their own builder then there may not be a need to allow them to override the global behavior. But if that option is desired it's possible to also use something like once_cell to ensure it's only set once. Or just wrapping it in a mutex:

const DEFAULT_BUILDER: Mutex<EntryBuilder> = Mutex::new(EntryBuilder {inner: None});

/// Set the credential builder used by default to create entries.
///
/// Use `entry_with_credential` if you are using multiple credential stores
/// and want precise control over which credential is in which store.
pub fn set_default_credential_builder(new: Box<CredentialBuilder>) {
    if let Ok(mut guard) = DEFAULT_BUILDER.try_lock() {
        guard.inner.replace(new);
    }
}

I'm not super against any option on this front, just thought I'd throw the idea out there. The docs seem clear to me.

@Sytten For the async discussion I definitely agree with @brotskydotcom and his comment here. On MacOS, Windows, and with Keyutils it seems all the platform APIs boil down to a single syscall to the OS. And they don't provide non-blocking options that you can then poll to wait for completion on. So wrapping them in an async method isn't really gaining you anything. Any async applications will need to block a thread on the call anyways. The exception is secret-service. And I'm not sure forcing users of this library to bring an async runtime in when they aren't using any async code is beneficial.

[Sytten]

Well I would not be against changing the underlining library but its a big task since you would also need to recreate a dbus library that is sync. That is why I suggested to use a single thread executor. I wish the standard library had a basic executor for this kind of situation.

[landhb]

Is the secret-service crate fully removing their blocking interface?

https://github.com/hwchen/secret-service-rs/tree/master/src/blocking

[Sytten]

No but they rely on the underlining dbus sync implementation which does what I said above aka spawn a single thread executor to run the async task.

[brotskydotcom]

@landhb I thought about using a mutex but I didn't particularly want to slow down entry creation by forcing it to obtain the mutex to read the default builder each time. But now that I've written that down I realize it's pretty silly: how long would that take :).

[landhb]

@Sytten would this not work for async applications using keyring-rs?

use keyring::{Entry, Error};

fn get_credential() -> Result<String, Error> {
        // This call blocks in zbus::blocking
        let entry = Entry::new("test", "test2")?;

        // This call blocks in zbus::blocking
        entry.get_password()
}


#[tokio::main]
async fn main() {
        // Obtain the credential
        let creds_future = tokio::task::spawn_blocking(|| {
                // This is running on a blocking thread.
                // Blocking here is ok.
                get_credential()
        });

        println!("Got {:?}", creds_future.await);
}

Just trying to understand the issue since I don't normally use a lot of async code. It seems it's okay to have block_on calls within a spawn_blocking call, according to:

https://github.com/tokio-rs/tokio/discussions/3717

And it seems the async_io::block_on method zbus is using doesn't spin up a new thread but just blocks the current one:

https://docs.rs/async-io/latest/async_io/fn.block_on.html

So the keyring::Entry API could remain fully sync. And async users can just spin up a blocking task within their existing runtime.

I'm curious what this forces on non-async users though. Since non-async users won't have an existing runtime.

[landhb]

Ah I see what you mean. With the tokio feature of zbus, a new single-threaded runtime is created (but for the current thread): https://github.com/zeenix/zbus/blob/main/zbus/src/utils.rs#L50

I'm not sure what will happen for non-async code with the async_io version: https://github.com/zeenix/zbus/blob/main/zbus/src/utils.rs#L33

[Sytten]

Apart from exposing an async interface this is usually the way it is done yes.

Its a bit annoying since it spawns a lot of single thread runtimes in the background but there is no way around this.

It will force non async users to choose a runtime and this is why I thought you wanted to eliminate dependency over zbus. Ideally zbus would have created interfaces for the backend they use (sync or async) similar to what diesel orm is doing but since they decided to go full async and that STD doesn't offer a basic runtime you must use a runtime one way or another.

[landhb]

Yeah I agree that ideally the sync/blocking interface shouldn't require a runtime.

[brotskydotcom]

OK, I have finished everything for this release other than updating to the new version of secret service, which I will start on now. Feel free to download and use the alpha.3 that I published this morning (that still uses secret-service v2): all feedback welcome.

[complexspaces]

FWIW, and I also noted this here, but I'd be willing to accept a different backend for the synchronous API in secret-service under a feature flag, if anyone is interested in that option to utilize in this crate.

[Sytten]

That could work. The dbus-rs crates (that binds to C) offers a real blocking interface and doesn't require an async runtime so that would be a good candidate for an alternate backend. And it also offers an async interface so we dont have to break the async interface of secret service when using that flag.

[complexspaces]

That sounds reasonable to me. I didn't even know it had an async interface 😄.