Allow use of both linux-native and secret-service keystores. #215
Conversation
Using both will make the mock keystore be the default, so clients will have to use `new_with_credential` to wrap their platform-specific credentials. Fixes hwchen#214.
src/lib.rs
Outdated
| feature = "sync-secret-service", | ||
| feature = "async-secret-service" | ||
| )) | ||
| any( |
There was a problem hiding this comment.
This condition illustrates well what I tried to explain here: #214 (comment).
It just looks wrong and messy. Also not sustainable: how will it look like in few years, with other new keystores?
There was a problem hiding this comment.
Thanks for the review. I agree this is messy. Unfortunately, in my experience, dealing with edge cases is always messy, and (as I explain further in #214) having more than one keystore enabled on a given platform is a fairly rare edge case. Using features to specify a single (default) keystore per platform allows the vast majority of clients to write platform-independent code.
As to the question about what this will look like if we integrate a whole bunch of new keystores on one platform, I'm not particularly concerned that's going to happen. The only reason the keyutils platform was integrated in the first place was to allow using this crate on headless Linux boxes where the SecretStore is generally not available.
Hope this helps explain my reasoning.
There was a problem hiding this comment.
The only reason the
keyutilsplatform was integrated in the first place was to allow using this crate on headless Linux boxes where the SecretStore is generally not available.
Could you explain a bit more, or give an example? I am not sure to understand.
There was a problem hiding this comment.
The secret service relies on the system to prompt users out of band with their current process, whenever it encounters a locked item or collection. Headless linux installs don't support this, so secret-service can only be used by unlocking the default collection when starting the service (see the docs in the secret-service module for more about this).
Because @landhb and I wanted to have a default keystore available on headless Linux boxes, he contributed and I integrated the keyutils implementation. There was never any expectation that the both would be used at the same time, because if the secret service can be used there's no reason to use the keyutils.
There was a problem hiding this comment.
Understood, thanks for the details. From the keyutils inline doc:
Consider the keyring a secure cache and plan for your application to handle cases where the entry is no-longer available in-memory.
For this particular (and unique) case, having both the keyutils and secret service makes sense (hence #222), right?
There was a problem hiding this comment.
Absolutely! See my review comments on #222, which I think would be a terrific addition to this crate (done as a separate "combo" keystore rather than as a modification of the existing keystore).
|
I propose a lighter version in #220. |
brotskydotcom
force-pushed
the
issue-214
branch
from
92e36c4
to
779ebd4
Compare
1. If both secret-service and linux-native are available, prefer secret-service. 2. Document why both secret-service and keyutils are available on linux. 3. Add CI test for both feature linux-native and feature sync-secret-service.
brotskydotcom
force-pushed
the
issue-214
branch
from
779ebd4
to
9e4e991
Compare
|
Based on discussion in various issues, this PR now makes secret-service the default if both it and keyutils are enabled. |
brotskydotcom
force-pushed
the
issue-214
branch
from
97d693d
to
72150fd
Compare
|
Looks great, I guess it can be merged! |
Using both will make the mock keystore be the default, so clients will have to use
new_with_credentialto wrap their platform-specific credentials.Fixes #214.