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

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

On 4/11/26 10:50 AM, Thomas Lamprecht wrote:
> 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).

Yes, you are absolutely right: I introduced the 
change-detection-fingerprint exactly for this after reports on v1 this 
being an issue, but  messed up the check here...

Will be fixed for the next iteration of the patches, including the nits 
below, thanks!

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