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

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

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).