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 A37021FF135 for ; Sun, 19 Apr 2026 23:08:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 58C26177CF; Sun, 19 Apr 2026 23:08:23 +0200 (CEST) From: Thomas Lamprecht To: c.ebner@proxmox.com Subject: Re: [PATCH proxmox-backup v3 19/30] fix #7251: api: push: encrypt snapshots using configured encryption key Date: Sun, 19 Apr 2026 22:41:55 +0200 Message-ID: <20260419210610.3915597-6-t.lamprecht@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260414125923.892345-20-c.ebner@proxmox.com> References: <20260419210610.3915597-1-t.lamprecht@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1776632780673 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.002 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: DICLAATJGUM65ZG4HXYASAJMOF3JKB7X X-Message-ID-Hash: DICLAATJGUM65ZG4HXYASAJMOF3JKB7X 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 CC: pbs-devel@lists.proxmox.com 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 14.04.26 um 14:59 schrieb Christian Ebner: > diff --git a/src/server/push.rs b/src/server/push.rs > > + if all_unencrypted { > + encrypt_using_key = params.crypt_config.clone(); > + info!("Encrypt and push unencrypted snapshot '{snapshot}'"); > + } else if any_unencrypted { > + warn!("Encountered partially encrypted snapshot '{snapshot}', refuse to re-encrypt and skip"); > + return Ok(stats); > + } else { > + info!("Pushing already encrypted snapshot '{snapshot}' without re-encryption"); > + } The "already fully encrypted" case pushes client-encrypted content through unchanged, regardless of whether the configured sync key matches the client key. That's the right behavior (re-encrypting client- encrypted content is impossible without the client key), but from the user side this is surprising: they assigned a sync key and the log only says "without re-encryption" - nothing indicates that the snapshot is *not* using the configured sync key at all. Please document this explicitly in the docs patch (30), and consider making the log message say something like "already encrypted with client key, configured sync key is not applied". Also, does a push job with an `encrypted-only=true` filter + a configured sync key make sense at all? If yes, the interaction is worth mentioning too. > - target_manifest.unprotected = source_manifest.unprotected; > - target_manifest.signature = source_manifest.signature; > + target_manifest.unprotected = source_manifest.unprotected.clone(); > + target_manifest.signature = source_manifest.signature.clone(); When encrypt_using_key is Some, the target manifest's per-file csums differ from the source (new ciphertexts), so a copied-over `signature` won't verify on the target anymore. PBS-to-PBS sync rarely carries client signatures in practice, but when it does, this produces a snapshot that fails signature verification. Probably safest to drop signature in the encrypt path and let the target manifest be unsigned. Minor observation on commit hygiene: patch 16 adds `let mut encrypt_using_key = None;` which is never assigned in that commit (only starting here in patch 19), triggering an unused_mut warning on the intermediate commit. Same pattern for `let mut new_manifest = None;` in patch 26 vs its assignment in patch 28. Not critical, but it means bisect-compiling each commit with -D warnings trips. Easy fix is to introduce the `mut` in the same patch that first assigns it, or to use `#[allow(unused_mut)]` on the intermediate.