[pbs-devel] [PATCH proxmox-backup v2] tape: skip setting encryption if we can't and don't want to

[Dominik Csapak]

On 7/11/25 17:09, Thomas Lamprecht wrote:
> Am 11.07.25 um 15:29 schrieb Dominik Csapak:
>>>> @@ -690,6 +693,14 @@ impl SgTape {
>>>>        }
>>>>    
>>>>        pub fn set_encryption(&mut self, key_data: Option<([u8; 32], Uuid)>) -> Result<(), Error> {
>>>> +        if self.encryption_possible == Some(false) {
>>>> +            if key_data.is_some() {
>>>> +                bail!("Drive does not support setting encryption mode");
>>>> +            } else {
>>>> +                // skip trying to set encryption if not supported and don't wanted
>>> s/don't/not/ and s/set/clear/ (still not ideal but slightly better IMO).
>>>
>>> btw. if one enabled encryption before changing the library to FIPS (or
>>> whatever blocks this setting then) and then cleared encryption we might
>>> then signal that it work the next time this method is called while it
>>> did not, seems a bit odd.
>>>
>>> Albeit, if that question even makes sense depends on the lifetime of the
>>> encryption_possible setting.
>>>
>> since it's just for one job/action I don't think that's an issue, since
>> modifying drive/changer settings during such an operation won't work most of
>> the time anyway (since those things like to block during operation)
>>
>>> Did you evaluate if exposing a setting the user has to explicitly enable?
>>> Not saying that is better, but might be a bit simpler to understand and
>>> probably also slightly safer to implement (or at least review hehe), I think.
>>> OTOTH., not much in the tape code atm., so my questions might be bogus and
>>> my reservation for this unfounded.
>> The original patch is already a few months old, so I'm not sure what exactly
>> i evaluated, but thinking about it now, I don't really want to expose
>> an option that 99% of users will never need and there is not much choice
>> in what to do anyway. One advantage of a user setting might be that
>> if the 'FIPS' mode (i'll just call it that for the moment) is enabled
>> by accident, we'd error out instead of just logging it.
> 
> IMO one really should not argue with 99% of users not requiring something,
> either this auto-magic is safe to do or not, if not a option is required
> in any way if the derivation of default (safe) behavior is required.
> You got already a handful reports, so the problem is obviously there, and
> while I certainly do not argue for adding a option for every little
> behavior, this is not just some small behavioral quirk, for which I'd agree
> with your assessment. But as this is relevant for data security, it means
> that if done wrong, backups might be either unprotected while the admin
> thinks they are, or not usable because encrypted while the admin doesn't
> expect them to be.
> 
> As said, that might not be possible and your implementation might be fully
> safe, but if you want to keep it as internal "auto-magic" behavior–which
> _might_ be fine–then all those edge cases should be closely looked at and
> explained in the commit message.
> 

I understand what you mean, and I think this implementation is ok the
way it is, maybe a short summary of situations that helps understand
it better:

* Enabling/Disabling encryption on the changer side must be done
   explicitly, since the encryption keys must be managed from there.
   So the Changer side mode is intentional by the admin.

* We have a very particular way to set encryption: The initial block
   is written unencrypted, while the remaining blocks are encrypted.

* If the encryption is forced (or forced off) changer side, we cannot
   modify the key or mode. Also the reads/writes are completely transparent to us.

This patch now ignores the following "errors":
* we try to write unencrypted but the changer forces encryption

When trying to set encryption from our side, but the changer/drive
does not let us, we bail with an error.

Note we only ever check the encryption mode when starting a new
tape (for performance reasons, if we'd check/set for every block/write
we'd be too slow for high tape throughput)

so if a user would/can change the encryption mode mid-backup, this
would impact current backups too, since it's only detected on
the start of a new tape.

(And while I don't exactly how these tape libs behave since we don't have
one, AFAIK you can't change the mode mid-backup, since our jobs
constantly use the drive, which makes it not available for such requests
from the changer/lib)

So in opinion, a setting is overkill, since if the admin sets
such a particular mode, it'll not be by accident, and the only
way forward then would be to set the option. While with my changes
it's used automatically. If the user wanted encryption on pbs side
he'll get an error when that does not happen. Only the other way around
(unencrypted on pbs side and encrypted on drive/changer side) is allowed
with this patch.

>> Though I hope not that anybody could enable that mode by accident
>> (since one must manage e.g. the encryption keys, etc.)
> 
> Yeah, I'd be also wary to create a generic and opaque "FIPS mode", besides
> being hard to relate to for those admins that actually care about security
> and don't just want to tick off some checkbox, if we cannot know/control
> the external HW such that it's actually upheld it's very dangerous.
> Specific simple question that map to specific behavior are much easier to
> maintain and understand; if we would get more of those then one could think
> about adding a setting that turns them all on at once.