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 BBE041FF140 for ; Fri, 24 Apr 2026 14:46:48 +0200 (CEST) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 9E7131951A; Fri, 24 Apr 2026 14:46:48 +0200 (CEST) Date: Fri, 24 Apr 2026 14:46:07 +0200 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= Subject: Re: [PATCH proxmox-backup 2/3] sync: pull: refactor decryption key loading checks To: Christian Ebner , pbs-devel@lists.proxmox.com References: <20260424103607.531400-1-c.ebner@proxmox.com> <20260424103607.531400-3-c.ebner@proxmox.com> In-Reply-To: <20260424103607.531400-3-c.ebner@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.17.0 (https://github.com/astroidmail/astroid) Message-Id: <1777033255.umlrtuml3i.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: 1777034680006 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. [pull.rs] Message-ID-Hash: FMDNKJ3NDQE6RPSTYNTR56WZFRQXZVDL X-Message-ID-Hash: FMDNKJ3NDQE6RPSTYNTR56WZFRQXZVDL 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 24, 2026 12:36 pm, Christian Ebner wrote: > In an effort to improve code readability and maintanablity. >=20 > 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. >=20 > Signed-off-by: Christian Ebner > --- > src/server/pull.rs | 225 +++++++++++++++++++++++++++------------------ > 1 file changed, 137 insertions(+), 88 deletions(-) >=20 > 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); > } > =20 > - let mut crypt_config =3D None; > - let mut new_manifest =3D None; > - if let Some(key_fp) =3D manifest.fingerprint().with_context(|| prefi= x.clone())? { > - // source got key fingerprint, expect contents to be signed or e= ncrypted > - if let Some((key_id, config)) =3D params > - .crypt_configs > - .iter() > - .find(|(_id, crypt_conf)| crypt_conf.fingerprint() =3D=3D *k= ey_fp.bytes()) > - { > - // check if source is encrypted or contents signed > - if !manifest > - .files() > - .iter() > - .all(|f| f.chunk_crypt_mode() =3D=3D CryptMode::Encrypt) > - { > - log_sender > - .log( > - Level::WARN, > - format!("Snapshot not fully encrypted, sync as i= s 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) =3D existing_target_manif= est { > - if let Some(existing_fingerprint) =3D existing_manif= est > - .fingerprint() > - .with_context(|| prefix.clone())? > - { > - if existing_fingerprint !=3D key_fp { > - bail!("Detected local encrypted or signed sn= apshot with encryption key mismatch!"); > - } > - } else if let Some(source_fp) =3D manifest > - .get_change_detection_fingerprint() > - .context("failed to parse change detection finge= rprint 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 =3D existing_manifest > - .signature(config) > - .with_context(|| prefix.clone())?; > - if target_fp =3D=3D *source_fp.bytes() { > - fetch_log().await?; > - cleanup().await?; > - return Ok(sync_stats); // nothing changed > - } > - > - bail!("Change detection fingerprint mismatch, re= fuse to continue"); > - } > - } else { > - log_sender > - .log( > - Level::INFO, > - format!("Found matching key '{key_id}' with = fingerprint {key_fp}, decrypt on pull"), > - ) > - .await?; > - crypt_config =3D Some(Arc::clone(config)); > - new_manifest =3D Some(Arc::new(Mutex::new(BackupMani= fest::new(snapshot.into())))); > - } > - } > - } else if let Some(existing_target_manifest) =3D existing_target= _manifest { > - if let Some(existing_fingerprint) =3D existing_target_manife= st > - .fingerprint() > - .with_context(|| prefix.clone())? > - { > - if existing_fingerprint !=3D key_fp { > - // pre-existing local manifest for encrypted snapsho= t with key mismatch > - bail!("Local encrypted or signed snapshot with diffe= rent key detected, refuse to sync"); > - } > - } else { > - // pre-existing local manifest without key-fingerprint w= as previously decrypted, > - // never overwrite with encrypted > - bail!( > - "local snapshot was previously decrypted but no matc= hing 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 witho= ut decryption"), > - ) > - .await?; > + let (crypt_config, new_manifest) =3D match optionally_use_decryption= _key( > + Arc::clone(¶ms), > + &manifest, > + existing_target_manifest.as_ref(), > + prefix.clone(), > + Arc::clone(&log_sender), > + ) > + .await? > + { > + (None, false) =3D> (None, None), // regular pull without decrypt= ion > + (Some(crypt_config), false) =3D> { > + // decrypt while pull > + let new_manifest =3D Arc::new(Mutex::new(BackupManifest::new= (snapshot.into()))); > + (Some(crypt_config), Some(new_manifest)) > } > - } > + (_, true) =3D> { > + // nothing changed > + fetch_log().await?; > + cleanup().await?; > + return Ok(sync_stats); > + } > + }; > =20 > for item in manifest.files() { > let mut path =3D snapshot.full_path(); > @@ -979,6 +912,122 @@ async fn pull_snapshot<'a>( > Ok(sync_stats) > } > =20 > +/// Check if the decryption key should be used to decrypt the snapshot d= uring > +/// pull based on given pull parameter, source and optionally already pr= esent > +/// target manifest. > +/// > +/// The boolean flag in the returned tuple indicates whether the pull ca= n 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 =3D match manifest.fingerprint().with_context(|| prefix.c= lone())? { > + Some(key_fp) =3D> key_fp, > + None =3D> return Ok((None, false)), // no fingerprint on source,= regular pull > + }; this can be a let else isntead > + > + // source got key fingerprint, expect contents to be signed or encry= pted > + let (key_id, config) =3D match params > + .crypt_configs > + .iter() > + .find(|(_id, crypt_conf)| crypt_conf.fingerprint() =3D=3D *key_f= p.bytes()) > + { > + Some(key_id_and_config) =3D> key_id_and_config, this as well > + None =3D> { > + // no matching key found in list of configured ones > + if let Some(existing_target_manifest) =3D existing_target_ma= nifest { > + if let Some(existing_fingerprint) =3D existing_target_ma= nifest > + .fingerprint() > + .with_context(|| prefix.clone())? > + { > + if existing_fingerprint !=3D key_fp { > + // pre-existing local manifest for encrypted sna= pshot with key mismatch > + bail!( > + "Local encrypted or signed snapshot with dif= ferent key detected, refuse to sync" > + ); > + } > + } else { > + // pre-existing local manifest without key-fingerpri= nt 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 w= ithout decryption"), > + ) > + .await?; > + } > + return Ok((None, false)); > + } > + }; > + > + // check if source is encrypted or contents signed > + if !manifest > + .files() > + .iter() > + .all(|f| f.chunk_crypt_mode() =3D=3D CryptMode::Encrypt) > + { > + log_sender > + .log( > + Level::WARN, > + format!("Snapshot not fully encrypted, sync as is despit= e 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())?; so at this point we are if - the source is encrypted, we found a matching key, and the signature is okay > + > + // avoid overwriting pre-existing target manifest > + if let Some(existing_manifest) =3D existing_target_manifest { > + if let Some(existing_fingerprint) =3D existing_manifest > + .fingerprint() > + .with_context(|| prefix.clone())? > + { > + if existing_fingerprint !=3D key_fp { > + bail!("Detected local encrypted or signed snapshot with = encryption key mismatch!"); > + } if it is signed we don't check the change detection fingerprint.. > + } else if let Some(source_fp) =3D manifest > + .get_change_detection_fingerprint() > + .context("failed to parse change detection fingerprint of so= urce 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 encryp= ted remote one. > + let target_fp =3D existing_manifest > + .signature(config) > + .with_context(|| prefix.clone())?; > + if target_fp =3D=3D *source_fp.bytes() { > + return Ok((None, true)); > + } > + > + bail!("Change detection fingerprint mismatch, refuse to cont= inue!"); > + } > + return Ok((None, false)); isn't this None here wrong? we know the source is encrypted, and the target is not, so we should either abort, or return the key we found. I sent some follow-ups, feel free to fold them in with either of these two variants (they currently abort, which seems like the safe choice to me). > + } > + > + 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 keepin= g existing ones in any case. > /// > /// The `reader` is configured to read from the source backup directory,= while the > --=20 > 2.47.3 >=20 >=20 >=20 >=20 >=20 >=20