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 79D5C1FF13C for ; Thu, 02 Apr 2026 02:24:51 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 0207E373F; Thu, 2 Apr 2026 02:25:20 +0200 (CEST) Message-ID: Date: Thu, 2 Apr 2026 02:25:14 +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 , pbs-devel@lists.proxmox.com References: <20260401075521.176354-1-c.ebner@proxmox.com> Content-Language: en-US From: Thomas Lamprecht In-Reply-To: <20260401075521.176354-1-c.ebner@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775089456854 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.498 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 1 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 1 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 1 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: EGQENZPPEVIHTCSQCFDZYTK6EQDRNOE7 X-Message-ID-Hash: EGQENZPPEVIHTCSQCFDZYTK6EQDRNOE7 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: 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. Thanks, the overall direction makes sense, a few design-level concerns below, but note that this is includes various stuff all over the place, I did not yet rechecked them with a fresh head, so do not take them for face value, rather input/questions to your own surely much more mature mental model. Pull resync after decryption is efficiency-wise not ideal. The local decrypted manifest will never byte-match the remote encrypted one, so the `raw_data()` fast path in pull_snapshot can never fire for already-decrypted snapshots. The per-file verification then also fails (different checksums), causing a full re-download. The `last_sync_time` filtering limits this to the latest snapshot per group, so it's not that bad, but probablly still worth to improve - especially for large snapshots that rarely change. On the push side, `upload_blob` is a raw byte passthrough, so an unencrypted source backup will then e.g. have `.conf.blob` or `.pxar.catalog.blob` get stored unencrypted on the untrusted remote. Might be nicer to also encrypt those here from security and an integrity standpoint? Also, `upload_blob_from_file` for the client log goes through `upload_blob_from_data` which calls `DataBlob::encode()` on already-encoded on-disk bytes, so you'd end up with a double-wrapped blob on the remote. Minor: `check_privs_and_load_key_config` uses `PRIV_SYS_AUDIT` to gate key access. It's not a real security issue since creating/running sync jobs already requires much heavier privileges (Datastore.Backup, Remote.Read, etc.), but Audit is semantically a read-only privilege. Might be worth using something like Sys.Modify instead to better express the intent. We could also create a few new privs for this, if it helps. 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? 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. 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. Password-protected keys can be uploaded but in the method `check_privs_and_load_key_config` you always pass an empty passphrase to its `key_config.decrypt()` call, failing with "Passphrase is too short!" in such a case. Verify state gets copied from the encrypted source manifest into the decrypted target. That verification was against encrypted data though, so might want to clear it on decryption. Albeit, the decryption itself basically was a verification, so not really problematic. Some other things: `load_previous_snapshot_known_chunks` always calls `download_previous_fixed_index` even for DynamicIndex archives, breaking being able to leverage the deduplication for cross-snapshot chunk dedup for .didx (error is silently swallowed via `let _res`) AFAICT. The `.dectmp` temp file from blob decryption on pull is not cleaned up on error, and `create_new(true)` means a retry fails then too. `log::info!` vs `tracing::info!` in check_privs_and_load_key_config; "indetical" typo (x2) in the index rewrite comments.