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 C3B2A60E9D for ; 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 ; 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 ; 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 ; 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 , Proxmox Backup Server development discussion References: <20220207124825.1116194-1-s.sterz@proxmox.com> From: Stefan Sterz In-Reply-To: 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-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 >>> --- >>>   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 >> } > 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.