From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 706CE1FF13A for ; Wed, 22 Jul 2026 16:49:15 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 40A09214AE; Wed, 22 Jul 2026 16:49:15 +0200 (CEST) Message-ID: Date: Wed, 22 Jul 2026 16:49:08 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH proxmox-backup v2 5/5] sync: push: gracefully handle previous manifest signature mismatches To: Robert Obkircher References: <20260507130135.589100-1-c.ebner@proxmox.com> <20260507130135.589100-6-c.ebner@proxmox.com> <178471986679.177215.10555006588074374450.b4-review@b4> Content-Language: en-US, de-DE From: Christian Ebner In-Reply-To: <178471986679.177215.10555006588074374450.b4-review@b4> 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: 1784731720590 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.176 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust 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: YJH4JTO2GUSIBHP65CDISVG4UAHSCVCZ X-Message-ID-Hash: YJH4JTO2GUSIBHP65CDISVG4UAHSCVCZ 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 7/22/26 1:30 PM, Robert Obkircher wrote: >> During push sync jobs with a given active encryption key, the key is >> loaded for the backup writer, used to encrypt the source snapshot on >> the fly. As optimization, the backup writer deduplicates chunks >> already present in the previous snapshot by loading them from the >> index files as stored in the previous manifest. >> >> Reuse of the previous manifest and index must however only happen if: >> - The previous manifest is not encrypted, neither will push encrypt. >> - The previous manifest is encrypted and passes signature >> verification using the same key to be used during push encryption. >> >> Since these checks are now performed when loading the previous >> manifest, drop redundant and now useless checks performed before >> loading the chunks from index files. >> >> Signed-off-by: Christian Ebner >> >> diff --git a/src/server/push.rs b/src/server/push.rs >> index 17fa605a4..6df83cbd8 100644 >> --- a/src/server/push.rs >> +++ b/src/server/push.rs >> @@ -979,25 +979,12 @@ pub(crate) async fn push_group( >> } >> >> async fn load_previous_snapshot_known_chunks( >> - params: &PushParameters, >> upload_options: &UploadOptions, >> backup_writer: &BackupWriter, >> archive_name: &BackupArchiveName, >> known_chunks: Arc>>, >> ) { >> if let Some(manifest) = upload_options.previous_manifest.as_ref() { >> - if let Some((_id, crypt_config)) = ¶ms.crypt_config { >> - // no fingerprint in previous manifest -> no reuse possible >> - let Ok(Some(fingerprint)) = manifest.fingerprint() else { >> - return; >> - }; >> - >> - if *fingerprint.bytes() != crypt_config.fingerprint() { >> - // key mismatch -> no reuse possible >> - return; >> - } >> - } >> - >> // Add known chunks, ignore errors since archive might not be present and it is better >> // to proceed on unrelated errors than to fail here. >> match archive_name.archive_type() { >> @@ -1128,18 +1115,73 @@ pub(crate) async fn push_snapshot( >> .await >> .with_context(|| prefix.to_string())?; >> >> - let mut previous_manifest = None; >> // Use manifest of previous snapshots in group on target for chunk upload deduplication >> - if fetch_previous_manifest { >> - match backup_writer.download_previous_manifest(false).await { >> - Ok(manifest) => previous_manifest = Some(Arc::new(manifest)), >> + let previous_manifest = if !fetch_previous_manifest { >> + None >> + } else { >> + let result = backup_writer >> + .download_previous_manifest(false) >> + .await >> + .map_err(|err| err.context("failed to download")) > nit: this could instead call Context::context without the map_err Ack, will adapt. >> + .and_then(|manifest| { >> + let fingerprint = manifest >> + .fingerprint() >> + .context("failed getting fingerprint")?; >> + >> + let Some(manifest_key_fp) = fingerprint else { >> + if encrypt_using_key.is_none() { >> + return Ok(Arc::new(manifest)); > What if encrypt_using_key is None because the local chunks are already > encrypted? Shouldn't we expect a fingerprint in that case? The fingerprint here is the one of the previous manifest on the remote, so if that has no fingerprint (not encrypted), and we have no key to encrypted (encrypt_using_key == None) it is fine to use the manifest for de-duplication during chunk upload of the contained matching index files, analog to an incremental backup. So if the local snapshot to be pushed is already encrypted (not on the fly encrypted), it will be allowed to reuse the manifest, but there will be no matching chunk digest. Just like for a regular push without any server side encryption. At least that is the intended behavior. > > (I'm really not sure if I understand this) >> + } >> + bail!("manifest not encrypted"); > This error message confused me initially because, according to the 3rd > commit, manifest blobs are never encrypted. Should it maybe be "backup > not encrypted" or "missing fingerprint"? Will improve, thanks! >> + }; >> + >> + let signed_only = manifest >> + .files() >> + .iter() >> + .all(|f| f.chunk_crypt_mode() == CryptMode::None); >> + >> + let Some((_key_id, crypt_config)) = &encrypt_using_key else { >> + if signed_only { >> + return Ok(Arc::new(manifest)); >> + } >> + bail!("manifest encrypted but no encryption key configured"); > My previous two comments might also apply here. >