From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id 16D7B1FF13B for ; Wed, 08 Apr 2026 10:28:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 256E73CE8; Wed, 8 Apr 2026 10:29:23 +0200 (CEST) Message-ID: <75b72b5a-10fe-4947-a51e-1088735cfbb0@proxmox.com> Date: Wed, 8 Apr 2026 10:29:13 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Beta Subject: Re: [PATCH proxmox{,-backup} 00/20] fix #7251: implement server side encryption support for push sync jobs To: Christian Ebner , =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pbs-devel@lists.proxmox.com References: <20260401075521.176354-1-c.ebner@proxmox.com> <185f5e2a-4c73-4e43-8089-f6383d5198cc@proxmox.com> <1775634036.o4lx1mdxmb.astroid@yuna.none> <0e17a3bc-84b8-4942-ad46-e8eae669a61c@proxmox.com> Content-Language: en-US, de-DE From: Thomas Lamprecht In-Reply-To: <0e17a3bc-84b8-4942-ad46-e8eae669a61c@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775636887764 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.001 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 RCVD_IN_VALIDITY_CERTIFIED_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_RPBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. RCVD_IN_VALIDITY_SAFE_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to Validity was blocked. See https://knowledge.validity.com/hc/en-us/articles/20961730681243 for more information. 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: 7U72ZPN44BUZNVOINYU6UBQQVZPVHJOC X-Message-ID-Hash: 7U72ZPN44BUZNVOINYU6UBQQVZPVHJOC X-MailFrom: t.lamprecht@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 08/04/2026 10:12, Christian Ebner wrote: > On 4/8/26 9:49 AM, Fabian Grünbichler wrote: >> On April 2, 2026 9:37 am, Christian Ebner wrote: >>> On 4/2/26 2:24 AM, Thomas Lamprecht wrote: >>>> Am 01.04.26 um 09:55 schrieb Christian Ebner: >>>>> This patch series implements support for encrypting backup snapshots >>>>> when pushing from a source PBS instance to an untrusted remote target >>>>> PBS instance. Further, it adds support to decrypt snapshots being >>>>> encrypted on the remote source PBS when pulling the contents to the >>>>> local target PBS instance. >>>> >> >> [..] >> >>> >>>> When a server-side key is configured on push, all snapshots with any >>>> encrypted content are silently skipped. That includes fully client-side >>>> encrypted ones that could just be synced as-is - they are already encrypted >>>> after all. Might be better to push those unchanged and only skip genuinely >>>> partially-encrypted snapshots? Or if unsure, allow to user to chose what >>>> they want. Wrapping encryption is probably never a good idea though, >>>> maybe just any parts that are not yet encrypted? >>> >>> Pushing fully encrypted ones as is makes sense, only log and skip >>> partially encrypted snapshots. >> >> pushing fully encrypted snapshots *if they use the same key*, or all >> fully encrypted snapshots? I think the former is okay, the latter might >> be confusing/unexpected and lead to "I have all my backups on the remote >> but can't decrypt them because of the wrong key"? > > after pondering about this a bit I must agree, mixing of content encrypted by different keys during sync will cause headache for users not expecting it. Well, can they really loose? Not having a backups synced might be unexpected too, and while the user could create another sync job, having to do so can be a bit of a hassle. Also, the contract of "everything gets encrypted, if it already is, it gets just send along as is" doesn't sound that complex to me. > Given this, I would rather keep the handling as is, meaning already encrypted backups are not pushed and maybe even add a check to detect if the previous manifest of the backup group (if present) was encrypted using the same key, refusing to sync at all if not? Confusion might be better resolved through docs and task log warnings, depending on gravity, over refusing to sync something, which can be fatal if the primary data source breaks and one searches for backups only to find that they did not get synced out of confusion protection. Don't get me wrong, I agree that this is not ideal and Fabian has a point, but with syncing or not syncing data one needs to be very careful for a backup server appliance. >> e.g.: >> >> client A uses key A to backup to PBS X >> client B uses plaintext to backup to PBS X >> PBS X uses key B to sync to PBS Y >> >> user thinks key B is enough to restore backups from Y (after all, it's >> the encryption key used for syncing), and destroys clients A and B and >> PBS X (all in the same location maybe?). now they can't recover client >> A.. >> >> I know this is technically on the user (they didn't pay attention to >> their key management), but seems like a potential foot gun. if we skip >> the groups of client A because of a key mismatch (and warn about that?) >> then it is at least obvious that those backups *are not on Y*. we could >> have a flag for allowing key mismatches to support such scenarios >> opt-in? > > I would rather not allow to flag this, but make the user explicitly sync such snapshots if truly wanted. After all we do already have the encrypted-only in place, which allows to sync encrypted snapshots without touching the others. It seems to me that making mixing contents the undesired, hard route is the better way, given also the pull decrypt mismatch issues with 2 jobs using different keys as reported by Manuel. Extra option is naturally adding complexity, but it would allow making the decision by the users, i.e. the ones that can actually know what the right choice for them is. But IMO we could imply the sync everything, but encrypt only unecrypted part now, given that datastores and namespaces often are organised such that most backups in there are the same and that we have already lots of possibilities to filter out stuff, and if user demand for skipping these comes up, then we could add a filter flag for exclude/include encrypted (and for another usecase also unencrypted) backup snapshots from the job selection, which would come relatively naturally and not be just another flag tacked on the job options. >> >>>> Nit: The same `encryption_key` field means "encrypt" for push and >>>> "decrypt" for pull. Maybe be literal here and name it decryption_key for >>>> the later. Or something generic that works for both. >>> >>> Yes, that make sense... Was primed by the fact that the entities >>> themself are referred to as encryption keys, but renaming this to >>> decryption_key for the pull makes more sense for making the code better >>> readable. Will adapt that. >>> >>>> A few key lifecycle things: >>>> >>>> Deleting a key does not check whether sync jobs reference it; the next run >>>> just fails. Might want to refuse deletion while references exist, or at >>>> least warn. >>> >>> Yes, also realized this just now, will add a check for this in the api >>> handler and not allow to remove it in that case. >> >> key rotation/lifecycle management would be a question in any case - we >> could have one "active" key per sync job, and a list of archived ones >> that are only used for decryption? a key rotation would obviously break >> deduplication chains.. such a scheme would also allow re-encryption on >> the fly of client-encrypted backups, if a matching key is provided - >> though the use case for that seems limited to me (why encrypt in the >> first place if you trust this PBS with the plaintext data?). >> >> or alternatively it would need to be documented that for rotating a key, >> you want to create a new key and target namespace, then switch the sync >> job over (syncing everything from scratch), and delete the old copies >> encrypted with the previous key once the re-sync is done? > > Given above, I think the alternative option might be the better way. Mixing snapshots in a group with different keys should not be encouraged due to possible key loss as described above. But making it possible to archive the key, so it is still kept around for decryption, but not usable to push new encrypted backups is definitely something I should implement, yes. And probably it should not be possible to remove a key, which has not been marked as archived first, even if currently not in use by any sync jobs? > Yeah, key removal might be well something that benefits from some extra steps and/or warnings needed.