Re: [PATCH proxmox-backup 04/20] pbs-config: implement encryption key config handling

[Christian Ebner <c.ebner@proxmox.com>]

On 4/2/26 1:26 AM, Thomas Lamprecht wrote:
> Am 01.04.26 um 09:55 schrieb Christian Ebner:
>> Implements the handling for encryption key configuration and files.
>>
>> Individual encryption keys with the secret key material are stored in
>> individual files, while the config stores duplicate key info, so the
>> actual key only needs to be loaded when accessed, not for listing.
>>
>> Key fingerprint is compared when loading the key in order to detect
>> possible mismatches.
>>
>> Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
>> ---
>>   pbs-config/Cargo.toml             |   1 +
>>   pbs-config/src/encryption_keys.rs | 159 ++++++++++++++++++++++++++++++
>>   pbs-config/src/lib.rs             |   1 +
>>   3 files changed, 161 insertions(+)
>>   create mode 100644 pbs-config/src/encryption_keys.rs
>>
>> diff --git a/pbs-config/Cargo.toml b/pbs-config/Cargo.toml
>> index eb81ce004..a27964cfd 100644
>> --- a/pbs-config/Cargo.toml
>> +++ b/pbs-config/Cargo.toml
>> @@ -30,3 +30,4 @@ proxmox-uuid.workspace = true
>>   
>>   pbs-api-types.workspace = true
>>   pbs-buildcfg.workspace = true
>> +pbs-key-config.workspace = true
>> diff --git a/pbs-config/src/encryption_keys.rs b/pbs-config/src/encryption_keys.rs
> 
>> +/// Delete the encryption key from config.
>> +///
>> +/// Deletes the key from the config but keeps a backup of the key file.
> 
> The implementation just calls `std::fs::remove_file`, so is there really a
> backup of the key kept?

Ah thanks for noticing! This is indeed outdated. An initial draft 
version kept a copy of the keyfile around. This was however removed at a 
later stage due to some offlist discussion with Fabian.

> 
>> +pub fn delete_key(id: &str) -> Result<(), Error> {
>> +    let _lock = lock_config()?;
>> +    let (mut config, _digest) = config()?;
>> +
>> +    // if the key with given id exists in config, try to remove also file on path.
>> +    if let Some((section_type, key)) = config.sections.get(id) {
>> +        if section_type == ENCRYPTION_KEYS_CFG_TYPE_ID {
>> +            let key = EncryptionKey::deserialize(key)
>> +                .map_err(|_err| format_err!("failed to parse pre-existing key"))?;
>> +
>> +            if let Some(path) = &key.info.path {
>> +                std::fs::remove_file(path)?;
>> +            }
> 
> This ordering seems slightly risky: the key file is deleted *before* the
> config is rewritten. If `replace_backup_config` fails after `remove_file`
> succeeds, the key material is gone but the config still references it.
> 
> Would be probably safer to write the config first (removing the section),
> then delete the file. Or was the idea here to avoid leaving a stale key
> file on disk?

No, this was again due to the changes mentioned above, will adapt and I 
see there is also still a check missing if the key is still in use by 
any sync job (which has to be performed on the api handler 
implementation though, not here).

> 
>> +        }
>> +
>> +        config.sections.remove(id);
>> +
>> +        let raw = CONFIG.write(ENCRYPTION_KEYS_CFG_FILENAME, &config)?;
>> +        replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_bytes())
>> +    } else {
>> +        bail!("key {id} not found in config");
>> +    }
>> +}