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 4ADAC1FF140 for ; Fri, 24 Apr 2026 12:36:27 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 8C821131FA; Fri, 24 Apr 2026 12:36:24 +0200 (CEST) From: Christian Ebner To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup 2/3] sync: pull: refactor decryption key loading checks Date: Fri, 24 Apr 2026 12:36:06 +0200 Message-ID: <20260424103607.531400-3-c.ebner@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260424103607.531400-1-c.ebner@proxmox.com> References: <20260424103607.531400-1-c.ebner@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1777026888735 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.070 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: AL3UQZ7RZHA5CE4TVSYE3OPN5RDTYQAI X-Message-ID-Hash: AL3UQZ7RZHA5CE4TVSYE3OPN5RDTYQAI 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 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: In an effort to improve code readability and maintanablity. By pulling out the logic into a dedicated helper, nested branches can be switched over to early return statements instead, greatly reducing the required nesting level. Signed-off-by: Christian Ebner --- src/server/pull.rs | 225 +++++++++++++++++++++++++++------------------ 1 file changed, 137 insertions(+), 88 deletions(-) diff --git a/src/server/pull.rs b/src/server/pull.rs index 051405ae7..d0f886cb5 100644 --- a/src/server/pull.rs +++ b/src/server/pull.rs @@ -755,95 +755,28 @@ async fn pull_snapshot<'a>( return Ok(sync_stats); } - let mut crypt_config = None; - let mut new_manifest = None; - if let Some(key_fp) = manifest.fingerprint().with_context(|| prefix.clone())? { - // source got key fingerprint, expect contents to be signed or encrypted - if let Some((key_id, config)) = params - .crypt_configs - .iter() - .find(|(_id, crypt_conf)| crypt_conf.fingerprint() == *key_fp.bytes()) - { - // check if source is encrypted or contents signed - if !manifest - .files() - .iter() - .all(|f| f.chunk_crypt_mode() == CryptMode::Encrypt) - { - log_sender - .log( - Level::WARN, - format!("Snapshot not fully encrypted, sync as is despite matching key '{key_id}' with fingerprint {key_fp}"), - ) - .await?; - } else { - manifest - .check_signature(config) - .context("failed to check source manifest signature") - .with_context(|| prefix.clone())?; - - 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 - .get_change_detection_fingerprint() - .context("failed to parse change detection fingerprint of source manifest") - .with_context(|| prefix.clone())? - { - // Stored fp is HMAC over the unencrypted source's protected fields; recompute - // over the locally decrypted manifest, not the fresh encrypted remote one. - let target_fp = existing_manifest - .signature(config) - .with_context(|| prefix.clone())?; - if target_fp == *source_fp.bytes() { - fetch_log().await?; - cleanup().await?; - return Ok(sync_stats); // nothing changed - } - - bail!("Change detection fingerprint mismatch, refuse to continue"); - } - } else { - log_sender - .log( - Level::INFO, - format!("Found matching key '{key_id}' with fingerprint {key_fp}, decrypt on pull"), - ) - .await?; - crypt_config = Some(Arc::clone(config)); - new_manifest = Some(Arc::new(Mutex::new(BackupManifest::new(snapshot.into())))); - } - } - } else 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() { - log_sender - .log( - Level::INFO, - format!("{prefix}: No matching key found, sync without decryption"), - ) - .await?; + let (crypt_config, new_manifest) = match optionally_use_decryption_key( + Arc::clone(¶ms), + &manifest, + existing_target_manifest.as_ref(), + prefix.clone(), + Arc::clone(&log_sender), + ) + .await? + { + (None, false) => (None, None), // regular pull without decryption + (Some(crypt_config), false) => { + // decrypt while pull + let new_manifest = Arc::new(Mutex::new(BackupManifest::new(snapshot.into()))); + (Some(crypt_config), Some(new_manifest)) } - } + (_, true) => { + // nothing changed + fetch_log().await?; + cleanup().await?; + return Ok(sync_stats); + } + }; for item in manifest.files() { let mut path = snapshot.full_path(); @@ -979,6 +912,122 @@ async fn pull_snapshot<'a>( Ok(sync_stats) } +/// Check if the decryption key should be used to decrypt the snapshot during +/// pull based on given pull parameter, source and optionally already present +/// target manifest. +/// +/// The boolean flag in the returned tuple indicates whether the pull can be +/// skipped altogether, since the already existing target is unchanged. +/// If decryption should happen, the matching decryption key is returned. +async fn optionally_use_decryption_key( + params: Arc, + manifest: &BackupManifest, + existing_target_manifest: Option<&BackupManifest>, + prefix: String, + log_sender: Arc, +) -> Result<(Option>, bool), Error> { + let key_fp = match manifest.fingerprint().with_context(|| prefix.clone())? { + Some(key_fp) => key_fp, + None => return Ok((None, false)), // no fingerprint on source, regular pull + }; + + // source got key fingerprint, expect contents to be signed or encrypted + let (key_id, config) = match params + .crypt_configs + .iter() + .find(|(_id, crypt_conf)| crypt_conf.fingerprint() == *key_fp.bytes()) + { + Some(key_id_and_config) => key_id_and_config, + None => { + // 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() { + log_sender + .log( + Level::INFO, + format!("{prefix}: No matching key found, sync without decryption"), + ) + .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) + { + log_sender + .log( + Level::WARN, + format!("Snapshot not fully encrypted, sync as is despite matching key '{key_id}' with fingerprint {key_fp}"), + ) + .await?; + return Ok((None, false)); + } + + manifest + .check_signature(config) + .context("failed to check source manifest signature") + .with_context(|| prefix.clone())?; + + // 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 + .get_change_detection_fingerprint() + .context("failed to parse change detection fingerprint of source manifest") + .with_context(|| prefix.clone())? + { + // Stored fp is HMAC over the unencrypted source's protected fields; recompute + // over the locally decrypted manifest, not the fresh encrypted remote one. + let target_fp = existing_manifest + .signature(config) + .with_context(|| prefix.clone())?; + if target_fp == *source_fp.bytes() { + return Ok((None, true)); + } + + bail!("Change detection fingerprint mismatch, refuse to continue!"); + } + return Ok((None, false)); + } + + log_sender + .log( + Level::INFO, + format!("Found matching key '{key_id}' with fingerprint {key_fp}, decrypt on pull"), + ) + .await?; + + Ok((Some(Arc::clone(config)), false)) +} + /// Pulls a `snapshot`, removing newly created ones on error, but keeping existing ones in any case. /// /// The `reader` is configured to read from the source backup directory, while the -- 2.47.3