From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: <s.sterz@proxmox.com> 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 C3B2A60E9D for <pbs-devel@lists.proxmox.com>; Mon, 7 Feb 2022 17:15:24 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id BB3342717B for <pbs-devel@lists.proxmox.com>; Mon, 7 Feb 2022 17:14:54 +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) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 36BBA2716F for <pbs-devel@lists.proxmox.com>; Mon, 7 Feb 2022 17:14: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 0893F46208 for <pbs-devel@lists.proxmox.com>; Mon, 7 Feb 2022 17:14:53 +0100 (CET) Message-ID: <10c2b794-7815-30b8-9957-a89acdafcab1@proxmox.com> Date: Mon, 7 Feb 2022 17:14:51 +0100 MIME-Version: 1.0 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101 Thunderbird/91.5.0 Content-Language: de-AT To: Thomas Lamprecht <t.lamprecht@proxmox.com>, Proxmox Backup Server development discussion <pbs-devel@lists.proxmox.com> References: <20220207124825.1116194-1-s.sterz@proxmox.com> <a6a5daee-5e44-3fa1-f832-1903bce75f59@proxmox.com> From: Stefan Sterz <s.sterz@proxmox.com> In-Reply-To: <a6a5daee-5e44-3fa1-f832-1903bce75f59@proxmox.com> Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.000 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 1/2] fix #3853: api: add force option to tape key change-passphrase X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion <pbs-devel.lists.proxmox.com> List-Unsubscribe: <https://lists.proxmox.com/cgi-bin/mailman/options/pbs-devel>, <mailto:pbs-devel-request@lists.proxmox.com?subject=unsubscribe> List-Archive: <http://lists.proxmox.com/pipermail/pbs-devel/> List-Post: <mailto:pbs-devel@lists.proxmox.com> List-Help: <mailto:pbs-devel-request@lists.proxmox.com?subject=help> List-Subscribe: <https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel>, <mailto:pbs-devel-request@lists.proxmox.com?subject=subscribe> X-List-Received-Date: Mon, 07 Feb 2022 16:15:24 -0000 On 2/7/22 16:57, Stefan Sterz wrote: > On 2/7/22 15:58, Thomas Lamprecht wrote: >> On 07.02.22 13:48, Stefan Sterz wrote: >>> When force is used, the current passphrase is not required. Instead >>> it will be read from the file pointed to by TAPE_KEYS_FILENAME and >>> the old key configuration will be overwritten using the new >>> passphrase. >>> >> looks quite ok, some nits/suggestions in line. >> >>> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com> >>> --- >>> src/api2/config/tape_encryption_keys.rs | 36 >>> ++++++++++++++++++++++--- >>> 1 file changed, 33 insertions(+), 3 deletions(-) >>> >>> diff --git a/src/api2/config/tape_encryption_keys.rs >>> b/src/api2/config/tape_encryption_keys.rs >>> index 1ad99377..b31f741d 100644 >>> --- a/src/api2/config/tape_encryption_keys.rs >>> +++ b/src/api2/config/tape_encryption_keys.rs >>> @@ -70,6 +70,7 @@ pub fn list_keys( >>> password: { >>> description: "The current password.", >>> min_length: 5, >>> + optional: true, >>> }, >>> "new-password": { >>> description: "The new password.", >>> @@ -78,6 +79,12 @@ pub fn list_keys( >>> hint: { >>> schema: PASSWORD_HINT_SCHEMA, >>> }, >>> + force: { >>> + optional: true, >>> + type: bool, >>> + description: "Don't ask for the old passphrase and >>> overwrite it. Root only.", >> Maybe we can better hint that we reset the password by >> restoring/re-using the >> original key, which is naturally only possible if the key available, >> e.g.: >> >> "Reset password for tape key-copy using original, root-only >> accessible key" >> >>> + default: false, >>> + }, >>> digest: { >>> optional: true, >>> schema: PROXMOX_CONFIG_DIGEST_SCHEMA, >>> @@ -91,9 +98,10 @@ pub fn list_keys( >>> /// Change the encryption key's password (and password hint). >>> pub fn change_passphrase( >>> kdf: Option<Kdf>, >>> - password: String, >>> + password: Option<String>, >>> new_password: String, >>> hint: String, >>> + force: bool, >>> fingerprint: Fingerprint, >>> digest: Option<String>, >>> _rpcenv: &mut dyn RpcEnvironment >>> @@ -116,10 +124,32 @@ pub fn change_passphrase( >>> let key_config = match config_map.get(&fingerprint) { >>> Some(key_config) => key_config, >>> - None => bail!("tape encryption key '{}' does not exist.", >>> fingerprint), >>> + None => bail!("tape encryption key configuration '{}' does >>> not exist.", fingerprint), >>> + }; >>> + >>> + // sanity checks for "password xor --force" >>> + if force && password.is_some() { >>> + bail!("password is not allowed when using force") >>> + } >>> + >>> + if !force && password.is_none() { >>> + bail!("missing parameter: password") >>> + } >> Above two if's could be written slightly shorter while IMO even >> improving readability >> >> match (force, password) { >> (true, Some(_)) => bail!("password is not allowed when using >> force"), >> (false, None) => bail!("missing parameter: password"), >> _ => (), // OK >> } > This does not work, because here password is moved into the match > expression. The borrow checker will complain about it being used later > on when trying to decrypt the key configuration. You could clone > password here, but this solution strikes me as rather "inelegant". >> We probably could even extend this over the "decrypt old key or force >> loading old key from plaintext >> file" part below, so that the whole things looks something like >> (untested): >> >> >> let (key, created, fingerprint) = match (force, password) { >> (true, Some(_)) => bail!("password is not allowed when using >> force"), >> (false, None) => bail!("missing parameter: password"), >> (true, Some(pass)) => key_config.decrypt(&|| >> Ok(pass.as_bytes().to_vec()))?, >> (false, None) => { >> let key = load_keys()?.map(|keys, _| >> key.get(&fingerprint)).unwrap_or_else(|| bail!("...")); >> (key, key_config.created, fingerprint) >> } >> } >> >> but not to hard feeling there, may get seen as a little bit too >> condensed... > > I considered this in a previous version, but dismissed it for the same > reason. It would, however, solve the borrow checker issue from before. > Note that this code doesn't work either due to trait and type > constraints/mismatches. You could do something like this: > > let key = match load_keys()?.0.get(&fingerprint) { > Some(k) => k.key, > None => bail!("failed to reset passphrase, could not find key > '{}'", fingerprint) > }; > > Please tell me your preference and I'll be happy to submit an updated > patch. > >>> + >>> + // decrypt old key or force loading old key from plaintext file >>> + let (key, created, fingerprint) = if let Some(pass) = password { >>> + key_config.decrypt(&|| Ok(pass.as_bytes().to_vec()))? >>> + } else { >>> + let (key_map, _) = load_keys()?; >>> + >>> + let key = match key_map.get(&fingerprint) { >>> + Some(k) => k.key, >>> + None => bail!("tape encryption key '{}' does not >>> exist.", fingerprint) >> error message could be slightly improved with context: >> "failed to reset key password, original tape enc..." >> >>> + }; >>> + >>> + (key, key_config.created, fingerprint) >>> }; >>> - let (key, created, fingerprint) = key_config.decrypt(&|| >>> Ok(password.as_bytes().to_vec()))?; >>> let mut new_key_config = KeyConfig::with_key(&key, >>> new_password.as_bytes(), kdf)?; >>> new_key_config.created = created; // keep original value >>> new_key_config.hint = Some(hint); > > Sorry forgot to hit reply-all.