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

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

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();