Re: [PATCH proxmox-backup v3 28/30] sync: pull: decrypt snapshots with matching encryption key fingerprint

[Thomas Lamprecht <t.lamprecht@proxmox.com>]

Am 14.04.26 um 14:59 schrieb Christian Ebner:
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> @@ -783,9 +781,15 @@ async fn pull_snapshot<'a>(
>
> +        new_manifest.unprotected = manifest.unprotected.clone();
> +        if let Some(unprotected) = new_manifest.unprotected.as_object_mut() {
> +            unprotected.remove("key-fingerprint");
> +            unprotected.remove("verify_state");
> +        }

Shouldn't this also remove the change-detection-fingerprint? That field was
computed from the source's encrypted file entries and doesn't match the
decrypted local content. On the next sync:

1. raw manifest comparison fails (encrypted vs decrypted blobs)
2. change_detection_fingerprint() gets recomputed from the local decrypted
   entries, which won't match the source's stored one that was computed from
   encrypted entries
3. no fast path fires and everything gets re-downloaded and re-decrypted

For large datastores that's a pretty significant hit on every re-sync even
when nothing actually changed on the source. I think either removing the
stale fingerprint here or storing the source fingerprint as-is (without
recomputing from local entries on the next round) would fix it.

> +        tmp_manifest_file.write_all(&manifest_data).await?;
> +        tmp_manifest_file.flush().await?;

nit: only flush() but no fsync() before the rename below. Patch 26 correctly
uses nix::unistd::fsync() before its rename, might want to be consistent
here for crash safety (albeit it won't help with peformance, but at least
would be consistent - your call).

> +    if local_manifest_file_fp.is_some() && crypt_config.is_none() {
> +        bail!("local unencrypted snapshot detected, refuse to sync without source decryption");
> +    }

nit: the error message is slightly misleading - the local snapshot isn't
"unencrypted" per se, it was previously *decrypted*. Something like "local
snapshot was previously decrypted but no matching decryption key is
configured" would probably better describe the situation for the user's POV.