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

["Michael Köppl" <m.koeppl@proxmox.com>]

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.

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.

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

[snip]