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 E8A7264C34 for ; Fri, 4 Mar 2022 12:02:53 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DEA8F350E for ; Fri, 4 Mar 2022 12:02:53 +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 1E5183505 for ; Fri, 4 Mar 2022 12:02:53 +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 E24C641D7B for ; Fri, 4 Mar 2022 12:02:52 +0100 (CET) Date: Fri, 4 Mar 2022 12:02:50 +0100 From: Wolfgang Bumiller To: Markus Frank , Dominik Csapak Cc: pbs-devel@lists.proxmox.com Message-ID: <20220304110250.wos6p2z2jby5qexf@wobu-vie.proxmox.com> References: <20220301112609.84755-1-m.frank@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20220301112609.84755-1-m.frank@proxmox.com> X-SPAM-LEVEL: Spam detection results: 0 AWL 0.369 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 v3] 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, 04 Mar 2022 11:02:54 -0000 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 > --- > 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!(": {}", 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, > ) -> 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)