Re: [PATCH proxmox-backup v3 19/30] fix #7251: api: push: encrypt snapshots using configured encryption key

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

On 4/19/26 11:06 PM, Thomas Lamprecht wrote:
> Am 14.04.26 um 14:59 schrieb Christian Ebner:
>> diff --git a/src/server/push.rs b/src/server/push.rs
>>
>> +        if all_unencrypted {
>> +            encrypt_using_key = params.crypt_config.clone();
>> +            info!("Encrypt and push unencrypted snapshot '{snapshot}'");
>> +        } else if any_unencrypted {
>> +            warn!("Encountered partially encrypted snapshot '{snapshot}', refuse to re-encrypt and skip");
>> +            return Ok(stats);
>> +        } else {
>> +            info!("Pushing already encrypted snapshot '{snapshot}' without re-encryption");
>> +        }
> 
> The "already fully encrypted" case pushes client-encrypted content
> through unchanged, regardless of whether the configured sync key matches
> the client key. That's the right behavior (re-encrypting client-
> encrypted content is impossible without the client key), but from the
> user side this is surprising: they assigned a sync key and the log only
> says "without re-encryption" - nothing indicates that the snapshot is
> *not* using the configured sync key at all.
> 
> Please document this explicitly in the docs patch (30), and consider

This is already mentioned in the docs patch though:
```
Already encrypted snapshots are not re-encrypted but rather pushed 
unmodified. Snapshots containing only partially encrypted contents are 
skipped for security reasons.
```

Any suggestions on how to improve the documentation to make this clearer?

> making the log message say something like "already encrypted with client
> key, configured sync key is not applied". Also, does a push job with an
> `encrypted-only=true` filter + a configured sync key make sense at all?
> If yes, the interaction is worth mentioning too.

It does not really make sense, no. It might be used to first push 
already encrypted snapshots to a target namespace with client encrypted 
only snapshots, then syncing all the snapshots with re-encryption to 
another namespace. But that is a bit far fetched.

Will explicitly mention this in the docs so that is clear as well. Or 
should I add checks to disallow configuring this in the first place?

> 
>> -    target_manifest.unprotected = source_manifest.unprotected;
>> -    target_manifest.signature = source_manifest.signature;
>> +    target_manifest.unprotected = source_manifest.unprotected.clone();
>> +    target_manifest.signature = source_manifest.signature.clone();
> 
> When encrypt_using_key is Some, the target manifest's per-file csums
> differ from the source (new ciphertexts), so a copied-over `signature`
> won't verify on the target anymore. PBS-to-PBS sync rarely carries
> client signatures in practice, but when it does, this produces a
> snapshot that fails signature verification. Probably safest to drop
> signature in the encrypt path and let the target manifest be unsigned.

That's true, will move this to be set for encrypt_using_key being false 
then, thanks!

> Minor observation on commit hygiene: patch 16 adds `let mut
> encrypt_using_key = None;` which is never assigned in that commit (only
> starting here in patch 19), triggering an unused_mut warning on the
> intermediate commit. Same pattern for `let mut new_manifest = None;` in
> patch 26 vs its assignment in patch 28. Not critical, but it means
> bisect-compiling each commit with -D warnings trips. Easy fix is to
> introduce the `mut` in the same patch that first assigns it, or to use
> `#[allow(unused_mut)]` on the intermediate.

Okay, will only make this mutable only once required then.