Re: [pbs-devel] [PATCH proxmox-backup v3] fix #3854 paperkey import to proxmox-tape

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

comments inline and I'd like to re-visit the file parameter name... ;-)

On Tue, Mar 01, 2022 at 12:26:09PM +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@proxmox.com>
> ---
> 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"
> 
(...)
> @@ -198,14 +207,27 @@ 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)
> +    match (hint, backupkey) {
> +        (_, Some(backupkey)) => {

I don't think we should just ignore the hint here and either allow
overriding it explicitly or error when both are set, otherwise this
feels a bit awkward.

> +            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), _) => {
> +            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)
> +        }
> +        (None, None) => {
> +            let err = ParameterError::from(("hint", format_err!("Please specify either a hint or a backupkey")));

^ line too long
Since you'll likely need a v4 now you can use the new `param_bail!` to
shorten  it even further.

> +            return Err(err.into());
> +        }
> +    }
>  }
>  
>  
> diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
> index 71df9ffa..31c573cc 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, ParameterError};
>  use proxmox_sys::linux::tty;
>  
>  use pbs_api_types::{
> @@ -233,6 +233,12 @@ async fn restore_key(
>                  type: String,
>                  min_length: 1,
>                  max_length: 32,
> +                optional: true,
> +            },
> +            "paperkey-file": {

nit: @Dominik (since you suggested it):
do we really want `-file` as a suffix here?

We have these currently:
in recover:
    --file
    --keyfile (without the dash), and
in 'inspect':
    --chunk => chunk file
    --decode => apparently where to decode *to* 🤔
    --keyfile (also no dash)
manager's 'acme':
    --data => path to plugin-data file

should we just use `--paperkey` here?

> +                description: "Paperkeyfile location for importing old backupkey",
> +                type: String,
> +                optional: true,
>              },
>          },
>      },
> @@ -241,12 +247,40 @@ async fn restore_key(
>  fn create_key(
>      mut param: Value,
>      rpcenv: &mut dyn RpcEnvironment,
> +    paperkey_file: Option<String>,
>  ) -> Result<(), Error> {
>  
>      if !tty::stdin_isatty() {
>          bail!("no password input mechanism available");
>      }
>  
> +    if param["hint"].is_null() && paperkey_file.is_none() {
> +        let err = ParameterError::from(("hint", format_err!("Please specify either a hint or a paperkey-file")));

^ as above, too long & can now use param_bail

> +        return Err(err.into());
> +    }
> +
> +    // 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 begin = "-----BEGIN PROXMOX BACKUP KEY-----";
> +        let start = data.find(begin);
> +        let end = data.find("-----END PROXMOX BACKUP KEY-----");

Since there's a good chance it'll at some point be moved to/available in
an utility crate, I'd like to see the content extraction in a separate
helper function
(like
  fn extract_text_between<'a>(text: &'a str, begin: &str, end: &str) -> Option<&'a str>
with invalid marker order just returning `None`, I'm not sure its really
worth an error message, otherwise of course it could return a `Result`,
too)