Re: [PATCH proxmox-backup v2 27/27] sync: pull: decrypt snapshots with matching encryption key fingerprint

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

Am 10.04.26 um 18:54 schrieb Christian Ebner:
> diff --git a/src/server/pull.rs b/src/server/pull.rs
> @@ -647,6 +704,22 @@ async fn pull_snapshot<'a>(
>
> +    // pre-existing local manifest for unencrypted snapshot, never overwrite with encrypted
> +    if local_manifest_key_fp.is_some() && crypt_config.is_none() {
> +        bail!("local unencrypted snapshot detected, refuse to sync without source decryption");
> +    }

This condition looks inverted AFAICT. After decrypt-on-pull the local manifest
has key-fingerprint: null, so manifest.fingerprint() returns None,
local_manifest_key_fp is None, and the is_some() check never fires.

The scenario it should catch: key removed from job after a previous
decrypt-on-pull, remote snapshot changed, local decrypted data gets silently
overwritten with encrypted content.

I think checking local_manifest_file_fp.is_some() (the
change-detection-fingerprint presence) would be the right indicator for this
situation, at least FWICT that field only exists in manifests produced by the
decrypt path (but might warrant a closer check).

> @@ -696,11 +769,38 @@ async fn pull_snapshot<'a>(
>
> +        new_manifest.unprotected["key-fingerprint"] = Value::Null;

nit: do we need to set the field explicitly to JSON null rather than removing
it via e.g. `.as_object_mut().unwrap().remove("key-fingerprint")` instead? but
no hard feelings here.

> +        new_manifest.unprotected = manifest.unprotected.clone();

nit: this copies verify state from the encrypted source manifest into the
decrypted target. The decryption itself acts as a verification of sorts (AEAD
would catch corruption), so this is fine, but a comment explaining the
reasoning would be IMO nice.

> +                info!("Found matching key fingerprint {source_fingerprint}, decrypt on pull");

nit: I'd suggest to also log which key ID matched, not just the fingerprint, as
that's then easier for the admin to cross-reference with the job config.