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

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

[Markus Frank]

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"

 src/api2/config/tape_encryption_keys.rs | 42 +++++++++++++++++++------
 src/bin/proxmox_tape/encryption_key.rs  | 38 ++++++++++++++++++++--
 2 files changed, 68 insertions(+), 12 deletions(-)

diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
index 25cc6cc0..09335933 100644
--- a/src/api2/config/tape_encryption_keys.rs
+++ b/src/api2/config/tape_encryption_keys.rs
@@ -3,7 +3,7 @@ use serde_json::Value;
 use hex::FromHex;
 
 use proxmox_router::{ApiMethod, Router, RpcEnvironment, Permission};
-use proxmox_schema::api;
+use proxmox_schema::{api, ParameterError};
 
 use pbs_api_types::{
     Authid, Fingerprint, KeyInfo, Kdf,
@@ -174,6 +174,14 @@ 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,
             },
         },
     },
@@ -188,7 +196,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> {
 
@@ -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)) => {
+            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")));
+            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": {
+                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")));
+        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-----");
+        match (start, end) {
+            (Some(start), Some(end)) => {
+                if start < end {
+                    let backupkey = &data[start + begin.len()..end];
+                    param["backupkey"] = backupkey.into();
+                    println!("backupkey to import: {}", backupkey);
+                } else {
+                    bail!("paperkey-file is incorrect: End-Marker of backupkey is before Begin-Marker");
+                }
+            }
+            (_, _) => {
+                bail!("Begin/End-Marker of backupkey in paperkey-file is missing");
+            }
+        }
+    }
+
     let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?;
 
     param["password"] = String::from_utf8(password)?.into();
-- 
2.30.2

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

[Wolfgang Bumiller]

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)

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

[Dominik Csapak]

On 3/4/22 12:02, Wolfgang Bumiller wrote:
> 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?

meh...

'--paperkey': would fit into the existing scheme, but implies
   giving the content, not a file...

'--paperkeyfile': it's ugly

so i suggested '--paperkey-file'...

but we can ofc use '--paperkey' here, maybe try
to use the content directly if it isn't an existing path?

then the implication would at least work...

idk, i'm not the best person to deal with naming things...

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

[Wolfgang Bumiller]

On Fri, Mar 04, 2022 at 12:50:07PM +0100, Dominik Csapak wrote:
> On 3/4/22 12:02, Wolfgang Bumiller wrote:
> > 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?
> 
> meh...
> 
> '--paperkey': would fit into the existing scheme, but implies
>   giving the content, not a file...

I disagree.

> 
> '--paperkeyfile': it's ugly
> 
> so i suggested '--paperkey-file'...
> 
> but we can ofc use '--paperkey' here, maybe try
> to use the content directly if it isn't an existing path?

Please no!

Well... @Markus, your choice, you can pick one of `--paperkey` or
`--paperkey-file`,
(and maybe we should rename `--keyfile` to `--key-file`...)