* Re: [pbs-devel] [PATCH proxmox-backup] fix #3854 paperkey import to proxmox-tape
2022-02-08 11:51 [pbs-devel] [PATCH proxmox-backup] fix #3854 paperkey import to proxmox-tape Markus Frank
@ 2022-02-09 13:21 ` Dominik Csapak
0 siblings, 0 replies; 2+ messages in thread
From: Dominik Csapak @ 2022-02-09 13:21 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Markus Frank
comments inline
On 2/8/22 12:51, Markus Frank wrote:
> added a parameter to the cli for reading a old paperkeyfile to restore
> the key from it. For that i added a json parameter for the api and made
> hint optional because hint is already in the proxmox-backupkey-json.
>
> functionality:
> proxmox-tape key paperkey [fingerprint of existing key] > paperkey.backup
> proxmox-tape key create --paperkeyfile paperkey.backup
>
> for importing the key it is irrelevant, if the paperkey got exported as html
> or txt.
>
> Signed-off-by: Markus Frank <m.frank@proxmox.com>
> ---
> src/api2/config/tape_encryption_keys.rs | 41 +++++++++++++++++++------
> src/bin/proxmox_tape/encryption_key.rs | 28 +++++++++++++++++
> 2 files changed, 60 insertions(+), 9 deletions(-)
>
> diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
> index 1ad99377..23de4acc 100644
> --- a/src/api2/config/tape_encryption_keys.rs
> +++ b/src/api2/config/tape_encryption_keys.rs
> @@ -145,6 +145,12 @@ pub fn change_passphrase(
> },
> hint: {
> schema: PASSWORD_HINT_SCHEMA,
> + optional: true,
> + },
> + backupkey: {
> + description: "json parameter for importing old backupkey",
since we want to import a previously exported paperkey, i'd use that
in the description instead, for example
'A previously exported paperkey in JSON format.'
> + type: String
while having a regex here is not practical (i think), we should
somewhat limit the input a user can send here, we should at least limit
the length here.
> + optional: true,
> },
> },
> },
> @@ -159,7 +165,8 @@ pub fn change_passphrase(
> pub fn create_key(
> kdf: Option<Kdf>,
> password: String,
> - hint: String,
> + hint: Option<String>,
> + backupkey: Option<String>,
> _rpcenv: &mut dyn RpcEnvironment
> ) -> Result<Fingerprint, Error> {
>
> @@ -169,14 +176,30 @@ pub fn create_key(
> bail!("Please specify a key derivation function (none is not allowed here).");
> }
>
> - let (key, mut key_config) = KeyConfig::new(password.as_bytes(), kdf)?;
> - key_config.hint = Some(hint);
> -
> - let fingerprint = key_config.fingerprint.clone().unwrap();
> -
> - insert_key(key, key_config, false)?;
> -
> - Ok(fingerprint)
> + if let Some(ref backupkey) = backupkey {
> + match serde_json::from_str::<KeyConfig>(backupkey) {
> + Ok(key_config) => {
> + let password_fn = || { Ok(password.as_bytes().to_vec()) };
using 'into_bytes()' here avoids copying it
> + let (key, _created, fingerprint) = key_config.decrypt(&password_fn)?;
> + insert_key(key, key_config, false)?;
> + Ok(fingerprint)
> + }
> + Err(err) => {
> + eprintln!("Couldn't parse data as KeyConfig - {}", err);
> + bail!("Neither a PEM-formatted private key, nor a PBS key file.");
two things: please do use log::error (or warning) instead of 'eprintln'
and when we return an error, there is no need to print an additional error,
this whole statement can be simplified by doing:
---
let key_config: KeyConfig = serde_json::from_str(backupkey)
.map_err(|err| format_err!("<errmsg>: {}", err))?;
...
---
> + }
> + }
> + } else {
> + if hint.is_none() {
> + bail!("Please specify either a hint or a backupkey.");
we have a special error to bubble parameter errors to the gui
look for 'ParameterError' in proxmox-schema
(basically:
let mut err = ParameterError::new();
err.push("parameter".to_string(), Err(...))
return Err(err);
using that, the api behaves similarly to when the schema verification fails
> + } else {
> + let (key, mut key_config) = KeyConfig::new(password.as_bytes(), kdf)?;
> + key_config.hint = hint;
> + let fingerprint = key_config.fingerprint.clone().unwrap();
> + insert_key(key, key_config, false)?;
> + Ok(fingerprint)
> + }
> + }
> }
the whole if/else could be better written as
match (hint, backupkey) {
(_, Some(backupkey)) => { }, // handle backupkey
(Some(hint), _) => { }, // handle hint
(None, None) => { } // handle parameter error
}
>
>
> diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
> index 156295fd..1863c9bc 100644
> --- a/src/bin/proxmox_tape/encryption_key.rs
> +++ b/src/bin/proxmox_tape/encryption_key.rs
> @@ -15,6 +15,8 @@ use pbs_config::tape_encryption_keys::{load_key_configs,complete_key_fingerprint
>
> use proxmox_backup::api2;
>
> +use std::fs;
> +
> pub fn encryption_key_commands() -> CommandLineInterface {
>
> let cmd_def = CliCommandMap::new()
> @@ -222,6 +224,12 @@ async fn restore_key(
> type: String,
> min_length: 1,
> max_length: 32,
> + optional: true,
> + },
> + paperkeyfile: {
nit: i'd prefer 'paperkey-file'
> + description: "Paperkeyfile location for importing old backupkey",
> + type: String,
> + optional: true,
> },
> },
> },
> @@ -230,12 +238,32 @@ async fn restore_key(
> fn create_key(
> mut param: Value,
> rpcenv: &mut dyn RpcEnvironment,
> + paperkeyfile: Option<String>,
> ) -> Result<(), Error> {
>
> if !tty::stdin_isatty() {
> bail!("no password input mechanism available");
> }
>
> + if param["hint"].is_null() && paperkeyfile.is_none(){
> + bail!("Please specify either a hint or a paperkeyfile.");
same as above with ParameterError
> + }
> +
> + // searching for PROXMOX BACKUP KEY if a paperkeyfile is defined
> + if let Some(paperkeyfile) = paperkeyfile {
> + let data = fs::read_to_string(paperkeyfile)?;
i'd use proxmox-sys::fs::file_read_string here
it's just a thin wrapper around read_to_string, but provides
a better error message
> + let begin = "-----BEGIN PROXMOX BACKUP KEY-----";
> + let start = data.find(begin);
> + let end = data.find("-----END PROXMOX BACKUP KEY-----");
> + if let Some(start) = start {
> + if let Some(end) = end {
> + let backupkey = &data[start+begin.len()..end];
i'd check here if start < end
> + param["backupkey"]=backupkey.into();
> + println!("backupkey to import: {}", backupkey);
> + }
> + }
this could be better written as:
match (start, end) {
(Some(start), Some(end)) => {} // set the parameter
(_, _) => {} // throw error
}
else a file with missing markers will be silently ignored..
> + }
> +
> let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?;
>
> param["password"] = String::from_utf8(password)?.into();
^ permalink raw reply [flat|nested] 2+ messages in thread