Invalid compile_error condition

[soywod]

keyring-rs/src/lib.rs

Lines 173 to 181 in 27a7b35

	#[cfg(all(
	not(doc),
	any(
	all(feature = "linux-native", feature = "sync-secret-service"),
	all(feature = "linux-native", feature = "async-secret-service"),
	all(feature = "sync-secret-service", feature = "async-secret-service")
	)
	))]
	compile_error!("You can enable at most one keystore per target architecture");

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 the v3:

keyring_native = { version = "3", package = "keyring", default-features = false, features = ["linux-native", "apple-native", "windows-native", "async-secret-service"] }

error[E0252]: the name `default` is defined multiple times
   --> /home/soywod/.cargo/registry/src/index.crates.io-6f17d22bba15001f/keyring-3.3.0/src/lib.rs:208:9
    |
197 | pub use keyutils as default;
    |         ------------------- previous import of the module `default` here
...
208 | pub use secret_service as default;
    |         ^^^^^^^^^^^^^^^^^^^^^^^^^ `default` reimported here
    |
    = note: `default` must be defined only once in the type namespace of this module
help: you can use `as` to change the binding name of the import
    |
208 | pub use secret_service as other_default;
    |         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

error: You can enable at most one keystore per target architecture
   --> /home/soywod/.cargo/registry/src/index.crates.io-6f17d22bba15001f/keyring-3.3.0/src/lib.rs:181:1
    |
181 | compile_error!("You can enable at most one keystore per target architecture");
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

[brotskydotcom]

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.

[soywod]

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.

[brotskydotcom]

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 default keystore is incredibly helpful: it gets rid of any need for explicit specification of which keystore one intends to use (already chosen via feature). That's why the original version of keyring only had the default keystore, and had no way of specifying you wanted to use some other one. It was only with the advent of v2 and the ability to bring your own keystore that the notion of a default keystore had to be made explicit, so that someone could specify a different keystore to be used by Entry::new (and the crate itself would know which keystore to use in the absence of that).

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 keyutils and secret_service modules don't even exist in a Mac build.) Can you tell me what use case you have that can't be addressed by using set_default_credential_builder to set the desired default and using new_with_credential to build a non-default credential where needed?

Once again, thanks for your thoughts!

[soywod]

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 keyutils and secret_service modules don't even exist in a Mac build.)

This is the point, every platform enables its own modules, and Credential implementations would need to be explicitly enabled. They are not platform independent because they provide different features. A Linux keyutils cannot be put at the same level with a MacOS Keychain, because they do not persist data the same way.

Can you tell me what use case you have that can't be addressed by using set_default_credential_builder to set the desired default and using new_with_credential to build a non-default credential where needed?

Managing 2 credentials at the same time, for example the Linux keyutils + secret service.

My main points are:

• The current implementation lack of flexibility, implementations should be accessible. Not only behind a static RwLock.
• Mixing up persistent and non-persistent keystores makes things more complicated: while in theory the API of Credential and CredentialPersistence make a lot of sense, in practice there is a huge difference between a persistent (disk) and a non-persistent (memory) keystore.

What about putting the default thing behind a cargo feature, enabled by default? If disabled, there is no more keystore as default. But then the whole build_default_credential would need a refactored. Maybe I could propose a draft PR? It would be simpler to explain what I have in mind.

---

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:

• either to make them both accessible at the same time
• expose a new keystore that uses both secret service for persistence and keyutils for in-memory cache

[soywod]

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:

• One low-level crate could expose CredentialApi (so people can implement their own keystore) and few OS-specific implementations. Such crate would be really simple and flexible: no default store, no conditions; just exposing a common keystore API and default implementations (that could remain behind feature gates to reduce dependencies).
• One higher-level crate could expose default credentials matching OS. Such crate would be intended for people who do not care about implementations and just need a working keystore for their current OS.

It does not mean to be necessary a dedicated crate: cargo features could be enough. I think a default-keystore feature could work as well. Would you like me to propose a PR, so we can discuss on it? It should not break anything and could even remain in the actual v3.

[brotskydotcom]

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:

1. Your work on Provide keyutils with persistence-after-reboot using secret-service #222 is very interesting, and I've left you a lot of review comments to think about. Let me know if you're interested in continuing to work on that as a separate module that makes use of both secret service and keyring.
2. Your Improve secret service and keyutils compile error condition #220 doesn't quite work (see comments on that PR), and I don't see any advantages it has over Allow use of both linux-native and secret-service keystores. #215, so I'm inclined to go with Allow use of both linux-native and secret-service keystores. #215 instead (but see next comment).
3. Between your comments on Allow use of both linux-native and secret-service keystores. #215 and your work on Provide keyutils with persistence-after-reboot using secret-service #222, you have convinced me that it's pointless to have the mock keystore be the default if someone has enabled both secret-service and keyutils, both of which offer more functionality. So I'm going to make secret-service the default in that case, since it offers persistence beyond reboot. Once I make this change, I think Allow use of both linux-native and secret-service keystores. #215 really just becomes an elaboration on your Improve secret service and keyutils compile error condition #220, with better commentary.

Please let me know what you think!

[brotskydotcom]

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:

• either to make them both accessible at the same time
• expose a new keystore that uses both secret service for persistence and keyutils for in-memory cache

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.