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

[Thomas Lamprecht]

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.

> 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.