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 2C85D6ABD3 for ; Thu, 17 Mar 2022 10:57:27 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 2A6D916C8 for ; Thu, 17 Mar 2022 10:57:27 +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 582B516BA for ; Thu, 17 Mar 2022 10:57:26 +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 2ED6A41B4B for ; Thu, 17 Mar 2022 10:57:26 +0100 (CET) Message-ID: Date: Thu, 17 Mar 2022 10:57:23 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:99.0) Gecko/20100101 Thunderbird/99.0 Content-Language: en-US To: Proxmox Backup Server development discussion , Markus Frank References: <20220309095610.73539-1-m.frank@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220309095610.73539-1-m.frank@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.057 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 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [lib.rs, text.rs] Subject: Re: [pbs-devel] [proxmox-backup v4] 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: Thu, 17 Mar 2022 09:57:27 -0000 On 09.03.22 10:56, 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 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" > > pbs-tools/src/lib.rs | 1 + > pbs-tools/src/text.rs | 20 ++++++++++ > src/api2/config/tape_encryption_keys.rs | 50 ++++++++++++++++++++----- > src/bin/proxmox_tape/encryption_key.rs | 31 ++++++++++++++- > 4 files changed, 90 insertions(+), 12 deletions(-) > create mode 100644 pbs-tools/src/text.rs > > diff --git a/pbs-tools/src/lib.rs b/pbs-tools/src/lib.rs > index 939ba7e6..0fb35482 100644 > --- a/pbs-tools/src/lib.rs > +++ b/pbs-tools/src/lib.rs > @@ -6,6 +6,7 @@ pub mod lru_cache; > pub mod nom; > pub mod sha; > pub mod ticket; > +pub mod text; > > pub mod async_lru_cache; > > diff --git a/pbs-tools/src/text.rs b/pbs-tools/src/text.rs > new file mode 100644 > index 00000000..b8371d06 > --- /dev/null > +++ b/pbs-tools/src/text.rs > @@ -0,0 +1,20 @@ > +use anyhow::{Error, bail}; > + fwiw, adding the helper could do better in its own, preparatory patch. missing a doc-comment here describing semantics (all pub should have one), e.g., it's may not be totally clear how begin/end is handled, e.g., one could think that begin is forward-searched while the end is backward-searched, or at least with begin as search start point. For example: text = "foobarfoo" with `begin = "bar"` and `end = "foo"` wouldn't find anything, even if a valid section is there. Such specific methods can be unit tested nicely too, so a hand full of (common + edge) cases would be great. It's also not obvious that allowing to transparently revering the marker order is nice, in the end that order may matter for the text in-between to be correct and not potentially bogus. What are your thoughts on this? > +pub fn extract_text_between<'a>(text: &'a str, begin: &str, end: &str) -> Result<&'a str, Error> { fwiw, switching to returning just the markers position would allow to drop the lifetime annotations (albeit that alone has no real benefit) but it'd also allow the calling site to decide about order importance. > + let begin_i = text.find(begin); > + let end_i = text.find(end); > + match (begin_i, end_i) { > + (Some(begin_i), Some(end_i)) => { > + if begin_i < end_i { > + let extracted_text = &text[begin_i + begin.len()..end_i]; > + return Ok(extracted_text); > + } else { > + let extracted_text = &text[end_i + end.len()..begin_i]; > + return Ok(extracted_text); > + } fwiw, with current semantics, above could be (using m1/m2 for marker 1 and marker 2 to avoid begin/end confusion): let (begin, end) = if m1 < m2 { (m1 + m1.len(), m2) } else { (m2 + m2.len(), m1) }; return Ok(&text[begin..end]); > + } > + (_, _) => { > + bail!("Begin/End-Marker is missing"); > + } no need for { } > + } > +} > > > diff --git a/src/bin/proxmox_tape/encryption_key.rs b/src/bin/proxmox_tape/encryption_key.rs > + // 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 backupkey = extract_text_between( > + &data, > + "-----BEGIN PROXMOX BACKUP KEY-----", > + "-----END PROXMOX BACKUP KEY-----", > + )?; I know Wolfgang suggest it, but I'm actually not really convinced about moving this out to a public helper, at least not with clear semantic rules about how the edge case behavior should be. Maybe rather expose the marker somewhere as pub const/static pub const (BEGIN_MARKER, END_MARKER) = ("-----BEGIN PROXMOX BACKUP KEY-----", "-----END PROXMOX BACKUP KEY-----"); Then a (untested and slightly condensed, but more edge-case aware) usage could be: let start = data.find(BEGIN_MARKER).unwrap_or_else(|| bail!("cannot find key start marker")) + BEGIN_MARKER.len(); let end = &data[start..].find(END_MARKER).unwrap_or_else(|| bail!("cannot find key start marker")); let backup_key = &data[start..end], IMO fine to have locally for now, but I don't want to "force" you to go for that, but if we have the helper then it's semantic should be clear, documented and unit-tested. > + param["backupkey"] = backupkey.into(); > + println!("backupkey to import: {}", backupkey); dumping the whole key may not be _that_ useful, I'd figure the fingerprint could be more relevant? (no overly hard feelings there) > + } > + > let password = tty::read_and_verify_password("Tape Encryption Key Password: ")?; > > param["password"] = String::from_utf8(password)?.into();