Re: [PATCH proxmox-backup v3 10/30] sync: add helper to check encryption key acls and load key

Thomas Lamprecht
2026-04-19T21:08:22Z

Am 14.04.26 um 14:59 schrieb Christian Ebner:
> diff --git a/src/server/sync.rs b/src/server/sync.rs
>
> +/// Helper to check user having access to the given encryption key and loading
> +/// the it using the passphrase from the config.

Two small doc nits:

- "loading the it" - typo, should be "and loading it".
- "using the passphrase from the config" might be slightly misleading.

> +pub(crate) fn check_privs_and_load_key_config(
> + key_id: &str,
> + user: &Authid,
> + fail_on_archived: bool,
> +) -> Result<Option<Arc<CryptConfig>>, Error> {
> + ...
> + let key_config = pbs_config::encryption_keys::load_key_config(key_id, fail_on_archived)?;
> + // pass empty passphrase to get raw key material of unprotected key
> + let (enc_key, _created, fingerprint) = key_config.decrypt(&|| Ok(Vec::new()))?;
> + ...
> + Ok(Some(crypt_config))
> +}

The return type is Option<Arc<CryptConfig>> but it seems that the function never
actually returns Ok(None), it's eithers Some() on success or an Err() on
failure. Caller like PullParameters::new, PushParameters::new, or
sync_user_can_access_optional_key then have basically dead code handling the
None case.

It's probably fine to just drop the Option wrapper and return
Result<Arc<CryptConfig>, Error> directly.

> + user_info.check_privs(
> + user,
> + &["system", "encryption-keys", key_id],
> + PRIV_SYS_MODIFY,
> + true,
> + )?;

Using PRIV_SYS_MODIFY here feels somewhat heavy for the "use a key for sync"
operation - SYS_MODIFY traditionally gates config-modifying actions, while
using a key to encrypt/decrypt is more of a "read/use" semantic. Not a
blocker since it's consistent with the other endpoints in the series, but
worth a design consideration for whether a dedicated privilege (or
PRIV_SYS_AUDIT for use-only) would fit better long-term (can definitively be
checked only after this landed first, no point in bloating up the scope now
to much).