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 667961FF141 for ; Mon, 13 Apr 2026 17:30:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 881983BB1; Mon, 13 Apr 2026 17:31:10 +0200 (CEST) Message-ID: Date: Mon, 13 Apr 2026 17:31:05 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 27/27] sync: pull: decrypt snapshots with matching encryption key fingerprint To: Thomas Lamprecht References: <20260411085154.1961287-1-t.lamprecht@proxmox.com> <20260411085154.1961287-11-t.lamprecht@proxmox.com> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <20260411085154.1961287-11-t.lamprecht@proxmox.com> 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: 1776094192103 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [pull.rs] Message-ID-Hash: Z3IZO4Y6IY3UVMOPBV4E24NGT56RC3IC X-Message-ID-Hash: Z3IZO4Y6IY3UVMOPBV4E24NGT56RC3IC 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 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: On 4/11/26 10:50 AM, Thomas Lamprecht wrote: > Am 10.04.26 um 18:54 schrieb Christian Ebner: >> diff --git a/src/server/pull.rs b/src/server/pull.rs >> @@ -647,6 +704,22 @@ async fn pull_snapshot<'a>( >> >> + // pre-existing local manifest for unencrypted snapshot, never overwrite with encrypted >> + if local_manifest_key_fp.is_some() && crypt_config.is_none() { >> + bail!("local unencrypted snapshot detected, refuse to sync without source decryption"); >> + } > > This condition looks inverted AFAICT. After decrypt-on-pull the local manifest > has key-fingerprint: null, so manifest.fingerprint() returns None, > local_manifest_key_fp is None, and the is_some() check never fires. > > The scenario it should catch: key removed from job after a previous > decrypt-on-pull, remote snapshot changed, local decrypted data gets silently > overwritten with encrypted content. > > I think checking local_manifest_file_fp.is_some() (the > change-detection-fingerprint presence) would be the right indicator for this > situation, at least FWICT that field only exists in manifests produced by the > decrypt path (but might warrant a closer check). Yes, you are absolutely right: I introduced the change-detection-fingerprint exactly for this after reports on v1 this being an issue, but messed up the check here... Will be fixed for the next iteration of the patches, including the nits below, thanks! > >> @@ -696,11 +769,38 @@ async fn pull_snapshot<'a>( >> >> + new_manifest.unprotected["key-fingerprint"] = Value::Null; > > nit: do we need to set the field explicitly to JSON null rather than removing > it via e.g. `.as_object_mut().unwrap().remove("key-fingerprint")` instead? but > no hard feelings here. > >> + new_manifest.unprotected = manifest.unprotected.clone(); > > nit: this copies verify state from the encrypted source manifest into the > decrypted target. The decryption itself acts as a verification of sorts (AEAD > would catch corruption), so this is fine, but a comment explaining the > reasoning would be IMO nice. > >> + info!("Found matching key fingerprint {source_fingerprint}, decrypt on pull"); > > nit: I'd suggest to also log which key ID matched, not just the fingerprint, as > that's then easier for the admin to cross-reference with the job config.