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

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

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.

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?

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.

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.

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?

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.

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.

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.

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.

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.