[pbs-devel] [PATCH proxmox-backup 0/7] master key improvements

[pbs-devel] [PATCH proxmox-backup 0/7] master key improvements

[Fabian Grünbichler]

this series fixes a bug preventing encrypted-key-restore that got
introduced with fingerprint checks when restoring files, and further
improves master key handling by providing an import command that takes a
restored encrypted key and a private master key to create a PBS keyfile.

Fabian Grünbichler (7):
  master key: store blob name in constant
  fix #3197: skip fingerprint check when restoring key
  key: move RSA-encryption to KeyConfig
  client: add 'import-with-master-key' command
  docs: replace openssl command with client
  KeyConfig: add encrypt/decrypt test
  KeyConfig: always calculate fingerprint

 docs/backup-client.rst               |   6 +-
 src/backup/crypt_config.rs           |  26 +------
 src/backup/key_derivation.rs         | 104 +++++++++++++++++++++++++--
 src/backup/manifest.rs               |   1 +
 src/bin/proxmox-backup-client.rs     |  33 +++++----
 src/bin/proxmox_backup_client/key.rs |  95 ++++++++++++++++++++++++
 6 files changed, 218 insertions(+), 47 deletions(-)

-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 1/7] master key: store blob name in constant

[Fabian Grünbichler]

since we will use it in more than one place.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/manifest.rs           | 1 +
 src/bin/proxmox-backup-client.rs | 3 ++-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs
index a64cbe15..8742cb0a 100644
--- a/src/backup/manifest.rs
+++ b/src/backup/manifest.rs
@@ -10,6 +10,7 @@ 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";
 pub const CLIENT_LOG_BLOB_NAME: &str = "client.log.blob";
+pub const ENCRYPTED_KEY_BLOB_NAME: &str = "rsa-encrypted.key.blob";
 
 mod hex_csum {
     use serde::{self, Deserialize, Serializer, Deserializer};
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 372ff268..1c456aab 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -54,6 +54,7 @@ use proxmox_backup::backup::{
     CryptConfig,
     CryptMode,
     DynamicIndexReader,
+    ENCRYPTED_KEY_BLOB_NAME,
     FixedChunkStream,
     FixedIndexReader,
     IndexFile,
@@ -1064,7 +1065,7 @@ async fn create_backup(
     }
 
     if let Some(rsa_encrypted_key) = rsa_encrypted_key {
-        let target = "rsa-encrypted.key.blob";
+        let target = ENCRYPTED_KEY_BLOB_NAME;
         println!("Upload RSA encoded key to '{:?}' as {}", repo, target);
         let stats = client
             .upload_blob_from_data(rsa_encrypted_key, target, false, false)
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 2/7] fix #3197: skip fingerprint check when restoring key

[Fabian Grünbichler]

when restoring an encrypted key, the original one is obviously not
available to check the fingerprint with.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/bin/proxmox-backup-client.rs | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 1c456aab..36da624e 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1273,10 +1273,15 @@ async fn restore(param: Value) -> Result<Value, Error> {
         true,
     ).await?;
 
+    let (archive_name, archive_type) = parse_archive_type(archive_name);
+
     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);
+    if archive_name == ENCRYPTED_KEY_BLOB_NAME && crypt_config.is_none() {
+        eprintln!("Restoring encrypted key blob without original key - skipping manifest fingerprint check!")
+    } else {
+        manifest.check_fingerprint(crypt_config.as_ref().map(Arc::as_ref))?;
+    }
 
     if archive_name == MANIFEST_BLOB_NAME {
         if let Some(target) = target {
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 3/7] key: move RSA-encryption to KeyConfig

[Fabian Grünbichler]

since that is what gets encrypted, and not a CryptConfig.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/crypt_config.rs       | 26 +-------------------------
 src/backup/key_derivation.rs     | 14 ++++++++++++++
 src/bin/proxmox-backup-client.rs | 13 +++++++++++--
 3 files changed, 26 insertions(+), 27 deletions(-)

diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs
index 711f90f6..2b04380a 100644
--- a/src/backup/crypt_config.rs
+++ b/src/backup/crypt_config.rs
@@ -11,7 +11,7 @@ use std::fmt;
 use std::fmt::Display;
 use std::io::Write;
 
-use anyhow::{bail, Error};
+use anyhow::{Error};
 use openssl::hash::MessageDigest;
 use openssl::pkcs5::pbkdf2_hmac;
 use openssl::symm::{decrypt_aead, Cipher, Crypter, Mode};
@@ -248,28 +248,4 @@ impl CryptConfig {
 
         Ok(decr_data)
     }
-
-    pub fn generate_rsa_encoded_key(
-        &self,
-        rsa: openssl::rsa::Rsa<openssl::pkey::Public>,
-        created: i64,
-    ) -> 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(),
-            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];
-        let len = rsa.public_encrypt(&data, &mut buffer, openssl::rsa::Padding::PKCS1)?;
-        if len != buffer.len() {
-            bail!("got unexpected length from rsa.public_encrypt().");
-        }
-        Ok(buffer)
-    }
 }
diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 046a8c8f..a2aa9469 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -245,3 +245,17 @@ pub fn decrypt_key(
 
     Ok((result, created, fingerprint))
 }
+
+pub fn rsa_encrypt_key_config(
+    rsa: openssl::rsa::Rsa<openssl::pkey::Public>,
+    key: &KeyConfig,
+) -> Result<Vec<u8>, Error> {
+    let data = serde_json::to_string(key)?.as_bytes().to_vec();
+
+    let mut buffer = vec![0u8; rsa.size() as usize];
+    let len = rsa.public_encrypt(&data, &mut buffer, openssl::rsa::Padding::PKCS1)?;
+    if len != buffer.len() {
+        bail!("got unexpected length from rsa.public_encrypt().");
+    }
+    Ok(buffer)
+}
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index 36da624e..c40bedc5 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -40,6 +40,7 @@ use proxmox_backup::pxar::catalog::*;
 use proxmox_backup::backup::{
     archive_type,
     decrypt_key,
+    rsa_encrypt_key_config,
     verify_chunk_size,
     ArchiveType,
     AsyncReadChunk,
@@ -57,6 +58,7 @@ use proxmox_backup::backup::{
     ENCRYPTED_KEY_BLOB_NAME,
     FixedChunkStream,
     FixedIndexReader,
+    KeyConfig,
     IndexFile,
     MANIFEST_BLOB_NAME,
     Shell,
@@ -914,13 +916,20 @@ async fn create_backup(
             let (key, created, fingerprint) = decrypt_key(&key, &key::get_encryption_key_password)?;
             println!("Encryption key fingerprint: {}", fingerprint);
 
-            let crypt_config = CryptConfig::new(key)?;
+            let crypt_config = CryptConfig::new(key.clone())?;
 
             match key::find_master_pubkey()? {
                 Some(ref path) if path.exists() => {
                     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)?;
+                    let key_config = KeyConfig {
+                        kdf: None,
+                        created,
+                        modified: proxmox::tools::time::epoch_i64(),
+                        data: key.to_vec(),
+                        fingerprint: Some(fingerprint),
+                    };
+                    let enc_key = rsa_encrypt_key_config(rsa, &key_config)?;
                     println!("Master key '{:?}'", path);
 
                     (Some(Arc::new(crypt_config)), Some(enc_key))
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 4/7] client: add 'import-with-master-key' command

[Fabian Grünbichler]

to import an encrypted encryption key using a master key.

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 src/backup/key_derivation.rs         | 12 ++++
 src/bin/proxmox-backup-client.rs     |  8 ---
 src/bin/proxmox_backup_client/key.rs | 95 ++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 8 deletions(-)

diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index a2aa9469..8289b86c 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -259,3 +259,15 @@ pub fn rsa_encrypt_key_config(
     }
     Ok(buffer)
 }
+
+pub fn rsa_decrypt_key_config(
+    rsa: openssl::rsa::Rsa<openssl::pkey::Private>,
+    key: &[u8],
+    passphrase: &dyn Fn() -> Result<Vec<u8>, Error>,
+) -> Result<([u8; 32], i64, Fingerprint), Error> {
+    let mut buffer = vec![0u8; rsa.size() as usize];
+    let decrypted = rsa
+        .private_decrypt(key, &mut buffer, openssl::rsa::Padding::PKCS1)
+        .map_err(|err| format_err!("failed to decrypt KeyConfig using RSA - {}", err))?;
+    decrypt_key(&mut buffer[..decrypted], passphrase)
+}
diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs
index c40bedc5..6cf81952 100644
--- a/src/bin/proxmox-backup-client.rs
+++ b/src/bin/proxmox-backup-client.rs
@@ -1081,14 +1081,6 @@ async fn create_backup(
             .await?;
         manifest.add_file(target.to_string(), stats.size, stats.csum, crypt_mode)?;
 
-        // openssl rsautl -decrypt -inkey master-private.pem -in rsa-encrypted.key -out t
-        /*
-        let mut buffer2 = vec![0u8; rsa.size() as usize];
-        let pem_data = file_get_contents("master-private.pem")?;
-        let rsa = openssl::rsa::Rsa::private_key_from_pem(&pem_data)?;
-        let len = rsa.private_decrypt(&buffer, &mut buffer2, openssl::rsa::Padding::PKCS1)?;
-        println!("TEST {} {:?}", len, buffer2);
-         */
     }
     // create manifest (index.json)
     // manifests are never encrypted, but include a signature
diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs
index 88fa5340..e49131c1 100644
--- a/src/bin/proxmox_backup_client/key.rs
+++ b/src/bin/proxmox_backup_client/key.rs
@@ -21,12 +21,14 @@ use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions};
 use proxmox_backup::backup::{
     encrypt_key_with_passphrase,
     load_and_decrypt_key,
+    rsa_decrypt_key_config,
     store_key_config,
     CryptConfig,
     Kdf,
     KeyConfig,
     KeyDerivationConfig,
 };
+
 use proxmox_backup::tools;
 
 #[api()]
@@ -152,6 +154,90 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
     Ok(())
 }
 
