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 92B581FF13A for ; Wed, 29 Apr 2026 13:43:14 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D139C33E7; Wed, 29 Apr 2026 13:43:12 +0200 (CEST) Date: Wed, 29 Apr 2026 13:43:04 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup 2/2] sync: push: gracefully handle previous manifest signature mismatches To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260429092847.381438-1-c.ebner@proxmox.com> <20260429092847.381438-3-c.ebner@proxmox.com> In-Reply-To: <20260429092847.381438-3-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1777462096.q4ecgbr8bo.astroid@yuna.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777462890217 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.054 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 URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [push.rs] Message-ID-Hash: VWWPAIV5OCRU2GEGXKTJTB4MG2IQXJBV X-Message-ID-Hash: VWWPAIV5OCRU2GEGXKTJTB4MG2IQXJBV X-MailFrom: f.gruenbichler@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 April 29, 2026 11:28 am, Christian Ebner 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. >=20 > If the previous backup snapshot in the same group (on the remote > target) is however encrypted or signed using a different key, the > signature check will fail, logging a rather alerting signature > mismatch, which is however benign. >=20 > Improve the logging behaviour by loading the manifest without > implicit signature check, check it on the call site and log > a more telling `Skip chunk deduplication from previous manifest`, > as that is the intended behaviour for this case. >=20 > Signed-off-by: Christian Ebner > --- > src/server/push.rs | 24 ++++++++++++++++++++++-- > 1 file changed, 22 insertions(+), 2 deletions(-) >=20 > diff --git a/src/server/push.rs b/src/server/push.rs > index dac62c84a..f4b68701d 100644 > --- a/src/server/push.rs > +++ b/src/server/push.rs > @@ -1131,8 +1131,28 @@ pub(crate) async fn push_snapshot( > let mut previous_manifest =3D None; > // Use manifest of previous snapshots in group on target for chunk u= pload deduplication > if fetch_previous_manifest { > - match backup_writer.download_previous_manifest().await { > - Ok(manifest) =3D> previous_manifest =3D Some(Arc::new(manife= st)), > + match backup_writer > + .download_previous_manifest_without_signature_check() > + .await > + { > + Ok(manifest) =3D> { > + if let Some((_id, crypt_config)) =3D &encrypt_using_key = { > + if manifest.check_signature(crypt_config).is_ok() { > + previous_manifest =3D Some(Arc::new(manifest)); see below, but this is incomplete - if there is no signature, this will also return ok! > + } else { > + log_sender > + .log( > + Level::INFO, > + format!( > + "{prefix}: Skip chunk deduplication = from previous manifest" > + ), > + ) > + .await? > + } this part should check if the signature was made with the configured key first, before attempting to validate the signature.. we basically have a matrix here of previous manifest state and whether we ware doing a pushing encrypt or not.. if we don't have a key, we only need to skip the previous manifest if it's encryption state doesn't match the currently pushed snapshot. this is missing (but maybe handled somewhere else?). if we have a key, we want to - check if the previous snapshot was encrypted, if not, we cannot use it but this is fine. - if it was encrypted, we want to check if it was encrypted using our key - if not, we cannot use it, but this is fine. - if it was encrypted using our key, we want to validate the signature - if that fails, we cannot use it and should probably emit a warning, something is very wrong (either the fingerprint or the signature is invalid, which should never happen) > + } else { > + previous_manifest =3D Some(Arc::new(manifest)); > + } > + } > Err(err) =3D> { > log_sender > .log( > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20