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

[Thomas Lamprecht]

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.

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

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

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.

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

> +                }
> +            }
> +        }
> +        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/ ?

> +            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})");