Provide keyutils with persistence-after-reboot using secret-service

[soywod]

This PR makes both async and sync secret service keystores using keyutils as cache, when available (on Linux only).

Since you mentioned that rare are users needing both secret service and keyutils, I assume the following statement: if a user enables both secret service and keyring, it implies that the user wants to benefit from the Linux secure cache. To disable cache, just remove the feature.

[soywod]

Requires #220 for the CI not to complain about dual keystores.

[brotskydotcom]

Hi Clément, thanks for this! It's a very interesting idea. I have reactions at three levels:

At the highest level, I think this captures very nicely the idea you were talking about in one of your comments, where you talked about having one credential store be primary and then another be fallback. That idea is really independent of credential store, and could be a separate crate that uses keyring to provide the credential stores.

At an intermediate level, this feels like it should be structured as a separate credential store module (and feature) that relies on (and requires) both secret-service and keyutils. Rather than thinking of it as "secret-service with keyutils cache," I tend to think of it as "keyutils with fallback to secret-service for persistence across reboots," because that's how it would work in practice (as has been noted in a number of closed issues against this crate, for example see #168).

At the lowest level, I think there are some logic changes that would be needed to really make this robust, in particular one would need to guard against the two stores going "out of sync" with one another as a result of separate access.

If you are willing to turn this into a separate credential module, and fix the out-of-sync issues, I would be delighted to include it as part of keyring.

[soywod]

At an intermediate level, this feels like it should be structured as a separate credential store module (and feature) that relies on (and requires) both secret-service and keyutils. Rather than thinking of it as "secret-service with keyutils cache," I tend to think of it as "keyutils with fallback to secret-service for persistence across reboots," because that's how it would work in practice (as has been noted in a number of closed issues against this crate, for example see #168).

I thought about it, I hesitated between the actual simpler (naive) solution and this one. What stopped me to implement the late one is the complexity around: how to name such store? how to deal with cargo features (sync/async secret service with/without keyring)? it brings even more conditions etc. I am not against, but knowing the low need of such feature we could start simpler, by just adding cache to secret service. It can be put behind a feature gate as well.

At the lowest level, I think there are some logic changes that would be needed to really make this robust, in particular one would need to guard against the two stores going "out of sync" with one another as a result of separate access.

Same here. While I am not against (because it makes sense), I find overkill to fine-tune the feature now knowing that it's an edge case of an edge case.

IMHO, better to start simpler and improve over time with users' feedback. What do you think?

[soywod]

After all, both ideas can be complementary, as they serve different purposes:

• A dedicated store for secret service + keyutils makes sense if you want to have a solid, safe and secure cache system. Integrity matters, in this case I agree with all your comments.
• A keyutils as light cache system for secret service makes sense in opportunistic way: if you already asked for a password that is in the cache → great, use it, otherwise it's fine → use secret service

Since nobody needed both secret service and keyutils (nor such cache feature) before, it makes sense to start first with the light one.

[brotskydotcom]

Thanks for your thoughtful comments; they make a lot of sense.

I absolutely agree that keeping things simple to begin with is the way to go. It doesn't need to be super robust as long as we document what the behavior is.

I agree there's a real need to have a standard way for keyring to support backing the keyutils with persistence. And I think that fills a real need for people who have systems they run as headless servers for the most part (relying on keyutils) but then can log into via UI to configure (via secret-service).

I still think this is best modeled as a separate linux-only keystore (perhaps called "linux-native-persistent") that is an exclusive alternative to both keyutils and secret-service. Because both the keyutils and dbus-secret-service modules are optional dependencies, we can have the "linux-native-persistent" feature enable both of those modules, and then simply use them as a client (using the pattern you've established in your existing PR). That way we don't complicate the existing keystore code and we give all users an easy way to use the double-keystore pattern that you are using.

Is this an approach that might work for you?

[soywod]

Maybe linux-native makes even more sense than linux-native-persistent: it aligns with other apple-native and windows-native (providing a persistent store). keyutils and {sync,async}-secret-service could still be used independently if needed. Let me update the PR tomorrow morning so we can discuss on it. Thank you!

[soywod]

I had hard time to deal with this new keystore with the actual cargo features management. As I expected, it became a mess of conditions in order to make default keystore exclusive.

I also discovered the potential reason behind the default keystore confusion: the word native implies that the current store is available, by default, on the target machine. linux-native(-persistent) is not really native because it depends now on other things like secret service or dbus.

In order to go forward, I suggest the following ideas:

• *-native cargo features stand for their meaning: available natively, without any other deps, on the target OS:
  • linux-native implies keyutils
  • apple-native implies security-framework
  • windows-native implies windows-sys
• default-* cargo features stand for persistent keystore that should be use by default on the target OS:
  • default-linux implies the new secret service + keyutils store
  • default-linux-headless implies the keyutils only (the only one not persistent)
  • default-bsd implies the secret service only
  • default-apple implies security-framework
  • default-windows implies windows-sys

default-linux and default-bsd need an explicit {sync,async}-secret-service feature to work, this way it prevents duplicate cargo features like default-linux-{sync,async}-secret-service and it makes compilation conditions easier to understand.

[brotskydotcom]

This is starting to look very good, thanks for working on it! Some notes:

1. WRT your suggested feature renames, changing existing feature behavior is a SemVer-incompatible change, and I would like to keep this change to a backwards-compatible enhancement (dot) release. So for now let's pick a new name for the feature controlling this keystore and just add it alongside the existing ones as an exclusive option. I know this makes the cfg logic even deeper on the *nix side, but that's purely an internal issue that we can manage and doesn't show to clients.
2. I like where you're going with the idea about -default vs -native features, and will keep track of it for the next major release. It also suggests to me that we don't need the -native feature set at all, because we can just include those keystores based on platform rather than feature, so maybe this will all get a lot simpler in v4 :). After all, there is only 1 platform (*nix) where there's a choice of keystores...
3. I don't see any reason to support both sync and async secret-service usage in this brand new keystore. Just supporting the dbus-secret-service will provide a lot of advantages: (a) no need to conditionalize a bunch of code, (b) much simpler feature logic (just have this feature rely on keyutils-rs and dbus-secret-service directly and prevent use of other *nix keystores), (c) much slimmer dependency footprint.
4. I think we should consider altering (or allowing the client to alter) one or more of the target, user, and service values that this code passes to keyutils and secret-service in order to avoid conflicts with legacy code that was already using those services for their passwords. But then again maybe we want this code just to overtake all those old entries if they exist?
5. When get_secret gets a miss in keyutils and then a hit in secret-service, it should repair the cache with set_password on the keyutils entry. If we do decide to overtake old entries with the same values, then perhaps the same should be done if we get a hit in keyutils but a miss in secret-service.

