[PATCH proxmox-backup v3 0/5] restrict previous manifest reuse checks for push sync jobs

[PATCH proxmox-backup v3 0/5] restrict previous manifest reuse checks for push sync jobs

[Christian Ebner]

Restrict and rework the checks for reuse of previous snapshots
manifest's for push sync jobs, in particular with key and signature
mismatches for sync jobs with active encryption key.
In particular, only allow reuse of the previous manifest if either
the previous manifest is not encrypted, and the push sync will also
not encrypt or the previous manifest is encrypted with the matching
active encryption key configured for the sync job, the manifest's
signature being verified with that key.

This patches thereby also more gracefully handle an otherwise rather
alerting log message during push sync jobs with configured active
encryption key, stating a key mismatch in the manifest signature
check error.

The series includes also some patches with code style cleanups
encountered while working on the code.

Changes since version 2 (thanks @Robert for review):
- Reduced intendataion level by using `let Some(config) = config else {..}`
- Dropped unnecessary map_err() when fetching previous manifest on push sync
- Improved log messages by clarifing that the snapshot, not the manifest itself
  is considered as entity being not-/encrypted.

Changes since version 1 (thanks @Fabian for review):
- use boolean flag to skip signature check in download_previous_manifest()
- do not pass crypt config on manifest blob decode
- refine checks for when the previous manifest should be reusable
- drop now outdated checks when pulling reusable index chunks
- refactor DataBlob methods for improved code style


proxmox-backup:

Christian Ebner (5):
  datastore: data blob: refactor crypt mode method
  datastore: data blob: refactor decoding method
  client: backup writer: pass no crypt config to manifest blob decoder
  client: allow skipping signature check on previous manifest fetching
  sync: push: gracefully handle previous manifest signature mismatches

 pbs-client/src/backup_writer.rs   | 23 ++++++---
 pbs-datastore/src/data_blob.rs    | 78 ++++++++++++++---------------
 proxmox-backup-client/src/main.rs |  2 +-
 src/server/push.rs                | 81 +++++++++++++++++++++++--------
 4 files changed, 114 insertions(+), 70 deletions(-)


Summary over all repositories:
  4 files changed, 114 insertions(+), 70 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox-backup v3 1/5] datastore: data blob: refactor crypt mode method

[Christian Ebner]

Improve code style and readability by using a simple match statement
instead of chained if blocks.

No functional changes.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/data_blob.rs | 16 +++++-----------
 1 file changed, 5 insertions(+), 11 deletions(-)

