From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 1E9A11FF13B for ; Wed, 22 Apr 2026 17:28:46 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id DD2A9244A2; Wed, 22 Apr 2026 17:28:45 +0200 (CEST) Message-ID: Date: Wed, 22 Apr 2026 17:28:41 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v4 13/30] api: config: allow encryption key manipulation for sync job To: =?UTF-8?Q?Michael_K=C3=B6ppl?= , pbs-devel@lists.proxmox.com References: <20260420161533.1055484-1-c.ebner@proxmox.com> <20260420161533.1055484-14-c.ebner@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776871634084 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.071 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: GBYNZ2O2EA6URDR6S2XXVK6MHOKNTRXS X-Message-ID-Hash: GBYNZ2O2EA6URDR6S2XXVK6MHOKNTRXS X-MailFrom: c.ebner@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 >