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

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

On 08/04/2026 10:12, Christian Ebner wrote:
> On 4/8/26 9:49 AM, Fabian Grünbichler wrote:
>> On April 2, 2026 9:37 am, Christian Ebner wrote:
>>> 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.
>>>>
>>
>> [..]
>>
>>>
>>>> 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.
>>
>> pushing fully encrypted snapshots *if they use the same key*, or all
>> fully encrypted snapshots? I think the former is okay, the latter might
>> be confusing/unexpected and lead to "I have all my backups on the remote
>> but can't decrypt them because of the wrong key"?
> 
> after pondering about this a bit I must agree, mixing of content encrypted by different keys during sync will cause headache for users not expecting it.

Well, can they really loose? Not having a backups synced might be
unexpected too, and while the user could create another sync job, having
to do so can be a bit of a hassle.
Also, the contract of "everything gets encrypted, if it already is, it
gets just send along as is" doesn't sound that complex to me.
> Given this, I would rather keep the handling as is, meaning already encrypted backups are not pushed and maybe even add a check to detect if the previous manifest of the backup group (if present) was encrypted using the same key, refusing to sync at all if not?

Confusion might be better resolved through docs and task log warnings,
depending on gravity, over refusing to sync something, which can be
fatal if the primary data source breaks and one searches for backups
only to find that they did not get synced out of confusion protection.

Don't get me wrong, I agree that this is not ideal and Fabian has a
point, but with syncing or not syncing data one needs to be very careful
for a backup server appliance.
>> e.g.:
>>
>> client A uses key A to backup to PBS X
>> client B uses plaintext to backup to PBS X
>> PBS X uses key B to sync to PBS Y
>>
>> user thinks key B is enough to restore backups from Y (after all, it's
>> the encryption key used for syncing), and destroys clients A and B and
>> PBS X (all in the same location maybe?). now they can't recover client
>> A..
>>
>> I know this is technically on the user (they didn't pay attention to
>> their key management), but seems like a potential foot gun. if we skip
>> the groups of client A because of a key mismatch (and warn about that?)
>> then it is at least obvious that those backups *are not on Y*. we could
>> have a flag for allowing key mismatches to support such scenarios
>> opt-in?
> 
> I would rather not allow to flag this, but make the user explicitly sync such snapshots if truly wanted. After all we do already have the encrypted-only in place, which allows to sync encrypted snapshots without touching the others. It seems to me that making mixing contents the undesired, hard route is the better way, given also the pull decrypt mismatch issues with 2 jobs using different keys as reported by Manuel.

Extra option is naturally adding complexity, but it would allow making
the decision by the users, i.e. the ones that can actually know what the
right choice for them is.

But IMO we could imply the sync everything, but encrypt only unecrypted
part now, given that datastores and namespaces often are organised such
that most backups in there are the same and that we have already lots of
possibilities to filter out stuff, and if user demand for skipping these
comes up, then we could add a filter flag for exclude/include encrypted
(and for another usecase also unencrypted) backup snapshots from the job
selection, which would come relatively naturally and not be just another
flag tacked on the job options.
>>
>>>> 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.
>>
>> key rotation/lifecycle management would be a question in any case - we
>> could have one "active" key per sync job, and a list of archived ones
>> that are only used for decryption? a key rotation would obviously break
>> deduplication chains.. such a scheme would also allow re-encryption on
>> the fly of client-encrypted backups, if a matching key is provided -
>> though the use case for that seems limited to me (why encrypt in the
>> first place if you trust this PBS with the plaintext data?).
>>
>> or alternatively it would need to be documented that for rotating a key,
>> you want to create a new key and target namespace, then switch the sync
>> job over (syncing everything from scratch), and delete the old copies
>> encrypted with the previous key once the re-sync is done?
> 
> Given above, I think the alternative option might be the better way. Mixing snapshots in a group with different keys should not be encouraged due to possible key loss as described above. But making it possible to archive the key, so it is still kept around for decryption, but not usable to push new encrypted backups is definitely something I should implement, yes. And probably it should not be possible to remove a key, which has not been marked as archived first, even if currently not in use by any sync jobs?
> 


Yeah, key removal might be well something that benefits from some extra
steps and/or warnings needed.