Re: [pbs-devel] [PATCH proxmox-backup 1/2] fix #3853: api: add force option to tape key change-passphrase

[Stefan Sterz <s.sterz@proxmox.com>]

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.