public inbox for pbs-devel@lists.proxmox.com
 help / color / mirror / Atom feed
* [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint
@ 2020-11-20 16:38 Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 01/13] crypt config: add fingerprint mechanism Fabian Grünbichler
                   ` (15 more replies)
  0 siblings, 16 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

changes since v1:
- switch to wrapping fingerprint in (transparent) Fingerprint struct
- display as 'short key ID' of 8 bytes (easily swappable to other
  format if desired)
- return in list_snapshots, display in GUI
- improve log messages for (non-)incremental backups
- handle fingerprint in paperkey
- adapt qemu library

proxmox-backup:

Fabian Grünbichler (13):
  crypt config: add fingerprint mechanism
  key: add fingerprint to key config
  client: print key fingerprint and master key
  client: add 'key show' command
  fix #3139: add key fingerprint to manifest
  manifest: check fingerprint when loading with key
  client: check fingerprint after downloading manifest
  paperkey: refactor common code
  paperkey: add short key ID to subject
  expose previous backup time in backup env
  refactor BackupInfo -> SnapshotListItem helper
  list_snapshots: return manifest fingerprint
  gui: add snapshot/file fingerprint tooltip

 src/api2/admin/datastore.rs                |  60 ++++---
 src/api2/backup.rs                         |  26 +++
 src/api2/types/mod.rs                      |   9 +-
 src/backup/crypt_config.rs                 |  38 +++-
 src/backup/key_derivation.rs               |  22 ++-
 src/backup/manifest.rs                     |  51 +++++-
 src/bin/proxmox-backup-client.rs           |  48 ++++-
 src/bin/proxmox_backup_client/benchmark.rs |   2 +-
 src/bin/proxmox_backup_client/catalog.rs   |   6 +-
 src/bin/proxmox_backup_client/key.rs       | 199 +++++++++++++++------
 src/bin/proxmox_backup_client/mount.rs     |   5 +-
 src/client/backup_writer.rs                |   7 +
 src/tools/format.rs                        |  34 ++++
 www/Utils.js                               |   5 +
 www/datastore/Content.js                   |  13 +-
 15 files changed, 430 insertions(+), 95 deletions(-)


proxmox-backup-qemu:

Fabian Grünbichler (2):
  adapt to proxmox-backup fingerprint changes
  restore: improve error if key is missing

 src/backup.rs  | 2 +-
 src/restore.rs | 3 ++-
 2 files changed, 3 insertions(+), 2 deletions(-)

-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 01/13] crypt config: add fingerprint mechanism
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config Fabian Grünbichler
                   ` (14 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

by computing the ID digest of a hash of a static string.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v2: wrap fingerprint in Struct

 src/backup/crypt_config.rs | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs
index 4be728d9..8a4fe0e3 100644
--- a/src/backup/crypt_config.rs
+++ b/src/backup/crypt_config.rs
@@ -7,6 +7,8 @@
 //! encryption](https://en.wikipedia.org/wiki/Authenticated_encryption)
 //! for a short introduction.
 
+use std::fmt;
+use std::fmt::Display;
 use std::io::Write;
 
 use anyhow::{bail, Error};
@@ -17,6 +19,11 @@ use serde::{Deserialize, Serialize};
 
 use proxmox::api::api;
 
+// openssl::sha::sha256(b"Proxmox Backup Encryption Key Fingerprint")
+const FINGERPRINT_INPUT: [u8; 32] = [ 110, 208, 239, 119,  71,  31, 255,  77,
+                                       85, 199, 168, 254,  74, 157, 182,  33,
+                                       97,  64, 127,  19,  76, 114,  93, 223,
+                                       48, 153,  45,  37, 236,  69, 237,  38, ];
 #[api(default: "encrypt")]
 #[derive(Copy, Clone, Debug, Eq, PartialEq, Deserialize, Serialize)]
 #[serde(rename_all = "kebab-case")]
@@ -30,6 +37,17 @@ pub enum CryptMode {
     SignOnly,
 }
 
+/// 32-byte fingerprint, usually calculated with SHA256.
+pub struct Fingerprint {
+    bytes: [u8; 32],
+}
+
+impl Display for Fingerprint {
+    fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
+        write!(f, "{:?}", self.bytes)
+    }
+}
+
 /// Encryption Configuration with secret key
 ///
 /// This structure stores the secret key and provides helpers for
@@ -101,6 +119,12 @@ impl CryptConfig {
         tag
     }
 
+    pub fn fingerprint(&self) -> Fingerprint {
+        Fingerprint {
+            bytes: self.compute_digest(&FINGERPRINT_INPUT)
+        }
+    }
+
     pub fn data_crypter(&self, iv: &[u8; 16], mode: Mode) -> Result<Crypter, Error>  {
         let mut crypter = openssl::symm::Crypter::new(self.cipher, mode, &self.enc_key, Some(iv))?;
         crypter.aad_update(b"")?; //??
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 01/13] crypt config: add fingerprint mechanism Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-23  8:07   ` Wolfgang Bumiller
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 03/13] client: print key fingerprint and master key Fabian Grünbichler
                   ` (13 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

and set/generate it on
- key creation
- key passphrase change
- key decryption if not already set
- key encryption with master key

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v2: simplify after switchover to Fingerprint struct, incorporate
    feedback
    
    an existing passwordless key can be 'updated' non-interactively with
    
    proxmox-backup-client key change-passphrase $path --kdf none
    
    something like this could go into the/some postinst to retro-actively give all
    the PVE generated storage keys a fingerprint field. this only affects the 'key
    show' subcommand (introduced later in this series) or manual checks of the key
    file, any operations actually using the key have access to its CryptConfig and
    thus the fingerprint() function anyway..
    
    an existing passwordless key can be 'updated' non-interactively with
    
    proxmox-backup-client key change-passphrase $path --kdf none
    
    something like this could go into the/some postinst to retro-actively give all
    the PVE generated storage keys a fingerprint field. this only affects the 'key
    show' subcommand (introduced later in this series) or manual checks of the key
    file, any operations actually using the key have access to its CryptConfig and
    thus the fingerprint() function anyway..

 src/backup/crypt_config.rs                 | 16 ++++++++--
 src/backup/key_derivation.rs               | 22 +++++++++++---
 src/bin/proxmox-backup-client.rs           |  6 ++--
 src/bin/proxmox_backup_client/benchmark.rs |  2 +-
 src/bin/proxmox_backup_client/catalog.rs   |  4 +--
 src/bin/proxmox_backup_client/key.rs       | 19 +++++++++---
 src/bin/proxmox_backup_client/mount.rs     |  2 +-
 src/tools/format.rs                        | 34 ++++++++++++++++++++++
 8 files changed, 88 insertions(+), 17 deletions(-)

diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs
index 8a4fe0e3..7d27706a 100644
--- a/src/backup/crypt_config.rs
+++ b/src/backup/crypt_config.rs
@@ -17,6 +17,8 @@ use openssl::pkcs5::pbkdf2_hmac;
 use openssl::symm::{decrypt_aead, Cipher, Crypter, Mode};
 use serde::{Deserialize, Serialize};
 
+use crate::tools::format::{as_fingerprint, bytes_as_fingerprint};
+
 use proxmox::api::api;
 
 // openssl::sha::sha256(b"Proxmox Backup Encryption Key Fingerprint")
@@ -37,14 +39,18 @@ pub enum CryptMode {
     SignOnly,
 }
 
+#[derive(Debug, Eq, PartialEq, Deserialize, Serialize)]
+#[serde(transparent)]
 /// 32-byte fingerprint, usually calculated with SHA256.
 pub struct Fingerprint {
+    #[serde(with = "bytes_as_fingerprint")]
     bytes: [u8; 32],
 }
 
+/// Display as short key ID
 impl Display for Fingerprint {
     fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
-        write!(f, "{:?}", self.bytes)
+        write!(f, "{}", as_fingerprint(&self.bytes[0..8]))
     }
 }
 
@@ -243,7 +249,13 @@ impl CryptConfig {
     ) -> Result<Vec<u8>, Error> {
 
         let modified = proxmox::tools::time::epoch_i64();
-        let key_config = super::KeyConfig { kdf: None, created, modified, data: self.enc_key.to_vec() };
+        let key_config = super::KeyConfig {
+            kdf: None,
+            created,
+            modified,
+            data: self.enc_key.to_vec(),
+            fingerprint: Some(self.fingerprint()),
+        };
         let data = serde_json::to_string(&key_config)?.as_bytes().to_vec();
 
         let mut buffer = vec![0u8; rsa.size() as usize];
diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 2c807723..e7b266ab 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -2,6 +2,8 @@ use anyhow::{bail, format_err, Context, Error};
 
 use serde::{Deserialize, Serialize};
 
+use crate::backup::{CryptConfig, Fingerprint};
+
 use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions};
 use proxmox::try_block;
 
