* [pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape
@ 2022-03-18 10:55 Markus Frank
2022-03-18 14:15 ` Wolfgang Bumiller
0 siblings, 1 reply; 2+ messages in thread
From: Markus Frank @ 2022-03-18 10:55 UTC (permalink / raw)
To: pbs-devel
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>
---
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)
+ 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..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
^ permalink raw reply [flat|nested] 2+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape
2022-03-18 10:55 [pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape Markus Frank
@ 2022-03-18 14:15 ` Wolfgang Bumiller
0 siblings, 0 replies; 2+ messages in thread
From: Wolfgang Bumiller @ 2022-03-18 14:15 UTC (permalink / raw)
To: Markus Frank; +Cc: pbs-devel
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@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
^ permalink raw reply [flat|nested] 2+ messages in thread
end of thread, other threads:[~2022-03-18 14:16 UTC | newest]
Thread overview: 2+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-18 10:55 [pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape Markus Frank
2022-03-18 14:15 ` Wolfgang Bumiller
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox