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 4340D1FF13C for ; Thu, 02 Apr 2026 09:37:00 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 1A7A0A90F; Thu, 2 Apr 2026 09:37:29 +0200 (CEST) Message-ID: <185f5e2a-4c73-4e43-8089-f6383d5198cc@proxmox.com> Date: Thu, 2 Apr 2026 09:37:25 +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: Thomas Lamprecht , pbs-devel@lists.proxmox.com References: <20260401075521.176354-1-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: 7bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1775115387527 X-SPAM-LEVEL: Spam detection results: 0 AWL -1.433 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: MBUZKCKB4LBDCCZHOLOOKE5X7WJ7HVA6 X-Message-ID-Hash: MBUZKCKB4LBDCCZHOLOOKE5X7WJ7HVA6 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/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. > > 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. Yes, you are right this is still needs optimization and I'm aware of it. What could work is to either store the previous manifest checksum for comparison. I didn't want to spent to much time on optimization just yet, this is still something I wanted to explore further if the overall approach is fine. > 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? That is not intentional and will be fixed, thanks! > 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. Will fix this as well! > 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. Okay, will escalate to Sys.Modify on the key acl also for loading the key. > 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. > 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. > 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. Good catch, password protected keys should not be uploadable for the time being. It was discussed off list that we might extend this to allow for password protected keys in the future. An intial draft version simply stored the key in the config as well, but since that does not add anything security wise and there are ideas to improve secret handling in general, supporting encrypted keys was dropped for now. > 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. Will strip this when decrypting, thanks! > 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. Will look into these as well, thanks a lot for comments and review!