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 ECE051FF13B for ; Wed, 08 Apr 2026 10:13:58 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 6CFAA3083; Wed, 8 Apr 2026 10:14:34 +0200 (CEST) Message-ID: <0e17a3bc-84b8-4942-ad46-e8eae669a61c@proxmox.com> Date: Wed, 8 Apr 2026 10:13:59 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox{,-backup} 00/20] fix #7251: implement server side encryption support for push sync jobs To: =?UTF-8?Q?Fabian_Gr=C3=BCnbichler?= , pbs-devel@lists.proxmox.com, Thomas Lamprecht References: <20260401075521.176354-1-c.ebner@proxmox.com> <185f5e2a-4c73-4e43-8089-f6383d5198cc@proxmox.com> <1775634036.o4lx1mdxmb.astroid@yuna.none> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <1775634036.o4lx1mdxmb.astroid@yuna.none> 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: 1775635973275 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.068 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: 5ET3HCDXVZ2U4HWN52N5VUCP3P7XSCHC X-Message-ID-Hash: 5ET3HCDXVZ2U4HWN52N5VUCP3P7XSCHC 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/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. 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? > > 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. > >>> 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?