diff --git a/pbs-datastore/src/data_blob.rs b/pbs-datastore/src/data_blob.rs
index 298c8f6e9..465bcb280 100644
--- a/pbs-datastore/src/data_blob.rs
+++ b/pbs-datastore/src/data_blob.rs
@@ -167,17 +167,11 @@ impl DataBlob {
 
     /// Get the encryption mode for this blob.
     pub fn crypt_mode(&self) -> Result<CryptMode, Error> {
-        let magic = self.magic();
-
-        Ok(
-            if magic == &UNCOMPRESSED_BLOB_MAGIC_1_0 || magic == &COMPRESSED_BLOB_MAGIC_1_0 {
-                CryptMode::None
-            } else if magic == &ENCR_COMPR_BLOB_MAGIC_1_0 || magic == &ENCRYPTED_BLOB_MAGIC_1_0 {
-                CryptMode::Encrypt
-            } else {
-                bail!("Invalid blob magic number.");
-            },
-        )
+        match *self.magic() {
+            UNCOMPRESSED_BLOB_MAGIC_1_0 | COMPRESSED_BLOB_MAGIC_1_0 => Ok(CryptMode::None),
+            ENCR_COMPR_BLOB_MAGIC_1_0 | ENCRYPTED_BLOB_MAGIC_1_0 => Ok(CryptMode::Encrypt),
+            _ => bail!("Invalid blob magic number."),
+        }
     }
 
     /// Decode blob data
-- 
2.47.3

[PATCH proxmox-backup v3 2/5] datastore: data blob: refactor decoding method

[Christian Ebner]

Improve code style and readability by using a single match statement
instead of chained if statements and deduplicate the common digest
verificaton, performed on the decoded blob data.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-datastore/src/data_blob.rs | 62 +++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 31 deletions(-)

diff --git a/pbs-datastore/src/data_blob.rs b/pbs-datastore/src/data_blob.rs
index 465bcb280..44ab41e16 100644
--- a/pbs-datastore/src/data_blob.rs
+++ b/pbs-datastore/src/data_blob.rs
@@ -180,33 +180,33 @@ impl DataBlob {
         config: Option<&CryptConfig>,
         digest: Option<&[u8; 32]>,
     ) -> Result<Vec<u8>, Error> {
-        let magic = self.magic();
+        let magic = *self.magic();
 
-        if magic == &UNCOMPRESSED_BLOB_MAGIC_1_0 {
-            let data_start = std::mem::size_of::<DataBlobHeader>();
-            let data = self.raw_data[data_start..].to_vec();
-            if let Some(digest) = digest {
-                Self::verify_digest(&data, None, digest)?;
+        let (data, crypt_config) = match magic {
+            UNCOMPRESSED_BLOB_MAGIC_1_0 => {
+                let data_start = std::mem::size_of::<DataBlobHeader>();
+                let data = self.raw_data[data_start..].to_vec();
+                (data, None)
             }
-            Ok(data)
-        } else if magic == &COMPRESSED_BLOB_MAGIC_1_0 {
-            let data_start = std::mem::size_of::<DataBlobHeader>();
-            let mut reader = &self.raw_data[data_start..];
-            let data = zstd::stream::decode_all(&mut reader)?;
-            // zstd::block::decompress is about 10% slower
-            // let data = zstd::block::decompress(&self.raw_data[data_start..], MAX_BLOB_SIZE)?;
-            if let Some(digest) = digest {
-                Self::verify_digest(&data, None, digest)?;
+            COMPRESSED_BLOB_MAGIC_1_0 => {
+                let data_start = std::mem::size_of::<DataBlobHeader>();
+                let mut reader = &self.raw_data[data_start..];
+                let data = zstd::stream::decode_all(&mut reader)?;
+                // zstd::block::decompress is about 10% slower
+                // let data = zstd::block::decompress(&self.raw_data[data_start..], MAX_BLOB_SIZE)?;
+                (data, None)
             }
-            Ok(data)
-        } else if magic == &ENCR_COMPR_BLOB_MAGIC_1_0 || magic == &ENCRYPTED_BLOB_MAGIC_1_0 {
-            let header_len = std::mem::size_of::<EncryptedDataBlobHeader>();
-            let head = unsafe {
-                (&self.raw_data[..header_len]).read_le_value::<EncryptedDataBlobHeader>()?
-            };
+            ENCR_COMPR_BLOB_MAGIC_1_0 | ENCRYPTED_BLOB_MAGIC_1_0 => {
+                let header_len = std::mem::size_of::<EncryptedDataBlobHeader>();
+                let head = unsafe {
+                    (&self.raw_data[..header_len]).read_le_value::<EncryptedDataBlobHeader>()?
+                };
 
-            if let Some(config) = config {
-                let data = if magic == &ENCR_COMPR_BLOB_MAGIC_1_0 {
+                let Some(config) = config else {
+                    bail!("unable to decrypt blob - missing CryptConfig");
+                };
+
+                let data = if magic == ENCR_COMPR_BLOB_MAGIC_1_0 {
                     Self::decode_compressed_chunk(
                         config,
                         &self.raw_data[header_len..],
@@ -221,16 +221,16 @@ impl DataBlob {
                         &head.tag,
                     )?
                 };
-                if let Some(digest) = digest {
-                    Self::verify_digest(&data, Some(config), digest)?;
-                }
-                Ok(data)
-            } else {
-                bail!("unable to decrypt blob - missing CryptConfig");
+                (data, Some(config))
             }
-        } else {
-            bail!("Invalid blob magic number.");
+            _ => bail!("Invalid blob magic number."),
+        };
+
+        if let Some(digest) = digest {
+            Self::verify_digest(&data, crypt_config, digest)?;
         }
+
+        Ok(data)
     }
 
     /// Load data blob via given sync ``reader`` and verify its CRC
-- 
2.47.3

[PATCH proxmox-backup v3 3/5] client: backup writer: pass no crypt config to manifest blob decoder

[Christian Ebner]

Data blobs storing a manifest are never encrypted, so passing the
backup writer crypt config on DataBlob::decode() while fetching the
previous manifest is useless, never used in that case anyways.

Simply do not pass the crypt config to make the code semantically
correct.

No functional changes intended.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/backup_writer.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index f0744ecaf..f1003f800 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -747,8 +747,8 @@ impl BackupWriter {
             .await?;
 
         let blob = DataBlob::load_from_reader(&mut &raw_data[..])?;
-        // no expected digest available
-        let data = blob.decode(self.crypt_config.as_ref().map(Arc::as_ref), None)?;
+        // manifest blobs are never encrypted and no expected digest available
+        let data = blob.decode(None, None)?;
 
         let manifest =
             BackupManifest::from_data(&data[..], self.crypt_config.as_ref().map(Arc::as_ref))?;
-- 
2.47.3

[PATCH proxmox-backup v3 4/5] client: allow skipping signature check on previous manifest fetching

[Christian Ebner]

Extends the BackupWriter method to download the previous manifest
from a PBS instance, by a flag which allows skipping the signature
check even if the backup writer has a crypt_config set.

Silences misleading logs during encrypting push sync jobs.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
Reviewed-by: Robert Obkircher <r.obkircher@proxmox.com>
---
 pbs-client/src/backup_writer.rs   | 19 ++++++++++++++-----
 proxmox-backup-client/src/main.rs |  2 +-
 src/server/push.rs                |  2 +-
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/pbs-client/src/backup_writer.rs b/pbs-client/src/backup_writer.rs
index f1003f800..269d74c56 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -737,8 +737,14 @@ impl BackupWriter {
         })
     }
 
-    /// Download backup manifest (index.json) of last backup
-    pub async fn download_previous_manifest(&self) -> Result<BackupManifest, Error> {
+    /// Download backup manifest (index.json) of last backup.
+    ///
+    /// If `check_signature` is set and the writer stores a crypt config,
+    /// also checks the manifest's signature.
+    pub async fn download_previous_manifest(
+        &self,
+        check_signature: bool,
+    ) -> Result<BackupManifest, Error> {
         let mut raw_data = Vec::with_capacity(64 * 1024);
 
         let param = json!({ "archive-name": MANIFEST_BLOB_NAME.to_string() });
@@ -750,10 +756,13 @@ impl BackupWriter {
         // manifest blobs are never encrypted and no expected digest available
         let data = blob.decode(None, None)?;
 
-        let manifest =
-            BackupManifest::from_data(&data[..], self.crypt_config.as_ref().map(Arc::as_ref))?;
+        let crypt_config = if check_signature {
+            self.crypt_config.as_ref().map(Arc::as_ref)
+        } else {
+            None
+        };
 
-        Ok(manifest)
+        BackupManifest::from_data(&data[..], crypt_config)
     }
 
     // We have no `self` here for `h2` and `verbose`, the only other arg "common" with 1 other
diff --git a/proxmox-backup-client/src/main.rs b/proxmox-backup-client/src/main.rs
index 5c52b7917..ecee8b4e3 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -1069,7 +1069,7 @@ async fn create_backup(
     };
 
     let previous_manifest = if download_previous_manifest {
-        match client.download_previous_manifest().await {
+        match client.download_previous_manifest(true).await {
             Ok(previous_manifest) => {
                 match previous_manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref)) {
                     Ok(()) => Some(Arc::new(previous_manifest)),
diff --git a/src/server/push.rs b/src/server/push.rs
index 7b3cac4cd..17fa605a4 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -1131,7 +1131,7 @@ pub(crate) async fn push_snapshot(
     let mut previous_manifest = None;
     // Use manifest of previous snapshots in group on target for chunk upload deduplication
     if fetch_previous_manifest {
-        match backup_writer.download_previous_manifest().await {
+        match backup_writer.download_previous_manifest(false).await {
             Ok(manifest) => previous_manifest = Some(Arc::new(manifest)),
             Err(err) => {
                 log_sender
-- 
2.47.3

[PATCH proxmox-backup v3 5/5] sync: push: gracefully handle previous manifest signature mismatches

[Christian Ebner]

During push sync jobs with a given active encryption key, the key is
loaded for the backup writer, used to encrypt the source snapshot on
the fly. As optimization, the backup writer deduplicates chunks
already present in the previous snapshot by loading them from the
index files as stored in the previous manifest.

Reuse of the previous manifest and index must however only happen if:
- The previous manifest is not encrypted, neither will push encrypt.
- The previous manifest is encrypted and passes signature
  verification using the same key to be used during push encryption.

Since these checks are now performed when loading the previous
manifest, drop redundant and now useless checks performed before
loading the chunks from index files.

Signed-off-by: Christian Ebner <c.ebner@proxmox.com>
---
 src/server/push.rs | 81 ++++++++++++++++++++++++++++++++++------------
 1 file changed, 61 insertions(+), 20 deletions(-)

diff --git a/src/server/push.rs b/src/server/push.rs
index 17fa605a4..5cc794a68 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -979,25 +979,12 @@ pub(crate) async fn push_group(
 }
 
 async fn load_previous_snapshot_known_chunks(
-    params: &PushParameters,
     upload_options: &UploadOptions,
     backup_writer: &BackupWriter,
     archive_name: &BackupArchiveName,
     known_chunks: Arc<Mutex<HashSet<[u8; 32]>>>,
 ) {
     if let Some(manifest) = upload_options.previous_manifest.as_ref() {
-        if let Some((_id, crypt_config)) = &params.crypt_config {
-            // no fingerprint in previous manifest -> no reuse possible
-            let Ok(Some(fingerprint)) = manifest.fingerprint() else {
-                return;
-            };
-
-            if *fingerprint.bytes() != crypt_config.fingerprint() {
-                // key mismatch -> no reuse possible
-                return;
-            }
-        }
-
         // Add known chunks, ignore errors since archive might not be present and it is better
         // to proceed on unrelated errors than to fail here.
         match archive_name.archive_type() {
@@ -1128,18 +1115,73 @@ pub(crate) async fn push_snapshot(
     .await
     .with_context(|| prefix.to_string())?;
 
-    let mut previous_manifest = None;
     // Use manifest of previous snapshots in group on target for chunk upload deduplication
-    if fetch_previous_manifest {
-        match backup_writer.download_previous_manifest(false).await {
-            Ok(manifest) => previous_manifest = Some(Arc::new(manifest)),
+    let previous_manifest = if !fetch_previous_manifest {
+        None
+    } else {
+        let result = backup_writer
+            .download_previous_manifest(false)
+            .await
+            .context("failed to download")
+            .and_then(|manifest| {
+                let fingerprint = manifest
+                    .fingerprint()
+                    .context("failed getting fingerprint")?;
+
+                let Some(manifest_key_fp) = fingerprint else {
+                    if encrypt_using_key.is_none() {
+                        return Ok(Arc::new(manifest));
+                    }
+                    bail!("previous snapshot not encrypted");
+                };
+
+                let signed_only = manifest
+                    .files()
+                    .iter()
+                    .all(|f| f.chunk_crypt_mode() == CryptMode::None);
+
+                let Some((_key_id, crypt_config)) = &encrypt_using_key else {
+                    if signed_only {
+                        return Ok(Arc::new(manifest));
+                    }
+                    bail!("previous snapshot encrypted but no encryption key configured");
+                };
+
+                if *manifest_key_fp.bytes() == crypt_config.fingerprint() {
+                    if let Some(expected_signature) = &manifest.signature {
+                        let calculated_signature = manifest
+                            .signature(crypt_config)
+                            .context("signature calculation failed")?;
+                        if hex::encode(calculated_signature) == *expected_signature {
+                            return Ok(Arc::new(manifest));
+                        }
+                        bail!("signature mismatch with configured encryption key");
+                    }
+                    bail!("previous snapshots manifest not signed with configured encryption key");
+                }
+                bail!(
+                    "encryption key of previous snapshot does not match configured encryption key"
+                );
+            });
+
+        match result {
+            Ok(prev) => {
+                log_sender
+                    .log(
+                        Level::INFO,
+                        format!("{prefix}: Reusing previous manifest for deduplication"),
+                    )
+                    .await?;
+                Some(prev)
+            }
             Err(err) => {
                 log_sender
                     .log(
                         Level::INFO,
-                        format!("{prefix}: Could not download previous manifest - {err}"),
+                        format!("{prefix}: Skip reuse of previous manifest - {err:#}"),
                     )
-                    .await?
+                    .await?;
+                None
             }
         }
     };
@@ -1184,7 +1226,6 @@ pub(crate) async fn push_snapshot(
             };
 
             load_previous_snapshot_known_chunks(
-                params,
                 &upload_options,
                 &backup_writer,
                 &archive_name,
-- 
2.47.3

applied-series: [PATCH proxmox-backup v3 0/5] restrict previous manifest reuse checks for push sync jobs

[Fabian Grünbichler]

On Wed, 29 Jul 2026 11:07:32 +0200, Christian Ebner wrote:
> Restrict and rework the checks for reuse of previous snapshots
> manifest's for push sync jobs, in particular with key and signature
> mismatches for sync jobs with active encryption key.
> In particular, only allow reuse of the previous manifest if either
> the previous manifest is not encrypted, and the push sync will also
> not encrypt or the previous manifest is encrypted with the matching
> active encryption key configured for the sync job, the manifest's
> signature being verified with that key.
> 
> [...]

Applied, thanks!

[1/5] datastore: data blob: refactor crypt mode method
      commit: b00fe46ee37479805bebff0eb3e2b131ca88c9b1
[2/5] datastore: data blob: refactor decoding method
      commit: 71ab92143a2e98a2bd9f4756f1e860348f1a72af
[3/5] client: backup writer: pass no crypt config to manifest blob decoder
      commit: 0f60e71d79ff02e0e65618609ea02981f4ca520d
[4/5] client: allow skipping signature check on previous manifest fetching
      commit: b80847b554be38f7a9bcae9bf09e14dfb64afa22
[5/5] sync: push: gracefully handle previous manifest signature mismatches
      commit: bb3a5fdfcfde5324a5b15a34e72138273df69c7e

Best regards,
-- 
Fabian Grünbichler <f.gruenbichler@proxmox.com>