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 03BBD60E8D for ; Mon, 7 Feb 2022 15:58:38 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9DF2626B64 for ; Mon, 7 Feb 2022 15:58:07 +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 AC9FE26B57 for ; Mon, 7 Feb 2022 15:58:02 +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 02EC7456AF for ; Mon, 7 Feb 2022 15:58:01 +0100 (CET) Message-ID: Date: Mon, 7 Feb 2022 15:58:00 +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 , Stefan Sterz References: <20220207124825.1116194-1-s.sterz@proxmox.com> From: Thomas Lamprecht In-Reply-To: <20220207124825.1116194-1-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.059 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 07 Feb 2022 14:58:38 -0000 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 > --- > 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, > - password: String, > + password: Option, > new_password: String, > hint: String, > + force: bool, > fingerprint: Fingerprint, > digest: Option, > _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 } 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... > + > + // 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);