From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id AE0B56B499 for ; Fri, 18 Mar 2022 15:16:07 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9E0891EADC for ; Fri, 18 Mar 2022 15:15:37 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 8D0721EAD3 for ; Fri, 18 Mar 2022 15:15:36 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 5A4944207D for ; Fri, 18 Mar 2022 15:15:36 +0100 (CET) Date: Fri, 18 Mar 2022 15:15:33 +0100 From: Wolfgang Bumiller To: Markus Frank Cc: pbs-devel@lists.proxmox.com Message-ID: <20220318141533.kijdakzj2tdghoeo@wobu-vie.proxmox.com> References: <20220318105501.101910-1-m.frank@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220318105501.101910-1-m.frank@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.354 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - Subject: Re: [pbs-devel] [PATCH proxmox-backup v5] fix #3854 paperkey import to proxmox-tape X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Fri, 18 Mar 2022 14:16:07 -0000 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 > --- > 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, > password: String, > - hint: String, > + hint: Option, > + backupkey: Option, > _rpcenv: &mut dyn RpcEnvironment > ) -> Result { > > 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!(": {}", 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!(": {}", 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!(": {}", 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, > 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