[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