[PATCH proxmox-backup 0/2] fix #6990: server: drop verify state on push & pull job

[PATCH proxmox-backup 0/2] fix #6990: server: drop verify state on push & pull job

[Jakob Klocker]

When syncing a snapshot to another datastore, the source's verify_state is
currently carried over to the target. This reports the target snapshot as
verified even though its stored copy was never checked there. Since verify
jobs skip snapshots that already carry a verify_state, the target's copy
can't be verified again.

Chunks are checksummed in memory while being transferred, but a verify
serves a different purpose: confirming the write to the (external) target
actually succeeded.

This series drops verify_state on sync so the target is verified
independently. The state is dropped by default.

Tested (verify_state correctly stripped on target):
 * Pull, verified source, new content, clean sync
 * Pull, verified source, existing content, clean sync
 * Pull, corrupted target, resync-corrupt
 * Push, verified source (has no corrupt path)
 * Pull, independently-verified target, clean re-sync - state preserved

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990


proxmox-backup:

Jakob Klocker (2):
  fix #6990: server: drop verify state on non-decrypt pull job
  fix #6990: server: drop verify state on push job

 src/server/pull.rs | 42 ++++++++++++++++++++++++++++--------------
 src/server/push.rs |  6 ++++++
 2 files changed, 34 insertions(+), 14 deletions(-)


Summary over all repositories:
  2 files changed, 34 insertions(+), 14 deletions(-)

-- 
Generated by murpp 0.12.0

[PATCH proxmox-backup 1/2] fix #6990: server: drop verify state on non-decrypt pull job

[Jakob Klocker]

On a non-decrypt pull the source manifest is written to the target
as-is, so the target inherits the source's verify_state flag instead of
being verified independently on its own storage. Because a snapshot
carrying a verify_state is skipped by verify jobs, the target's copy can
never be checked.

The decrypt path already drops verify_state; do the same on the
non-decrypt path when the snapshot is newly pulled or re-synced due to
corruption. Also factor the manifest blob encoding and writing into a
helper, shared by both paths.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990

Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/pull.rs | 42 ++++++++++++++++++++++++++++--------------
 1 file changed, 28 insertions(+), 14 deletions(-)

diff --git a/src/server/pull.rs b/src/server/pull.rs
index e86a9c329..f8107911e 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -745,7 +745,8 @@ async fn pull_snapshot<'a>(
     };
 
     let mut manifest_data = tmp_manifest_blob.raw_data().to_vec();
-    let manifest = BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
+    let mut manifest =
+        BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?;
 
     if ignore_not_verified_or_encrypted(
         &manifest,
@@ -885,19 +886,15 @@ async fn pull_snapshot<'a>(
             new_manifest.set_sync_source_signature(expected.bytes())?;
         }
 
-        let manifest_string = new_manifest.to_string(None)?;
-        let manifest_blob = DataBlob::encode(manifest_string.as_bytes(), None, true)?;
-        // update contents to be uploaded to backend
-        manifest_data = manifest_blob.raw_data().to_vec();
-
-        let mut tmp_manifest_file = OpenOptions::new()
-            .write(true)
-            .truncate(true) // clear pre-existing manifest content
-            .open(&tmp_manifest_name)
-            .await?;
-        tmp_manifest_file.write_all(&manifest_data).await?;
-        tmp_manifest_file.flush().await?;
-        nix::unistd::fsync(tmp_manifest_file.as_raw_fd())?;
+        manifest_data = write_manifest_blob(&new_manifest, &tmp_manifest_name).await?;
+    } else if (is_new || corrupt) && manifest.unprotected.get("verify_state").is_some() {
+        // the manifest is copied as-is on the non-decrypted path, drop verify_state explicitly
+        if let Some(unprotected) = manifest.unprotected.as_object_mut() {
+            unprotected.remove("verify_state");
+        } else {
+            bail!("Encountered unexpected manifest without 'unprotected' section.");
+        }
+        manifest_data = write_manifest_blob(&manifest, &tmp_manifest_name).await?;
     }
 
     if let Err(err) = std::fs::rename(&tmp_manifest_name, &manifest_name) {
@@ -927,6 +924,23 @@ async fn pull_snapshot<'a>(
     Ok(Some(sync_stats))
 }
 
+async fn write_manifest_blob(
+    manifest: &BackupManifest,
+    path: &std::path::Path,
+) -> Result<Vec<u8>, Error> {
+    let manifest_blob = DataBlob::encode(manifest.to_string(None)?.as_bytes(), None, true)?;
+    let manifest_data = manifest_blob.raw_data().to_vec();
+    let mut manifest_file = OpenOptions::new()
+        .write(true)
+        .truncate(true)
+        .open(path)
+        .await?;
+    manifest_file.write_all(&manifest_data).await?;
+    manifest_file.flush().await?;
+    nix::unistd::fsync(manifest_file.as_raw_fd())?;
+    Ok(manifest_data)
+}
+
 /// 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.
-- 
2.47.3

[PATCH proxmox-backup 2/2] fix #6990: server: drop verify state on push job

[Jakob Klocker]

When pushing a snapshot the target manifest is recreated from the source
manifest, copying its verify_state. As with pull, this makes the pushed
snapshot appear verified on the remote without its stored copy having
been checked there.
Strip verify_state after copying the unprotected section so the pushed
snapshot is verified independently on the target.

Link: https://bugzilla.proxmox.com/show_bug.cgi?id=6990

Signed-off-by: Jakob Klocker <j.klocker@proxmox.com>
---
 src/server/push.rs | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/src/server/push.rs b/src/server/push.rs
index 7b3cac4cd..92d32bceb 100644
--- a/src/server/push.rs
+++ b/src/server/push.rs
@@ -1364,6 +1364,12 @@ pub(crate) async fn push_snapshot(
     // Rewrite manifest for pushed snapshot, recreating manifest from source on target,
     // needs to update all relevant info for new manifest.
     target_manifest.unprotected = source_manifest.unprotected.clone();
+
+    if let Some(unprotected) = target_manifest.unprotected.as_object_mut() {
+        unprotected.remove("verify_state");
+    } else {
+        bail!("Encountered unexpected manifest without 'unprotected' section.");
+    }
     let manifest_string = if let Some((_id, crypt_config)) = &encrypt_using_key {
         let sync_source_signature = source_manifest.signature(crypt_config)?;
         target_manifest.set_sync_source_signature(&sync_source_signature)?;
-- 
2.47.3