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

[pbs-devel] [proxmox-backup v4] 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 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"

 pbs-tools/src/lib.rs                    |  1 +
 pbs-tools/src/text.rs                   | 20 ++++++++++
 src/api2/config/tape_encryption_keys.rs | 50 ++++++++++++++++++++-----
 src/bin/proxmox_tape/encryption_key.rs  | 31 ++++++++++++++-
 4 files changed, 90 insertions(+), 12 deletions(-)
 create mode 100644 pbs-tools/src/text.rs

diff --git a/pbs-tools/src/lib.rs b/pbs-tools/src/lib.rs
index 939ba7e6..0fb35482 100644
--- a/pbs-tools/src/lib.rs
+++ b/pbs-tools/src/lib.rs
@@ -6,6 +6,7 @@ pub mod lru_cache;
 pub mod nom;
 pub mod sha;
 pub mod ticket;
+pub mod text;
 
 pub mod async_lru_cache;
 
diff --git a/pbs-tools/src/text.rs b/pbs-tools/src/text.rs
new file mode 100644
index 00000000..b8371d06
--- /dev/null
+++ b/pbs-tools/src/text.rs
@@ -0,0 +1,20 @@
+use anyhow::{Error, bail};
+
+pub fn extract_text_between<'a>(text: &'a str, begin: &str, end: &str) -> Result<&'a str, Error> {
+    let begin_i = text.find(begin);
+    let end_i = text.find(end);
+    match (begin_i, end_i) {
+        (Some(begin_i), Some(end_i)) => {
+            if begin_i < end_i {
+                let extracted_text = &text[begin_i + begin.len()..end_i];
+                return Ok(extracted_text);
+            } else {
+                let extracted_text = &text[end_i + end.len()..begin_i];
+                return Ok(extracted_text);
+            }
+        }
+        (_, _) => {
+            bail!("Begin/End-Marker is missing");
+        }
+    }
+}
diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs
index 3e9a60d1..d8910788 100644
--- a/src/api2/config/tape_encryption_keys.rs
+++ b/src/api2/config/tape_encryption_keys.rs
@@ -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> {
 
@@ -197,15 +206,36 @@ pub fn create_key(
     if let Kdf::None = kdf {
         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)
+ 
+    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)?;
+            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();
+            insert_key(key, key_config, false)?;
+            Ok(fingerprint)
+        }
+        (_, _) => {
+            param_bail!("hint", format_err!("Please specify either a hint or a backupkey"));
+        }
+    }
 }
 
 
diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs
index 71df9ffa..ffda0537 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::{format_err, bail, 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::{
@@ -12,6 +12,7 @@ use pbs_api_types::{
 
 use pbs_datastore::paperkey::{PaperkeyFormat, generate_paper_key};
 use pbs_config::tape_encryption_keys::{load_key_configs,complete_key_fingerprint};
+use pbs_tools::text::extract_text_between;
 
 use proxmox_backup::api2;
 
@@ -233,6 +234,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 +247,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 +255,25 @@ 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 backupkey = extract_text_between(
+            &data,
+            "-----BEGIN PROXMOX BACKUP KEY-----",
+            "-----END PROXMOX BACKUP KEY-----",
+        )?;
+        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

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

[Thomas Lamprecht]

On 09.03.22 10:56, 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 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"
> 
>  pbs-tools/src/lib.rs                    |  1 +
>  pbs-tools/src/text.rs                   | 20 ++++++++++
>  src/api2/config/tape_encryption_keys.rs | 50 ++++++++++++++++++++-----
>  src/bin/proxmox_tape/encryption_key.rs  | 31 ++++++++++++++-
>  4 files changed, 90 insertions(+), 12 deletions(-)
>  create mode 100644 pbs-tools/src/text.rs
> 
> diff --git a/pbs-tools/src/lib.rs b/pbs-tools/src/lib.rs
> index 939ba7e6..0fb35482 100644
> --- a/pbs-tools/src/lib.rs
> +++ b/pbs-tools/src/lib.rs
> @@ -6,6 +6,7 @@ pub mod lru_cache;
>  pub mod nom;
>  pub mod sha;
>  pub mod ticket;
> +pub mod text;
>  
>  pub mod async_lru_cache;
>  
> diff --git a/pbs-tools/src/text.rs b/pbs-tools/src/text.rs
> new file mode 100644
> index 00000000..b8371d06
> --- /dev/null
> +++ b/pbs-tools/src/text.rs
> @@ -0,0 +1,20 @@
> +use anyhow::{Error, bail};
> +

fwiw, adding the helper could do better in its own, preparatory patch.

missing a doc-comment here describing semantics (all pub should have one), e.g.,
it's may not be totally clear how begin/end is handled, e.g., one could think that
begin is forward-searched while the end is backward-searched, or at least with
begin as search start point.

For example:
text = "foobar<test>foo" with `begin = "bar"` and `end = "foo"`

wouldn't find anything, even if a valid section is there. Such specific methods
can be unit tested nicely too, so a hand full of (common + edge) cases would
be great.

It's also not obvious that allowing to transparently revering the marker order is
nice, in the end that order may matter for the text in-between to be correct and
not potentially bogus. What are your thoughts on this?

> +pub fn extract_text_between<'a>(text: &'a str, begin: &str, end: &str) -> Result<&'a str, Error> {

fwiw, switching to returning just the markers position would allow to drop the
lifetime annotations (albeit that alone has no real benefit) but it'd also allow 
the calling site to decide about order importance.

> +    let begin_i = text.find(begin);
> +    let end_i = text.find(end);
> +    match (begin_i, end_i) {
> +        (Some(begin_i), Some(end_i)) => {
> +            if begin_i < end_i {
> +                let extracted_text = &text[begin_i + begin.len()..end_i];
> +                return Ok(extracted_text);
> +            } else {
> +                let extracted_text = &text[end_i + end.len()..begin_i];
> +                return Ok(extracted_text);
> +            }

fwiw, with current semantics, above could be (using m1/m2 for marker 1 and marker 2 to
avoid begin/end confusion):

let (begin, end) = if m1 < m2 { (m1 + m1.len(), m2) } else { (m2 + m2.len(), m1) };

return Ok(&text[begin..end]);

> +        }
> +        (_, _) => {
> +            bail!("Begin/End-Marker is missing");
> +        }

no need for { }

> +    }
> +}

>  
>  
> diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs

> +    // 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 backupkey = extract_text_between(
> +            &data,
> +            "-----BEGIN PROXMOX BACKUP KEY-----",
> +            "-----END PROXMOX BACKUP KEY-----",
> +        )?;

I know Wolfgang suggest it, but I'm actually not really convinced about
moving this out to a public helper, at least not with clear semantic rules
about how the edge case behavior should be.

Maybe rather expose the marker somewhere as pub const/static

pub const (BEGIN_MARKER, END_MARKER) = ("-----BEGIN PROXMOX BACKUP KEY-----", "-----END PROXMOX BACKUP KEY-----");

Then a (untested and slightly condensed, but more edge-case aware) usage could be:

let start = data.find(BEGIN_MARKER).unwrap_or_else(|| bail!("cannot find key start marker")) + BEGIN_MARKER.len();
let end = &data[start..].find(END_MARKER).unwrap_or_else(|| bail!("cannot find key start marker"));

let backup_key = &data[start..end],

IMO fine to have locally for now, but I don't want to "force" you to go for that, but if
we have the helper then it's semantic should be clear, documented and unit-tested.

> +        param["backupkey"] = backupkey.into();
> +        println!("backupkey to import: {}", backupkey);

dumping the whole key may not be _that_ useful, I'd figure the fingerprint could be more
relevant? (no overly hard feelings there)

> +    }
> +
>      let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?;
>  
>      param["password"] = String::from_utf8(password)?.into();