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 62D57616AC for ; Wed, 9 Feb 2022 14:22:09 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 43CCF260F for ; Wed, 9 Feb 2022 14:21:39 +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 C87302602 for ; Wed, 9 Feb 2022 14:21:34 +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 962A746C5F; Wed, 9 Feb 2022 14:21:34 +0100 (CET) Message-ID: <035844da-48ae-c5a5-cd15-5b267d31da27@proxmox.com> Date: Wed, 9 Feb 2022 14:21:32 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:97.0) Gecko/20100101 Thunderbird/97.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Markus Frank References: <20220208115119.74931-1-m.frank@proxmox.com> From: Dominik Csapak In-Reply-To: <20220208115119.74931-1-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.159 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 NICE_REPLY_A -0.001 Looks like a legit reply (A) 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] 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: Wed, 09 Feb 2022 13:22:09 -0000 comments inline On 2/8/22 12:51, 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 --paperkeyfile paperkey.backup > > for importing the key it is irrelevant, if the paperkey got exported as html > or txt. > > Signed-off-by: Markus Frank > --- > src/api2/config/tape_encryption_keys.rs | 41 +++++++++++++++++++------ > src/bin/proxmox_tape/encryption_key.rs | 28 +++++++++++++++++ > 2 files changed, 60 insertions(+), 9 deletions(-) > > diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_encryption_keys.rs > index 1ad99377..23de4acc 100644 > --- a/src/api2/config/tape_encryption_keys.rs > +++ b/src/api2/config/tape_encryption_keys.rs > @@ -145,6 +145,12 @@ pub fn change_passphrase( > }, > hint: { > schema: PASSWORD_HINT_SCHEMA, > + optional: true, > + }, > + backupkey: { > + description: "json parameter for importing old backupkey", since we want to import a previously exported paperkey, i'd use that in the description instead, for example 'A previously exported paperkey in JSON format.' > + type: String while having a regex here is not practical (i think), we should somewhat limit the input a user can send here, we should at least limit the length here. > + optional: true, > }, > }, > }, > @@ -159,7 +165,8 @@ pub fn change_passphrase( > pub fn create_key( > kdf: Option, > password: String, > - hint: String, > + hint: Option, > + backupkey: Option, > _rpcenv: &mut dyn RpcEnvironment > ) -> Result { > > @@ -169,14 +176,30 @@ 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) > + if let Some(ref backupkey) = backupkey { > + match serde_json::from_str::(backupkey) { > + Ok(key_config) => { > + let password_fn = || { Ok(password.as_bytes().to_vec()) }; using 'into_bytes()' here avoids copying it > + let (key, _created, fingerprint) = key_config.decrypt(&password_fn)?; > + insert_key(key, key_config, false)?; > + Ok(fingerprint) > + } > + Err(err) => { > + eprintln!("Couldn't parse data as KeyConfig - {}", err); > + bail!("Neither a PEM-formatted private key, nor a PBS key file."); two things: please do use log::error (or warning) instead of 'eprintln' and when we return an error, there is no need to print an additional error, this whole statement can be simplified by doing: --- let key_config: KeyConfig = serde_json::from_str(backupkey) .map_err(|err| format_err!(": {}", err))?; ... --- > + } > + } > + } else { > + if hint.is_none() { > + bail!("Please specify either a hint or a backupkey."); we have a special error to bubble parameter errors to the gui look for 'ParameterError' in proxmox-schema (basically: let mut err = ParameterError::new(); err.push("parameter".to_string(), Err(...)) return Err(err); using that, the api behaves similarly to when the schema verification fails > + } else { > + let (key, mut key_config) = KeyConfig::new(password.as_bytes(), kdf)?; > + key_config.hint = hint; > + let fingerprint = key_config.fingerprint.clone().unwrap(); > + insert_key(key, key_config, false)?; > + Ok(fingerprint) > + } > + } > } the whole if/else could be better written as match (hint, backupkey) { (_, Some(backupkey)) => { }, // handle backupkey (Some(hint), _) => { }, // handle hint (None, None) => { } // handle parameter error } > > > diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs > index 156295fd..1863c9bc 100644 > --- a/src/bin/proxmox_tape/encryption_key.rs > +++ b/src/bin/proxmox_tape/encryption_key.rs > @@ -15,6 +15,8 @@ use pbs_config::tape_encryption_keys::{load_key_configs,complete_key_fingerprint > > use proxmox_backup::api2; > > +use std::fs; > + > pub fn encryption_key_commands() -> CommandLineInterface { > > let cmd_def = CliCommandMap::new() > @@ -222,6 +224,12 @@ async fn restore_key( > type: String, > min_length: 1, > max_length: 32, > + optional: true, > + }, > + paperkeyfile: { nit: i'd prefer 'paperkey-file' > + description: "Paperkeyfile location for importing old backupkey", > + type: String, > + optional: true, > }, > }, > }, > @@ -230,12 +238,32 @@ async fn restore_key( > fn create_key( > mut param: Value, > rpcenv: &mut dyn RpcEnvironment, > + paperkeyfile: Option, > ) -> Result<(), Error> { > > if !tty::stdin_isatty() { > bail!("no password input mechanism available"); > } > > + if param["hint"].is_null() && paperkeyfile.is_none(){ > + bail!("Please specify either a hint or a paperkeyfile."); same as above with ParameterError > + } > + > + // searching for PROXMOX BACKUP KEY if a paperkeyfile is defined > + if let Some(paperkeyfile) = paperkeyfile { > + let data = fs::read_to_string(paperkeyfile)?; i'd use proxmox-sys::fs::file_read_string here it's just a thin wrapper around read_to_string, but provides a better error message > + let begin = "-----BEGIN PROXMOX BACKUP KEY-----"; > + let start = data.find(begin); > + let end = data.find("-----END PROXMOX BACKUP KEY-----"); > + if let Some(start) = start { > + if let Some(end) = end { > + let backupkey = &data[start+begin.len()..end]; i'd check here if start < end > + param["backupkey"]=backupkey.into(); > + println!("backupkey to import: {}", backupkey); > + } > + } this could be better written as: match (start, end) { (Some(start), Some(end)) => {} // set the parameter (_, _) => {} // throw error } else a file with missing markers will be silently ignored.. > + } > + > let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?; > > param["password"] = String::from_utf8(password)?.into();