[soywod]

3. I don't see any reason to support both sync and async secret-service usage in this brand new keystore. Just supporting the dbus-secret-service will provide a lot of advantages: (a) no need to conditionalize a bunch of code, (b) much simpler feature logic (just have this feature rely on keyutils-rs and dbus-secret-service directly and prevent use of other *nix keystores), (c) much slimmer dependency footprint.

And what if someone already relies on zbus (it may be my case within my app, I need to check) and want to have keyutils + zbus secret service in order to avoid having both dbus and zbus as dependencies?

4. I think we should consider altering (or allowing the client to alter) one or more of the target, user, and service values that this code passes to keyutils and secret-service in order to avoid conflicts with legacy code that was already using those services for their passwords. But then again maybe we want this code just to overtake all those old entries if they exist?

I think it's fine to overtake old entries, at least for now. Let's wait for users feedback?

5. When get_secret gets a miss in keyutils and then a hit in secret-service, it should repair the cache with set_password on the keyutils entry. If we do decide to overtake old entries with the same values, then perhaps the same should be done if we get a hit in keyutils but a miss in secret-service.

Nice catch, I take care of it tomorrow morning.

---

I don't get the reason of the CI failure, nothing changed except for cargo features. Any idea?

[brotskydotcom]

Wrt using dbus-secret-service with tokio and zbus: I have tested that thoroughly and there's no issue there. I'd really like to keep the footprint as light as possible and I don't want to have multiple features for this new store, so let's stay with just the one.

Not sure about the test failures, often they fail once randomly but when I ran them again they failed at the same test. Once you finish your changes we can take a closer look.

[soywod]

I just pushed changes. Somehow, by keeping the secret-service feature guard it makes conditions lighter. Plus it enables the ability to use both keyutils with zbus secret service (which is still a plus for me at application level, I don't want to rely on external lib like libdbus). And it looks like it does not break the current API, do you confirm?

2. I like where you're going with the idea about -default vs -native features, and will keep track of it for the next major release. It also suggests to me that we don't need the -native feature set at all, because we can just include those keystores based on platform rather than feature, so maybe this will all get a lot simpler in v4 :). After all, there is only 1 platform (*nix) where there's a choice of keystores...

I thought about this, but native features are still convenient for testing purpose (for example to force the mock store). It joins a bit the idea I proposed last time about having the whole default keystore system behind a feature gate (on by default).

Native features also allow apps to choose on which OS they want to be compatible: if you have an app that works only on Linux and MacOS, you can omit the windows-native which will make the crate not buildable on Windows (better that still building but app failing at runtime).

5. If we do decide to overtake old entries with the same values, then perhaps the same should be done if we get a hit in keyutils but a miss in secret-service.

If we get a hit in keyutils then it returns straight without hitting secret service: the role if this cache is to prevent as much as possible to call the slower secret service. It's part of the opportunistic approach I was explaining, it's fine (at least with this particular store) if keys are not sync.

[soywod]

The cargo feature condition you were proposing did not work, so I adjusted them. Ready for the next review!

[brotskydotcom]

Excellent! One last feature name change (or three if you like my suggested change to the top-level features names) and adding logging and the content is done!

Now we need documentation and unit tests. You should pretty much be able to crib the tests from the keyring crate. If you can take a first pass at the docs - I suggest keeping them simple and just referring to the docs of the other crates if necessary - I am happy to do copy editing in a review.

[soywod]

I am happy to do copy editing in a review

Sure, maybe it is better this way as I may not word properly things. You can also push a commit directly on my branch, as you wish.

Ready for another review.

[brotskydotcom]

Almost! You lost a config test that's required for non-linux systems. I've called it out. (actually, I didn't before, I will add it below.)

What do you think about adding logging for all the error conditions that aren't returned. For example, when you miss in keyutils we would log that happened even if you then hit in secret-service. As long as we have logging I think it would be good to be transparent in this way.

[soywod]

Not sure to understand what's wrong with the CI. Feel free to push commits on the branch if needed for the merge, I will check tomorrow morning.

[brotskydotcom]

Don't worry about the test failures until we have unit tests and docs. Then, if necessary, I'll pull it and look at it.

[soywod]

I initiated doc and unit tests, feel free to adjust at your convenience.

[brotskydotcom]

Great, thank you!! I will be making tweaks for a while by pushing commits to your repo - feel free to review them. Then once we agree it's done I'll merge it.

[brotskydotcom]

OK, I've updated docs and tweaked a few things. Please take a look and try it out and let me know if you think we are ready to pull this.

[soywod]

Thank you for fixing typos. Looks good to me, ready to merge!

[soywod]

Could I ask you to release a new version as soon as you merge? It will allow me to release my versions as well.