hwchen / secret-service-rs

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

Source on main is at version 3, crates.io is still at version 2.0.2? #62

Closed brotskydotcom closed 1 year ago

brotskydotcom commented 1 year ago

@complexspaces I'm finally getting around to the next release of the keyring-rs crate, and I'm trying to figure out which released version of secret-service it should use. I am going to stay with dbus (rather than zbus) because I don't want to force my clients to use an async runtime, so in that sense I could stay with 2.0.2, but then again I'd like to include the unlock improvements of a269ee7 by @jkhsjdhjs, which are only in 3.0. Are you going to release 3.0 to crates.io anytime soon? And will I be able to use feature flags to stay with the dbus interface in 3.0?

cc @Sytten, @landhb

Sytten commented 1 year ago

Thing is v2 still uses zbus just the sync api which spawns a single thread tokio executor to call the async api. Even worst it pulls in by default both tokio and async-io runtimes... Last time I looked at it, my conclusion was that the secret service crate should have two implementations, one sync that maybe uses the dbus crate (C wrapper) and one async interface that uses zbus (or dbus-tokio).

I just wish zbus would have decoupled the transport from the dbus implementation (similar to what diesel did with diesel-async).

Right now even if you dont want to impose an async runtime @brotskydotcom you kinda do because it will use tokio under the hood. Though I agree that keyring should remain sync. One could really wish for an STD runtime that would just block on async code 😑

brotskydotcom commented 1 year ago

Wow, thanks @Sytten for laying that out for me. As you may recall, I don't spend a lot of time in Linux-land so this is really helpful education for me.

Does this mean that the current keyring-rs release is also causing its clients to pull in asynchronous runtimes ("under the hood", as we say in the US)? So whether I use v2 or v3 of secret service in the next release of keyring makes no difference in that respect?

Sytten commented 1 year ago

I am not super versed in Linux either but glad I can help with the async /sync stuff.

Exactly it pulls in async runtimes under the hood (zbus does not this crate) since its the only way to run async from sync code.

I believe that v3 is fully async so would have to do this yourself if you want to keep a async api for keyring.

brotskydotcom commented 1 year ago

@complexspaces Are you open to a contribution (under feature flag, I would expect) that uses dbus rather than zbus? (I seem to remember you suggested as much in some other thread.)

Sytten commented 1 year ago

Unsure if it is a good idea re-reading the code since literally all the api is async now so that would basically mean forking from v2 and writing another lib...

