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

Christian Ebner
2026-04-15T06:48:23Z

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]