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

Christian Ebner
2026-04-02T07:09:13Z

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");
>> + }
>> +}