From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [45.144.208.40]) by lore.proxmox.com (Postfix) with ESMTPS id 44B391FF0AA for ; Fri, 21 Aug 2026 13:18:53 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id F26C9215F4; Fri, 21 Aug 2026 13:18:47 +0200 (CEST) From: Jakob Klocker To: pbs-devel@lists.proxmox.com Subject: [PATCH proxmox-backup v2 3/3] fix #6990: server: drop verify state on non-decrypt pull job Date: Fri, 21 Aug 2026 13:18:26 +0200 Message-ID: <20260821111826.299588-4-j.klocker@proxmox.com> X-Mailer: git-send-email 2.47.3 In-Reply-To: <20260821111826.299588-1-j.klocker@proxmox.com> References: <20260821111826.299588-1-j.klocker@proxmox.com> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 1 AWL -0.520 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) KAM_LAZY_DOMAIN_SECURITY 1 Sending domain does not have any anti-forgery methods RDNS_NONE 1.274 Delivered to internal network by a host with no rDNS SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_NONE 0.001 SPF: sender does not publish an SPF Record Message-ID-Hash: OIUNMI5WQESVSATOG42EOARBK35FOIZX X-Message-ID-Hash: OIUNMI5WQESVSATOG42EOARBK35FOIZX X-MailFrom: jklocker@iris.proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header CC: Jakob Klocker X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: 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 --- 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, 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 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