Re: [PATCH proxmox-backup v2 06/27] pbs-config: implement encryption key config handling

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 10.04.26 um 18:54 schrieb Christian Ebner:
> diff --git a/pbs-config/src/encryption_keys.rs b/pbs-config/src/encryption_keys.rs
> @@ -0,0 +1,210 @@
>
> +    let key_path = format!("{ENCRYPTION_KEYS_DIR}{id}.enc");
> +
> +    // lock to avoid race with key deletion, file ownership and permissions will
> +    // be adapted by replacing file on key store below.
> +    open_backup_lockfile(&key_path, None, true)?;
> +
> +    // assert the key file is empty (new)
> +    let metadata = std::fs::metadata(&key_path)?;
> +    if metadata.len() > 0 {
> +        bail!("detected pre-existing key file, refusing to overwrite.");
> +    }

The lockfile _is_ the key file path, which for one makes the check bogus
AFAICT, as a previous open_backup_lockfile creates it if it does not exist.  So
the metadata check always succeeds on a fresh create (file exists, length 0),
and on a stale leftover from a previous failed attempt it also passes (empty
lockfile). And for another reusing the file to protect as lock file can have
problems as the lock file never must be deleted, but for atomic (config, key,
...) writes one needs to use write-tmp+rename, which then breaks locking.

I'd add and use something like:

let key_lock_path = format!("{key_path}.lck");

Then the metadata len check afterwards should also work.

> +pub fn delete_key(id: &str, mut config: SectionConfigData) -> Result<bool, Error> {

nit: this takes an owned SectionConfigData, which is unusual compared to the
other config helpers that manage their own lock.
Also, the caller (API handler) holds the lock, which is fine, but a doc comment
noting that requirment might be good here.

> +        replace_backup_config(ENCRYPTION_KEYS_CFG_FILENAME, raw.as_bytes())?;
> +
> +        // drop key file lock
> +        std::fs::remove_file(key_path)?;

nit: if remove_file fails here, the config was already updated (key section
removed). Not dangerous since the key is no longer referenced, but you'd leave
a stale .enc file. Maybe just log a warning and return Ok instead of
propagating the error?