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

[PATCH proxmox-backup v2 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 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    | 102 ++++++++++++++----------------
 proxmox-backup-client/src/main.rs |   2 +-
 src/server/push.rs                |  81 ++++++++++++++++++------
 4 files changed, 126 insertions(+), 82 deletions(-)


Summary over all repositories:
  4 files changed, 126 insertions(+), 82 deletions(-)

-- 
Generated by murpp 0.11.0

[PATCH proxmox-backup v2 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>
---
 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 b434243c0..7761bbae7 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 v2 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>
---
 pbs-datastore/src/data_blob.rs | 86 +++++++++++++++++-----------------
 1 file changed, 43 insertions(+), 43 deletions(-)

diff --git a/pbs-datastore/src/data_blob.rs b/pbs-datastore/src/data_blob.rs
index 7761bbae7..3c05a6106 100644
--- a/pbs-datastore/src/data_blob.rs
+++ b/pbs-datastore/src/data_blob.rs
@@ -180,57 +180,57 @@ 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 {
-                    Self::decode_compressed_chunk(
-                        config,
-                        &self.raw_data[header_len..],
-                        &head.iv,
-                        &head.tag,
-                    )?
+                if let Some(config) = config {
+                    let data = if magic == ENCR_COMPR_BLOB_MAGIC_1_0 {
+                        Self::decode_compressed_chunk(
+                            config,
+                            &self.raw_data[header_len..],
+                            &head.iv,
+                            &head.tag,
+                        )?
+                    } else {
+                        Self::decode_uncompressed_chunk(
+                            config,
+                            &self.raw_data[header_len..],
+                            &head.iv,
+                            &head.tag,
+                        )?
+                    };
+                    (data, Some(config))
                 } else {
-                    Self::decode_uncompressed_chunk(
-                        config,
-                        &self.raw_data[header_len..],
-                        &head.iv,
-                        &head.tag,
-                    )?
-                };
-                if let Some(digest) = digest {
-                    Self::verify_digest(&data, Some(config), digest)?;
+                    bail!("unable to decrypt blob - missing CryptConfig");
                 }
-                Ok(data)
-            } else {
-                bail!("unable to decrypt blob - missing CryptConfig");
             }
-        } 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 v2 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>
---
 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 49aff3fdd..47f08840e 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -745,8 +745,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 v2 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>
---
 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 47f08840e..b60d71d3f 100644
--- a/pbs-client/src/backup_writer.rs
+++ b/pbs-client/src/backup_writer.rs
@@ -735,8 +735,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() });
@@ -748,10 +754,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 5e8bb5393..c96db321f 100644
--- a/proxmox-backup-client/src/main.rs
+++ b/proxmox-backup-client/src/main.rs
@@ -1064,7 +1064,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 dac62c84a..afa78b751 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 v2 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 afa78b751..4495b68ed 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
+            .map_err(|err| err.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!("manifest 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!("manifest 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!("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