+#[api(
+    input: {
+        properties: {
+            "master-keyfile": {
+                description: "(Private) master key to use.",
+            },
+            "encrypted-keyfile": {
+                description: "RSA-encrypted keyfile to import.",
+            },
+            kdf: {
+                type: Kdf,
+                optional: true,
+            },
+            "path": {
+                description:
+                    "Output file. Without this the key will become the new default encryption key.",
+                optional: true,
+            }
+        },
+    },
+)]
+/// Import an encrypted backup of an encryption key using a (private) master key.
+async fn import_with_master_key(
+    master_keyfile: String,
+    encrypted_keyfile: String,
+    kdf: Option<Kdf>,
+    path: Option<String>,
+) -> Result<(), Error> {
+    let path = match path {
+        Some(path) => PathBuf::from(path),
+        None => {
+            let path = place_default_encryption_key()?;
+            if path.exists() {
+                bail!("Please remove default encryption key at {:?} before importing to default location (or choose a non-default one).", path);
+            }
+            println!("Importing key to default location at: {:?}", path);
+            path
+        }
+    };
+
+    let encrypted_key = file_get_contents(&encrypted_keyfile)?;
+    let master_key = file_get_contents(&master_keyfile)?;
+    let password = tty::read_password("Master Key Password: ")?;
+
+    let master_key =
+        openssl::pkey::PKey::private_key_from_pem_passphrase(&master_key, &password)
+        .map_err(|err| format_err!("failed to read PEM-formatted private key - {}", err))?
+        .rsa()
+        .map_err(|err| format_err!("not a valid private RSA key - {}", err))?;
+
+    let (key, created, fingerprint) =
+        rsa_decrypt_key_config(master_key, &encrypted_key, &get_encryption_key_password)?;
+
+    let kdf = kdf.unwrap_or_default();
+    match kdf {
+        Kdf::None => {
+            let modified = proxmox::tools::time::epoch_i64();
+
+            store_key_config(
+                &path,
+                true,
+                KeyConfig {
+                    kdf: None,
+                    created, // keep original value
+                    modified,
+                    data: key.to_vec(),
+                    fingerprint: Some(fingerprint),
+                },
+            )?;
+        }
+        Kdf::Scrypt | Kdf::PBKDF2 => {
+            let password = tty::read_and_verify_password("New Password: ")?;
+
+            let mut new_key_config = encrypt_key_with_passphrase(&key, &password, kdf)?;
+            new_key_config.created = created; // keep original value
+            new_key_config.fingerprint = Some(fingerprint);
+
+            store_key_config(&path, true, new_key_config)?;
+        }
+    }
+
+    Ok(())
+}
+
 #[api(
     input: {
         properties: {
@@ -446,6 +532,14 @@ pub fn cli() -> CliCommandMap {
         .arg_param(&["path"])
         .completion_cb("path", tools::complete_file_name);
 
+    let key_import_with_master_key_cmd_def = CliCommand::new(&API_METHOD_IMPORT_WITH_MASTER_KEY)
+        .arg_param(&["master-keyfile"])
+        .completion_cb("master-keyfile", tools::complete_file_name)
+        .arg_param(&["encrypted-keyfile"])
+        .completion_cb("encrypted-keyfile", tools::complete_file_name)
+        .arg_param(&["path"])
+        .completion_cb("path", tools::complete_file_name);
+
     let key_change_passphrase_cmd_def = CliCommand::new(&API_METHOD_CHANGE_PASSPHRASE)
         .arg_param(&["path"])
         .completion_cb("path", tools::complete_file_name);
@@ -465,6 +559,7 @@ pub fn cli() -> CliCommandMap {
 
     CliCommandMap::new()
         .insert("create", key_create_cmd_def)
+        .insert("import-with-master-key", key_import_with_master_key_cmd_def)
         .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)
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 5/7] docs: replace openssl command with client

[Fabian Grünbichler]

Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>
---
 docs/backup-client.rst | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/docs/backup-client.rst b/docs/backup-client.rst
index b462f399..1c35818f 100644
--- a/docs/backup-client.rst
+++ b/docs/backup-client.rst
@@ -353,8 +353,10 @@ To set up a master key:
 
    .. code-block:: console
 
-     # openssl rsautl -decrypt -inkey master-private.pem -in rsa-encrypted.key -out /path/to/target
-     Enter pass phrase for ./master-private.pem: *********
+     # proxmox-backup-client key import-with-master-key /path/to/target --master-keyfile /path/to/master-private.pem --encrypted-keyfile /path/to/rsa-encrypted.key
+     Master Key Password: ******
+     New Password: ******
+     Verify Password: ******
 
 7. The target file will now contain the encryption key information in plain
    text. The success of this can be confirmed by passing the resulting ``json``
-- 
2.20.1

[pbs-devel] [PATCH proxmox-backup 6/7] KeyConfig: add encrypt/decrypt test

[Fabian Grünbichler]

the RSA key and the encryption key itself are hard-coded to avoid
stalling the test runs because of lack of entropy, they have no special
significance otherwise.

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

diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 8289b86c..a91b21ca 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -271,3 +271,47 @@ pub fn rsa_decrypt_key_config(
         .map_err(|err| format_err!("failed to decrypt KeyConfig using RSA - {}", err))?;
     decrypt_key(&mut buffer[..decrypted], passphrase)
 }
+
+#[test]
+fn encrypt_decrypt_test() -> Result<(), Error> {
+    use openssl::bn::BigNum;
+
+    // hard-coded RSA key to avoid RNG load
+    let n = BigNum::from_dec_str("763297775503047285270249195475255390818773358873206395804367739392073195066702037300664538507287660511788773520960052020490020500131532096848837840341808263208238432840771609456175669299183585737297710099814398628316822920397690811697390531460556770185920171717205255045261184699028939408338685227311360280561223048029934992213591164033485740834987719043448066906674761591422028943934961140687347873900379335604823288576732038392698785999130614670054444889172406669687648738432457362496418067100853836965448514921778954255248154805294695304544857640397043149235605321195443660560591215396460879078079707598866525981810195613239506906019678159905700662365794059211182357495974678620101388841934629146959674859076053348229540838236896752745251734302737503775744293828247434369307467826891918526442390310055226655466835862319406221740752718258752129277114593279326227799698036058425261999904258111333276380007458144919278763944469942242338999234161147188585579934794573969834141472487673642778646170134259790130811461184848743147137198639341697548363179639042991358823669057297753206096865332303845149920379065177826748710006313272747133642274061146677367740923397358666767242901746171920401890395722806446280380164886804469750825832083").expect("converting to bignum failed");
+    let e = BigNum::from_dec_str("65537").expect("converting to bignum failed");
+    let d = BigNum::from_dec_str("19834537920284564853674022001226176519590018312725185651690468898251379391772488358073023011091610629897253174637151053464371346136136825929376853412608136964518211867003891708559549030570664609466682947037305962494828103719078802086159819263581307957743290849968728341884428605863043529798446388179368090663224786773806846388143274064254180335413340334940446739125488182098535411927937482988091512111514808559058456451259207186517021416246081401087976557460070014777577029793101223558164090029643622447657946212243306210181845486266030884899215596710196751196243890657122549917370139613045651724521564033154854414253451612565268626314358200247667906740226693180923631251719053819020017537699856142036238058103150388959616397059243552685604990510867544536282659146915388522812398795915840913802745825670833498941795568293354230962683054249223513028733221781409833526268687556063636480230666207346771664323325175723577540510559973905170578206847160551684632855673373061549848844186260938182413805301541655002820734307939021848604620517318497220269398148326924299176570233223593669359192722153811016413065311904503101005564780859010942238851216519088762587394817890851764597501374473176420295837906296738426781972820833509964922715585").expect("converting to bignum failed");
+    let p = BigNum::from_dec_str("29509637001892646371585718218450720181675215968655693119622290166463846337874978909899277049204111617901784460858811114760264767076166751445502024396748257412446297522757119324882999179307561418697097464139952930737249422485899639568595470472222197161276683797577982497955467948265299386993875583089675892019886767032750524889582030672594405810531152141432362873209548569385820623081973262550874468619670422387868884561012170536839449407663630232422905779693831681822257822783504983493794208329832510955061326579888576047912149807967610736616238778237407615015312695567289456675371922184276823263863231190560557676339").expect("converting to bignum failed");
+    let q = BigNum::from_dec_str("25866050993920799422553175902510303878636288340476152724026122959148470649546748310678170203350410878157245623372422271950639190884394436256045773535202161325882791039345330048364703416719823181485853395688815455066122599160191671526435061804017559815713791273329637690511813515454721229797045837580571003198471014420883727461348135261877384657284061678787895040009197824032371314493780688519536250146270701914875469190776765810821706480996720025323321483843112182646061748043938180130013308823672610860230340094502643614566152670758944502783858455501528490806234504795239898001698524105646533910560293336400403204897").expect("converting to bignum failed");
+    let dmp1 = BigNum::from_dec_str("21607770579166338313924278588690558922108583912962897316392792781303188398339022047518905458553289108745759383366535358272664077428797321640702979183532285223743426240475893650342331272664468275332046219832278884297711602396407401980831582724583041600551528176116883960387063733484217876666037528133838392148714866050744345765006980605100330287254053877398358630385580919903058731105447806937933747350668236714360621211130384969129674812319182867594036995223272269821421615266717078107026511273509659211002684589097654567453625356436054504001404801715927134738465685565147724902539753143706245247513141254140715042985").expect("converting to bignum failed");
+    let dmq1 = BigNum::from_dec_str("294824909477987048059069264677589712640818276551195295555907561384926187881828905626998384758270243160099828809057470393016578048898219996082612765778049262408020582364022789357590879232947921274546172186391582540158896220038500063021605980859684104892476037676079761887366292263067835858498149757735119694054623308549371262243115446856316376077501168409517640338844786525965200908293851935915491689568704919822573134038943559526432621897623477713604851434011395096458613085567264607124524187730342254186063812054159860538030670385536895853938115358646898433438472543479930479076991585011794266310458811393428158049").expect("converting to bignum failed");
+    let iqmp = BigNum::from_dec_str("19428066064824171668277167138275898936765006396600005071379051329779053619544399695639107933588871625444213173194462077344726482973273922001955114108600584475883837715007613468112455972196002915686862701860412263935895363086514864873592142686096117947515832613228762197577036084559813332497101195090727973644165586960538914545531208630624795512138060798977135902359295307626262953373309121954863020224150277262533638440848025788447039555055470985052690506486164836957350781708784380677438638580158751807723730202286612196281022183410822668814233870246463721184575820166925259871133457423401827024362448849298618281053").expect("converting to bignum failed");
+    let public =
+        openssl::rsa::Rsa::from_public_components(n.to_owned().unwrap(), e.to_owned().unwrap())
+            .expect("creating hard-coded RSA public key instance failed");
+    let private = openssl::rsa::Rsa::from_private_components(n, e, d, p, q, dmp1, dmq1, iqmp)
+        .expect("creating hard-coded RSA key instance failed");
+
+    let passphrase = || -> Result<Vec<u8>, Error> { Ok(Vec::new()) };
+
+    let key = KeyConfig {
+        kdf: None,
+        created: proxmox::tools::time::epoch_i64(),
+        modified: proxmox::tools::time::epoch_i64(),
+        data: (0u8..32u8).collect(),
+        fingerprint: Some(Fingerprint::new([
+            14, 171, 212, 70, 11, 110, 185, 202, 52, 80, 35, 222, 226, 183, 120, 199, 144, 229, 74,
+            22, 131, 185, 101, 156, 10, 87, 174, 25, 144, 144, 21, 155,
+        ])),
+    };
+
+    let encrypted = rsa_encrypt_key_config(public.clone(), &key).expect("encryption failed");
+    let (decrypted, created, fingerprint) =
+        rsa_decrypt_key_config(private.clone(), &encrypted, &passphrase)
+            .expect("decryption failed");
+
+    assert_eq!(key.created, created);
+    assert_eq!(key.data, decrypted);
+    assert_eq!(key.fingerprint, Some(fingerprint));
+
+    Ok(())
+}
-- 
2.20.1

[pbs-devel] [RFC proxmox-backup 7/7] KeyConfig: always calculate fingerprint

[Fabian Grünbichler]

and warn if stored and calculated fingerprint don't match.

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

Notes:
    should not happen in practice, but when it does, it's probably not a good idea
    to display/use the wrong fingerprint..
    
    calculating the fingerprint should be cheap anyway:
    - derive ID key
    - calculate single digest with it

 src/backup/key_derivation.rs | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index a91b21ca..7e8480d3 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -235,13 +235,16 @@ pub fn decrypt_key(
     let mut result = [0u8; 32];
     result.copy_from_slice(&key);
 
-    let fingerprint = match key_config.fingerprint {
-        Some(fingerprint) => fingerprint,
-        None => {
-            let crypt_config = CryptConfig::new(result.clone())?;
-            crypt_config.fingerprint()
-        },
-    };
+    let crypt_config = CryptConfig::new(result.clone())?;
+    let fingerprint = crypt_config.fingerprint();
+    if let Some(stored_fingerprint) = key_config.fingerprint {
+        if fingerprint != stored_fingerprint {
+            eprintln!(
+                "KeyConfig contains wrong fingerprint {}, contained key has fingerprint {}",
+                stored_fingerprint, fingerprint
+            );
+        }
+    }
 
     Ok((result, created, fingerprint))
 }
@@ -313,5 +316,22 @@ fn encrypt_decrypt_test() -> Result<(), Error> {
     assert_eq!(key.data, decrypted);
     assert_eq!(key.fingerprint, Some(fingerprint));
 
+    let key = KeyConfig {
+        kdf: None,
+        created: proxmox::tools::time::epoch_i64(),
+        modified: proxmox::tools::time::epoch_i64(),
+        data: (0u8..32u8).collect(),
+        fingerprint: Some(Fingerprint::new([0u8; 32])), // wrong FP
+    };
+    let encrypted = rsa_encrypt_key_config(public.clone(), &key).expect("encryption failed");
+    let (decrypted, created, fingerprint) =
+        rsa_decrypt_key_config(private.clone(), &encrypted, &passphrase)
+            .expect("decryption failed");
+
+    assert_eq!(key.created, created);
+    assert_eq!(key.data, decrypted);
+    // wrong FP update by round-trip through encrypt/decrypt
+    assert_ne!(key.fingerprint, Some(fingerprint));
+
     Ok(())
 }
-- 
2.20.1

[pbs-devel] applied: [PATCH proxmox-backup 0/7] master key improvements

[Dietmar Maurer]

applied

> On 12/16/2020 2:41 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> this series fixes a bug preventing encrypted-key-restore that got
> introduced with fingerprint checks when restoring files, and further
> improves master key handling by providing an import command that takes a
> restored encrypted key and a private master key to create a PBS keyfile.
> 
> Fabian Grünbichler (7):
>   master key: store blob name in constant
>   fix #3197: skip fingerprint check when restoring key
>   key: move RSA-encryption to KeyConfig
>   client: add 'import-with-master-key' command
>   docs: replace openssl command with client
>   KeyConfig: add encrypt/decrypt test
>   KeyConfig: always calculate fingerprint
> 
>  docs/backup-client.rst               |   6 +-
>  src/backup/crypt_config.rs           |  26 +------
>  src/backup/key_derivation.rs         | 104 +++++++++++++++++++++++++--
>  src/backup/manifest.rs               |   1 +
>  src/bin/proxmox-backup-client.rs     |  33 +++++----
>  src/bin/proxmox_backup_client/key.rs |  95 ++++++++++++++++++++++++
>  6 files changed, 218 insertions(+), 47 deletions(-)
> 
> -- 
> 2.20.1
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] [RFC proxmox-backup 7/7] KeyConfig: always calculate fingerprint

[Dietmar Maurer]

> On 12/16/2020 2:41 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
> 
>  
> and warn if stored and calculated fingerprint don't match.

applied, but I wonder why this is only a warning (should generate an error instead)?

[pbs-devel] applied: [RFC proxmox-backup 7/7] KeyConfig: always calculate fingerprint

[Fabian Grünbichler]

On December 17, 2020 6:55 am, Dietmar Maurer wrote:
> 
>> On 12/16/2020 2:41 PM Fabian Grünbichler <f.gruenbichler@proxmox.com> wrote:
>> 
>>  
>> and warn if stored and calculated fingerprint don't match.
> 
> applied, but I wonder why this is only a warning (should generate an error instead)?

as discussed this morning, switched to bail! and adapted the test cases:

commit c01742855ae95231d0b964bd9434c74d2e9dffd1
Author:     Fabian Grünbichler <f.gruenbichler@proxmox.com>
AuthorDate: Thu Dec 17 10:53:21 2020 +0100
Commit:     Fabian Grünbichler <f.gruenbichler@proxmox.com>
CommitDate: Thu Dec 17 11:27:06 2020 +0100

    KeyConfig: bail on wrong fingerprint
    
    instead of just logging the error. this should never happen in practice
    unless someone is messing with the keyfile, in which case, it's better
    to abort.
    
    update tests accordingly (wrong fingerprint should fail, no fingerprint
    should get the expected one).
    
    Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com>

diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs
index 7e8480d3..a215670a 100644
--- a/src/backup/key_derivation.rs
+++ b/src/backup/key_derivation.rs
@@ -239,7 +239,7 @@ pub fn decrypt_key(
     let fingerprint = crypt_config.fingerprint();
     if let Some(stored_fingerprint) = key_config.fingerprint {
         if fingerprint != stored_fingerprint {
-            eprintln!(
+            bail!(
                 "KeyConfig contains wrong fingerprint {}, contained key has fingerprint {}",
                 stored_fingerprint, fingerprint
             );
@@ -316,6 +316,11 @@ fn encrypt_decrypt_test() -> Result<(), Error> {
     assert_eq!(key.data, decrypted);
     assert_eq!(key.fingerprint, Some(fingerprint));
 
+    Ok(())
+}
+
+#[test]
+fn fingerprint_checks() -> Result<(), Error> {
     let key = KeyConfig {
         kdf: None,
         created: proxmox::tools::time::epoch_i64(),
@@ -323,15 +328,30 @@ fn encrypt_decrypt_test() -> Result<(), Error> {
         data: (0u8..32u8).collect(),
         fingerprint: Some(Fingerprint::new([0u8; 32])), // wrong FP
     };
-    let encrypted = rsa_encrypt_key_config(public.clone(), &key).expect("encryption failed");
-    let (decrypted, created, fingerprint) =
-        rsa_decrypt_key_config(private.clone(), &encrypted, &passphrase)
-            .expect("decryption failed");
 
+    let expected_fingerprint = Fingerprint::new([
+            14, 171, 212, 70, 11, 110, 185, 202, 52, 80, 35, 222, 226, 183, 120, 199, 144, 229, 74,
+            22, 131, 185, 101, 156, 10, 87, 174, 25, 144, 144, 21, 155,
+        ]);
+
+    let mut data = serde_json::to_vec(&key).expect("encoding KeyConfig failed");
+    decrypt_key(&mut data, &{ || { Ok(Vec::new()) }}).expect_err("decoding KeyConfig with wrong fingerprint worked");
+
+    let key = KeyConfig {
+        kdf: None,
+        created: proxmox::tools::time::epoch_i64(),
+        modified: proxmox::tools::time::epoch_i64(),
+        data: (0u8..32u8).collect(),
+        fingerprint: None,
+    };
+
+
+    let mut data = serde_json::to_vec(&key).expect("encoding KeyConfig failed");
+    let (key_data, created, fingerprint) = decrypt_key(&mut data, &{ || { Ok(Vec::new()) }}).expect("decoding KeyConfig without fingerprint failed");
+
+    assert_eq!(key.data, key_data);
     assert_eq!(key.created, created);
-    assert_eq!(key.data, decrypted);
-    // wrong FP update by round-trip through encrypt/decrypt
-    assert_ne!(key.fingerprint, Some(fingerprint));
+    assert_eq!(expected_fingerprint, fingerprint);
 
     Ok(())
 }