Configurable keyring backends#278
Conversation
This refactors `warg_client::keyring` to make everything a method of a `Keyring` struct which can support multiple `CredentialBuilder`s. Currently, all CLI commands still just use the system default everywhere, but next, a config option can be added to customize it.
dfoxfranke
force-pushed
the
feature/pluggable-keyring
branch
from
8a7570a to
01e3d40
Compare
Create a `KeyringError` type providing structured information about what went wrong. In the case of a platform error when using the secret-service backend, provide a tip on how to remedy it.
|
I'm marking this ready for review. The |
| pub type Result<T, E = KeyringError> = std::result::Result<T, E>; | ||
|
|
||
| impl Keyring { | ||
| #[cfg(target_os = "linux")] |
There was a problem hiding this comment.
There was a problem hiding this comment.
I didn't think it worth pulling in the dependency just to use it in this one place where it barely provides any simplification. But if it gets pulled in later for wider use then, yes, this should be refactored.
| if let Some(ref backend) = config.keyring_backend { | ||
| Self::new(backend.as_str()) | ||
| } else { | ||
| Self::new(Self::SUPPORTED_BACKENDS[0]) |
There was a problem hiding this comment.
| Self::new(Self::SUPPORTED_BACKENDS[0]) | |
| Self::new(Self::DEFAULT_BACKEND) |
There was a problem hiding this comment.
Calvin already merged, so I'll fix this in my next PR.
| pub const DEFAULT_BACKEND: &'static str = Self::SUPPORTED_BACKENDS[0]; | ||
|
|
||
| /// Returns a human-readable description of a keyring backend. | ||
| pub fn describe_backend(backend: &str) -> &'static str { |
There was a problem hiding this comment.
Would it be difficult to make backend an enum?
There was a problem hiding this comment.
I considered this but decided it would create more problems than it would solve. The set of supported backends is platform-dependent, so there would be a distinction between the enumeration of all understood backend identifiers, and the subset of those which are supported on the user's platform. So there would be a design trade-off between:
- Defining two separate enums, one which covers all backends and one which covers those which are supported on the platform.
- Defining only the supported enum, and needing a bunch of conditional attributes every time you want to match on it.
- Defining only the all-backends enum, and putting an
is_supported(&self) -> boolmethod on it.
Then we have to decide how exactly we want our Serialize and Deserialize implementations to work and which type they should use, and probably put custom error reporting into this traits to not be too cryptic if the user copies a configuration file from one system to another where the backend isn't supported. Altogether it didn't seem worth it, so I just kept this stringly typed.
|
Thank you! This looks great. |
This adds support for storing warg credentials as flat files in the warg config directory, following up on #278.
4 participants
Makes keyring backends user-selectable, as discussed in #271 (comment).
client::keyringfunctions into aKeyringstruct which can be instantiated with a backend selection.secret-servicebackend isn't working.flat-filebackend that stores keys in plaintext alongside the config file.