* [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint @ 2020-11-17 17:57 Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 1/7] crypt config: add fingerprint mechanism Fabian Grünbichler ` (7 more replies) 0 siblings, 8 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 UTC (permalink / raw) To: pbs-devel next improvements/follow-ups in this area: - return fingerprint via SnapshotListItem, display somehow in GUI (pending list_snapshot refactor which is waiting for review) - filter snapshot list by fingerprint and/or crypt mode (for GUI, client, PVE)? - display in PVE (storage key fingerprint in storage config view, manifest fingerprint via new attribute mechanism once its returned by list_snapshot?) - postinst to fixup PVE generated keys via change-passphrase - switch libproxmox-backup-qemu to just call crypt_config.fingerprint() longer term ideas: - some sort of keyring? multiple keys in a single keyfile to allow rotation? Fabian Grünbichler (7): 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 src/backup/crypt_config.rs | 17 +++++- src/backup/key_derivation.rs | 23 ++++++-- src/backup/manifest.rs | 33 ++++++++++++ src/bin/proxmox-backup-client.rs | 14 +++-- src/bin/proxmox_backup_client/benchmark.rs | 2 +- src/bin/proxmox_backup_client/catalog.rs | 6 ++- src/bin/proxmox_backup_client/key.rs | 63 ++++++++++++++++++++-- src/bin/proxmox_backup_client/mount.rs | 7 ++- src/tools/format.rs | 58 ++++++++++++++++++++ 9 files changed, 208 insertions(+), 15 deletions(-) -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 1/7] crypt config: add fingerprint mechanism 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config Fabian Grünbichler ` (6 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 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: obviously the input could be whatever, but sizeof(input) >= sizeof(output) seemed like a good idea and we use the same scheme for the magic header strings.. src/backup/crypt_config.rs | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs index 4be728d9..01ab0942 100644 --- a/src/backup/crypt_config.rs +++ b/src/backup/crypt_config.rs @@ -17,6 +17,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")] @@ -101,6 +106,10 @@ impl CryptConfig { tag } + pub fn fingerprint(&self) -> [u8; 32] { + 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] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 1/7] crypt config: add fingerprint mechanism Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-18 8:48 ` Wolfgang Bumiller 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key Fabian Grünbichler ` (5 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 UTC (permalink / raw) To: pbs-devel and set/generate it on - key creation - key passphrase change - key decryption if not already set (not persisted, but returned) - key encryption with master key Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- Notes: 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 | 8 ++- src/backup/key_derivation.rs | 23 +++++++-- 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 | 17 +++++-- src/bin/proxmox_backup_client/mount.rs | 2 +- src/tools/format.rs | 58 ++++++++++++++++++++++ 8 files changed, 105 insertions(+), 15 deletions(-) diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs index 01ab0942..177923b3 100644 --- a/src/backup/crypt_config.rs +++ b/src/backup/crypt_config.rs @@ -228,7 +228,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..dcf18cfa 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; + use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions}; use proxmox::try_block; @@ -66,6 +68,10 @@ 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)] + #[serde(with = "crate::tools::format::opt_sha256_as_fingerprint")] + pub fingerprint: Option<[u8; 32]>, } pub fn store_key_config( @@ -142,13 +148,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, [u8;32]), Error> { do_load_and_decrypt_key(path, passphrase) .with_context(|| format!("failed to load decryption key from {:?}", path)) } @@ -156,14 +163,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, [u8;32]), 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, [u8;32]), Error> { let key_config: KeyConfig = serde_json::from_reader(&mut keydata)?; let raw_data = key_config.data; @@ -203,5 +210,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..915ee970 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; @@ -121,6 +125,9 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> { let kdf = kdf.unwrap_or_default(); let key = proxmox::sys::linux::random_data(32)?; + use std::convert::TryInto; + let key_array: [u8; 32] = key.clone()[0..32].try_into().unwrap(); + let crypt_config = CryptConfig::new(key_array)?; 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..c16559a7 100644 --- a/src/tools/format.rs +++ b/src/tools/format.rs @@ -102,6 +102,64 @@ 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 opt_sha256_as_fingerprint { + use serde::{self, Serializer, Deserializer}; + + pub fn serialize<S>( + csum: &Option<[u8; 32]>, + serializer: S, + ) -> Result<S::Ok, S::Error> + where + S: Serializer, + { + crate::tools::format::sha256_as_fingerprint::serialize(&csum.unwrap(), serializer) + } + + pub fn deserialize<'de, D>( + deserializer: D, + ) -> Result<Option<[u8; 32]>, D::Error> + where + D: Deserializer<'de>, + { + crate::tools::format::sha256_as_fingerprint::deserialize(deserializer) + .map(|digest| Some(digest)) + } +} + +pub mod sha256_as_fingerprint { + use serde::{self, Deserialize, Serializer, Deserializer}; + + pub fn serialize<S>( + csum: &[u8; 32], + serializer: S, + ) -> Result<S::Ok, S::Error> + where + S: Serializer, + { + let s = crate::tools::format::as_fingerprint(csum); + 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] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config Fabian Grünbichler @ 2020-11-18 8:48 ` Wolfgang Bumiller 0 siblings, 0 replies; 17+ messages in thread From: Wolfgang Bumiller @ 2020-11-18 8:48 UTC (permalink / raw) To: Fabian Grünbichler; +Cc: pbs-devel On Tue, Nov 17, 2020 at 06:57:20PM +0100, Fabian Grünbichler wrote: > and set/generate it on > - key creation > - key passphrase change > - key decryption if not already set (not persisted, but returned) > - key encryption with master key > > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> > --- > > Notes: > 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 | 8 ++- > src/backup/key_derivation.rs | 23 +++++++-- > 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 | 17 +++++-- > src/bin/proxmox_backup_client/mount.rs | 2 +- > src/tools/format.rs | 58 ++++++++++++++++++++++ > 8 files changed, 105 insertions(+), 15 deletions(-) > > diff --git a/src/backup/crypt_config.rs b/src/backup/crypt_config.rs > index 01ab0942..177923b3 100644 > --- a/src/backup/crypt_config.rs > +++ b/src/backup/crypt_config.rs > @@ -66,6 +68,10 @@ 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)] > + #[serde(with = "crate::tools::format::opt_sha256_as_fingerprint")] > + pub fingerprint: Option<[u8; 32]>, > } > > pub fn store_key_config( (...) > diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs > index 8b802b3e..915ee970 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; > > @@ -121,6 +125,9 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> { > let kdf = kdf.unwrap_or_default(); > > let key = proxmox::sys::linux::random_data(32)?; This allocates, and calls `fill_with_random_data`. You could shorten this to: let mut key_array = [0u8; 32]; proxmox::sys::linux::random_data(&mut key_array)?; > + use std::convert::TryInto; > + let key_array: [u8; 32] = key.clone()[0..32].try_into().unwrap(); > + let crypt_config = CryptConfig::new(key_array)?; > > match kdf { > Kdf::None => { > diff --git a/src/tools/format.rs b/src/tools/format.rs > index 8fe6aa82..c16559a7 100644 > --- a/src/tools/format.rs > +++ b/src/tools/format.rs > @@ -102,6 +102,64 @@ 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 opt_sha256_as_fingerprint { > + use serde::{self, Serializer, Deserializer}; ^ `self` not needed there > + > + pub fn serialize<S>( > + csum: &Option<[u8; 32]>, > + serializer: S, > + ) -> Result<S::Ok, S::Error> > + where > + S: Serializer, > + { > + crate::tools::format::sha256_as_fingerprint::serialize(&csum.unwrap(), serializer) This unwrap makes skipping `None` a hard requirement, so any user of this without the `#[serde(skip_serializing_if = "Option::is_none")]` attribute will panic. Simply translate `None` to `serialize_none()`: match csum { Some(csum) => super::sha256_as_fingerprint::serialize(csum, serializer), None => serializer.serialize_none(), } (Also note the possible use of `super::` instead of the full `crate::tools::format::` prefix. > + } > + > + pub fn deserialize<'de, D>( > + deserializer: D, > + ) -> Result<Option<[u8; 32]>, D::Error> > + where > + D: Deserializer<'de>, > + { > + crate::tools::format::sha256_as_fingerprint::deserialize(deserializer) > + .map(|digest| Some(digest)) Similarly this fails if there the option type is not skipped and serialized eg. as json `null`. You can go via `Option<String>` here, but unfortunately lose the ability to reuse the existing code from below. match Option::<String>::deserialize(deserializer)? { Some(mut s) => { <code from sha256_as_fingerprint::deserialize>, } None => Ok(None), } I've also played around a bit with making this more general. Not sure we need/want this, but this did work: Assuming a `fn hex_to_bin_exact(hex: &str, out: &mut [u8]) -> Result<(), Error>` being in `proxmox::tools` (same as `hex_to_bin` but writing to `out` expecting the string to have an exactly matching length): ---8<--- pub mod opt_bytes_as_fingerprint { use serde::{Deserialize, Deserializer, Serializer}; pub fn serialize<S, T>(csum: &Option<T>, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer, T: AsRef<[u8]>, { match csum { Some(csum) => super::bytes_as_fingerprint::serialize(csum, serializer), None => serializer.serialize_none(), } } pub fn deserialize<'de, D, T>(deserializer: D) -> Result<Option<T>, D::Error> where D: Deserializer<'de>, T: Default + AsMut<[u8]>, { match Option::<String>::deserialize(deserializer)? { Some(mut s) => { s.retain(|c| c != ':'); let mut out = T::default(); proxmox::tools::hex_to_bin_exact(&s, out.as_mut()) .map_err(serde::de::Error::custom)?; Ok(Some(out)) } None => Ok(None), } } } pub mod bytes_as_fingerprint { use serde::{Deserialize, Serializer, Deserializer}; pub fn serialize<S, T>(csum: &T, serializer: S) -> Result<S::Ok, S::Error> where S: Serializer, T: AsRef<[u8]>, { let s = super::as_fingerprint(csum.as_ref()); serializer.serialize_str(&s) } pub fn deserialize<'de, D, T>(deserializer: D) -> Result<T, D::Error> where D: Deserializer<'de>, T: Default + AsMut<[u8]>, { let mut s = String::deserialize(deserializer)?; s.retain(|c| c != ':'); let mut out = T::default(); proxmox::tools::hex_to_bin_exact(&s, out.as_mut()) .map_err(serde::de::Error::custom)?; Ok(out) } } ^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 1/7] crypt config: add fingerprint mechanism Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-17 18:38 ` Thomas Lamprecht 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 4/7] client: add 'key show' command Fabian Grünbichler ` (4 subsequent siblings) 7 siblings, 1 reply; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 UTC (permalink / raw) To: pbs-devel for operations where it makes sense. Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- src/bin/proxmox-backup-client.rs | 11 +++++++++-- src/bin/proxmox_backup_client/mount.rs | 6 +++++- 2 files changed, 14 insertions(+), 3 deletions(-) diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs index c63c7eb6..54f7911d 100644 --- a/src/bin/proxmox-backup-client.rs +++ b/src/bin/proxmox-backup-client.rs @@ -1069,7 +1069,9 @@ 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: {}", + crate::tools::format::as_fingerprint(&fingerprint)); let crypt_config = CryptConfig::new(key)?; @@ -1078,6 +1080,9 @@ 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 +1385,9 @@ 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: '{}'", + crate::tools::format::as_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..a1c2ed86 100644 --- a/src/bin/proxmox_backup_client/mount.rs +++ b/src/bin/proxmox_backup_client/mount.rs @@ -182,7 +182,11 @@ 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: '{}'", + crate::tools::format::as_fingerprint(&fingerprint)); Some(Arc::new(CryptConfig::new(key)?)) } }; -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key Fabian Grünbichler @ 2020-11-17 18:38 ` Thomas Lamprecht 0 siblings, 0 replies; 17+ messages in thread From: Thomas Lamprecht @ 2020-11-17 18:38 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Fabian Grünbichler On 17.11.20 18:57, Fabian Grünbichler wrote: > for operations where it makes sense. > > Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> > --- > src/bin/proxmox-backup-client.rs | 11 +++++++++-- > src/bin/proxmox_backup_client/mount.rs | 6 +++++- > 2 files changed, 14 insertions(+), 3 deletions(-) > > diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs > index c63c7eb6..54f7911d 100644 > --- a/src/bin/proxmox-backup-client.rs > +++ b/src/bin/proxmox-backup-client.rs > @@ -1069,7 +1069,9 @@ 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: {}", > + crate::tools::format::as_fingerprint(&fingerprint)); > > let crypt_config = CryptConfig::new(key)?; > > @@ -1078,6 +1080,9 @@ 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); nit, above would better fit in the same line as the println! (just as reminder for whoever applies this series, or you if a v2 is warranted for other reasons) > + > (Some(Arc::new(crypt_config)), Some(enc_key)) > } > _ => (Some(Arc::new(crypt_config)), None), > @@ -1380,7 +1385,9 @@ 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: '{}'", > + crate::tools::format::as_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..a1c2ed86 100644 > --- a/src/bin/proxmox_backup_client/mount.rs > +++ b/src/bin/proxmox_backup_client/mount.rs > @@ -182,7 +182,11 @@ 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: '{}'", > + crate::tools::format::as_fingerprint(&fingerprint)); > Some(Arc::new(CryptConfig::new(key)?)) > } > }; > ^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 4/7] client: add 'key show' command 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler ` (2 preceding siblings ...) 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 5/7] add key fingerprint to manifest Fabian Grünbichler ` (3 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 UTC (permalink / raw) To: pbs-devel for (pretty-)printing a keyfile. Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- src/bin/proxmox_backup_client/key.rs | 46 ++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_client/key.rs index 915ee970..9d0951e6 100644 --- a/src/bin/proxmox_backup_client/key.rs +++ b/src/bin/proxmox_backup_client/key.rs @@ -16,6 +16,7 @@ use proxmox_backup::backup::{ store_key_config, CryptConfig, KeyConfig, + KeyDerivationConfig, }; use proxmox_backup::tools; @@ -229,6 +230,46 @@ 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, + } + }, + }, +)] +/// Print the encryption key's metadata. +fn show_key(path: Option<String>) -> 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 + } + }; + + println!("Path: {:?}", path); + let config: KeyConfig = serde_json::from_slice(&file_get_contents(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: {}", crate::tools::format::as_fingerprint(&fp)), + None => println!("Fingerprint: none (legacy key)"), + }; + + Ok(()) +} + #[api( input: { properties: { @@ -348,6 +389,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 +402,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] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 5/7] add key fingerprint to manifest 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler ` (3 preceding siblings ...) 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 4/7] client: add 'key show' command Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 6/7] fix #3139: manifest: check fingerprint when loading with key Fabian Grünbichler ` (2 subsequent siblings) 7 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 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> --- src/backup/manifest.rs | 28 ++++++++++++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs index 51980a07..5922144d 100644 --- a/src/backup/manifest.rs +++ b/src/backup/manifest.rs @@ -223,12 +223,40 @@ 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 = crate::tools::format::as_fingerprint(&crypt_config.fingerprint()); + manifest["unprotected"]["key-fingerprint"] = fingerprint.into(); } let manifest = serde_json::to_string_pretty(&manifest).unwrap().into(); Ok(manifest) } + fn check_fingerprint_value(value: &Value, crypt_config: &CryptConfig) -> Result<(), Error> { + let config_fp = crate::tools::format::as_fingerprint(&crypt_config.fingerprint()); + if *value == config_fp { + Ok(()) + } else { + bail!("wrong key - manifest fingerprint {} does not match key fingerprint '{}'", + value, + config_fp) + } + } + + /// 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> { + match (&self.unprotected["key-fingerprint"], crypt_config) { + (Value::Null, _) => Ok(()), + (manifest_fp, None) => bail!("missing key - manifest was created with key {}", + manifest_fp), + (manifest_fp, Some(crypt_config)) => { + BackupManifest::check_fingerprint_value(manifest_fp, crypt_config) + }, + } + } + /// 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] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 6/7] fix #3139: manifest: check fingerprint when loading with key 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler ` (4 preceding siblings ...) 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 5/7] add key fingerprint to manifest Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 7/7] client: check fingerprint after downloading manifest Fabian Grünbichler 2020-11-18 5:27 ` [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Dietmar Maurer 7 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 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 | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/backup/manifest.rs b/src/backup/manifest.rs index 5922144d..eb204c96 100644 --- a/src/backup/manifest.rs +++ b/src/backup/manifest.rs @@ -265,6 +265,11 @@ 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 { + BackupManifest::check_fingerprint_value(fingerprint, crypt_config)?; + } if signature != expected_signature { bail!("wrong signature in manifest"); } -- 2.20.1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 7/7] client: check fingerprint after downloading manifest 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler ` (5 preceding siblings ...) 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 6/7] fix #3139: manifest: check fingerprint when loading with key Fabian Grünbichler @ 2020-11-17 17:57 ` Fabian Grünbichler 2020-11-18 5:27 ` [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Dietmar Maurer 7 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-17 17:57 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. Signed-off-by: Fabian Grünbichler <f.gruenbichler@proxmox.com> --- src/bin/proxmox-backup-client.rs | 1 + src/bin/proxmox_backup_client/catalog.rs | 2 ++ src/bin/proxmox_backup_client/mount.rs | 1 + 3 files changed, 4 insertions(+) diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.rs index 54f7911d..9299cada 100644 --- a/src/bin/proxmox-backup-client.rs +++ b/src/bin/proxmox-backup-client.rs @@ -1403,6 +1403,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 a1c2ed86..b00892d3 100644 --- a/src/bin/proxmox_backup_client/mount.rs +++ b/src/bin/proxmox_backup_client/mount.rs @@ -216,6 +216,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] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler ` (6 preceding siblings ...) 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 7/7] client: check fingerprint after downloading manifest Fabian Grünbichler @ 2020-11-18 5:27 ` Dietmar Maurer 2020-11-18 5:47 ` Dietmar Maurer 7 siblings, 1 reply; 17+ messages in thread From: Dietmar Maurer @ 2020-11-18 5:27 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Fabian Grünbichler Do we really need/want a 256bit long fingerprint? I thought 64bit (or maybe 32bit) would be large enough? If not, why does it have to be that large? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-18 5:27 ` [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Dietmar Maurer @ 2020-11-18 5:47 ` Dietmar Maurer 2020-11-18 6:47 ` Thomas Lamprecht 0 siblings, 1 reply; 17+ messages in thread From: Dietmar Maurer @ 2020-11-18 5:47 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Fabian Grünbichler > On 11/18/2020 6:27 AM Dietmar Maurer <dietmar@proxmox.com> wrote: > > > Do we really need/want a 256bit long fingerprint? > > I thought 64bit (or maybe 32bit) would be large enough? > If not, why does it have to be that large? some quick math: max keys a user generate in his live: 1024 (2¹⁰) so the likelihood of a 32bit fingerprint clash is W = 1/2^²² (1/4Millions) which is, unlikely, but possible. But with 64bit it is extremely unlikely (1/2⁵⁴). ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-18 5:47 ` Dietmar Maurer @ 2020-11-18 6:47 ` Thomas Lamprecht 2020-11-18 8:27 ` Fabian Grünbichler 0 siblings, 1 reply; 17+ messages in thread From: Thomas Lamprecht @ 2020-11-18 6:47 UTC (permalink / raw) To: Proxmox Backup Server development discussion, Dietmar Maurer, Fabian Grünbichler On 18.11.20 06:47, Dietmar Maurer wrote: >> On 11/18/2020 6:27 AM Dietmar Maurer <dietmar@proxmox.com> wrote: >> >> >> Do we really need/want a 256bit long fingerprint? >> >> I thought 64bit (or maybe 32bit) would be large enough? >> If not, why does it have to be that large? > > some quick math: > > max keys a user generate in his live: 1024 (2¹⁰) > > so the likelihood of a 32bit fingerprint clash is > > W = 1/2^²² (1/4Millions) > > which is, unlikely, but possible. > > But with 64bit it is extremely unlikely (1/2⁵⁴). From a pure user experience I think it could be better to present 8 byte of fingerprint information - as the nerves/stress ratio is probably not to good at times where this is required. 8 byte: "90:A1:CA:44:BE:0B:D4:1C" vs. 32 byte: "90:A1:CA:44:BE:0B:D4:1C:F7:D9:F5:2F:7C:92:DB:69:B2:2A:AF:6A:1C:7A:DB:0C:03:93:3E:EA:95:EC:26:92" I mean, after all, this is rather informal and even if there would be a unlikely collision it does not actually compromises security in any way I can think of. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-18 6:47 ` Thomas Lamprecht @ 2020-11-18 8:27 ` Fabian Grünbichler 2020-11-18 8:54 ` Dietmar Maurer 2020-11-23 7:55 ` Dietmar Maurer 0 siblings, 2 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-18 8:27 UTC (permalink / raw) To: Dietmar Maurer, Proxmox Backup Server development discussion, Thomas Lamprecht On November 18, 2020 7:47 am, Thomas Lamprecht wrote: > On 18.11.20 06:47, Dietmar Maurer wrote: >>> On 11/18/2020 6:27 AM Dietmar Maurer <dietmar@proxmox.com> wrote: >>> >>> >>> Do we really need/want a 256bit long fingerprint? >>> >>> I thought 64bit (or maybe 32bit) would be large enough? >>> If not, why does it have to be that large? >> >> some quick math: >> >> max keys a user generate in his live: 1024 (2¹⁰) >> >> so the likelihood of a 32bit fingerprint clash is >> >> W = 1/2^²² (1/4Millions) >> >> which is, unlikely, but possible. >> >> But with 64bit it is extremely unlikely (1/2⁵⁴). > > From a pure user experience I think it could be better to present 8 byte of fingerprint > information - as the nerves/stress ratio is probably not to good at times where this is > required. > > 8 byte: "90:A1:CA:44:BE:0B:D4:1C" > > vs. > > 32 byte: "90:A1:CA:44:BE:0B:D4:1C:F7:D9:F5:2F:7C:92:DB:69:B2:2A:AF:6A:1C:7A:DB:0C:03:93:3E:EA:95:EC:26:92" > > I mean, after all, this is rather informal and even if there would be a unlikely > collision it does not actually compromises security in any way I can think of. I'd be fine with that, although I think we should probably mention somewhere why we think it's fine to use a truncated hash here: - the actual verification happens via the signature of the manifest - we are talking about your own keys, not keys of other parties that you need to verify via a fingerprint (which is an entirely different problem) - the fingerprint is just used as an automatically/mathematically generated 'name' of the key should we switch it altogether, or just truncate it on display? IMHO for Qemu I'd like to keep the full digest/fingerprint, since there a skipped collision means corrupt backups, not running into the next error and bailing out.. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-18 8:27 ` Fabian Grünbichler @ 2020-11-18 8:54 ` Dietmar Maurer 2020-11-23 7:55 ` Dietmar Maurer 1 sibling, 0 replies; 17+ messages in thread From: Dietmar Maurer @ 2020-11-18 8:54 UTC (permalink / raw) To: Fabian Grünbichler, Proxmox Backup Server development discussion, Thomas Lamprecht > should we switch it altogether, or just truncate it on display? i would switch to 8 bytes. > IMHO for > Qemu I'd like to keep the full digest/fingerprint, since there a > skipped collision means corrupt backups, not running into the next > error and bailing out.. There are no collission (very very very very unlikely) ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-18 8:27 ` Fabian Grünbichler 2020-11-18 8:54 ` Dietmar Maurer @ 2020-11-23 7:55 ` Dietmar Maurer 2020-11-23 8:16 ` Fabian Grünbichler 1 sibling, 1 reply; 17+ messages in thread From: Dietmar Maurer @ 2020-11-23 7:55 UTC (permalink / raw) To: Fabian Grünbichler, Proxmox Backup Server development discussion, Thomas Lamprecht > should we switch it altogether, or just truncate it on display? IMHO for > Qemu I'd like to keep the full digest/fingerprint, since there a > skipped collision means corrupt backups, not running into the next > error and bailing out.. Just noticed that is a different use case, where we need to be exact. To be 100% sure, we would even need to compare the key raw data. But yes, we want to avoid keeping the old key in memory). But we already have code there to do it correctly, so why do you thing an 8byte fingerprint affects that at all? see proxmox-backup-qemu commit 5a82749a29821bae756bb8c25dc459a3c08301d1 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint 2020-11-23 7:55 ` Dietmar Maurer @ 2020-11-23 8:16 ` Fabian Grünbichler 0 siblings, 0 replies; 17+ messages in thread From: Fabian Grünbichler @ 2020-11-23 8:16 UTC (permalink / raw) To: Dietmar Maurer, Proxmox Backup Server development discussion, Thomas Lamprecht On November 23, 2020 8:55 am, Dietmar Maurer wrote: >> should we switch it altogether, or just truncate it on display? IMHO for >> Qemu I'd like to keep the full digest/fingerprint, since there a >> skipped collision means corrupt backups, not running into the next >> error and bailing out.. > > Just noticed that is a different use case, where we need to be exact. To be > 100% sure, we would even need to compare the key raw data. But yes, we want > to avoid keeping the old key in memory). > > But we already have code there to do it correctly, so why do you thing > an 8byte fingerprint affects that at all? > > see proxmox-backup-qemu commit 5a82749a29821bae756bb8c25dc459a3c08301d1 > I did that change ;) I meant that we can switch that over to just use the fingerprint() function in CryptConfig, but obviously not if that returns some collision-prone, shortened version.. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2020-11-23 8:17 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-11-17 17:57 [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 1/7] crypt config: add fingerprint mechanism Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config Fabian Grünbichler 2020-11-18 8:48 ` Wolfgang Bumiller 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 3/7] client: print key fingerprint and master key Fabian Grünbichler 2020-11-17 18:38 ` Thomas Lamprecht 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 4/7] client: add 'key show' command Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 5/7] add key fingerprint to manifest Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 6/7] fix #3139: manifest: check fingerprint when loading with key Fabian Grünbichler 2020-11-17 17:57 ` [pbs-devel] [PATCH proxmox-backup 7/7] client: check fingerprint after downloading manifest Fabian Grünbichler 2020-11-18 5:27 ` [pbs-devel] [PATCH proxmox-backup 0/7] add, persist and check key fingerprint Dietmar Maurer 2020-11-18 5:47 ` Dietmar Maurer 2020-11-18 6:47 ` Thomas Lamprecht 2020-11-18 8:27 ` Fabian Grünbichler 2020-11-18 8:54 ` Dietmar Maurer 2020-11-23 7:55 ` Dietmar Maurer 2020-11-23 8:16 ` Fabian Grünbichler
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox