Re: [PATCH proxmox-backup 2/3] sync: pull: refactor decryption key loading checks

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

On April 24, 2026 12:36 pm, Christian Ebner wrote:
> 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 <c.ebner@proxmox.com>
> ---
>  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(&params),
> +        &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<PullParameters>,
> +    manifest: &BackupManifest,
> +    existing_target_manifest: Option<&BackupManifest>,
> +    prefix: String,
> +    log_sender: Arc<LogLineSender>,
> +) -> Result<(Option<Arc<CryptConfig>>, 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
> +    };

this can be a let else isntead

> +
> +    // 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,

this as well

> +        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())?;

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) = 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!");
> +            }

if it is signed we don't check the change detection fingerprint..

> +        } 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));

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 keeping existing ones in any case.
>  ///
>  /// The `reader` is configured to read from the source backup directory, while the
> -- 
> 2.47.3
> 
> 
> 
> 
> 
>