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

[Jakob Klocker <j.klocker@proxmox.com>]

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>
---
 pbs-datastore/src/manifest.rs | 25 ++++++++++++++++++-
 src/server/pull.rs            | 47 +++++++++++++++++++++++------------
 2 files changed, 55 insertions(+), 17 deletions(-)

diff --git a/pbs-datastore/src/manifest.rs b/pbs-datastore/src/manifest.rs
index 11de1085b..c87797474 100644
--- a/pbs-datastore/src/manifest.rs
+++ b/pbs-datastore/src/manifest.rs
@@ -1,5 +1,8 @@
-use anyhow::{Error, bail, format_err};
+use std::io::Write;
+use std::os::fd::AsRawFd;
+use std::path::Path;
 
+use anyhow::{Context, Error, bail, format_err};
 use serde::{Deserialize, Serialize};
 use serde_json::{Value, json};
 
@@ -278,6 +281,26 @@ impl BackupManifest {
 
         Ok(Some(Deserialize::deserialize(value)?))
     }
+
+    /// Encode the manifest and write the raw blob data to `path`, fsync'ing it.
+    ///
+    /// Returns the raw blob data.
+    pub fn write_to_path(&self, path: &Path) -> Result<Vec<u8>, Error> {
+        let raw_data = self.to_data_blob(None)?.raw_data().to_vec();
+
+        let mut file = std::fs::OpenOptions::new()
+            .write(true)
+            .create(true)
+            .truncate(true)
+            .open(path)
+            .with_context(|| format!("failed to open manifest {path:?}"))?;
+
+        file.write_all(&raw_data)?;
+        file.flush()?;
+        nix::unistd::fsync(file.as_raw_fd())?;
+
+        Ok(raw_data)
+    }
 }
 
 impl TryFrom<DataBlob> for BackupManifest {
diff --git a/src/server/pull.rs b/src/server/pull.rs
index fd401ac85..acb7a98dc 100644
--- a/src/server/pull.rs
+++ b/src/server/pull.rs
@@ -3,7 +3,6 @@
 use std::collections::hash_map::Entry;
 use std::collections::{HashMap, HashSet};
 use std::io::Seek;
-use std::os::fd::AsRawFd;
 use std::sync::atomic::{AtomicU64, AtomicUsize, Ordering};
 use std::sync::{Arc, Mutex};
 use std::time::{Duration, SystemTime};
@@ -16,8 +15,6 @@ use super::sync::{
 use crate::backup::{check_ns_modification_privs, check_ns_privs};
 use crate::server::sync::SharedGroupProgress;
 use anyhow::{Context, Error, bail, format_err};
-use tokio::fs::OpenOptions;
-use tokio::io::AsyncWriteExt;
 
 use pbs_api_types::{
     ArchiveType, Authid, BackupDir, BackupGroup, BackupNamespace, CLIENT_LOG_BLOB_NAME, CryptMode,
@@ -741,7 +738,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 =
+        Arc::new(BackupManifest::try_from(tmp_manifest_blob).with_context(|| prefix.clone())?);
 
     if ignore_not_verified_or_encrypted(
         &manifest,
@@ -849,6 +847,10 @@ async fn pull_snapshot<'a>(
         sync_stats.add(stats);
     }
 
+    let target_verify_state = existing_target_manifest
+        .as_ref()
+        .and_then(|m| m.unprotected.get("verify_state").cloned());
+
     if let Some(new_manifest) = new_manifest {
         let mut new_manifest = Arc::try_unwrap(new_manifest)
             .map_err(|_arc| {
@@ -875,19 +877,32 @@ async fn pull_snapshot<'a>(
             new_manifest.set_sync_source_signature(expected.bytes())?;
         }
 
-        // keep signature
-        let manifest_blob = new_manifest.to_data_blob(None)?;
-        // update contents to be uploaded to backend
-        manifest_data = manifest_blob.raw_data().to_vec();
+        let tmp_manifest_name = tmp_manifest_name.clone();
+        manifest_data =
+            tokio::task::spawn_blocking(move || new_manifest.write_to_path(&tmp_manifest_name))
+                .await??;
+    } else if manifest.unprotected.get("verify_state") != target_verify_state.as_ref() {
+        // the manifest is copied as-is on the non-decrypted path: never inherit the
+        // source's verify state, but keep one the target obtained on its own
+        let manifest_mut = Arc::get_mut(&mut manifest)
+            .ok_or_else(|| format_err!("{prefix}: manifest unexpectedly shared"))?;
+        let Some(unprotected) = manifest_mut.unprotected.as_object_mut() else {
+            bail!("{prefix}: unexpected manifest without 'unprotected' section");
+        };
+        match &target_verify_state {
+            Some(state) => {
+                unprotected.insert("verify_state".to_string(), state.clone());
+            }
+            None => {
+                unprotected.remove("verify_state");
+            }
+        }
 
-        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())?;
+        let manifest = Arc::clone(&manifest);
+        let tmp_manifest_name = tmp_manifest_name.clone();
+        manifest_data =
+            tokio::task::spawn_blocking(move || manifest.write_to_path(&tmp_manifest_name))
+                .await??;
     }
 
     if let Err(err) = tokio::fs::rename(&tmp_manifest_name, &manifest_name).await {
-- 
2.47.3