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

[Dominik Csapak <d.csapak@proxmox.com>]

On 7/11/25 13:45, Thomas Lamprecht wrote:
> The subject reads a bit sassy ^^
> 
> 
> Am 16.04.25 um 09:07 schrieb Dominik Csapak:
>> Some settings on changers prevents changing the encryption parameters
>> via the application, e.g. some libraries have a 'encryption disabled' or
>> 'encryption is library managed' option. While the former situation can
>> be fixed by setting the library to 'application managed', the latter is
>> sometimes necessary for FIPS compliance (to ensure the tape data is
>> encrypted).
>>
>> When libraries are configured this way, the code currently fails with
>> 'drive does not support AES-GCM encryption'. Instead of failing, check
>> on first call to set_encryption if we could set it, and save that
>> result.
> 
> Could that result in a transient transport/HW/... error becoming
> permanent? I mean, probably not hugely likely, but with such "automagic"
> behavior it might be worth about thinking of some potential exit
> strategy for the user, or is the lifetime bound to only a single job
> anyway – I could naturally check myself, but might be nice to mention
> that here.

no that is just per job/action so each time we open the tape drive.
can reword that part to make it clearer

> 
>>
>> Only fail when encryption is to be enabled but it is not allowed, but
>> ignore the error when the backup should be done unencrypted.
>>
>> `assert_encryption_mode` must also check if it's possible, and skip any
>> error if it's not possible and we wanted no encryption.
>>
>> With these changes, it should be possible to use such configured libraries
>> when there is no encryption configured on the PBS side. (We currently
>> don't have a library with such capabilities to test.)
>>
>> Note that in contrast to normal operation, the tape label will also be
>> encrypted then and will not be readable in case the encryption key is
>> lost or changed.
>>
>> Additionally, return an error for 'drive_set_encryption' in case the
>> drive reports that it does not support hardware encryption, because this
>> is now already caught one level above in 'set_encryption'.
> 
> nit: "already" implies to me that it's checked upfront or handled by a
> method called, not one that calls us.
> 
> "[...] because this is now handled a level above"
> 
> is IMO a bit better fitting
> 

ok

>>
>> Also, slightly change the error message to make it clear that the drive
>> does not support *setting* encryption, not that it does not support
>> it at all.
>>
>> This was reported in the community forum:
>>
>> https://forum.proxmox.com/threads/107383/
>> https://forum.proxmox.com/threads/164941/
>>
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> changes from v1:
>> * adapted error message to make it clear what is not supported
>> * also adapt `assert_encryption_mode`, otherwise a backup is not possible
>>
>>   pbs-tape/src/sg_tape.rs            | 30 +++++++++++++++++++++++++++++-
>>   pbs-tape/src/sg_tape/encryption.rs | 12 ++----------
>>   src/tape/drive/lto/mod.rs          |  4 ++++
>>   3 files changed, 35 insertions(+), 11 deletions(-)
>>
>> diff --git a/pbs-tape/src/sg_tape.rs b/pbs-tape/src/sg_tape.rs
>> index 39e6a2f94..9c7a254d5 100644
>> --- a/pbs-tape/src/sg_tape.rs
>> +++ b/pbs-tape/src/sg_tape.rs
>> @@ -136,6 +136,8 @@ pub struct SgTape {
>>       file: File,
>>       locate_offset: Option<i64>,
>>       info: InquiryInfo,
>> +    // auto-detect if we can set the encryption mode
>> +    encryption_possible: Option<bool>,
>>   }
>>   
>>   impl SgTape {
>> @@ -158,6 +160,7 @@ impl SgTape {
>>               file,
>>               info,
>>               locate_offset: None,
>> +            encryption_possible: None,
>>           })
>>       }
>>   
>> @@ -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.

Though I hope not that anybody could enable that mode by accident
(since one must manage e.g. the encryption keys, etc.)

