From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id B6D5C1FF13A for ; Wed, 22 Jul 2026 13:31:49 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 7D3EB214E3; Wed, 22 Jul 2026 13:31:49 +0200 (CEST) MIME-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit Subject: Re: [PATCH proxmox-backup v2 5/5] sync: push: gracefully handle previous manifest signature mismatches From: Robert Obkircher To: Christian Ebner In-Reply-To: <20260507130135.589100-6-c.ebner@proxmox.com> References: <20260507130135.589100-1-c.ebner@proxmox.com> <20260507130135.589100-6-c.ebner@proxmox.com> Date: Wed, 22 Jul 2026 13:31:06 +0200 Message-Id: <178471986679.177215.10555006588074374450.b4-review@b4> X-Mailer: b4 0.16-dev X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784719847085 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.230 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: CILC7OFU7X5TT4NHEPMHDODE6SMGRZIH X-Message-ID-Hash: CILC7OFU7X5TT4NHEPMHDODE6SMGRZIH X-MailFrom: r.obkircher@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: > 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 > + .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? (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"? > + }; > + > + 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. -- Robert Obkircher