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 046111FF0E0 for ; Thu, 23 Jul 2026 11:33:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id BB4E9214D6; Thu, 23 Jul 2026 11:32:59 +0200 (CEST) Message-ID: Date: Thu, 23 Jul 2026 11:32:55 +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: Christian Ebner 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-AT From: Robert Obkircher In-Reply-To: Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1784799146079 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.224 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: DKUO4K3WCYUPAOWCNYWU7WEMSBZY36CC X-Message-ID-Hash: DKUO4K3WCYUPAOWCNYWU7WEMSBZY36CC 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: On 22.07.26 16:48, Christian Ebner wrote: > 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.  > [..] >>> +            .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.  Isn't it technically always fine to use it for deduplication and just inefficient when nothing matches? The commit message states that we must not reuse when going from unencrypted to "push encrypted". Why is that forbidden while going from unencrypted to before-push encrypted seems to be allowed? Is it because we know for sure that "push encrypt" will encrypt everything, while the other case could still reference unencrypted chunks? > >> >> (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. >> >