feat(init-wallet-enc): optionally create enc identity file#320
feat(init-wallet-enc): optionally create enc identity file#320distractedm1nd wants to merge 2 commits into
Conversation
str4d
left a comment
str4d
left a comment
There was a problem hiding this comment.
Initial feedback. Depending on how the UX ends up shaping out for configuring the different possible alternative wallet encryption mechanisms (including plugins #252), it may be better to have a dedicated CLI method for each, and remove init-wallet-encryption (instead doing what it does now inside each of the dedicated methods). But we can start by prototyping the combined UX inside init-wallet-encryption.
| let sk = age::x25519::Identity::generate(); | ||
| let pk = sk.to_public(); | ||
|
|
||
| // Q: should we also include timestamp? | ||
| writeln!(out, "# public key: {}", pk).map_err(|e| ErrorKind::Init.context(e))?; | ||
| writeln!(out, "{}", sk.to_string().expose_secret()) | ||
| .map_err(|e| ErrorKind::Init.context(e))?; |
There was a problem hiding this comment.
Instead of this, what I should do is expose the file-generator code from inside rage-keygen somewhere under age::cli_common for reuse. Then it would also benefit from translations.
There was a problem hiding this comment.
tried to tackle this for a bit but gave up - moving the generator (where the localized strings are found) into cli_common means moving those strings over to age/i18n, and adding chrono as a dependency. Is this what you meant?
Exposing instead only the file creation might be an option? But then we don't really gain much at all from doing so.
I've updated the code now without making a PR to rage - maybe it'll help thinking about what to expose upstream.
| .map_err(|e| ErrorKind::Init.context(e))?; | ||
|
|
||
| info!("Encryption identity file created at {}", path.display()); | ||
| info!("Public key: {}", pk); |
There was a problem hiding this comment.
The recipient is irrelevant to Zallet users.
| info!("Public key: {}", pk); |
| pub(crate) struct InitWalletEncryptionCmd { | ||
| /// Creates the encryption identity file if it does not already exist. | ||
| #[arg(short)] | ||
| pub(crate) create: bool, |
There was a problem hiding this comment.
Instead of this being a bool, we should take an enum with variants corresponding to both unencrypted (what is currently implemented) and passphrase-encrypted, so we cover both of the currently-documented use cases.
|
@str4d can you please create an issue for the follow-up work that will be needed to support plugins? |
|
I summarised what needs to happen here: #252 (comment) For now, this PR can be reviewed and merged as-is, and we can then iterate on the UX in later alphas. |
schell
left a comment
schell
left a comment
There was a problem hiding this comment.
The only thing left is to add some unit tests to ensure this creates valid identity files and that there's no clobbering when given a path that already exists.
See my changes at distractedm1nd#1.
| })? | ||
| .to_string(); | ||
|
|
||
| generate_identity_file(&path, self.mode)?; |
There was a problem hiding this comment.
I think we should probably be checking the path to ensure we're not clobbering an existing identity file.
There was a problem hiding this comment.
Ok, I see that OutputWriter has a parameter allow_overwrite that you're setting to false to not overwrite an existing file. So we're trusting that library here, which might be fine, but I'd still prefer to have a manual check before calling generate_identity_file just to be sure.
| .ok_or_else(|| { | ||
| ErrorKind::Init.context(format!( | ||
| "{} is not currently supported (not UTF-8)", | ||
| config.encryption_identity().display(), |
There was a problem hiding this comment.
We have translations strings to use here:
fl!("err-init-path-not-utf8", ...)
| // TODO: Not a big fan of this, at all. Is there a better | ||
| // way to log it that will capture more attention? | ||
| warn!("Using an autogenerated passphrase: {}", p.expose_secret()); | ||
| warn!("Please store this passphrase securely."); |
There was a problem hiding this comment.
I don't think there's a better way to log it.
I'd like to make this case unrepresentable by requiring generation have a filepath to put the passphrase into once generated, making that action explicit. But we're dealing with an external library here, so I guess this is the best we can do for now.
We could write the passphrase to a file and warn! but they could miss that - and then they have a sensitive passphrase sitting on their filesystem...
| })? | ||
| .to_string(), | ||
| ) | ||
| age::IdentityFile::from_file(path.to_string()) |
There was a problem hiding this comment.
path is already String here - we should use .clone() :)
| } | ||
| Err(e) => { | ||
| return Err(ErrorKind::Init | ||
| .context(format!("Failed to read or generate passphrase: {}", e)) |
There was a problem hiding this comment.
We should use translations through fl! here.
| })? | ||
| .to_string(); | ||
|
|
||
| generate_identity_file(&path, self.mode)?; |
There was a problem hiding this comment.
Probably want to generate the file AFTER initializing the keystore. If the mode is use-existing, and the keystore file doesn't exist, I believe this would fail with an IO error
5 participants
Resolves #302
Resolves COR-333
Wanted to open as a suggestion for dealing with #302, and starting a discussion:
Would it not be better to integrate this behavior directly into the initialization command (with a flag), rather than adding a new command? 2 steps rather than 3 is already a big improvement from a product perspective, let alone removing the requirement of having the extra binary.
But this draft PR is just a placeholder to discuss it, just don't want to waste time in a direction that isn't useful. Obviously there would still be a lot to change+improve here.