I guess you could use the futures crate single threaded executor (https://docs.rs/futures/latest/futures/executor/fn.block_on.html) but it isn't working super well for people calling from tokio I think (https://stackoverflow.com/questions/66035290/how-do-i-await-a-future-inside-a-non-async-method-which-was-called-from-an-async).

landhb commented 1 year ago

I think a possible course of action is:

That allows downstream users of kerying-rs to not need an async runtime or have libdbus-1-dev installed to compile keyring-rs.

I started the dbus/libdbus vendoring work in a branch here. Just to see if it's viable.

Edit, draft PR here: https://github.com/diwic/dbus-rs/pull/408

complexspaces commented 1 year ago

Hi everyone, sorry for the delay. Let me try and answer everyone's questions:

Are you going to release 3.0 to crates.io anytime soon? And will I be able to use feature flags to stay with the dbus interface in 3.0?

As I think was discovered above, no, there are not currently a set of feature flags that will let downstream users use the libdbus C library (without any Rust async runtimes). Regarding the 3.0 release, everything is ready to do so. There aren't any technical blockers, I just got cold feet (sorry 😞) and then ran out of time to balance with.

If there are no breaking changes required/planned for the proposal to add another backend, then my plan is to release 3.0, merge some of the cryptography updates (at work, we're stuck on some older versions and I would prefer to have a 3.0 release for use internally that doesn't duplicate a bunch of dependencies) and then release 3.1 with those.

I am currently out of town until January for vacation, so I won't be able to do much more then comment on GitHub until then sorry. But once I'm back, a 3.0 release would take 1-2 days at most.

Are you open to a contribution (under feature flag, I would expect) that uses dbus rather than zbus? (I seem to remember you suggested as much in some other thread.)

re-reading the code since literally all the api is async now so that would basically mean forking from v2 and writing another lib...

Yes, I am still open to contributions adding this. Given that the crate now has a blocking module, I imagine that this would be the best place to plug in the alternate backend. @Sytten this should also hopefully resolve your concern about there being no synchronous API remaining, right? Instead of blocking on async tasks, it would use truly blocking I/O via libdbus instead.

If possible, it would be nice to give users of the keyring-rs crate a way to opt-out of the C backend, primary so that having the crate in your tree doesn't force any independent use of secret-service-rs to use libdbus too. That may be a nicety for anyone who already has Tokio or async-std in-tree and wants to avoid a C dependency.

brotskydotcom commented 1 year ago

Hi all,

Thanks so much for the education and context. I think I have a pretty good picture of the situation right now, and here's what I'm planning. All feedback welcome!

  1. As soon as I finish testing, I'm going to release a new alpha of keyring-rs that uses the blocking API of the current main branch of secret-service. There will be features, analogous to those in secret-service, that control which async runtime gets used and which encryption mechanism gets used, with the default being async-io and crypto-rust.
  2. The new alpha will have a feature to disable the use of secret-service completely (so that none of zbus or its dependencies are linked at all). Clients who disable secret-service can use the linux keyring as their credential store (or bring their own).
  3. Once secret-service 3.0 is released, I will release an rc of keyring-rs 2.0 that uses it.
  4. If anyone gets around to adding a secret-service feature that links the blocking API (or both APIs) to dbus rather than zbus, I will expose that feature in a single-dot release of keyring-rs (assuming the dbus-version is blocking-API-compatible with the zbus version).

This plan seems to me to balance backward-compatibility for keyring-rs (for async-naive clients) with sufficient controls over zbus behavior (for async-aware clients) with sufficient controls over dependencies (for embedded or otherwise zbus-averse clients). And it leaves the door open for an API-compatible release of keyring-rs if anyone ever moves forward with a dbus-based secret-service.

Your thoughts?

landhb commented 1 year ago

After merging https://github.com/diwic/dbus-rs/pull/408, I began trying to get a start on updating the secret-service crate to use the dbus crate for its blocking interface.

It's looking like a larger lift than I originally anticipated. Since most of the core library is tightly coupled with zbus.

Ideally the secret-service Cargo.toml would end up looking something like the following, to make zbus fully optional:

[features]
# Default to using rust crypto backend, force user to choose a Mode
default = ["crypto-rust"]
# Cryptography backends
crypto-rust = ["dep:aes", "dep:block-modes", "dep:sha2", "dep:hkdf"]
crypto-openssl = ["dep:openssl"]
# Modes (Blocking or specifying a runtime)
# The async runtime features mirror those of `zbus` for compatibility.
blocking = ["dep:dbus"]
rt-async-io = ["zbus/async-io"]
rt-tokio = ["zbus/tokio"]

[dependencies]
# Crypto: Rust backend
# TODO: Update these when Rust 1.56 isn't so new.
aes = { version = "0.7.0", optional = true }
block-modes = { version = "0.8.0", optional = true }
hkdf = { version = "0.12.0", optional = true }
sha2 = { version = "0.10.0", optional = true }
# Crypto: Openssl backend
openssl = { version = "^0.10.40", optional = true }
# Mode: Blocking/sync backend
dbus = { git = "https://github.com/diwic/dbus-rs.git", features = ["vendored"], optional = true}
# Mode: Async backend (zbus will pull in the relevant run time)
zbus = { version = "3", default-features = false, optional = true}
...

And then the top-level lib.rs looking something like:

/// Error when a runtime has not been selected
#[cfg(not(any(
    feature = "rt-async-io",
    feature = "rt-tokio",
    feature = "blocking",
)))]
mod error_message {
    compile_error!(
        "A Mode must be selected, either \"blocking\", \
        \"rt-async-io\" or \"rt-tokio\" must be enabled."
    );
}

