Re: [PATCH proxmox{,-backup} 00/20] fix #7251: implement server side encryption support for push sync jobs

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

On 4/2/26 2:24 AM, Thomas Lamprecht wrote:
> Am 01.04.26 um 09:55 schrieb Christian Ebner:
>> This patch series implements support for encrypting backup snapshots
>> when pushing from a source PBS instance to an untrusted remote target
>> PBS instance. Further, it adds support to decrypt snapshots being
>> encrypted on the remote source PBS when pulling the contents to the
>> local target PBS instance.
> 
> Thanks, the overall direction makes sense, a few design-level concerns
> below, but note that this is includes various stuff all over the place, I
> did not yet rechecked them with a fresh head, so do not take them for face
> value, rather input/questions to your own surely much more mature mental
> model.
> 
> Pull resync after decryption is efficiency-wise not ideal. The local
> decrypted manifest will never byte-match the remote encrypted one, so the
> `raw_data()` fast path in pull_snapshot can never fire for already-decrypted
> snapshots. The per-file verification then also fails (different checksums),
> causing a full re-download. The `last_sync_time` filtering limits this to
> the latest snapshot per group, so it's not that bad, but probablly still
> worth to improve - especially for large snapshots that rarely change.

Yes, you are right this is still needs optimization and I'm aware of it. 
What could work is to either store the previous manifest checksum for 
comparison. I didn't want to spent to much time on optimization just 
yet, this is still something I wanted to explore further if the overall 
approach is fine.

> On the push side, `upload_blob` is a raw byte passthrough, so an unencrypted
> source backup will then e.g. have `.conf.blob` or `.pxar.catalog.blob` get
> stored unencrypted on the untrusted remote. Might be nicer to also encrypt
> those here from security and an integrity standpoint?

That is not intentional and will be fixed, thanks!

> Also, `upload_blob_from_file` for the client log goes through
> `upload_blob_from_data` which calls `DataBlob::encode()` on already-encoded
> on-disk bytes, so you'd end up with a double-wrapped blob on the remote.

Will fix this as well!

> Minor: `check_privs_and_load_key_config` uses `PRIV_SYS_AUDIT` to gate key
> access. It's not a real security issue since creating/running sync jobs
> already requires much heavier privileges (Datastore.Backup, Remote.Read,
> etc.), but Audit is semantically a read-only privilege. Might be worth using
> something like Sys.Modify instead to better express the intent. We could
> also create a few new privs for this, if it helps.

Okay, will escalate to Sys.Modify on the key acl also for loading the key.

> When a server-side key is configured on push, all snapshots with any
> encrypted content are silently skipped. That includes fully client-side
> encrypted ones that could just be synced as-is - they are already encrypted
> after all. Might be better to push those unchanged and only skip genuinely
> partially-encrypted snapshots? Or if unsure, allow to user to chose what
> they want. Wrapping encryption is probably never a good idea though,
> maybe just any parts that are not yet encrypted?

Pushing fully encrypted ones as is makes sense, only log and skip 
partially encrypted snapshots.

> Nit: The same `encryption_key` field means "encrypt" for push and
> "decrypt" for pull. Maybe be literal here and name it decryption_key for
> the later. Or something generic that works for both.

Yes, that make sense... Was primed by the fact that the entities 
themself are referred to as encryption keys, but renaming this to 
decryption_key for the pull makes more sense for making the code better 
readable. Will adapt that.

> A few key lifecycle things:
> 
> Deleting a key does not check whether sync jobs reference it; the next run
> just fails. Might want to refuse deletion while references exist, or at
> least warn.

Yes, also realized this just now, will add a check for this in the api 
handler and not allow to remove it in that case.

> Password-protected keys can be uploaded but in the method
> `check_privs_and_load_key_config` you always pass an empty passphrase
> to its `key_config.decrypt()` call, failing with "Passphrase is too short!"
> in such a case.

Good catch, password protected keys should not be uploadable for the 
time being. It was discussed off list that we might extend this to allow 
for password protected keys in the future. An intial draft version 
simply stored the key in the config as well, but since that does not add 
anything security wise and there are ideas to improve secret handling in 
general, supporting encrypted keys was dropped for now.

> Verify state gets copied from the encrypted source manifest into the
> decrypted target. That verification was against encrypted data though, so
> might want to clear it on decryption. Albeit, the decryption itself
> basically was a verification, so not really problematic.

Will strip this when decrypting, thanks!

> Some other things:
> 
> `load_previous_snapshot_known_chunks` always calls
> `download_previous_fixed_index` even for DynamicIndex archives, breaking
> being able to leverage the deduplication for cross-snapshot chunk dedup
>   for .didx (error is silently swallowed via `let _res`) AFAICT.
> 
> The `.dectmp` temp file from blob decryption on pull is not cleaned up on
> error, and `create_new(true)` means a retry fails then too.
> 
> `log::info!` vs `tracing::info!` in check_privs_and_load_key_config;
> "indetical" typo (x2) in the index rewrite comments.

Will look into these as well, thanks a lot for comments and review!