From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [IPv6:2a01:7e0:0:424::9]) by lore.proxmox.com (Postfix) with ESMTPS id BAD151FF140 for ; Fri, 24 Apr 2026 14:46:22 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 84902194CC; Fri, 24 Apr 2026 14:46:21 +0200 (CEST) From: =?UTF-8?q?Fabian=20Gr=C3=BCnbichler?= To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup 2/2] pull: decrypt: re-order encryption key/state checks Date: Fri, 24 Apr 2026 14:46:09 +0200 Message-ID: <20260424124615.654666-2-f.gruenbichler@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260424124615.654666-1-f.gruenbichler@proxmox.com> References: <20260424103607.531400-1-c.ebner@proxmox.com> <20260424124615.654666-1-f.gruenbichler@proxmox.com> MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777034686764 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 Message-ID-Hash: HBINJQZLOG5JV2T2BFQORLDIROPQHOIB X-Message-ID-Hash: HBINJQZLOG5JV2T2BFQORLDIROPQHOIB 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: make the full sequence of checks: - is source encrypted? if not -> regular pull - if a local snapshot exists -- is it encrypted or signed using the same key -> regular pull -- is it encrypted using a different key -> abort -- if it is signed, we either abort later or log + regular pull - find decryption key -- if none is found, but a local snapshot exists and the source is full encrypted -> abort -- if none is found, and no local snapshot exists -> regular pull -- only proceed for plain snapshots or signed-with-same key, since those could be the result of a decrypting pull - check signature -> abort if it doesn't match - if a local snapshot exists, check change-detection fingerprint, and abort if it doesn't match or doesn't exist, otherwise assume we can proceed with a decrypting sync Signed-off-by: Fabian Grünbichler --- still not super happy with this, but AFAICT this should at least handle one case better (the final bail) and deduplicate some code.. IMHO the encrypted = part should move to BackupManifest itself, since we have it in quite a few places already, but unfortunately ENOTIME right now ;) src/server/pull.rs | 80 ++++++++++++++++++++++++---------------------- 1 file changed, 42 insertions(+), 38 deletions(-) diff --git a/src/server/pull.rs b/src/server/pull.rs index 3dc5770f9..b4882718e 100644 --- a/src/server/pull.rs +++ b/src/server/pull.rs @@ -930,32 +930,43 @@ async fn optionally_use_decryption_key( return Ok((None, false)); // no fingerprint on source, regular pull }; + // check if source is encrypted or contents signed + let encrypted = manifest + .files() + .iter() + .all(|f| f.chunk_crypt_mode() == CryptMode::Encrypt); + + if let Some(existing_manifest) = existing_target_manifest { + if let Some(existing_fingerprint) = existing_manifest.fingerprint()? { + if existing_fingerprint == key_fp { + let target_encrypted = existing_manifest + .files() + .iter() + .all(|f| f.chunk_crypt_mode() == CryptMode::Encrypt); + if encrypted == target_encrypted { + // both sides are signed or encrypted with the same key, just resync + return Ok((None, false)); + } + } else { + // pre-existing local manifest for encrypted snapshot with key mismatch + bail!("Local encrypted or signed snapshot with different key detected, refuse to sync"); + } + } + }; + // source got key fingerprint, expect contents to be signed or encrypted let Some((key_id, config)) = params .crypt_configs .iter() .find(|(_id, crypt_conf)| crypt_conf.fingerprint() == *key_fp.bytes()) else { - // no matching key found in list of configured ones - if let Some(existing_target_manifest) = existing_target_manifest { - if let Some(existing_fingerprint) = existing_target_manifest - .fingerprint() - .with_context(|| prefix.clone())? - { - if existing_fingerprint != key_fp { - // pre-existing local manifest for encrypted snapshot with key mismatch - bail!( - "Local encrypted or signed snapshot with different key detected, refuse to sync" - ); - } - } else { - // pre-existing local manifest without key-fingerprint was previously decrypted, - // never overwrite with encrypted - bail!( - "local snapshot was previously decrypted but no matching decryption key is configured, refuse to sync" - ); - } - } else if !params.crypt_configs.is_empty() { + // all the other cases are handled above + if encrypted && existing_target_manifest.is_some() { + bail!("No matching key found, refusing sync"); + } + + // regular sync + if !params.crypt_configs.is_empty() { log_sender .log( Level::INFO, @@ -963,15 +974,12 @@ async fn optionally_use_decryption_key( ) .await?; } + return Ok((None, false)); }; // check if source is encrypted or contents signed - if !manifest - .files() - .iter() - .all(|f| f.chunk_crypt_mode() == CryptMode::Encrypt) - { + if !encrypted { log_sender .log( Level::WARN, @@ -986,16 +994,11 @@ async fn optionally_use_decryption_key( .context("failed to check source manifest signature") .with_context(|| prefix.clone())?; + let mut skip_resync = false; + // avoid overwriting pre-existing target manifest if let Some(existing_manifest) = existing_target_manifest { - if let Some(existing_fingerprint) = existing_manifest - .fingerprint() - .with_context(|| prefix.clone())? - { - if existing_fingerprint != key_fp { - bail!("Detected local encrypted or signed snapshot with encryption key mismatch!"); - } - } else if let Some(source_fp) = manifest + if let Some(source_fp) = manifest .get_change_detection_fingerprint() .context("failed to parse change detection fingerprint of source manifest") .with_context(|| prefix.clone())? @@ -1006,12 +1009,13 @@ async fn optionally_use_decryption_key( .signature(config) .with_context(|| prefix.clone())?; if target_fp == *source_fp.bytes() { - return Ok((None, true)); + skip_resync = true; + } else { + bail!("Change detection fingerprint mismatch, refuse to continue!"); } - - bail!("Change detection fingerprint mismatch, refuse to continue!"); + } else { + bail!("No change detection fingerprint found, refuse to continue!"); } - return Ok((None, false)); } log_sender @@ -1021,7 +1025,7 @@ async fn optionally_use_decryption_key( ) .await?; - Ok((Some(Arc::clone(config)), false)) + Ok((Some(Arc::clone(config)), skip_resync)) } /// Pulls a `snapshot`, removing newly created ones on error, but keeping existing ones in any case. -- 2.47.3