> 
>> +                return Ok(());
>> +            }
>> +        }
>>           let key = if let Some((ref key, ref uuid)) = key_data {
>>               // derive specialized key for each media-set
>>   
>> @@ -710,7 +721,24 @@ impl SgTape {
>>               None
>>           };
>>   
>> -        drive_set_encryption(&mut self.file, key)
>> +        match drive_set_encryption(&mut self.file, key) {
>> +            Ok(()) => self.encryption_possible = Some(true),
>> +            Err(err) => {
>> +                self.encryption_possible = Some(false);
>> +                if key.is_some() {
>> +                    bail!("could not set encryption mode on drive: {err}");
>> +                } else {
>> +                    log::info!("could not set encryption mode on drive: {err}, ignoring.");
> 
> While internally naming the function "set" might be fine, it always sets some
> value after all, but I'd not leak that outside and rather do s/set/clear/ for
> above log message, as otherwise the user will interpret this as we tried to
> set (= enable) encryption, failed and just ignore it, which seems rather unsafe
> from their POV.

yes makes sense

> 
>> +                }
>> +            }
>> +        }
>> +        Ok(())
>> +    }
>> +
>> +    /// Returns if encryption is possible. Returns [`None`] if it's unknown, because
>> +    /// no attempt was made to set the mode yet.
>> +    pub fn encryption_possible(&self) -> Option<bool> {
>> +        self.encryption_possible
>>       }
>>   
>>       // Note: use alloc_page_aligned_buffer to alloc data transfer buffer
>> diff --git a/pbs-tape/src/sg_tape/encryption.rs b/pbs-tape/src/sg_tape/encryption.rs
>> index 7247d257f..c24a6c658 100644
>> --- a/pbs-tape/src/sg_tape/encryption.rs
>> +++ b/pbs-tape/src/sg_tape/encryption.rs
>> @@ -12,15 +12,7 @@ use crate::sgutils2::{alloc_page_aligned_buffer, SgRaw};
>>   ///
>>   /// We always use mixed mode,
>>   pub fn drive_set_encryption<F: AsRawFd>(file: &mut F, key: Option<[u8; 32]>) -> Result<(), Error> {
>> -    let data = match sg_spin_data_encryption_caps(file) {
>> -        Ok(data) => data,
>> -        Err(_) if key.is_none() => {
>> -            // Assume device does not support HW encryption
>> -            // We can simply ignore the clear key request
>> -            return Ok(());
>> -        }
>> -        Err(err) => return Err(err),
>> -    };
>> +    let data = sg_spin_data_encryption_caps(file)?;
>>   
>>       let algorithm_index = decode_spin_data_encryption_caps(&data)?;
>>   
>> @@ -266,7 +258,7 @@ fn decode_spin_data_encryption_caps(data: &[u8]) -> Result<u8, Error> {
>>   
>>           match aes_gcm_index {
>>               Some(index) => Ok(index),
>> -            None => bail!("drive does not support AES-GCM encryption"),
>> +            None => bail!("drive does not support setting AES-GCM encryption"),
>>           }
>>       })
>>       .map_err(|err: Error| format_err!("decode data encryption caps page failed - {}", err))
>> diff --git a/src/tape/drive/lto/mod.rs b/src/tape/drive/lto/mod.rs
>> index 23e043ce6..bd5ec8ae6 100644
>> --- a/src/tape/drive/lto/mod.rs
>> +++ b/src/tape/drive/lto/mod.rs
>> @@ -264,6 +264,10 @@ impl TapeDriver for LtoTapeHandle {
>>       }
>>   
>>       fn assert_encryption_mode(&mut self, encryption_wanted: bool) -> Result<(), Error> {
>> +        if Some(false) == self.sg_tape.encryption_possible() && !encryption_wanted {
>> +            // ignore assertion for unencrypted mode, because we can't set it anyway
> 
> s/because/if/ ?

sure, sounds also good to me

I'll send a v3. Since the changes are only rewordings of the message and
comments, I'll include the 'tested-by' tag from Andrew Stone, alright?

> 
>> +            return Ok(());
>> +        }
>>           let encryption_set = drive_get_encryption(self.sg_tape.file_mut())?;
>>           if encryption_wanted != encryption_set {
>>               bail!("Set encryption mode not what was desired (set: {encryption_set}, wanted: {encryption_wanted})");
> 



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel