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