[pbs-devel] [PATCH proxmox-backup] tape: fix regression in restoring key from medium

[pbs-devel] [PATCH proxmox-backup] tape: fix regression in restoring key from medium

[Dominik Csapak]

when trying to restore a key from a tape, we automatically try to load
the key into the drive after reading the media-set label.

Since the key restore is only useful when the key is not already on
disk, this fails of course.

To fix it but leave the automatism in place, introduce a function
'read_label_without_loading_key' that does the heavy lifting of reading
the labels but does not load the encryption key. Then in read_label,
call that function and explictely load the key (when one exists).

Then, we only have to use this new function in the 'restore-key' api
call for the restore to work as intended.

Fixes: 1343dcaf ("tape: move 'set_encryption' calls to the TapeDriver (and implementation)")
Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
---
should be cherry-pickable for stable-2 if necessary

 src/api2/tape/drive.rs |  2 +-
 src/tape/drive/mod.rs  | 42 ++++++++++++++++++++++++++++++------------
 2 files changed, 31 insertions(+), 13 deletions(-)

diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs
index 53b14bfd..5a5d39d9 100644
--- a/src/api2/tape/drive.rs
+++ b/src/api2/tape/drive.rs
@@ -630,7 +630,7 @@ pub async fn restore_key(drive: String, password: String) -> Result<(), Error> {
     run_drive_blocking_task(drive.clone(), "restore key".to_string(), move |config| {
         let mut drive = open_drive(&config, &drive)?;
 
-        let (_media_id, key_config) = drive.read_label()?;
+        let (_media_id, key_config) = drive.read_label_without_loading_key()?;
 
         if let Some(key_config) = key_config {
             let password_fn = || Ok(password.as_bytes().to_vec());
diff --git a/src/tape/drive/mod.rs b/src/tape/drive/mod.rs
index 73886f14..2fc88ef8 100644
--- a/src/tape/drive/mod.rs
+++ b/src/tape/drive/mod.rs
@@ -105,11 +105,13 @@ pub trait TapeDriver {
         key_config: Option<&KeyConfig>,
     ) -> Result<(), Error>;
 
-    /// Read the media label
+    /// Read the media label without setting the encryption key
     ///
-    /// This tries to read both media labels (label and
-    /// media_set_label). Also returns the optional encryption key configuration.
-    fn read_label(&mut self) -> Result<(Option<MediaId>, Option<KeyConfig>), Error> {
+    /// This is used internally by 'read_label' and when restoring the encryption
+    /// key from the drive. Should not be used or overwritten otherwise!
+    fn read_label_without_loading_key(
+        &mut self,
+    ) -> Result<(Option<MediaId>, Option<KeyConfig>), Error> {
         self.rewind()?;
 
         let label = {
@@ -182,18 +184,34 @@ pub trait TapeDriver {
             bail!("got unexpected data after media set label");
         }
 
-        drop(reader);
+        media_id.media_set_label = Some(media_set_label);
 
-        let encrypt_fingerprint = media_set_label
-            .encryption_key_fingerprint
-            .clone()
-            .map(|fp| (fp, media_set_label.uuid.clone()));
+        Ok((Some(media_id), key_config))
+    }
 
-        self.set_encryption(encrypt_fingerprint)?;
+    /// Read the media label
+    ///
+    /// This tries to read both media labels (label and
+    /// media_set_label). Also returns the optional encryption key configuration.
+    ///
+    /// Automatically sets the encryption key on the drive
+    fn read_label(&mut self) -> Result<(Option<MediaId>, Option<KeyConfig>), Error> {
+        let (media_id, key_config) = self.read_label_without_loading_key()?;
 
-        media_id.media_set_label = Some(media_set_label);
+        let encrypt_fingerprint = if let Some(media_set_label) =
+            media_id.as_ref().and_then(|id| id.media_set_label.clone())
+        {
+            media_set_label
+                .encryption_key_fingerprint
+                .clone()
+                .map(|fp| (fp, media_set_label.uuid.clone()))
+        } else {
+            None
+        };
 
-        Ok((Some(media_id), key_config))
+        self.set_encryption(encrypt_fingerprint)?;
+
+        Ok((media_id, key_config))
     }
 
     /// Eject media
-- 
2.39.2

[pbs-devel] applied: [PATCH proxmox-backup] tape: fix regression in restoring key from medium

[Thomas Lamprecht]

Am 31/01/2024 um 14:42 schrieb Dominik Csapak:
> when trying to restore a key from a tape, we automatically try to load
> the key into the drive after reading the media-set label.
> 
> Since the key restore is only useful when the key is not already on
> disk, this fails of course.
> 
> To fix it but leave the automatism in place, introduce a function
> 'read_label_without_loading_key' that does the heavy lifting of reading
> the labels but does not load the encryption key. Then in read_label,
> call that function and explictely load the key (when one exists).
> 
> Then, we only have to use this new function in the 'restore-key' api
> call for the restore to work as intended.
> 
> Fixes: 1343dcaf ("tape: move 'set_encryption' calls to the TapeDriver (and implementation)")
> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
> ---
> should be cherry-pickable for stable-2 if necessary
> 
>  src/api2/tape/drive.rs |  2 +-
>  src/tape/drive/mod.rs  | 42 ++++++++++++++++++++++++++++++------------
>  2 files changed, 31 insertions(+), 13 deletions(-)
> 
>

applied, with a reword commit message, thanks!

But I'm wondering, why exactly caused the key load issues if we are
in the mixed mode anyway?

A reference to the forum post, bugzilla with the report of the user
running into this would have been nice too..

But yeah, as this should not hurt and we got other fixes to roll out
I'm keeping continuing to roll this out nonetheless for now.

Re: [pbs-devel] applied: [PATCH proxmox-backup] tape: fix regression in restoring key from medium

[Dominik Csapak]

On 2/2/24 17:07, Thomas Lamprecht wrote:
> Am 31/01/2024 um 14:42 schrieb Dominik Csapak:
>> when trying to restore a key from a tape, we automatically try to load
>> the key into the drive after reading the media-set label.
>>
>> Since the key restore is only useful when the key is not already on
>> disk, this fails of course.
>>
>> To fix it but leave the automatism in place, introduce a function
>> 'read_label_without_loading_key' that does the heavy lifting of reading
>> the labels but does not load the encryption key. Then in read_label,
>> call that function and explictely load the key (when one exists).
>>
>> Then, we only have to use this new function in the 'restore-key' api
>> call for the restore to work as intended.
>>
>> Fixes: 1343dcaf ("tape: move 'set_encryption' calls to the TapeDriver (and implementation)")
>> Signed-off-by: Dominik Csapak <d.csapak@proxmox.com>
>> ---
>> should be cherry-pickable for stable-2 if necessary
>>
>>   src/api2/tape/drive.rs |  2 +-
>>   src/tape/drive/mod.rs  | 42 ++++++++++++++++++++++++++++++------------
>>   2 files changed, 31 insertions(+), 13 deletions(-)
>>
>>
> 
> applied, with a reword commit message, thanks!
> 
> But I'm wondering, why exactly caused the key load issues if we are
> in the mixed mode anyway?

the error was from our side, since we did not find the key in a
disaster recovery case (when you want to restore the key from tape)

> 
> A reference to the forum post, bugzilla with the report of the user
> running into this would have been nice too..

it was a customer in the enterprise support, so there's nothing public
to link to

> 
> But yeah, as this should not hurt and we got other fixes to roll out
> I'm keeping continuing to roll this out nonetheless for now.

thanks!