[PATCH proxmox-backup 0/3] fixup for server side decryption

[PATCH proxmox-backup 0/3] fixup for server side decryption

[Christian Ebner]

When decryption keys are set for a pull job, currently it is not
correctly distinguished whether the snapshot to be pulled is trueley
encrypted or just signed, for both cases the key fingerprint being
present on the manifest. Fix this by explicitley checking each files
blob crypt mode as registered in the manifest. Further, extend error
messages on key mismatch with pre-existing local manifests to also
include the signed-only case.

Path 1 is a bugfix for a missing check of snapshots being encrypted
or signed only on decrypting pull syncs. Patch 2 and 3 are related
code cleanups only.

Christian Ebner (3):
  sync: pull decrypt: check if contents only signed or fully encrypted
  sync: pull: refactor decryption key loading checks
  api: encryption keys: refactor associated keys check

 src/api2/config/encryption_keys.rs |  18 +--
 src/server/pull.rs                 | 211 +++++++++++++++++++----------
 2 files changed, 147 insertions(+), 82 deletions(-)

-- 
2.47.3

[PATCH proxmox-backup 1/3] sync: pull decrypt: check if contents only signed or fully encrypted

[Christian Ebner]

The key-fingerprint stored on the source manifest might be present if
the snapshot contents are encrypted or signed.

Checking the key fingerprints presence on the source manifest is
therefore not enough to determine whether the snapshot is fully
encrypted, rather each of the registered files chunk crypt mode has
to be checked.

Only decrypt on pull with a matching key if the snapshots contains
only contains encrypted files, warn and fallback to regular pull if
not. Adapt error messages on key mismatch to reflect that snapshot
might be signed but not encrypted.

Reported-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/pull.rs | 90 ++++++++++++++++++++++++++--------------------
 1 file changed, 52 insertions(+), 38 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index 7fa273edb..051405ae7 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -758,52 +758,66 @@ async fn pull_snapshot<'a>(
     let mut crypt_config = None;
     let mut new_manifest = None;
     if let Some(key_fp) = manifest.fingerprint().with_context(|| prefix.clone())? {
-        // source is encrypted, find matching key
+        // 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())
         {
-            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 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 {
+            // check if source is encrypted or contents signed
+            if !manifest
+                .files()
+                .iter()
+                .all(|f| f.chunk_crypt_mode() == CryptMode::Encrypt)
+            {
                 log_sender
                     .log(
-                        Level::INFO,
-                        format!("Found matching key '{key_id}' with fingerprint {key_fp}, decrypt on pull"),
+                        Level::WARN,
+                        format!("Snapshot not fully encrypted, sync as is despite matching key '{key_id}' with fingerprint {key_fp}"),
                     )
                     .await?;
-                crypt_config = Some(Arc::clone(config));
-                new_manifest = Some(Arc::new(Mutex::new(BackupManifest::new(snapshot.into()))));
+            } 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
@@ -812,7 +826,7 @@ async fn pull_snapshot<'a>(
             {
                 if existing_fingerprint != key_fp {
                     // pre-existing local manifest for encrypted snapshot with key mismatch
-                    bail!("Local encrypted snapshot with different key detected, refuse to sync");
+                    bail!("Local encrypted or signed snapshot with different key detected, refuse to sync");
                 }
             } else {
                 // pre-existing local manifest without key-fingerprint was previously decrypted,
-- 
2.47.3

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

[Christian Ebner]

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

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

[Fabian Grünbichler]

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
> 
> 
> 
> 
> 
> 

[PATCH proxmox-backup 3/3] api: encryption keys: refactor associated keys check

[Christian Ebner]

Fixes an if_same_then_else clippy warning and improves the
readability for the check of sync jobs having a key assigned as
associated key.

Both branches have the same push logic, so combine into a common if
statement. Since the if statement would then however be hard to
parse, move the associated key check logic into a closue instead.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/api2/config/encryption_keys.rs | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/src/api2/config/encryption_keys.rs b/src/api2/config/encryption_keys.rs
index 81483302e..5c0c08e52 100644
--- a/src/api2/config/encryption_keys.rs
+++ b/src/api2/config/encryption_keys.rs
@@ -195,15 +195,17 @@ fn check_encryption_key_in_use(id: &str, include_associated: bool) -> Result<(),
     let mut used_by_jobs = Vec::new();
 
     let job_list: Vec<SyncJobConfig> = config.convert_to_typed_array("sync")?;
+
+    let contains_associated_key = |job: &SyncJobConfig| {
+        job.associated_key
+            .as_deref()
+            .unwrap_or(&[])
+            .contains(&id.to_string())
+    };
+
     for job in job_list {
-        if job.active_encryption_key.as_deref() == Some(id) {
-            used_by_jobs.push(job.id.clone());
-        } else if include_associated
-            && job
-                .associated_key
-                .as_deref()
-                .unwrap_or(&[])
-                .contains(&id.to_string())
+        if job.active_encryption_key.as_deref() == Some(id)
+            || (include_associated && contains_associated_key(&job))
         {
             used_by_jobs.push(job.id.clone());
         }
-- 
2.47.3

[PATCH proxmox-backup 1/2] pull: decrypt: use let else to reduce indentation

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
best viewed with -w

 src/server/pull.rs | 56 +++++++++++++++++++++-------------------------
 1 file changed, 26 insertions(+), 30 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index d0f886cb5..3dc5770f9 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -926,48 +926,44 @@ async fn optionally_use_decryption_key(
     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
+    let Some(key_fp) = manifest.fingerprint().with_context(|| prefix.clone())? else {
+        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
+    let Some((key_id, config)) = 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!(
+    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!(
+                }
+            } 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));
+        } 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
-- 
2.47.3

[PATCH proxmox-backup 2/2] pull: decrypt: re-order encryption key/state checks

[Fabian Grünbichler]

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 <f.gruenbichler@proxmox.com>
---
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

applied: [PATCH proxmox-backup 0/3] fixup for server side decryption

[Thomas Lamprecht]

On Fri, 24 Apr 2026 12:36:04 +0200, Christian Ebner wrote:
> When decryption keys are set for a pull job, currently it is not
> correctly distinguished whether the snapshot to be pulled is trueley
> encrypted or just signed, for both cases the key fingerprint being
> present on the manifest. Fix this by explicitley checking each files
> blob crypt mode as registered in the manifest. Further, extend error
> messages on key mismatch with pre-existing local manifests to also
> include the signed-only case.
> 
> [...]

Applied, thanks!

[1/3] sync: pull decrypt: check if contents only signed or fully encrypted
      commit: f515516448250afc96f3227d1323e0323ded0dda
[2/3] sync: pull: refactor decryption key loading checks
      commit: 14b580739d9f0e3ad2098812a3a9b251ae5e48c1
[3/3] api: encryption keys: refactor associated keys check
      commit: a6dfa6abdea8045b82209bb357e21862d88b0abc