@@ -66,6 +68,9 @@ pub struct KeyConfig {
     pub modified: i64,
     #[serde(with = "proxmox::tools::serde::bytes_as_base64")]
     pub data: Vec<u8>,
+    #[serde(skip_serializing_if = "Option::is_none")]
+    #[serde(default)]
+    pub fingerprint: Option<Fingerprint>,
  }
 
 pub fn store_key_config(
@@ -142,13 +147,14 @@ pub fn encrypt_key_with_passphrase(
         created,
         modified: created,
         data: enc_data,
+        fingerprint: None,
     })
 }
 
 pub fn load_and_decrypt_key(
     path: &std::path::Path,
     passphrase: &dyn Fn() -> Result<Vec<u8>, Error>,
-) -> Result<([u8;32], i64), Error> {
+) -> Result<([u8;32], i64, Fingerprint), Error> {
     do_load_and_decrypt_key(path, passphrase)
         .with_context(|| format!("failed to load decryption key from {:?}", path))
 }
@@ -156,14 +162,14 @@ pub fn load_and_decrypt_key(
 fn do_load_and_decrypt_key(
     path: &std::path::Path,
     passphrase: &dyn Fn() -> Result<Vec<u8>, Error>,
-) -> Result<([u8;32], i64), Error> {
+) -> Result<([u8;32], i64, Fingerprint), Error> {
     decrypt_key(&file_get_contents(&path)?, passphrase)
 }
 
 pub fn decrypt_key(
     mut keydata: &[u8],
     passphrase: &dyn Fn() -> Result<Vec<u8>, Error>,
-) -> Result<([u8;32], i64), Error> {
+) -> Result<([u8;32], i64, Fingerprint), Error> {
     let key_config: KeyConfig = serde_json::from_reader(&mut keydata)?;
 
     let raw_data = key_config.data;
@@ -203,5 +209,13 @@ pub fn decrypt_key(
     let mut result = [0u8; 32];
     result.copy_from_slice(&key);
 
-    Ok((result, created))
+    let fingerprint = match key_config.fingerprint {
+        Some(fingerprint) => fingerprint,
+        None => {
+            let crypt_config = CryptConfig::new(result.clone())?;
+            crypt_config.fingerprint()
+        },
+    };
+
+    Ok((result, created, fingerprint))
 }
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 70aba77f..c63c7eb6 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1069,7 +1069,7 @@ async fn create_backup(
     let (crypt_config, rsa_encrypted_key) = match keydata {
         None => (None, None),
         Some(key) => {
-            let (key, created) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            let (key, created, _fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
 
             let crypt_config = CryptConfig::new(key)?;
 
@@ -1380,7 +1380,7 @@ async fn restore(param: Value) -> Result<Value, Error> {
     let crypt_config = match keydata {
         None => None,
         Some(key) => {
-            let (key, _) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            let (key, _, _) = decrypt_key(&key, &key::get_encryption_key_password)?;
             Some(Arc::new(CryptConfig::new(key)?))
         }
     };
@@ -1538,7 +1538,7 @@ async fn upload_log(param: Value) -> Result<Value, Error> {
     let crypt_config = match keydata {
         None => None,
         Some(key) => {
-            let (key, _created) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            let (key, _created, _) = decrypt_key(&key, &key::get_encryption_key_password)?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
diff --git a/src/bin/proxmox_backup_client/benchmark.rs b/src/bin/proxmox_backup_client/benchmark.rs
index b0d769e8..e53e43ce 100644
--- a/src/bin/proxmox_backup_client/benchmark.rs
+++ b/src/bin/proxmox_backup_client/benchmark.rs
@@ -151,7 +151,7 @@ pub async fn benchmark(
     let crypt_config = match keyfile {
         None => None,
         Some(path) => {
-            let (key, _) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
+            let (key, _, _) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs
index e4931e10..37ad842f 100644
--- a/src/bin/proxmox_backup_client/catalog.rs
+++ b/src/bin/proxmox_backup_client/catalog.rs
@@ -73,7 +73,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
     let crypt_config = match keydata {
         None => None,
         Some(key) => {
-            let (key, _created) = decrypt_key(&key, &get_encryption_key_password)?;
+            let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?;
             let crypt_config = CryptConfig::new(key)?;
             Some(Arc::new(crypt_config))
         }
@@ -170,7 +170,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
     let crypt_config = match keydata {
         None => None,
         Some(key) => {
-            let (key, _created) = decrypt_key(&key, &get_encryption_key_password)?;
+            let (key, _created, _fingerprint) = decrypt_key(&key, &get_encryption_key_password)?;
             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 8b802b3e..193367e3 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -11,7 +11,11 @@ use proxmox::sys::linux::tty;
 use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions};
 
 use proxmox_backup::backup::{
-    encrypt_key_with_passphrase, load_and_decrypt_key, store_key_config, KeyConfig,
+    encrypt_key_with_passphrase,
+    load_and_decrypt_key,
+    store_key_config,
+    CryptConfig,
+    KeyConfig,
 };
 use proxmox_backup::tools;
 
@@ -120,7 +124,10 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
 
     let kdf = kdf.unwrap_or_default();
 
-    let key = proxmox::sys::linux::random_data(32)?;
+    let mut key_array = [0u8; 32];
+    proxmox::sys::linux::fill_with_random_data(&mut key_array)?;
+    let crypt_config = CryptConfig::new(key_array.clone())?;
+    let key = key_array.to_vec();
 
     match kdf {
         Kdf::None => {
@@ -134,6 +141,7 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
                     created,
                     modified: created,
                     data: key,
+                    fingerprint: Some(crypt_config.fingerprint()),
                 },
             )?;
         }
@@ -145,7 +153,8 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
 
             let password = tty::read_and_verify_password("Encryption Key Password: ")?;
 
-            let key_config = encrypt_key_with_passphrase(&key, &password)?;
+            let mut key_config = encrypt_key_with_passphrase(&key, &password)?;
+            key_config.fingerprint = Some(crypt_config.fingerprint());
 
             store_key_config(&path, false, key_config)?;
         }
@@ -188,7 +197,7 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
         bail!("unable to change passphrase - no tty");
     }
 
-    let (key, created) = load_and_decrypt_key(&path, &get_encryption_key_password)?;
+    let (key, created, fingerprint) = load_and_decrypt_key(&path, &get_encryption_key_password)?;
 
     match kdf {
         Kdf::None => {
@@ -202,6 +211,7 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
                     created, // keep original value
                     modified,
                     data: key.to_vec(),
+                    fingerprint: Some(fingerprint),
                 },
             )?;
         }
@@ -210,6 +220,7 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
 
             let mut new_key_config = encrypt_key_with_passphrase(&key, &password)?;
             new_key_config.created = created; // keep original value
+            new_key_config.fingerprint = Some(fingerprint);
 
             store_key_config(&path, true, new_key_config)?;
         }
diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index 415fbf9d..6283961e 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -182,7 +182,7 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
     let crypt_config = match keyfile {
         None => None,
         Some(path) => {
-            let (key, _) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
+            let (key, _, _) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
             Some(Arc::new(CryptConfig::new(key)?))
         }
     };
diff --git a/src/tools/format.rs b/src/tools/format.rs
index 8fe6aa82..c9f50053 100644
--- a/src/tools/format.rs
+++ b/src/tools/format.rs
@@ -102,6 +102,40 @@ impl From<u64> for HumanByte {
     }
 }
 
+pub fn as_fingerprint(bytes: &[u8]) -> String {
+    proxmox::tools::digest_to_hex(bytes)
+        .as_bytes()
+        .chunks(2)
+        .map(|v| std::str::from_utf8(v).unwrap())
+        .collect::<Vec<&str>>().join(":")
+}
+
+pub mod bytes_as_fingerprint {
+    use serde::{Deserialize, Serializer, Deserializer};
+
+    pub fn serialize<S>(
+        bytes: &[u8; 32],
+        serializer: S,
+    ) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        let s = crate::tools::format::as_fingerprint(bytes);
+        serializer.serialize_str(&s)
+    }
+
+    pub fn deserialize<'de, D>(
+        deserializer: D,
+    ) -> Result<[u8; 32], D::Error>
+    where
+        D: Deserializer<'de>,
+    {
+        let mut s = String::deserialize(deserializer)?;
+        s.retain(|c| c != ':');
+        proxmox::tools::hex_to_digest(&s).map_err(serde::de::Error::custom)
+    }
+}
+
 #[test]
 fn correct_byte_convert() {
     fn convert(b: usize) -> String {
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 03/13] client: print key fingerprint and master key
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 01/13] crypt config: add fingerprint mechanism Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 04/13] client: add 'key show' command Fabian Grünbichler
                   ` (12 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

for operations where it makes sense.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v2: print FP directly (short key ID via Display)

 src/bin/proxmox-backup-client.rs       | 9 +++++++--
 src/bin/proxmox_backup_client/mount.rs | 4 +++-
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index c63c7eb6..ee2623f0 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1069,7 +1069,8 @@ async fn create_backup(
     let (crypt_config, rsa_encrypted_key) = match keydata {
         None => (None, None),
         Some(key) => {
-            let (key, created, _fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            let (key, created, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            println!("Encryption key fingerprint: {}", fingerprint);
 
             let crypt_config = CryptConfig::new(key)?;
 
@@ -1078,6 +1079,8 @@ async fn create_backup(
                     let pem_data = file_get_contents(path)?;
                     let rsa = openssl::rsa::Rsa::public_key_from_pem(&pem_data)?;
                     let enc_key = crypt_config.generate_rsa_encoded_key(rsa, created)?;
+                    println!("Master key '{:?}'", path);
+
                     (Some(Arc::new(crypt_config)), Some(enc_key))
                 }
                 _ => (Some(Arc::new(crypt_config)), None),
@@ -1380,7 +1384,8 @@ async fn restore(param: Value) -> Result<Value, Error> {
     let crypt_config = match keydata {
         None => None,
         Some(key) => {
-            let (key, _, _) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            let (key, _, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
+            println!("Encryption key fingerprint: '{}'", fingerprint);
             Some(Arc::new(CryptConfig::new(key)?))
         }
     };
diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index 6283961e..187deab5 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -182,7 +182,9 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
     let crypt_config = match keyfile {
         None => None,
         Some(path) => {
-            let (key, _, _) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
+            println!("Encryption key file: '{:?}'", path);
+            let (key, _, fingerprint) = load_and_decrypt_key(&path, &crate::key::get_encryption_key_password)?;
+            println!("Encryption key fingerprint: '{}'", fingerprint);
             Some(Arc::new(CryptConfig::new(key)?))
         }
     };
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 04/13] client: add 'key show' command
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (2 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 03/13] client: print key fingerprint and master key Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 05/13] fix #3139: add key fingerprint to manifest Fabian Grünbichler
                   ` (11 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

for (pretty-)printing a keyfile.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v2: display fp directly as well, add output-format support

 src/bin/proxmox_backup_client/key.rs | 68 +++++++++++++++++++++++++++-
 1 file changed, 67 insertions(+), 1 deletion(-)

diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index 915ee970..ea7e8c82 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -4,9 +4,16 @@ use std::process::{Stdio, Command};
 
 use anyhow::{bail, format_err, Error};
 use serde::{Deserialize, Serialize};
+use serde_json::Value;
 
 use proxmox::api::api;
-use proxmox::api::cli::{CliCommand, CliCommandMap};
+use proxmox::api::cli::{
+    CliCommand,
+    CliCommandMap,
+    format_and_print_result,
+    get_output_format,
+    OUTPUT_FORMAT,
+};
 use proxmox::sys::linux::tty;
 use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions};
 
@@ -16,6 +23,7 @@ use proxmox_backup::backup::{
     store_key_config,
     CryptConfig,
     KeyConfig,
+    KeyDerivationConfig,
 };
 use proxmox_backup::tools;
 
@@ -229,6 +237,59 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
     Ok(())
 }
 
+#[api(
+    input: {
+        properties: {
+            path: {
+                description: "Key file. Without this the default key's metadata will be shown.",
+                optional: true,
+            },
+            "output-format": {
+                schema: OUTPUT_FORMAT,
+                optional: true,
+            },
+        },
+    },
+)]
+/// Print the encryption key's metadata.
+fn show_key(
+    path: Option<String>,
+    param: Value,
+) -> Result<(), Error> {
+    let path = match path {
+        Some(path) => PathBuf::from(path),
+        None => {
+            let path = find_default_encryption_key()?
+                .ok_or_else(|| {
+                    format_err!("no encryption file provided and no default file found")
+                })?;
+            path
+        }
+    };
+
+    let output_format = get_output_format(&param);
+    let config: KeyConfig = serde_json::from_slice(&file_get_contents(path.clone())?)?;
+
+    if output_format == "text" {
+        println!("Path: {:?}", path);
+        match config.kdf {
+            Some(KeyDerivationConfig::PBKDF2 { .. }) => println!("KDF: pbkdf2"),
+            Some(KeyDerivationConfig::Scrypt { .. }) => println!("KDF: scrypt"),
+            None => println!("KDF: none (plaintext key)"),
+        };
+        println!("Created: {}", proxmox::tools::time::epoch_to_rfc3339_utc(config.created)?);
+        println!("Modified: {}", proxmox::tools::time::epoch_to_rfc3339_utc(config.modified)?);
+        match config.fingerprint {
+            Some(fp) => println!("Fingerprint: {}", fp),
+            None => println!("Fingerprint: none (legacy key)"),
+        };
+    } else {
+        format_and_print_result(&serde_json::to_value(config)?, &output_format);
+    }
+
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -348,6 +409,10 @@ pub fn cli() -> CliCommandMap {
         .arg_param(&["path"])
         .completion_cb("path", tools::complete_file_name);
 
+    let key_show_cmd_def = CliCommand::new(&API_METHOD_SHOW_KEY)
+        .arg_param(&["path"])
+        .completion_cb("path", tools::complete_file_name);
+
     let paper_key_cmd_def = CliCommand::new(&API_METHOD_PAPER_KEY)
         .arg_param(&["path"])
         .completion_cb("path", tools::complete_file_name);
@@ -357,6 +422,7 @@ pub fn cli() -> CliCommandMap {
         .insert("create-master-key", key_create_master_key_cmd_def)
         .insert("import-master-pubkey", key_import_master_pubkey_cmd_def)
         .insert("change-passphrase", key_change_passphrase_cmd_def)
+        .insert("show", key_show_cmd_def)
         .insert("paperkey", paper_key_cmd_def)
 }
 
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 05/13] fix #3139: add key fingerprint to manifest
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (3 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 04/13] client: add 'key show' command Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 06/13] manifest: check fingerprint when loading with key Fabian Grünbichler
                   ` (10 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

if the manifest is signed/the contained archives/blobs are encrypted.
stored in 'unprotected' area, since there is already a strong binding
between key and manifest via the signature, and this avoids breaking
backwards compatibility for a simple usability improvement.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    v2: use Fingerprint struct here as well

 src/backup/manifest.rs | 38 +++++++++++++++++++++++++++++++++++++-
 1 file changed, 37 insertions(+), 1 deletion(-)

diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index 51980a07..00dafbd6 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -5,7 +5,7 @@ use std::path::Path;
 use serde_json::{json, Value};
 use ::serde::{Deserialize, Serialize};
 
-use crate::backup::{BackupDir, CryptMode, CryptConfig};
+use crate::backup::{BackupDir, CryptMode, CryptConfig, Fingerprint};
 
 pub const MANIFEST_BLOB_NAME: &str = "index.json.blob";
 pub const MANIFEST_LOCK_NAME: &str = ".index.json.lck";
@@ -223,12 +223,48 @@ impl BackupManifest {
         if let Some(crypt_config) = crypt_config {
             let sig = self.signature(crypt_config)?;
             manifest["signature"] = proxmox::tools::digest_to_hex(&sig).into();
+            let fingerprint = &crypt_config.fingerprint();
+            manifest["unprotected"]["key-fingerprint"] = serde_json::to_value(fingerprint)?;
         }
 
         let manifest = serde_json::to_string_pretty(&manifest).unwrap().into();
         Ok(manifest)
     }
 
+    pub fn fingerprint(&self) -> Result<Option<Fingerprint>, Error> {
+        match &self.unprotected["key-fingerprint"] {
+            Value::Null => Ok(None),
+            value => Ok(Some(serde_json::from_value(value.clone())?))
+        }
+    }
+
+    /// Checks if a BackupManifest and a CryptConfig share a valid fingerprint combination.
+    ///
+    /// An unsigned manifest is valid with any or no CryptConfig.
+    /// A signed manifest is only valid with a matching CryptConfig.
+    pub fn check_fingerprint(&self, crypt_config: Option<&CryptConfig>) -> Result<(), Error> {
+        if let Some(fingerprint) = self.fingerprint()? {
+            match crypt_config {
+                None => bail!(
+                    "missing key - manifest was created with key {}",
+                    fingerprint,
+                ),
+                Some(crypt_config) => {
+                    let config_fp = crypt_config.fingerprint();
+                    if config_fp != fingerprint {
+                        bail!(
+                            "wrong key - manifest's key {} does not match provided key {}",
+                            fingerprint,
+                            config_fp
+                        );
+                    }
+                }
+            }
+        };
+
+        Ok(())
+    }
+
     /// Try to read the manifest. This verifies the signature if there is a crypt_config.
     pub fn from_data(data: &[u8], crypt_config: Option<&CryptConfig>) -> Result<BackupManifest, Error> {
         let json: Value = serde_json::from_slice(data)?;
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 06/13] manifest: check fingerprint when loading with key
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (4 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 05/13] fix #3139: add key fingerprint to manifest Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 07/13] client: check fingerprint after downloading manifest Fabian Grünbichler
                   ` (9 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

otherwise loading will run into the signature mismatch which is
technically true, but not the complete picture in this case.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/manifest.rs | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index 00dafbd6..a64cbe15 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -273,6 +273,19 @@ impl BackupManifest {
         if let Some(ref crypt_config) = crypt_config {
             if let Some(signature) = signature {
                 let expected_signature = proxmox::tools::digest_to_hex(&Self::json_signature(&json, crypt_config)?);
+
+                let fingerprint = &json["unprotected"]["key-fingerprint"];
+                if fingerprint != &Value::Null {
+                    let fingerprint = serde_json::from_value(fingerprint.clone())?;
+                    let config_fp = crypt_config.fingerprint();
+                    if config_fp != fingerprint {
+                        bail!(
+                            "wrong key - unable to verify signature since manifest's key {} does not match provided key {}",
+                            fingerprint,
+                            config_fp
+                        );
+                    }
+                }
                 if signature != expected_signature {
                     bail!("wrong signature in manifest");
                 }
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 07/13] client: check fingerprint after downloading manifest
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (5 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 06/13] manifest: check fingerprint when loading with key Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 08/13] paperkey: refactor common code Fabian Grünbichler
                   ` (8 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

this is stricter than the check that happened on manifest load, as it
also fails if the manifest is signed but we don't have a key available.

add some additional output at the start of a backup to indicate whether
a previous manifest is available to base the backup on.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox-backup-client.rs         | 22 ++++++++++++++++++----
 src/bin/proxmox_backup_client/catalog.rs |  2 ++
 src/bin/proxmox_backup_client/mount.rs   |  1 +
 3 files changed, 21 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index ee2623f0..b4f87071 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1100,10 +1100,23 @@ async fn create_backup(
         false
     ).await?;
 
-    let previous_manifest = if let Ok(previous_manifest) = client.download_previous_manifest().await {
-        Some(Arc::new(previous_manifest))
-    } else {
-        None
+    let previous_manifest = match client.download_previous_manifest().await {
+        Ok(previous_manifest) => {
+            match previous_manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref)) {
+                Ok(()) => {
+                    println!("Successfully downloaded previous manifest.");
+                    Some(Arc::new(previous_manifest))
+                },
+                Err(err) => {
+                    println!("Couldn't re-use pevious manifest - {}", err);
+                    None
+                },
+            }
+        },
+        Err(err) => {
+            println!("Couldn't download pevious manifest - {}", err);
+            None
+        },
     };
 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
@@ -1401,6 +1414,7 @@ async fn restore(param: Value) -> Result<Value, Error> {
     ).await?;
 
     let (manifest, backup_index_data) = client.download_manifest().await?;
+    manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
 
     let (archive_name, archive_type) = parse_archive_type(archive_name);
 
diff --git a/src/bin/proxmox_backup_client/catalog.rs b/src/bin/proxmox_backup_client/catalog.rs
index 37ad842f..61bcc57f 100644
--- a/src/bin/proxmox_backup_client/catalog.rs
+++ b/src/bin/proxmox_backup_client/catalog.rs
@@ -92,6 +92,7 @@ async fn dump_catalog(param: Value) -> Result<Value, Error> {
     ).await?;
 
     let (manifest, _) = client.download_manifest().await?;
+    manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
 
     let index = client.download_dynamic_index(&manifest, CATALOG_NAME).await?;
 
@@ -199,6 +200,7 @@ async fn catalog_shell(param: Value) -> Result<(), Error> {
         .open("/tmp")?;
 
     let (manifest, _) = client.download_manifest().await?;
+    manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
 
     let index = client.download_dynamic_index(&manifest, &server_archive_name).await?;
     let most_used = index.find_most_used_chunks(8);
diff --git a/src/bin/proxmox_backup_client/mount.rs b/src/bin/proxmox_backup_client/mount.rs
index 187deab5..c586b764 100644
--- a/src/bin/proxmox_backup_client/mount.rs
+++ b/src/bin/proxmox_backup_client/mount.rs
@@ -214,6 +214,7 @@ async fn mount_do(param: Value, pipe: Option<RawFd>) -> Result<Value, Error> {
     ).await?;
 
     let (manifest, _) = client.download_manifest().await?;
+    manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
 
     let file_info = manifest.lookup_file_info(&server_archive_name)?;
 
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 08/13] paperkey: refactor common code
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (6 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 07/13] client: check fingerprint after downloading manifest Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject Fabian Grünbichler
                   ` (7 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

from formatting functions to main function, and pass along the key data
lines instead of the full string.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox_backup_client/key.rs | 104 +++++++++++++--------------
 1 file changed, 52 insertions(+), 52 deletions(-)

diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index ea7e8c82..e56308b7 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -385,13 +385,47 @@ fn paper_key(
     };
 
     let data = file_get_contents(&path)?;
-    let data = std::str::from_utf8(&data)?;
+    let data = String::from_utf8(data)?;
+
+    let (data, is_private_key) = if data.starts_with("-----BEGIN ENCRYPTED PRIVATE KEY-----\n") {
+        let lines: Vec<String> = data
+            .lines()
+            .map(|s| s.trim_end())
+            .filter(|s| !s.is_empty())
+            .map(String::from)
+            .collect();
+
+        if !lines[lines.len()-1].starts_with("-----END ENCRYPTED PRIVATE KEY-----") {
+            bail!("unexpected key format");
+        }
+
+        if lines.len() < 20 {
+            bail!("unexpected key format");
+        }
+
+        (lines, true)
+    } else {
+        match serde_json::from_str::<KeyConfig>(&data) {
+            Ok(key_config) => {
+                let lines = serde_json::to_string_pretty(&key_config)?
+                    .lines()
+                    .map(String::from)
+                    .collect();
+
+                (lines, false)
+            },
+            Err(err) => {
+                eprintln!("Couldn't parse '{:?}' as KeyConfig - {}", path, err);
+                bail!("Neither a PEM-formatted private key, nor a PBS key file.");
+            },
+        }
+    };
 
     let format = output_format.unwrap_or(PaperkeyFormat::Html);
 
     match format {
-        PaperkeyFormat::Html => paperkey_html(data, subject),
-        PaperkeyFormat::Text => paperkey_text(data, subject),
+        PaperkeyFormat::Html => paperkey_html(&data, subject, is_private_key),
+        PaperkeyFormat::Text => paperkey_text(&data, subject, is_private_key),
     }
 }
 
@@ -426,7 +460,7 @@ pub fn cli() -> CliCommandMap {
         .insert("paperkey", paper_key_cmd_def)
 }
 
-fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
+fn paperkey_html(lines: &[String], subject: Option<String>, is_private: bool) -> Result<(), Error> {
 
     let img_size_pt = 500;
 
@@ -455,21 +489,7 @@ fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
         println!("<p>Subject: {}</p>", subject);
     }
 
-    if data.starts_with("-----BEGIN ENCRYPTED PRIVATE KEY-----\n") {
-        let lines: Vec<String> = data.lines()
-            .map(|s| s.trim_end())
-            .filter(|s| !s.is_empty())
-            .map(String::from)
-            .collect();
-
-        if !lines[lines.len()-1].starts_with("-----END ENCRYPTED PRIVATE KEY-----") {
-            bail!("unexpected key format");
-        }
-
-        if lines.len() < 20 {
-            bail!("unexpected key format");
-        }
-
+    if is_private {
         const BLOCK_SIZE: usize = 20;
         let blocks = (lines.len() + BLOCK_SIZE -1)/BLOCK_SIZE;
 
@@ -490,8 +510,7 @@ fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
 
             println!("</p>");
 
-            let data = data.join("\n");
-            let qr_code = generate_qr_code("svg", data.as_bytes())?;
+            let qr_code = generate_qr_code("svg", data)?;
             let qr_code = base64::encode_config(&qr_code, base64::STANDARD_NO_PAD);
 
             println!("<center>");
@@ -507,16 +526,13 @@ fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
         return Ok(());
     }
 
-    let key_config: KeyConfig = serde_json::from_str(&data)?;
-    let key_text = serde_json::to_string_pretty(&key_config)?;
-
     println!("<div style=\"page-break-inside: avoid\">");
 
     println!("<p>");
 
     println!("-----BEGIN PROXMOX BACKUP KEY-----");
 
-    for line in key_text.lines() {
+    for line in lines {
         println!("{}", line);
     }
 
@@ -524,7 +540,7 @@ fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
 
     println!("</p>");
 
-    let qr_code = generate_qr_code("svg", key_text.as_bytes())?;
+    let qr_code = generate_qr_code("svg", lines)?;
     let qr_code = base64::encode_config(&qr_code, base64::STANDARD_NO_PAD);
 
     println!("<center>");
@@ -541,27 +557,13 @@ fn paperkey_html(data: &str, subject: Option<String>) -> Result<(), Error> {
     Ok(())
 }
 
-fn paperkey_text(data: &str, subject: Option<String>) -> Result<(), Error> {
+fn paperkey_text(lines: &[String], subject: Option<String>, is_private: bool) -> Result<(), Error> {
 
     if let Some(subject) = subject {
         println!("Subject: {}\n", subject);
     }
 
-    if data.starts_with("-----BEGIN ENCRYPTED PRIVATE KEY-----\n") {
-        let lines: Vec<String> = data.lines()
-            .map(|s| s.trim_end())
-            .filter(|s| !s.is_empty())
-            .map(String::from)
-            .collect();
-
-        if !lines[lines.len()-1].starts_with("-----END ENCRYPTED PRIVATE KEY-----") {
-            bail!("unexpected key format");
-        }
-
-        if lines.len() < 20 {
-            bail!("unexpected key format");
-        }
-
+    if is_private {
         const BLOCK_SIZE: usize = 5;
         let blocks = (lines.len() + BLOCK_SIZE -1)/BLOCK_SIZE;
 
@@ -576,8 +578,7 @@ fn paperkey_text(data: &str, subject: Option<String>) -> Result<(), Error> {
             for l in start..end {
                 println!("{:-2}: {}", l, lines[l]);
             }
-            let data = data.join("\n");
-            let qr_code = generate_qr_code("utf8i", data.as_bytes())?;
+            let qr_code = generate_qr_code("utf8i", data)?;
             let qr_code = String::from_utf8(qr_code)
                 .map_err(|_| format_err!("Failed to read qr code (got non-utf8 data)"))?;
             println!("{}", qr_code);
@@ -587,14 +588,13 @@ fn paperkey_text(data: &str, subject: Option<String>) -> Result<(), Error> {
         return Ok(());
     }
 
-    let key_config: KeyConfig = serde_json::from_str(&data)?;
-    let key_text = serde_json::to_string_pretty(&key_config)?;
-
     println!("-----BEGIN PROXMOX BACKUP KEY-----");
-    println!("{}", key_text);
+    for line in lines {
+        println!("{}", line);
+    }
     println!("-----END PROXMOX BACKUP KEY-----");
 
-    let qr_code = generate_qr_code("utf8i", key_text.as_bytes())?;
+    let qr_code = generate_qr_code("utf8i", &lines)?;
     let qr_code = String::from_utf8(qr_code)
         .map_err(|_| format_err!("Failed to read qr code (got non-utf8 data)"))?;
 
@@ -603,8 +603,7 @@ fn paperkey_text(data: &str, subject: Option<String>) -> Result<(), Error> {
     Ok(())
 }
 
-fn generate_qr_code(output_type: &str, data: &[u8]) -> Result<Vec<u8>, Error> {
-
+fn generate_qr_code(output_type: &str, lines: &[String]) -> Result<Vec<u8>, Error> {
     let mut child = Command::new("qrencode")
         .args(&["-t", output_type, "-m0", "-s1", "-lm", "--output", "-"])
         .stdin(Stdio::piped())
@@ -614,7 +613,8 @@ fn generate_qr_code(output_type: &str, data: &[u8]) -> Result<Vec<u8>, Error> {
     {
         let stdin = child.stdin.as_mut()
             .ok_or_else(|| format_err!("Failed to open stdin"))?;
-        stdin.write_all(data)
+        let data = lines.join("\n");
+        stdin.write_all(data.as_bytes())
             .map_err(|_| format_err!("Failed to write to stdin"))?;
     }
 
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (7 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 08/13] paperkey: refactor common code Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-23  7:07   ` Dietmar Maurer
  2020-11-20 16:38 ` [pbs-devel] [RFC proxmox-backup 10/13] expose previous backup time in backup env Fabian Grünbichler
                   ` (6 subsequent siblings)
  15 siblings, 1 reply; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

and strip fingerprint from keyfile data, since it's rather long and only
informational anyway.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox_backup_client/key.rs | 18 ++++++++++++++----
 1 file changed, 14 insertions(+), 4 deletions(-)

diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index e56308b7..baae8726 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -387,7 +387,7 @@ fn paper_key(
     let data = file_get_contents(&path)?;
     let data = String::from_utf8(data)?;
 
-    let (data, is_private_key) = if data.starts_with("-----BEGIN ENCRYPTED PRIVATE KEY-----\n") {
+    let (data, subject, is_private_key) = if data.starts_with("-----BEGIN ENCRYPTED PRIVATE KEY-----\n") {
         let lines: Vec<String> = data
             .lines()
             .map(|s| s.trim_end())
@@ -403,16 +403,26 @@ fn paper_key(
             bail!("unexpected key format");
         }
 
-        (lines, true)
+        (lines, subject, true)
     } else {
         match serde_json::from_str::<KeyConfig>(&data) {
-            Ok(key_config) => {
+            Ok(mut key_config) => {
+                // add display version of fingerprint to subject and strip from key data
+                let subject = match (subject, key_config.fingerprint.take()) {
+                    (Some(mut subject), Some(fingerprint)) => {
+                        subject.push_str(&format!(" ({})", fingerprint));
+                        Some(subject)
+                    },
+                    (None, Some(fingerprint)) => Some(format!("Fingerprint: {}", fingerprint)),
+                    (subject, _) => subject,
+                };
+
                 let lines = serde_json::to_string_pretty(&key_config)?
                     .lines()
                     .map(String::from)
                     .collect();
 
-                (lines, false)
+                (lines, subject, false)
             },
             Err(err) => {
                 eprintln!("Couldn't parse '{:?}' as KeyConfig - {}", path, err);
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [RFC proxmox-backup 10/13] expose previous backup time in backup env
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (8 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 11/13] refactor BackupInfo -> SnapshotListItem helper Fabian Grünbichler
                   ` (5 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

and use this information to add more information to client backup log
and guide the download manifest decision.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    new in v2

    backwards-compatible API change!

 src/api2/backup.rs               | 26 ++++++++++++++++
 src/bin/proxmox-backup-client.rs | 51 ++++++++++++++++++++++----------
 src/client/backup_writer.rs      |  7 +++++
 3 files changed, 68 insertions(+), 16 deletions(-)

diff --git a/src/api2/backup.rs b/src/api2/backup.rs
index ce9a34ae..f4eed074 100644
--- a/src/api2/backup.rs
+++ b/src/api2/backup.rs
@@ -311,6 +311,10 @@ pub const BACKUP_API_SUBDIRS: SubdirMap = &[
         "previous", &Router::new()
             .download(&API_METHOD_DOWNLOAD_PREVIOUS)
     ),
+    (
+        "previous_backup_time", &Router::new()
+            .get(&API_METHOD_GET_PREVIOUS_BACKUP_TIME)
+    ),
     (
         "speedtest", &Router::new()
             .upload(&API_METHOD_UPLOAD_SPEEDTEST)
@@ -694,6 +698,28 @@ fn finish_backup (
     Ok(Value::Null)
 }
 
+#[sortable]
+pub const API_METHOD_GET_PREVIOUS_BACKUP_TIME: ApiMethod = ApiMethod::new(
+    &ApiHandler::Sync(&get_previous_backup_time),
+    &ObjectSchema::new(
+        "Get previous backup time.",
+        &[],
+    )
+);
+
+fn get_previous_backup_time(
+    _param: Value,
+    _info: &ApiMethod,
+    rpcenv: &mut dyn RpcEnvironment,
+) -> Result<Value, Error> {
+
+    let env: &BackupEnvironment = rpcenv.as_ref();
+
+    let backup_time = env.last_backup.as_ref().map(|info| info.backup_dir.backup_time());
+
+    Ok(json!(backup_time))
+}
+
 #[sortable]
 pub const API_METHOD_DOWNLOAD_PREVIOUS: ApiMethod = ApiMethod::new(
     &ApiHandler::AsyncHttp(&download_previous),
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index b4f87071..3e9931b8 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1100,23 +1100,42 @@ async fn create_backup(
         false
     ).await?;
 
-    let previous_manifest = match client.download_previous_manifest().await {
-        Ok(previous_manifest) => {
-            match previous_manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref)) {
-                Ok(()) => {
-                    println!("Successfully downloaded previous manifest.");
-                    Some(Arc::new(previous_manifest))
-                },
-                Err(err) => {
-                    println!("Couldn't re-use pevious manifest - {}", err);
-                    None
-                },
+    let download_previous_manifest = match client.previous_backup_time().await {
+        Ok(Some(backup_time)) => {
+            println!(
+                "Downloading previous manifest ({})",
+                strftime_local("%c", backup_time)?
+            );
+            true
+        }
+        Ok(None) => {
+            println!("No previous manifest available.");
+            false
+        }
+        Err(_) => {
+            // Fallback for outdated server, TODO remove/bubble up with 2.0
+            true
+        }
+    };
+
+    let previous_manifest = if download_previous_manifest {
+        match client.download_previous_manifest().await {
+            Ok(previous_manifest) => {
+                match previous_manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref)) {
+                    Ok(()) => Some(Arc::new(previous_manifest)),
+                    Err(err) => {
+                        println!("Couldn't re-use previous manifest - {}", err);
+                        None
+                    }
+                }
             }
-        },
-        Err(err) => {
-            println!("Couldn't download pevious manifest - {}", err);
-            None
-        },
+            Err(err) => {
+                println!("Couldn't download previous manifest - {}", err);
+                None
+            }
+        }
+    } else {
+        None
     };
 
     let snapshot = BackupDir::new(backup_type, backup_id, backup_time)?;
diff --git a/src/client/backup_writer.rs b/src/client/backup_writer.rs
index c5ce5b42..39cd574d 100644
--- a/src/client/backup_writer.rs
+++ b/src/client/backup_writer.rs
@@ -475,6 +475,13 @@ impl BackupWriter {
         Ok(index)
     }
 
+    /// Retrieve backup time of last backup
+    pub async fn previous_backup_time(&self) -> Result<Option<i64>, Error> {
+        let data = self.h2.get("previous_backup_time", None).await?;
+        serde_json::from_value(data)
+            .map_err(|err| format_err!("Failed to parse backup time value returned by server - {}", err))
+    }
+
     /// Download backup manifest (index.json) of last backup
     pub async fn download_previous_manifest(&self) -> Result<BackupManifest, Error> {
 
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 11/13] refactor BackupInfo -> SnapshotListItem helper
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (9 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [RFC proxmox-backup 10/13] expose previous backup time in backup env Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 12/13] list_snapshots: return manifest fingerprint Fabian Grünbichler
                   ` (4 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

before adding more fields to the tuple, let's just create the struct
inside the match arms to improve readability.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    new in v2

 src/api2/admin/datastore.rs | 50 +++++++++++++++++++++----------------
 1 file changed, 28 insertions(+), 22 deletions(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index 872d081e..de0c8de3 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -391,9 +391,11 @@ pub fn list_snapshots (
     };
 
     let info_to_snapshot_list_item = |group: &BackupGroup, owner, info: BackupInfo| {
+        let backup_type = group.backup_type().to_string();
+        let backup_id = group.backup_id().to_string();
         let backup_time = info.backup_dir.backup_time();
 
-        let (comment, verification, files, size) = match get_all_snapshot_files(&datastore, &info) {
+        match get_all_snapshot_files(&datastore, &info) {
             Ok((manifest, files)) => {
                 // extract the first line from notes
                 let comment: Option<String> = manifest.unprotected["notes"]
@@ -401,8 +403,8 @@ pub fn list_snapshots (
                     .and_then(|notes| notes.lines().next())
                     .map(String::from);
 
-                let verify = manifest.unprotected["verify_state"].clone();
-                let verify: Option<SnapshotVerifyState> = match serde_json::from_value(verify) {
+                let verification = manifest.unprotected["verify_state"].clone();
+                let verification: Option<SnapshotVerifyState> = match serde_json::from_value(verification) {
                     Ok(verify) => verify,
                     Err(err) => {
                         eprintln!("error parsing verification state : '{}'", err);
@@ -412,14 +414,20 @@ pub fn list_snapshots (
 
                 let size = Some(files.iter().map(|x| x.size.unwrap_or(0)).sum());
 
-                (comment, verify, files, size)
+                SnapshotListItem {
+                    backup_type,
+                    backup_id,
+                    backup_time,
+                    comment,
+                    verification,
+                    files,
+                    size,
+                    owner,
+                }
             },
             Err(err) => {
                 eprintln!("error during snapshot file listing: '{}'", err);
-                (
-                    None,
-                    None,
-                    info
+                let files = info
                         .files
                         .into_iter()
                         .map(|x| BackupContent {
@@ -427,21 +435,19 @@ pub fn list_snapshots (
                             size: None,
                             crypt_mode: None,
                         })
-                        .collect(),
-                    None,
-                )
+                        .collect();
+
+                SnapshotListItem {
+                    backup_type,
+                    backup_id,
+                    backup_time,
+                    comment: None,
+                    verification: None,
+                    files,
+                    size: None,
+                    owner,
+                }
             },
-        };
-
-        SnapshotListItem {
-            backup_type: group.backup_type().to_string(),
-            backup_id: group.backup_id().to_string(),
-            backup_time,
-            comment,
-            verification,
-            files,
-            size,
-            owner,
         }
     };
 
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 12/13] list_snapshots: return manifest fingerprint
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (10 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 11/13] refactor BackupInfo -> SnapshotListItem helper Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 13/13] gui: add snapshot/file fingerprint tooltip Fabian Grünbichler
                   ` (3 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

for display in clients.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    new in v2
    
    backwards-compatible API change (adds new field)

 src/api2/admin/datastore.rs | 10 ++++++++++
 src/api2/types/mod.rs       |  9 ++++++++-
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/src/api2/admin/datastore.rs b/src/api2/admin/datastore.rs
index de0c8de3..00f98c66 100644
--- a/src/api2/admin/datastore.rs
+++ b/src/api2/admin/datastore.rs
@@ -403,6 +403,14 @@ pub fn list_snapshots (
                     .and_then(|notes| notes.lines().next())
                     .map(String::from);
 
+                let fingerprint = match manifest.fingerprint() {
+                    Ok(fp) => fp,
+                    Err(err) => {
+                        eprintln!("error parsing fingerprint: '{}'", err);
+                        None
+                    },
+                };
+
                 let verification = manifest.unprotected["verify_state"].clone();
                 let verification: Option<SnapshotVerifyState> = match serde_json::from_value(verification) {
                     Ok(verify) => verify,
@@ -420,6 +428,7 @@ pub fn list_snapshots (
                     backup_time,
                     comment,
                     verification,
+                    fingerprint,
                     files,
                     size,
                     owner,
@@ -443,6 +452,7 @@ pub fn list_snapshots (
                     backup_time,
                     comment: None,
                     verification: None,
+                    fingerprint: None,
                     files,
                     size: None,
                     owner,
diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs
index d7d5a8af..b347fc29 100644
--- a/src/api2/types/mod.rs
+++ b/src/api2/types/mod.rs
@@ -5,7 +5,7 @@ use proxmox::api::{api, schema::*};
 use proxmox::const_regex;
 use proxmox::{IPRE, IPRE_BRACKET, IPV4RE, IPV6RE, IPV4OCTET, IPV6H16, IPV6LS32};
 
-use crate::backup::{CryptMode, BACKUP_ID_REGEX};
+use crate::backup::{CryptMode, Fingerprint, BACKUP_ID_REGEX};
 use crate::server::UPID;
 
 #[macro_use]
@@ -484,6 +484,10 @@ pub struct SnapshotVerifyState {
             type: SnapshotVerifyState,
             optional: true,
         },
+        fingerprint: {
+            type: String,
+            optional: true,
+        },
         files: {
             items: {
                 schema: BACKUP_ARCHIVE_NAME_SCHEMA
@@ -508,6 +512,9 @@ pub struct SnapshotListItem {
     /// The result of the last run verify task
     #[serde(skip_serializing_if="Option::is_none")]
     pub verification: Option<SnapshotVerifyState>,
+    /// Fingerprint of encryption key
+    #[serde(skip_serializing_if="Option::is_none")]
+    pub fingerprint: Option<Fingerprint>,
     /// List of contained archive files.
     pub files: Vec<BackupContent>,
     /// Overall snapshot size (sum of all archive sizes).
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup 13/13] gui: add snapshot/file fingerprint tooltip
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (11 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 12/13] list_snapshots: return manifest fingerprint Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes Fabian Grünbichler
                   ` (2 subsequent siblings)
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

display short key ID, like backend's Display trait.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---

Notes:
    new in v2

 www/Utils.js             |  5 +++++
 www/datastore/Content.js | 13 ++++++++++++-
 2 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/www/Utils.js b/www/Utils.js
index ab48bdcf..775c03d7 100644
--- a/www/Utils.js
+++ b/www/Utils.js
@@ -161,6 +161,11 @@ Ext.define('PBS.Utils', {
 	return `Datastore ${what} ${id}`;
     },
 
+    // mimics Display trait in backend
+    renderKeyID: function(fingerprint) {
+	return fingerprint.substring(0, 23);
+    },
+
     parse_datastore_worker_id: function(type, id) {
 	let result;
 	let res;
diff --git a/www/datastore/Content.js b/www/datastore/Content.js
index acd4184b..12dacb89 100644
--- a/www/datastore/Content.js
+++ b/www/datastore/Content.js
@@ -12,6 +12,7 @@ Ext.define('pbs-data-store-snapshots', {
 	'files',
 	'owner',
 	'verification',
+	'fingerprint',
 	{ name: 'size', type: 'int', allowNull: true },
 	{
 	    name: 'crypt-mode',
@@ -182,6 +183,7 @@ Ext.define('PBS.DataStoreContent', {
 		for (const file of data.files) {
 		    file.text = file.filename;
 		    file['crypt-mode'] = PBS.Utils.cryptmap.indexOf(file['crypt-mode']);
+		    file.fingerprint = data.fingerprint;
 		    file.leaf = true;
 		    file.matchesFilter = true;
 
@@ -699,7 +701,16 @@ Ext.define('PBS.DataStoreContent', {
 		if (iconCls) {
 		    iconTxt = `<i class="fa fa-fw fa-${iconCls}"></i> `;
 		}
-		return (iconTxt + PBS.Utils.cryptText[v]) || Proxmox.Utils.unknownText;
+		let tip;
+		if (v !== PBS.Utils.cryptmap.indexOf('none') && record.data.fingerprint !== undefined) {
+		    tip = "Key: " + PBS.Utils.renderKeyID(record.data.fingerprint);
+		}
+		let txt = (iconTxt + PBS.Utils.cryptText[v]) || Proxmox.Utils.unknownText;
+		if (record.parentNode.id === 'root' || tip === undefined) {
+		    return txt;
+		} else {
+		    return `<span data-qtip="${tip}">${txt}</span>`;
+		}
 	    },
 	},
 	{
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (12 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 13/13] gui: add snapshot/file fingerprint tooltip Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-24  8:07   ` [pbs-devel] applied: " Dietmar Maurer
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 2/2] restore: improve error if key is missing Fabian Grünbichler
  2020-11-24  7:47 ` [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Dietmar Maurer
  15 siblings, 1 reply; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
requires proxmox-backup with other patches from this series applied

 src/backup.rs  | 2 +-
 src/restore.rs | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backup.rs b/src/backup.rs
index 184ac64..c3a7c11 100644
--- a/src/backup.rs
+++ b/src/backup.rs
@@ -47,7 +47,7 @@ impl BackupTask {
         let crypt_config = match setup.keyfile {
             None => None,
             Some(ref path) => {
-                let (key, _) = load_and_decrypt_key(path, & || {
+                let (key, _, _) = load_and_decrypt_key(path, & || {
                     match setup.key_password {
                         Some(ref key_password) => Ok(key_password.as_bytes().to_vec()),
                         None => bail!("no key_password specified"),
diff --git a/src/restore.rs b/src/restore.rs
index ad825bd..6cba71f 100644
--- a/src/restore.rs
+++ b/src/restore.rs
@@ -41,7 +41,7 @@ impl RestoreTask {
         let crypt_config = match setup.keyfile {
             None => None,
             Some(ref path) => {
-                let (key, _) = load_and_decrypt_key(path, & || {
+                let (key, _, _) = load_and_decrypt_key(path, & || {
                     match setup.key_password {
                         Some(ref key_password) => Ok(key_password.as_bytes().to_vec()),
                         None => bail!("no key_password specified"),
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] [PATCH proxmox-backup-qemu 2/2] restore: improve error if key is missing
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (13 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes Fabian Grünbichler
@ 2020-11-20 16:38 ` Fabian Grünbichler
  2020-11-24  7:47 ` [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Dietmar Maurer
  15 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-20 16:38 UTC (permalink / raw)
  To: pbs-devel

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
Notes: regular users should never run into this check, since we first
restore the config file via proxmox-backup-client, which already checks
this and fails.

requires proxmox-backup patches from this series

 src/restore.rs | 1 +
 1 file changed, 1 insertion(+)

diff --git a/src/restore.rs b/src/restore.rs
index 6cba71f..24983dd 100644
--- a/src/restore.rs
+++ b/src/restore.rs
@@ -92,6 +92,7 @@ impl RestoreTask {
         ).await?;
 
         let (manifest, _) = client.download_manifest().await?;
+        manifest.check_fingerprint(self.crypt_config.as_ref().map(Arc::as_ref))?;
 
         self.manifest.set(Arc::new(manifest))
             .map_err(|_| format_err!("already connected!"))?;
-- 
2.20.1





^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject Fabian Grünbichler
@ 2020-11-23  7:07   ` Dietmar Maurer
  2020-11-23  8:16     ` Fabian Grünbichler
  0 siblings, 1 reply; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-23  7:07 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Fabian Grünbichler


> +            Ok(mut key_config) => {
> +                // add display version of fingerprint to subject and strip from key data
> +                let subject = match (subject, key_config.fingerprint.take()) {
> +                    (Some(mut subject), Some(fingerprint)) => {
> +                        subject.push_str(&format!(" ({})", fingerprint));
> +                        Some(subject)
> +                    },
> +                    (None, Some(fingerprint)) => Some(format!("Fingerprint: {}", fingerprint)),
> +                    (subject, _) => subject,
> +                };
> +

I still don't get why we need a 32byte fingerprint - this is the same length as the key itself!

I want to avoid having keys with different content (strip fingerprint), because this only
confuse people.




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config Fabian Grünbichler
@ 2020-11-23  8:07   ` Wolfgang Bumiller
  0 siblings, 0 replies; 26+ messages in thread
From: Wolfgang Bumiller @ 2020-11-23  8:07 UTC (permalink / raw)
  To: Fabian Grünbichler; +Cc: pbs-devel

On Fri, Nov 20, 2020 at 05:38:32PM +0100, Fabian Grünbichler wrote:
> diff --git a/src/tools/format.rs b/src/tools/format.rs
> index 8fe6aa82..c9f50053 100644
> --- a/src/tools/format.rs
> +++ b/src/tools/format.rs
> @@ -102,6 +102,40 @@ impl From<u64> for HumanByte {
>      }
>  }
>  

Minor nit (maybe for a follow-up cleanup depending on how the rest of
the series goes):

> +pub fn as_fingerprint(bytes: &[u8]) -> String {
> +    proxmox::tools::digest_to_hex(bytes)
> +        .as_bytes()
> +        .chunks(2)
> +        .map(|v| std::str::from_utf8(v).unwrap())

^ this performs an unnecessary check, even if you just unwrap it, use

    unsafe { std::str::from_utf8_unchecked(v) }

> +        .collect::<Vec<&str>>().join(":")

^ this allocates multiple tiny vectors.

Since you already know even the length of the resulting string it's
nicer to just allocate it before hand and fill it in a loop.

    lex hex = proxmox::tools::digest_to_hex(bytes);

    // avoid underflow:
    if hex.is_empty() {
        return String::new();
    }

    let mut out = String::with_capacity((hex.len() / 2) - 1);
    for pair in hex.as_bytes().chunks(2) {
        if !out.is_empty() {
            out.push(':');
        }
        out.push_str(unsafe { std::str::from_utf8_unchecked(pair) });
    }
    out




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  7:07   ` Dietmar Maurer
@ 2020-11-23  8:16     ` Fabian Grünbichler
  2020-11-23  8:30       ` Dietmar Maurer
                         ` (2 more replies)
  0 siblings, 3 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-23  8:16 UTC (permalink / raw)
  To: Dietmar Maurer, Proxmox Backup Server development discussion

On November 23, 2020 8:07 am, Dietmar Maurer wrote:
> 
>> +            Ok(mut key_config) => {
>> +                // add display version of fingerprint to subject and strip from key data
>> +                let subject = match (subject, key_config.fingerprint.take()) {
>> +                    (Some(mut subject), Some(fingerprint)) => {
>> +                        subject.push_str(&format!(" ({})", fingerprint));
>> +                        Some(subject)
>> +                    },
>> +                    (None, Some(fingerprint)) => Some(format!("Fingerprint: {}", fingerprint)),
>> +                    (subject, _) => subject,
>> +                };
>> +
> 
> I still don't get why we need a 32byte fingerprint - this is the same length as the key itself!

the key (on disk) is 8 + 8 + 8 + 32 (key derivation) + 64 (encrypted key 
data) + 8 + 8 (timestamps), totalling 136 bytes. serialized it's a bit 
more, although there the fingerprint skews the numbers more heavily 
(because I opted for a readable serialization, not one optimized for 
size). even in-memory, the key is not 32-byte long, but 32+32+however 
long the PKey struct from openssl is.

I want to have the "full" fingerprint because we persist this in places 
where we can't (easily) update it, so it's more future-proof to keep the 
full value there. it also makes it possible to use the full value for 
the actual comparison done on manifest load/check (where we not only 
have to think about collisions for a single user, but potentially 
hundreds/thousands if they share a datastore!).

> I want to avoid having keys with different content (strip fingerprint), because this only
> confuse people.

I originally wanted to keep the full fingerprint in, but conceded to 
your space arguments here and only included the short version.. I also 
don't really buy the confusion, since unless the user has manually 
looked inside the key file they don't even know that or how the 
fingerprint is stored there - in the user-facing parts we only ever show 
the short key ID. furthermore we already paperkey the pretty-printed 
version so if the user restores that the checksum of the keyfile is 
different anyhow.




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  8:16     ` Fabian Grünbichler
@ 2020-11-23  8:30       ` Dietmar Maurer
  2020-11-23  8:47         ` Fabian Grünbichler
  2020-11-23  8:41       ` Dietmar Maurer
  2020-11-23  8:55       ` Dietmar Maurer
  2 siblings, 1 reply; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-23  8:30 UTC (permalink / raw)
  To: Fabian Grünbichler, Proxmox Backup Server development discussion

> I originally wanted to keep the full fingerprint in, but conceded to 
> your space arguments here and only included the short version.. I also 
> don't really buy the confusion, since unless the user has manually 
> looked inside the key file they don't even know that or how the 
> fingerprint is stored there - in the user-facing parts we only ever show 
> the short key ID. furthermore we already paperkey the pretty-printed 
> version so if the user restores that the checksum of the keyfile is 
> different anyhow.

Don't get that. You delete the fingerprint, so if a user restores that key
he don't have a fingerprint to compare?




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  8:16     ` Fabian Grünbichler
  2020-11-23  8:30       ` Dietmar Maurer
@ 2020-11-23  8:41       ` Dietmar Maurer
  2020-11-23  8:55       ` Dietmar Maurer
  2 siblings, 0 replies; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-23  8:41 UTC (permalink / raw)
  To: Fabian Grünbichler, Proxmox Backup Server development discussion

> I want to have the "full" fingerprint because we persist this in places 
> where we can't (easily) update it, so it's more future-proof to keep the 
> full value there. it also makes it possible to use the full value for 
> the actual comparison done on manifest load/check (where we not only 
> have to think about collisions for a single user, but potentially 
> hundreds/thousands if they share a datastore!).

We have backup "owners", so users can only access backups they own.
But even with thousands of keys, collisions are very unlikely...




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  8:30       ` Dietmar Maurer
@ 2020-11-23  8:47         ` Fabian Grünbichler
  0 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-23  8:47 UTC (permalink / raw)
  To: Dietmar Maurer, Proxmox Backup Server development discussion

On November 23, 2020 9:30 am, Dietmar Maurer wrote:
>> I originally wanted to keep the full fingerprint in, but conceded to 
>> your space arguments here and only included the short version.. I also 
>> don't really buy the confusion, since unless the user has manually 
>> looked inside the key file they don't even know that or how the 
>> fingerprint is stored there - in the user-facing parts we only ever show 
>> the short key ID. furthermore we already paperkey the pretty-printed 
>> version so if the user restores that the checksum of the keyfile is 
>> different anyhow.
> 
> Don't get that. You delete the fingerprint, so if a user restores that key
> he don't have a fingerprint to compare?

the short key ID is put into the subject now to make human matching of 
paperkey printouts and manifest/snapshot lists easier.

if the user restores the paperkey into a keyfile, anytime that keyfile 
is used it will re-generate the fingerprint on the fly. if the keyfile 
is modified (e.g. on passphrase/KDF change), the generated fingerprint 
will also be persisted again. we don't have a 'restore paperkey' command 
(yet), but if we ever do, that could of course also regenerate the full 
fingerprint and persist it on writing out the keyfile.




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  8:16     ` Fabian Grünbichler
  2020-11-23  8:30       ` Dietmar Maurer
  2020-11-23  8:41       ` Dietmar Maurer
@ 2020-11-23  8:55       ` Dietmar Maurer
  2020-11-23  9:44         ` Fabian Grünbichler
  2 siblings, 1 reply; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-23  8:55 UTC (permalink / raw)
  To: Fabian Grünbichler, Proxmox Backup Server development discussion

> > I still don't get why we need a 32byte fingerprint - this is the same length as the key itself!
> 
> the key (on disk) is 8 + 8 + 8 + 32 (key derivation) + 64 (encrypted key 
> data) + 8 + 8 (timestamps), totalling 136 bytes. serialized it's a bit 
> more, although there the fingerprint skews the numbers more heavily 
> (because I opted for a readable serialization, not one optimized for 
> size). even in-memory, the key is not 32-byte long, but 32+32+however 
> long the PKey struct from openssl is.

For the record, the fingerprint is computed using 32 secret bytes (not more). 

Note: FINGERPRINT_INPUT is const

+    pub fn fingerprint(&self) -> Fingerprint {
+        Fingerprint {
+            bytes: self.compute_digest(&FINGERPRINT_INPUT)
+        }
+    }
+

    pub fn compute_digest(&self, data: &[u8]) -> [u8; 32] {
        let mut hasher = openssl::sha::Sha256::new();
        hasher.update(data); // this is const
        hasher.update(&self.id_key); // this is 32 bytes
        hasher.finish()
    }

So the fingerprint has exactly the same size as the secret key.
I really do not understand your arguments...




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject
  2020-11-23  8:55       ` Dietmar Maurer
@ 2020-11-23  9:44         ` Fabian Grünbichler
  0 siblings, 0 replies; 26+ messages in thread
From: Fabian Grünbichler @ 2020-11-23  9:44 UTC (permalink / raw)
  To: Dietmar Maurer, Proxmox Backup Server development discussion

On November 23, 2020 9:55 am, Dietmar Maurer wrote:
>> > I still don't get why we need a 32byte fingerprint - this is the same length as the key itself!
>> 
>> the key (on disk) is 8 + 8 + 8 + 32 (key derivation) + 64 (encrypted key 
>> data) + 8 + 8 (timestamps), totalling 136 bytes. serialized it's a bit 
>> more, although there the fingerprint skews the numbers more heavily 
>> (because I opted for a readable serialization, not one optimized for 
>> size). even in-memory, the key is not 32-byte long, but 32+32+however 
>> long the PKey struct from openssl is.
> 
> For the record, the fingerprint is computed using 32 secret bytes (not more). 
> 
> Note: FINGERPRINT_INPUT is const
> 
> +    pub fn fingerprint(&self) -> Fingerprint {
> +        Fingerprint {
> +            bytes: self.compute_digest(&FINGERPRINT_INPUT)
> +        }
> +    }
> +
> 
>     pub fn compute_digest(&self, data: &[u8]) -> [u8; 32] {
>         let mut hasher = openssl::sha::Sha256::new();
>         hasher.update(data); // this is const
>         hasher.update(&self.id_key); // this is 32 bytes
>         hasher.finish()
>     }
> 
> So the fingerprint has exactly the same size as the secret key.
> I really do not understand your arguments...

I specifically wrote what my numbers refer to (structures kept on-disk 
and in-memory, both for the FULL key). yes, the PART of the key that is 
used to calculated digests (and the fingerprint) is only 32 bytes. my 
argument is that for both on-disk and in-memory, adding a full 32 bytes 
fingerprint does not 'double' the size, as you imply, since the 
fingerprint is for the full key and not just the derived ID key part.

if everybody besides me thinks that storing the truncated 8 byte 
"fingerprint" is a good idea, then I can send a v3 that does that.

IMHO we don't have much to gain by storing it truncated:
- key files are a bit smaller (they are client-side only anyway)
- manifests are a bit smaller (they are in a different order of 
  magnitude anyway)

while storing the full one has advantages:
- fingerprint has same level of security as chunk digest (if that breaks 
  everything is broken anyway!), allowing us to base non-informational 
  features on it as well (such as Qemu key tracking for bitmap 
  invalidation)
- we can change how to display it in the future and still have the full 
  value to derive the display value from




^ permalink raw reply	[flat|nested] 26+ messages in thread

* Re: [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint
  2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
                   ` (14 preceding siblings ...)
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 2/2] restore: improve error if key is missing Fabian Grünbichler
@ 2020-11-24  7:47 ` Dietmar Maurer
  15 siblings, 0 replies; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-24  7:47 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Fabian Grünbichler

finally applied, without patch 9/13

also changed the "key show" command to use format_and_print_result_full()


> On 11/20/2020 5:38 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> changes since v1:
> - switch to wrapping fingerprint in (transparent) Fingerprint struct
> - display as 'short key ID' of 8 bytes (easily swappable to other
>   format if desired)
> - return in list_snapshots, display in GUI
> - improve log messages for (non-)incremental backups
> - handle fingerprint in paperkey
> - adapt qemu library
>




^ permalink raw reply	[flat|nested] 26+ messages in thread

* [pbs-devel] applied: [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes
  2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes Fabian Grünbichler
@ 2020-11-24  8:07   ` Dietmar Maurer
  0 siblings, 0 replies; 26+ messages in thread
From: Dietmar Maurer @ 2020-11-24  8:07 UTC (permalink / raw)
  To: Proxmox Backup Server development discussion, Fabian Grünbichler

applied both patches




^ permalink raw reply	[flat|nested] 26+ messages in thread

end of thread, other threads:[~2020-11-24  8:07 UTC | newest]

Thread overview: 26+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-11-20 16:38 [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 01/13] crypt config: add fingerprint mechanism Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 02/13] key: add fingerprint to key config Fabian Grünbichler
2020-11-23  8:07   ` Wolfgang Bumiller
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 03/13] client: print key fingerprint and master key Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 04/13] client: add 'key show' command Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 05/13] fix #3139: add key fingerprint to manifest Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 06/13] manifest: check fingerprint when loading with key Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 07/13] client: check fingerprint after downloading manifest Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 08/13] paperkey: refactor common code Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 09/13] paperkey: add short key ID to subject Fabian Grünbichler
2020-11-23  7:07   ` Dietmar Maurer
2020-11-23  8:16     ` Fabian Grünbichler
2020-11-23  8:30       ` Dietmar Maurer
2020-11-23  8:47         ` Fabian Grünbichler
2020-11-23  8:41       ` Dietmar Maurer
2020-11-23  8:55       ` Dietmar Maurer
2020-11-23  9:44         ` Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [RFC proxmox-backup 10/13] expose previous backup time in backup env Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 11/13] refactor BackupInfo -> SnapshotListItem helper Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 12/13] list_snapshots: return manifest fingerprint Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup 13/13] gui: add snapshot/file fingerprint tooltip Fabian Grünbichler
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 1/2] adapt to proxmox-backup fingerprint changes Fabian Grünbichler
2020-11-24  8:07   ` [pbs-devel] applied: " Dietmar Maurer
2020-11-20 16:38 ` [pbs-devel] [PATCH proxmox-backup-qemu 2/2] restore: improve error if key is missing Fabian Grünbichler
2020-11-24  7:47 ` [pbs-devel] [PATCH v2 proxmox-backup(-qemu) 00/15] add, persist and check fingerprint Dietmar Maurer

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Service provided by Proxmox Server Solutions GmbH | Privacy | Legal