/// Error when a crypto backend has not been selected
#[cfg(not(any(
    feature = "crypto-rust",
    feature = "crypto-openssl",
)))]
mod error_message {
    compile_error!(
        "A crypto backend must be selected, either \
        \"crypto-rust\" or \"openssl\" must be enabled."
    );
}

// Generic modules
mod error;

/// Async mode
#[cfg(any(feature = "rt-async-io", feature = "rt-tokio"))]
pub mod runtime;

/// Sync mode
#[cfg(feature = "blocking")]
pub mod blocking;

So I think that your point number 4 may run into issues, since I anticipate potential breaking changes to occur within secret-service when moving all the async code into a separate module. Since all the zbus dependent code will need to be feature gated.

And a lot of the core code, like the Proxy/Session/Error definitions are defined using zbus macros and types.

landhb commented 1 year ago

Actually it may not affect users of keyring aside from removing the runtime features you're exposing.

When/if the swap happens under the hood in secret-service.

So the breaking change would be exposing the runtime features now and later removing them.

Your point 2 is definitely a plus for people trying to avoid a runtime entirely.

brotskydotcom commented 1 year ago

Thanks @landhb for the feedback on number 4. I am planning to make introducing a new credential store into keyring-rs fully backwards compatible (with respect to the client API) and thus something that can be done in a dot release. Assuming I succeed, then: if the secret-service blocking API were to change a lot with the move to dbus, or if we needed to split dbus support into a separate sub-module completely, I could handle it in keyring-rs simply by introducing a new credential store that used the new API.

What this does make me realize, however, is that I probably should make the error enum returned by keyring be explicitly non-exhaustive, so that new credential stores can introduce new error types without breaking the clients.

brotskydotcom commented 1 year ago

I have published keyring-rs v2 alpha 4 to GitHub so folks can try it out. It can't be published to crates.io until after secret-service v3 is published.

brotskydotcom commented 1 year ago

Hi @complexspaces, Happy New Year! Just checking in on this. It would be great to have a 3.0 release; that will allow me to release keyring 2.0. If you think it will be a while before you do the 3.0 release, that's fine! Just let me know and I will release keyring 2.0 against secret-service 2.0.2 and then update it in a dot once 3.0 comes out.

brotskydotcom commented 1 year ago

I've reverted back to secret service 2.0.2 so I can release keyring-rs v2. I just pushed beta.1 to crates.io. I know exactly what's needed to move back to secret service 3.0 so I can do a dot whenever that happens.

complexspaces commented 1 year ago

Hey @brotskydotcom, I'm really sorry for the silence. This month has been far rougher then I expected. I've had to slow down on many non-primary obligations.

Given some of the research done above (thank you everyone), I think that we should be good to cut v3 and keep it for a while. The dbus crate offers a non-blocking backend as well so I still believe adding it as an alternative to zbus should be possible without breaking changes. Even if that turns out to be wrong, hopefully a v4 release won't be that painful for downstream users.

I will do my best to have v3 out by the weekend. Please speak up in the next day or so if you have any issues with the current API.

brotskydotcom commented 1 year ago

Hi @complexspaces, wonderful news and thanks for all your effort! If you can do a release in the next week I will happily use it in keyring 2.0.

complexspaces commented 1 year ago

I've now published 3.0 on crates.io. Please let me know if anything about it ended up not working (hopefully nothing! 😅).

Regarding the dbus-powered backend, I'm still open to accepting a PR for this. I'm going to close this issue, since crates.io now matches master, and point discussion of that topic to https://github.com/hwchen/secret-service-rs/issues/64 instead.

brotskydotcom commented 1 year ago

Thanks much @complexspaces for doing this release! I will move keyring-rs onto it and release this week.