[pbs-devel] [PATCH proxmox-backup 09/10] client: track key source, print when used

["Fabian Grünbichler" <f.gruenbichler@proxmox.com>]

to avoid confusing messages about using encryption keys when restoring
plaintext backups, or about loading master keys when they are not
actually used for the current operation.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox-backup-client.rs          | 185 +++++++++++++++-------
 src/bin/proxmox_backup_client/catalog.rs  |  13 +-
 src/bin/proxmox_backup_client/key.rs      |  24 +--
 src/bin/proxmox_backup_client/snapshot.rs |   2 +-
 4 files changed, 155 insertions(+), 69 deletions(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 89d77d04..2db5cf7d 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -606,12 +606,56 @@ fn spawn_catalog_upload(
     Ok(CatalogUploadResult { catalog_writer, result: catalog_result_rx })
 }
 
+#[derive(Clone, Debug, Eq, PartialEq)]
+enum KeySource {
+    DefaultKey,
+    Fd,
+    Path(String),
+}
+
+fn format_key_source(source: &KeySource, key_type: &str) -> String {
+    match source {
+        KeySource::DefaultKey => format!("Using default {} key..", key_type),
+        KeySource::Fd => format!("Using {} key from file descriptor..", key_type),
+        KeySource::Path(path) => format!("Using {} key from '{}'..", key_type, path),
+    }
+}
+
+#[derive(Clone, Debug, Eq, PartialEq)]
+struct KeyWithSource {
+    pub source: KeySource,
+    pub key: Vec<u8>,
+}
+
+impl KeyWithSource {
+    pub fn from_fd(key: Vec<u8>) -> Self {
+        Self {
+            source: KeySource::Fd,
+            key,
+        }
+    }
+
+    pub fn from_default(key: Vec<u8>) -> Self {
+        Self {
+            source: KeySource::DefaultKey,
+            key,
+        }
+    }
+
+    pub fn from_path(path: String, key: Vec<u8>) -> Self {
+        Self {
+            source: KeySource::Path(path),
+            key,
+        }
+    }
+}
+
 #[derive(Debug, Eq, PartialEq)]
 struct CryptoParams {
     mode: CryptMode,
-    enc_key: Option<Vec<u8>>,
+    enc_key: Option<KeyWithSource>,
     // FIXME switch to openssl::rsa::rsa<openssl::pkey::Public> once that is Eq?
-    master_pubkey: Option<Vec<u8>>,
+    master_pubkey: Option<KeyWithSource>,
 }
 
 fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
@@ -656,52 +700,47 @@ fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
         None => None,
     };
 
-    let keydata = match (keyfile, key_fd) {
+    let key = match (keyfile, key_fd) {
         (None, None) => None,
         (Some(_), Some(_)) => bail!("--keyfile and --keyfd are mutually exclusive"),
-        (Some(keyfile), None) => {
-            eprintln!("Using encryption key file: {}", keyfile);
-            Some(file_get_contents(keyfile)?)
-        },
+        (Some(keyfile), None) => Some(KeyWithSource::from_path(
+            keyfile.clone(),
+            file_get_contents(keyfile)?,
+        )),
         (None, Some(fd)) => {
             let input = unsafe { std::fs::File::from_raw_fd(fd) };
             let mut data = Vec::new();
-            let _len: usize = { input }.read_to_end(&mut data)
-                .map_err(|err| {
-                    format_err!("error reading encryption key from fd {}: {}", fd, err)
-                })?;
-            eprintln!("Using encryption key from file descriptor");
-            Some(data)
+            let _len: usize = { input }.read_to_end(&mut data).map_err(|err| {
+                format_err!("error reading encryption key from fd {}: {}", fd, err)
+            })?;
+            Some(KeyWithSource::from_fd(data))
         }
     };
 
-    let master_pubkey_data = match (master_pubkey_file, master_pubkey_fd) {
+    let master_pubkey = match (master_pubkey_file, master_pubkey_fd) {
         (None, None) => None,
         (Some(_), Some(_)) => bail!("--keyfile and --keyfd are mutually exclusive"),
-        (Some(keyfile), None) => {
-            eprintln!("Using master key from file: {}", keyfile);
-            Some(file_get_contents(keyfile)?)
-        },
+        (Some(keyfile), None) => Some(KeyWithSource::from_path(
+            keyfile.clone(),
+            file_get_contents(keyfile)?,
+        )),
         (None, Some(fd)) => {
             let input = unsafe { std::fs::File::from_raw_fd(fd) };
             let mut data = Vec::new();
-            let _len: usize = { input }.read_to_end(&mut data)
-                .map_err(|err| {
-                    format_err!("error reading master key from fd {}: {}", fd, err)
-                })?;
-            eprintln!("Using master key from file descriptor");
-            Some(data)
+            let _len: usize = { input }
+                .read_to_end(&mut data)
+                .map_err(|err| format_err!("error reading master key from fd {}: {}", fd, err))?;
+            Some(KeyWithSource::from_fd(data))
         }
     };
 
     let res = match mode {
         // no crypt mode, enable encryption if keys are available
-        None => match (keydata, master_pubkey_data) {
+        None => match (key, master_pubkey) {
             // only default keys if available
             (None, None) => match key::read_optional_default_encryption_key()? {
                 None => CryptoParams { mode: CryptMode::None, enc_key: None, master_pubkey: None },
                 enc_key => {
-                    eprintln!("Encrypting with default encryption key!");
                     let master_pubkey = key::read_optional_default_master_pubkey()?;
                     CryptoParams {
                         mode: CryptMode::Encrypt,
@@ -715,7 +754,6 @@ fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
             (None, master_pubkey) => match key::read_optional_default_encryption_key()? {
                 None => bail!("--master-pubkey-file/--master-pubkey-fd specified, but no key available"),
                 enc_key => {
-                    eprintln!("Encrypting with default encryption key!");
                     CryptoParams {
                         mode: CryptMode::Encrypt,
                         enc_key,
@@ -732,7 +770,7 @@ fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
         },
 
         // explicitly disabled encryption
-        Some(CryptMode::None) => match (keydata, master_pubkey_data) {
+        Some(CryptMode::None) => match (key, master_pubkey) {
             // no keys => OK, no encryption
             (None, None) => CryptoParams { mode: CryptMode::None, enc_key: None, master_pubkey: None },
 
@@ -744,7 +782,7 @@ fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
         },
 
         // explicitly enabled encryption
-        Some(mode) => match (keydata, master_pubkey_data) {
+        Some(mode) => match (key, master_pubkey) {
             // no key, maybe master key
             (None, master_pubkey) => match key::read_optional_default_encryption_key()? {
                 None => bail!("--crypt-mode without --keyfile and no default key file available"),
@@ -782,11 +820,15 @@ fn crypto_parameters(param: &Value) -> Result<CryptoParams, Error> {
 // WARNING: there must only be one test for crypto_parameters as the default key handling is not
 // safe w.r.t. concurrency
 fn test_crypto_parameters_handling() -> Result<(), Error> {
-    let some_key = Some(vec![1;1]);
-    let default_key = Some(vec![2;1]);
+    let some_key = vec![1;1];
+    let default_key = vec![2;1];
 
-    let some_master_key = Some(vec![3;1]);
-    let default_master_key = Some(vec![4;1]);
+    let some_master_key = vec![3;1];
+    let default_master_key = vec![4;1];
+
+    let keypath = "./tests/keyfile.test";
+    let master_keypath = "./tests/masterkeyfile.test";
+    let invalid_keypath = "./tests/invalid_keyfile.test";
 
     let no_key_res = CryptoParams {
         enc_key: None,
@@ -794,42 +836,54 @@ fn test_crypto_parameters_handling() -> Result<(), Error> {
         mode: CryptMode::None,
     };
     let some_key_res = CryptoParams {
-        enc_key: some_key.clone(),
+        enc_key: Some(KeyWithSource::from_path(
+            keypath.to_string(),
+            some_key.clone(),
+        )),
         master_pubkey: None,
         mode: CryptMode::Encrypt,
     };
     let some_key_some_master_res = CryptoParams {
-        enc_key: some_key.clone(),
-        master_pubkey: some_master_key.clone(),
+        enc_key: Some(KeyWithSource::from_path(
+            keypath.to_string(),
+            some_key.clone(),
+        )),
+        master_pubkey: Some(KeyWithSource::from_path(
+            master_keypath.to_string(),
+            some_master_key.clone(),
+        )),
         mode: CryptMode::Encrypt,
     };
     let some_key_default_master_res = CryptoParams {
-        enc_key: some_key.clone(),
-        master_pubkey: default_master_key.clone(),
+        enc_key: Some(KeyWithSource::from_path(
+            keypath.to_string(),
+            some_key.clone(),
+        )),
+        master_pubkey: Some(KeyWithSource::from_default(default_master_key.clone())),
         mode: CryptMode::Encrypt,
     };
 
     let some_key_sign_res = CryptoParams {
-        enc_key: some_key.clone(),
+        enc_key: Some(KeyWithSource::from_path(
+            keypath.to_string(),
+            some_key.clone(),
+        )),
         master_pubkey: None,
         mode: CryptMode::SignOnly,
     };
     let default_key_res = CryptoParams {
-        enc_key: default_key.clone(),
+        enc_key: Some(KeyWithSource::from_default(default_key.clone())),
         master_pubkey: None,
         mode: CryptMode::Encrypt,
     };
     let default_key_sign_res = CryptoParams {
-        enc_key: default_key.clone(),
+        enc_key: Some(KeyWithSource::from_default(default_key.clone())),
         master_pubkey: None,
         mode: CryptMode::SignOnly,
     };
 
-    let keypath = "./tests/keyfile.test";
-    replace_file(&keypath, some_key.as_ref().unwrap(), CreateOptions::default())?;
-    let master_keypath = "./tests/masterkeyfile.test";
-    replace_file(&master_keypath, some_master_key.as_ref().unwrap(), CreateOptions::default())?;
-    let invalid_keypath = "./tests/invalid_keyfile.test";
+    replace_file(&keypath, &some_key, CreateOptions::default())?;
+    replace_file(&master_keypath, &some_master_key, CreateOptions::default())?;
 
     // no params, no default key == no key
     let res = crypto_parameters(&json!({}));
@@ -863,7 +917,7 @@ fn test_crypto_parameters_handling() -> Result<(), Error> {
     assert!(crypto_parameters(&json!({"keyfile": invalid_keypath, "crypt-mode": "encrypt"})).is_err());
 
     // now set a default key
-    unsafe { key::set_test_encryption_key(Ok(default_key.clone())); }
+    unsafe { key::set_test_encryption_key(Ok(Some(default_key.clone()))); }
 
     // and repeat
 
@@ -938,7 +992,7 @@ fn test_crypto_parameters_handling() -> Result<(), Error> {
     // now remove default key again
     unsafe { key::set_test_encryption_key(Ok(None)); }
     // set a default master key
-    unsafe { key::set_test_default_master_pubkey(Ok(default_master_key.clone())); }
+    unsafe { key::set_test_default_master_pubkey(Ok(Some(default_master_key.clone()))); }
 
     // and use an explicit master key
     assert!(crypto_parameters(&json!({"master-pubkey-file": master_keypath})).is_err());
@@ -1206,15 +1260,23 @@ async fn create_backup(
 
     let (crypt_config, rsa_encrypted_key) = match crypto.enc_key {
         None => (None, None),
-        Some(key) => {
-            let (key, created, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
+        Some(key_with_source) => {
+            println!(
+                "{}",
+                format_key_source(&key_with_source.source, "encryption")
+            );
+
+            let (key, created, fingerprint) =
+                decrypt_key(&key_with_source.key, &key::get_encryption_key_password)?;
             println!("Encryption key fingerprint: {}", fingerprint);
 
             let crypt_config = CryptConfig::new(key)?;
 
             match crypto.master_pubkey {
-                Some(pem_data) => {
-                    let rsa = openssl::rsa::Rsa::public_key_from_pem(&pem_data)?;
+                Some(pem_with_source) => {
+                    println!("{}", format_key_source(&pem_with_source.source, "master"));
+
+                    let rsa = openssl::rsa::Rsa::public_key_from_pem(&pem_with_source.key)?;
 
                     let mut key_config = KeyConfig::without_password(key)?;
                     key_config.created = created; // keep original value
@@ -1222,7 +1284,7 @@ async fn create_backup(
                     let enc_key = rsa_encrypt_key_config(rsa, &key_config)?;
 
                     (Some(Arc::new(crypt_config)), Some(enc_key))
-                }
+                },
                 _ => (Some(Arc::new(crypt_config)), None),
             }
         }
@@ -1574,9 +1636,12 @@ async fn restore(param: Value) -> Result<Value, Error> {
 
     let crypt_config = match crypto.enc_key {
         None => None,
-        Some(key) => {
-            let (key, _, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
-            eprintln!("Encryption key fingerprint: '{}'", fingerprint);
+        Some(ref key) => {
+            let (key, _, _) =
+                decrypt_key(&key.key, &key::get_encryption_key_password).map_err(|err| {
+                    eprintln!("{}", format_key_source(&key.source, "encryption"));
+                    err
+                })?;
             Some(Arc::new(CryptConfig::new(key)?))
         }
     };
@@ -1598,6 +1663,14 @@ async fn restore(param: Value) -> Result<Value, Error> {
     if archive_name == ENCRYPTED_KEY_BLOB_NAME && crypt_config.is_none() {
         eprintln!("Restoring encrypted key blob without original key - skipping manifest fingerprint check!")
     } else {
+        if manifest.signature.is_some() {
+            if let Some(key) = &crypto.enc_key {
+                eprintln!("{}", format_key_source(&key.source, "encryption"));
+            }
+            if let Some(config) = &crypt_config {
+                eprintln!("Fingerprint: {}", config.fingerprint());
+            }
+        }
         manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
     }
 
diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs
index b69d9c37..659200ff 100644
--- a/src/bin/proxmox_backup_client/catalog.rs
+++ b/src/bin/proxmox_backup_client/catalog.rs
@@ -15,6 +15,7 @@ use crate::{
     REPO_URL_SCHEMA,
     KEYFD_SCHEMA,
     extract_repository_from_value,
+    format_key_source,
     record_repository,
     key::get_encryption_key_password,
     decrypt_key,
@@ -73,7 +74,11 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
     let crypt_config = match crypto.enc_key {
         None => None,
         Some(key) => {
-            let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?;
+            let (key, _created, _fingerprint) = decrypt_key(&key.key, &get_encryption_key_password)
+                .map_err(|err| {
+                    eprintln!("{}", format_key_source(&key.source, "encryption"));
+                    err
+                })?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
@@ -171,7 +176,11 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
     let crypt_config = match crypto.enc_key {
         None => None,
         Some(key) => {
-            let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?;
+            let (key, _created, _fingerprint) = decrypt_key(&key.key, &get_encryption_key_password)
+                .map_err(|err| {
+                    eprintln!("{}", format_key_source(&key.source, "encryption"));
+                    err
+                })?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index 7dd28473..6e18a026 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -20,6 +20,8 @@ use proxmox_backup::{
     tools::paperkey::{generate_paper_key, PaperkeyFormat},
 };
 
+use crate::KeyWithSource;
+
 pub const DEFAULT_ENCRYPTION_KEY_FILE_NAME: &str = "encryption-key.json";
 pub const DEFAULT_MASTER_PUBKEY_FILE_NAME: &str = "master-public.pem";
 
@@ -52,16 +54,16 @@ pub fn place_default_encryption_key() -> Result<PathBuf, Error> {
 }
 
 #[cfg(not(test))]
-pub fn read_optional_default_encryption_key() -> Result<Option<Vec<u8>>, Error> {
+pub(crate) fn read_optional_default_encryption_key() -> Result<Option<KeyWithSource>, Error> {
     find_default_encryption_key()?
-        .map(file_get_contents)
+        .map(|path| file_get_contents(path).map(KeyWithSource::from_default))
         .transpose()
 }
 
 #[cfg(not(test))]
-pub fn read_optional_default_master_pubkey() -> Result<Option<Vec<u8>>, Error> {
+pub(crate) fn read_optional_default_master_pubkey() -> Result<Option<KeyWithSource>, Error> {
     find_default_master_pubkey()?
-        .map(file_get_contents)
+        .map(|path| file_get_contents(path).map(KeyWithSource::from_default))
         .transpose()
 }
 
@@ -69,11 +71,12 @@ pub fn read_optional_default_master_pubkey() -> Result<Option<Vec<u8>>, Error> {
 static mut TEST_DEFAULT_ENCRYPTION_KEY: Result<Option<Vec<u8>>, Error> = Ok(None);
 
 #[cfg(test)]
-pub fn read_optional_default_encryption_key() -> Result<Option<Vec<u8>>, Error> {
+pub(crate) fn read_optional_default_encryption_key() -> Result<Option<KeyWithSource>, Error> {
     // not safe when multiple concurrent test cases end up here!
     unsafe {
         match &TEST_DEFAULT_ENCRYPTION_KEY {
-            Ok(key) => Ok(key.clone()),
+            Ok(Some(key)) => Ok(Some(KeyWithSource::from_default(key.clone()))),
+            Ok(None) => Ok(None),
             Err(_) => bail!("test error"),
         }
     }
@@ -81,7 +84,7 @@ pub fn read_optional_default_encryption_key() -> Result<Option<Vec<u8>>, Error>
 
 #[cfg(test)]
 // not safe when multiple concurrent test cases end up here!
-pub unsafe fn set_test_encryption_key(value: Result<Option<Vec<u8>>, Error>) {
+pub(crate) unsafe fn set_test_encryption_key(value: Result<Option<Vec<u8>>, Error>) {
     TEST_DEFAULT_ENCRYPTION_KEY = value;
 }
 
@@ -89,11 +92,12 @@ pub unsafe fn set_test_encryption_key(value: Result<Option<Vec<u8>>, Error>) {
 static mut TEST_DEFAULT_MASTER_PUBKEY: Result<Option<Vec<u8>>, Error> = Ok(None);
 
 #[cfg(test)]
-pub fn read_optional_default_master_pubkey() -> Result<Option<Vec<u8>>, Error> {
+pub(crate) fn read_optional_default_master_pubkey() -> Result<Option<KeyWithSource>, Error> {
     // not safe when multiple concurrent test cases end up here!
     unsafe {
         match &TEST_DEFAULT_MASTER_PUBKEY {
-            Ok(key) => Ok(key.clone()),
+            Ok(Some(key)) => Ok(Some(KeyWithSource::from_default(key.clone()))),
+            Ok(None) => Ok(None),
             Err(_) => bail!("test error"),
         }
     }
@@ -101,7 +105,7 @@ pub fn read_optional_default_master_pubkey() -> Result<Option<Vec<u8>>, Error> {
 
 #[cfg(test)]
 // not safe when multiple concurrent test cases end up here!
-pub unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>>, Error>) {
+pub(crate) unsafe fn set_test_default_master_pubkey(value: Result<Option<Vec<u8>>, Error>) {
     TEST_DEFAULT_MASTER_PUBKEY = value;
 }
 
diff --git a/src/bin/proxmox_backup_client/snapshot.rs b/src/bin/proxmox_backup_client/snapshot.rs
index 0c694598..5988ebf6 100644
--- a/src/bin/proxmox_backup_client/snapshot.rs
+++ b/src/bin/proxmox_backup_client/snapshot.rs
@@ -239,7 +239,7 @@ async fn upload_log(param: Value) -> Result<Value, Error> {
     let crypt_config = match crypto.enc_key {
         None => None,
         Some(key) => {
-            let (key, _created, _) = decrypt_key(&key, &crate::key::get_encryption_key_password)?;
+            let (key, _created, _) = decrypt_key(&key.key, &crate::key::get_encryption_key_password)?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
-- 
2.20.1