- Sponsor
-
Notifications
You must be signed in to change notification settings - Fork 54
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
Invalid compile_error
condition
#214
Comments
I completely agree that this is a bug, Clément, and should be fixed. But I have some question about how to fix it. When I first did v3, I figured I would just have the two secret-service modules be conflicting, and let you combine them with the keyutils store. But that raised the question of which of the two services (secret-service or keyutils) should be the default. I really didn't like the v2 features that let you set the default, so instead I made them all conflict, figuring I would force people to choose which one they wanted. But that eliminates the ability to combine them, as you discovered :). So... shall I just make the mock keystore be the default if both secret-service and keyutils are included, which forces clients to be explicit about which keystore they want? Shall I arbitrarily pick one or the other? I don't think I'm willing to bring back a feature to choose the default, because it feels way too clumsy. Unless you (or other clients) can make a good argument for handling it some other way, I think I will just have the mock keystore be the default when multiple keystores on the same platform are included. Please let me know what you think. |
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.
Why do you need a default keystore? It makes things way more complicated than it should be. You cannot know in advance how people will use your lib. There are too many edge cases: someone could work an a legacy code refactor and may need the sync AND async secret service keystores at the same time, someone may need both async secret service AND keyutils, tomorrow you may integrate a new keystore for MacOS which would bring the same default question etc. The only useful case I see for default keystores is about OS-related ones (Linux keyutils, MacOS Keychain and Windows Credential Manager), but even there I doubt. When I first used your lib, I did not know about the Linux keyutils and its memory-only property. I thought it was similar to the MacOS and Windows keystores until I rebooted my computer. I faced it in production, so did my users at that time. In reality, keyutils does not really compete with the MacOS nor Windows keystores. It even looks like keyutils is the only keystore acting on memory? In my opinion (with my actual understanding of the lib, which may be wrong), forcing users to select keystore(s) they want is the simplest way to go: let keyutils_entry = EntryBuilder::new().with_credential(keyring::macos).build(service, user);
let secret_service_entry = EntryBuilder::new().with_credential(keyring::secret_service).build(service, user); Multiple credentials could be given, acting like fallback/cache/proxy/replicate (a nice API could be designed): let entry = EntryBuilder::new()
.with_strategy(CredentialStrategy::Fallback) // Switch to the next credential in case of failure
.with_credential(keyring::keyutils)
.with_credential(keyring::secret_service)
.build(service, user); Better to propose an API that pushes users to compose their keystores rather than restricting them to use only one default that may not match / be enough. |
Thanks for the feedback! What I have learned from client feedback is that using more than one keystore on a particular platform is very rare (even on Linux, where two are available). With v3, now that clients have to explicitly specify which keystore they want to use, you are the only one I've heard from who has noticed that you can't use more than one together. In cases where a client is using only one keystore per platform, the notion of a While I am intrigued by your EntryBuilder idea, I can't really see the utility of it, and it's not platform independent. (For example the Once again, thanks for your thoughts! |
This is the point, every platform enables its own modules, and
Managing 2 credentials at the same time, for example the Linux keyutils + secret service. My main points are:
What about putting the default thing behind a cargo feature, enabled by default? If disabled, there is no more Side note: do you have any feedback of users using only the Linux keyutils? If so, for which purpose? As I said earlier, my use-case is to have a secret service keystore backed up by a keyutils for cache. Which implies:
|
After sleeping on it, my thoughts became slightly more precise. Trying to propose a common credential API and a default one matching the current OS is what makes things messy. IMO, what misses is an intermediate crate:
It does not mean to be necessary a dedicated crate: cargo features could be enough. I think a |
Hi Clément, and thanks for all your work in and around this issue. I'm sorry I've been quiet for a few weeks, but I was busy with personal matters so I am just now getting back to this. Here are my thoughts:
Please let me know what you think! |
People use Linux keyutils by itself on headless Linux boxes, where the secret service cannot be used because prompting is unavailable and there is no way to unlock the storage. |
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 linux-native and secret-service.
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.
keyring-rs/src/lib.rs
Lines 173 to 181 in 27a7b35
I think the condition is not correct, at least for Linux. It prevents Linux users to combine both the secret service based keystore (for persistent storage, on disk) and the keyutils (for cache, in memory). I actually have a combination of both with your
v2
, and this compile error prevents me to upgrade to thev3
:The text was updated successfully, but these errors were encountered: