[pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape

Wolfgang Bumiller w.bumiller at proxmox.com
Fri Mar 18 15:15:33 CET 2022


On Fri, Mar 18, 2022 at 11:55:01AM +0100, 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 --paperkey-file 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 at proxmox.com>
> ---
> Comments:
>  * I would show the backupkey before sending it to the api2. a few reasons:
>  1. The user can validate the key that will be imported
>  2. It shows the hint before the password input function
>  3. To get fingerprint the cli tool also would need to parse the json
>  4. The API returns the fingerprint after importing
> 
> version 5:
>  * removed extract_text_between. I would say we can move it to a
>  public function if another component does a similar text-extraction.
>  * simplified text extraction
> 
> version 4:
>  * ParameterError::from to param_bail!
>  * when hint and paperkey-file used at the same time, old hint get overwritten
>  * added text in pbs-tools with function extract_text_between
> 
> version 3:
>  * ParameterError with method ParameterError::from
>  * changed --paperkey_file to --paperkey-file
> 
> version 2:
>  * added format_err! and ParameterError
>  * changed a few "ifs" to "match"
> 
>  src/api2/config/tape_encryption_keys.rs | 57 ++++++++++++++++++++-----
>  src/bin/proxmox_tape/encryption_key.rs  | 37 +++++++++++++++-
>  2 files changed, 82 insertions(+), 12 deletions(-)
> 
> diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
> index 3e9a60d1..d2e668ba 100644
> --- a/src/api2/config/tape_encryption_keys.rs
> +++ b/src/api2/config/tape_encryption_keys.rs
> @@ -174,7 +174,16 @@ pub fn change_passphrase(
>              },
>              hint: {
>                  schema: PASSWORD_HINT_SCHEMA,
> +                optional: true,
>              },
> +            backupkey: {
> +                description: "A previously exported paperkey in JSON format.",
> +                type: String,
> +                min_length: 300,
> +                max_length: 600,
> +                optional: true,
> +             },
> +
>          },
>      },
>      returns: {
> @@ -188,24 +197,52 @@ 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> {
>  
>      let kdf = kdf.unwrap_or_default();
>  
>      if let Kdf::None = kdf {
> -        param_bail!("kdf", format_err!("Please specify a key derivation function (none is not allowed here)."));
> +        param_bail!(
> +            "kdf",
> +            format_err!("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)

IMO there's too much identical code below now.
While Dominik initially suggested the match, that was mostly because it
explicilty ignored the `hint` when `backupkey` was set.
But now...

> +    match (hint, backupkey) {
> +        (Some(hint), Some(backupkey)) => {
> +            let mut key_config: KeyConfig =
> +                serde_json::from_str(&backupkey).map_err(|err| format_err!("<errmsg>: {}", err))?;
> +            let password_fn = || Ok(password.as_bytes().to_vec());
> +            let (key, _created, fingerprint) = key_config.decrypt(&password_fn)?;

... the first 2 match arms are identical with the exception of the
single line below...

> +            key_config.hint = Some(hint);
> +            insert_key(key, key_config, false)?;
> +            Ok(fingerprint)
> +        }
> +        (None, Some(backupkey)) => {
> +            let key_config: KeyConfig =
> +                serde_json::from_str(&backupkey).map_err(|err| format_err!("<errmsg>: {}", err))?;
> +            let password_fn = || Ok(password.as_bytes().to_vec());
> +            let (key, _created, fingerprint) = key_config.decrypt(&password_fn)?;
> +            insert_key(key, key_config, false)?;
> +            Ok(fingerprint)
> +        }
> +        (Some(hint), None) => {
> +            let (key, mut key_config) = KeyConfig::new(password.as_bytes(), kdf)?;
> +            key_config.hint = Some(hint);
> +            let fingerprint = key_config.fingerprint.clone().unwrap();

... and the insertion & return value is also identical everywhere.

> +            insert_key(key, key_config, false)?;
> +            Ok(fingerprint)
> +        }
> +        (_, _) => {

... and this case, which is now actually `(None, None)` can just be handled
right at the beginning rather than as part of the match.

> +            param_bail!(
> +                "hint",
> +                format_err!("Please specify either a hint or a backupkey")
> +            );
> +        }
> +    }

Without testing, how about restructuring it like this:

handle error early
    if hint.is_none && backupkey.is_none() {
        param_bail!...
    }

backupkey is the only real difference:
    let (key, mut key_config, fingerprint) = match backupkey {
        Some(backupkey) => {
            let mut key_config: KeyConfig =
                serde_json::from_str(&backupkey).map_err(|err| format_err!("<errmsg>: {}", err))?;
            let password_fn = || Ok(password.as_bytes().to_vec());
            let (key, _created, fingerprint) = key_config.decrypt(&password_fn)?;
            (key, key_config, fingerprint)
        }
        None => {
            let (key, key_config) = KeyConfig::new(password.as_bytes(), kdf)?;
            let fingerprint = key_config.fingerprint.clone().unwrap();
            (key, key_config, fingerprint)
        }
    };

hint handling is always the same
    if hint.is_some() {
       key_config.hint = hint;
    }

    insert_key(key, key_config, false)?;
    Ok(fingerprint)

>  }
>  
>  
> diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
> index 71df9ffa..26d27812 100644
> --- a/src/bin/proxmox_tape/encryption_key.rs
> +++ b/src/bin/proxmox_tape/encryption_key.rs
> @@ -1,8 +1,8 @@
> -use anyhow::{bail, Error};
> +use anyhow::{bail, format_err, Error};
>  use serde_json::Value;
>  
>  use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
> -use proxmox_schema::api;
> +use proxmox_schema::{api, param_bail};
>  use proxmox_sys::linux::tty;
>  
>  use pbs_api_types::{
> @@ -221,6 +221,9 @@ async fn restore_key(
>      Ok(())
>  }
>  
> +pub const BEGIN_MARKER: &str = "-----BEGIN PROXMOX BACKUP KEY-----";
> +pub const END_MARKER: &str = "-----END PROXMOX BACKUP KEY-----";
> +
>  #[api(
>      input: {
>          properties: {
> @@ -233,6 +236,12 @@ async fn restore_key(
>                  type: String,
>                  min_length: 1,
>                  max_length: 32,
> +                optional: true,
> +            },
> +            "paperkey-file": {
> +                description: "Paperkeyfile location for importing old backupkey",
> +                type: String,
> +                optional: true,
>              },
>          },
>      },
> @@ -240,6 +249,7 @@ async fn restore_key(
>  /// Create key (read password from stdin)
>  fn create_key(
>      mut param: Value,
> +    paperkey_file: Option<String>,
>      rpcenv: &mut dyn RpcEnvironment,
>  ) -> Result<(), Error> {
>  
> @@ -247,6 +257,29 @@ fn create_key(
>          bail!("no password input mechanism available");
>      }
>  
> +    if param["hint"].is_null() && paperkey_file.is_none() {
> +        param_bail!(
> +            "hint",
> +            format_err!("Please specify either a hint or a paperkey-file")
> +        );
> +    }
> +
> +    // searching for PROXMOX BACKUP KEY if a paperkeyfile is defined
> +    if let Some(paperkey_file) = paperkey_file {
> +        let data = proxmox_sys::fs::file_read_string(paperkey_file)?;
> +        let start = data
> +            .find(BEGIN_MARKER)
> +            .ok_or_else(|| format_err!("cannot find key start marker"))?
> +            + BEGIN_MARKER.len();
> +        let data_remain = &data[start..];
> +        let end = data_remain
> +            .find(END_MARKER)
> +            .ok_or_else(|| format_err!("cannot find key end marker below start marker"))?;
> +        let backupkey = &data_remain[..end];
> +        param["backupkey"] = backupkey.into();
> +        println!("backupkey to import: {}", backupkey);
> +    }
> +
>      let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?;
>  
>      param["password"] = String::from_utf8(password)?.into();
> -- 
> 2.30.2




More information about the pbs-devel mailing list