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

[Christian Ebner <c.ebner@proxmox.com>]

On 4/22/26 5:16 PM, Michael Köppl wrote:
> Noticed a problem when updating sync jobs. Tried to explain it below.
> Hope it's comprehensible and I didn't miss anything.

no, all clear! Good catch! Should be easy to fix by additional checking 
and will be fixed for the upcoming version.

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