Re: [PATCH proxmox-backup v4 13/30] api: config: allow encryption key manipulation for sync job

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

Noticed a problem when updating sync jobs. Tried to explain it below.
Hope it's comprehensible and I didn't miss anything.

On Mon Apr 20, 2026 at 6:15 PM CEST, Christian Ebner wrote:

[snip]

>      if let Some(group_filter) = update.group_filter {
>          data.group_filter = Some(group_filter);
> @@ -555,6 +603,34 @@ pub fn update_sync_job(
>          data.sync_direction = Some(sync_direction);
>      }
>  
> +    if let Some(active_encryption_key) = update.active_encryption_key {
> +        // owner updated above already, so can use the one in data
> +        let owner = data
> +            .owner
> +            .as_ref()
> +            .unwrap_or_else(|| Authid::root_auth_id());
> +        sync_user_can_access_optional_key(Some(&active_encryption_key), owner, true)?;
> +
> +        keep_previous_key_as_associated(
> +            data.active_encryption_key.as_ref(),
> +            &mut update.associated_key,
> +        );
> +        data.active_encryption_key = Some(active_encryption_key);


Suppose the user has a sync job configured with the following keys:

- Active key: key-a
- Associated keys: key-b

Now the user updates the sync job with

    proxmox-backup-manager sync-job update "${SYNC_JOB_ID}" \
        --active-encryption-key "key-c"

Note that there's explicitly no associated keys set as part of the
update. Then `update.associated_key` is None when passed into
`keep_previous_key_as_associated`, therefore following the match arm

```
    ...
    None => *associated_keys = Some(vec![prev.clone()]),
    ...
```

putting the previously active key as the only element of the list of
associated keys.

continue in the next comment...

> +    }
> +
> +    if let Some(associated_key) = update.associated_key {
> +        // owner updated above already, so can use the one in data
> +        let owner = data
> +            .owner
> +            .as_ref()
> +            .unwrap_or_else(|| Authid::root_auth_id());
> +        // Don't allow associating keys the local user/owner can't access
> +        for key in &associated_key {
> +            sync_user_can_access_optional_key(Some(key), owner, false)?;
> +        }
> +        data.associated_key = Some(associated_key);

Since above `update.associated key` is set to a vector containing only
the previously active key, this point is reached and the list of
associated keys is overwritten. The resulting sync job will then have
the following:

- Active key: key-c
- Associated keys: key-a

key-b is gone without the user explicitly removing it or updating the
list of associated keys.

> +    }
> +
>      if update.limit.rate_in.is_some() {
>          data.limit.rate_in = update.limit.rate_in;
>      }
> @@ -727,6 +803,8 @@ acl:1:/remote/remote1/remotestore1:write@pbs:RemoteSyncOperator
>          run_on_mount: None,
>          unmount_on_done: None,
>          sync_direction: None, // use default
> +        active_encryption_key: None,
> +        associated_key: None,
>      };
>  
>      // should work without ACLs