Re: [PATCH proxmox-backup v3 07/30] pbs-config: implement encryption key config handling

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

On 4/14/26 4:31 PM, Michael Köppl wrote:
> 2 comments inline
> 
> On Tue Apr 14, 2026 at 2:59 PM CEST, Christian Ebner wrote:
> 
> [snip]
> 
>> +/// Store the encryption key to file.
>> +///
>> +/// Inserts the key in the config and stores it to the given file.
>> +pub fn store_key(id: &str, key: &KeyConfig) -> Result<(), Error> {
>> +    let _lock = lock_config()?;
>> +    let (mut config, _digest) = config()?;
>> +
>> +    if config.sections.contains_key(id) {
>> +        bail!("key with id '{id}' already exists.");
>> +    }
>> +
>> +    let backup_user = crate::backup_user()?;
>> +    let dir_options = CreateOptions::new()
>> +        .perm(Mode::from_bits_truncate(0o0750))
>> +        .owner(Uid::from_raw(0))
>> +        .group(backup_user.gid);
>> +
>> +    proxmox_sys::fs::ensure_dir_exists(ENCRYPTION_KEYS_DIR, &dir_options, true)?;
>> +
>> +    let key_path = format!("{ENCRYPTION_KEYS_DIR}{id}.enc");
>> +    let key_lock_path = format!("{key_path}.lck");
>> +
>> +    // lock to avoid race with key deletion
>> +    open_backup_lockfile(&key_lock_path, None, true)?;
> 
> This needs to be assigned to a variable, no? Otherwise, the lock would
> be immediately dropped.

Oh, good catch! Indeed without this the lock would be immediately 
dropped, will be fixed. Thanks!

> 
> In other places we have something like let _lock = lock_config()?;
> 
>> +
>> +    // assert the key file is empty or does not exist
>> +    match std::fs::metadata(&key_path) {
>> +        Ok(metadata) => {
>> +            if metadata.len() > 0 {
>> +                bail!("detected pre-existing key file, refusing to overwrite.");
>> +            }
>> +        }
>> +        Err(err) if err.kind() == std::io::ErrorKind::NotFound => (),
>> +        Err(err) => return Err(err.into()),
>> +    }
>> +
> 
> [snip]
> 
>> +/// Delete the encryption key from config.
>> +///
>> +/// Returns true if the key was removed successfully, false if there was no matching key.
>> +/// Safety: caller must acquire and hold config lock.
>> +pub fn delete_key(id: &str, mut config: SectionConfigData) -> Result<bool, Error> {
>> +    if let Some((_, key)) = config.sections.remove(id) {
>> +        let key =
>> +            CryptKey::deserialize(key).map_err(|_err| format_err!("failed to parse key config"))?;
>> +
>> +        if key.archived_at.is_none() {
>> +            bail!("key still active, deleting is only possible for archived keys");
>> +        }
>> +
>> +        if let Some(key_path) = &key.info.path {
>> +            let key_lock_path = format!("{key_path}.lck");
>> +            // Avoid races with key insertion
>> +            let _lock = open_backup_lockfile(key_lock_path, None, true)?;
>> +
>> +            let key_config = KeyConfig::load(key_path)?;
>> +            let stored_key_info = KeyInfo::from(&key_config);
>> +            // Check the key is the expected one
>> +            if key.info.fingerprint != stored_key_info.fingerprint {
>> +                bail!("unexpected key detected in key file, refuse to delete");
>> +            }
>> +
>> +            let raw = CONFIG.write(ENCRYPTION_KEYS_CFG_FILENAME, &config)?;
>> +            // drops config lock
>> +            replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_bytes())?;
>> +
>> +            std::fs::remove_file(key_path)?;
> 
> Wouldn't it make more sense to delete the key before writing the config?
> If removing the key fails, an error would be returned, the key file
> would (possibly) still be there, but the config would already have been
> updated, leaving an orphaned key file.

The ordering here was chosen as is on purpose, see 
https://lore.proxmox.com/pbs-devel/b7d0f730-a574-4454-bf18-933db202db8b@proxmox.com/

> 
>> +            return Ok(true);
>> +        }
>> +
>> +        bail!("missing key file path for key '{id}'");
>> +    }
>> +    Ok(false)
>> +}
>> +
> 
> [snip]