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

[Christian Ebner <c.ebner@proxmox.com>]

Thanks for the patch, mostly looks good to me.

Just one comment and nit inline.

On 7/9/26 2:22 PM, Jakob Klocker wrote:
> 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())?;

nit: pre-existing but this is a blocking call and we are in an async 
context, so we should spawn a blocking tokio task for this I guess. Same 
is true for the std::fs::rename which should be replaced by it's 
tokio::fs::rename counterpart and the call to 
BackupDir::cleanup_unreferenced_files().

Could you also include additional patches to fix these since touching this?

> +    Ok(manifest_data)
> +}

comment: This should rather live as method impl on BackupManifest itself 
IMO. It could further be refactored to reduce common code with 
pbs-datastores BackupDir::update_manifest().

>   /// 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.