From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id D83E569D56 for ; Wed, 20 Jan 2021 17:13:56 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id CA1DBAA98 for ; Wed, 20 Jan 2021 17:13:56 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [212.186.127.180]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS id 4544BAA8B for ; Wed, 20 Jan 2021 17:13:54 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 12CBC4607B for ; Wed, 20 Jan 2021 17:13:54 +0100 (CET) Date: Wed, 20 Jan 2021 17:13:43 +0100 From: Fabian =?iso-8859-1?q?Gr=FCnbichler?= To: Proxmox Backup Server development discussion References: <20210119120943.19363-1-dietmar@proxmox.com> In-Reply-To: <20210119120943.19363-1-dietmar@proxmox.com> MIME-Version: 1.0 User-Agent: astroid/0.15.0 (https://github.com/astroidmail/astroid) Message-Id: <1611157977.5ji5ovitgo.astroid@nora.none> Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-SPAM-LEVEL: Spam detection results: 0 AWL 0.027 Adjusted score from AWL reputation of From: address KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment RCVD_IN_DNSWL_MED -2.3 Sender listed at https://www.dnswl.org/, medium trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [proxmox-backup-client.rs, key.rs, proxmox.com, drive.rs, mod.rs] Subject: Re: [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 20 Jan 2021 16:13:56 -0000 in general this looks okay, some nits inline, and the following=20 question: does it make sense to split the "public" (misleading naming ;)) and=20 private part in tape_encryption_keys.rs? it seems to me that we=20 access/modify them together, so we might want to make it a single file=20 to avoid running out of sync? i.e., EncryptionKeyInfo could just get the=20 KeyConfig as field as well, and we store a list/map of those.. On January 19, 2021 1:09 pm, Dietmar Maurer wrote: > --- > src/api2/config/tape_encryption_keys.rs | 12 ++-- > src/api2/tape/drive.rs | 18 +++-- > src/api2/types/mod.rs | 7 ++ > src/backup/key_derivation.rs | 17 ++++- > src/bin/proxmox-backup-client.rs | 1 + > src/bin/proxmox_backup_client/key.rs | 88 +++++++++++++++++++------ > src/config/tape_encryption_keys.rs | 41 +++--------- > src/tape/pool_writer.rs | 5 +- > 8 files changed, 122 insertions(+), 67 deletions(-) >=20 > diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/ta= pe_encryption_keys.rs > index 31a4fdfa..c652cbc5 100644 > --- a/src/api2/config/tape_encryption_keys.rs > +++ b/src/api2/config/tape_encryption_keys.rs > @@ -26,6 +26,7 @@ use crate::{ > api2::types::{ > TAPE_ENCRYPTION_KEY_FINGERPRINT_SCHEMA, > PROXMOX_CONFIG_DIGEST_SCHEMA, > + PASSWORD_HINT_SCHEMA, > TapeKeyMetadata, > }, > backup::{ > @@ -57,7 +58,7 @@ pub fn list_keys( > =20 > for (fingerprint, item) in key_map { > list.push(TapeKeyMetadata { > - hint: item.hint, > + hint: item.hint.unwrap_or(String::new()), unwrap_or_default > fingerprint: as_fingerprint(fingerprint.bytes()), > }); > } > @@ -75,9 +76,7 @@ pub fn list_keys( > min_length: 5, > }, > hint: { > - description: "Password restore hint.", > - min_length: 1, > - max_length: 32, > + schema: PASSWORD_HINT_SCHEMA, > }, > }, > }, > @@ -92,11 +91,12 @@ pub fn create_key( > _rpcenv: &mut dyn RpcEnvironment > ) -> Result { > =20 > - let (key, key_config) =3D generate_tape_encryption_key(password.as_b= ytes())?; > + let (key, mut key_config) =3D generate_tape_encryption_key(password.= as_bytes())?; we could pass the hint to encrypt_key_with_passphrase (via generate_tape_en= cryption_key), since hints and KDF go hand-in-hand.. > + key_config.hint =3D Some(hint); > =20 > let fingerprint =3D key_config.fingerprint.clone().unwrap(); > =20 > - insert_key(key, key_config, hint)?; > + insert_key(key, key_config)?; > =20 > Ok(fingerprint) > } > diff --git a/src/api2/tape/drive.rs b/src/api2/tape/drive.rs > index 5cf18fe5..2bef55d9 100644 > --- a/src/api2/tape/drive.rs > +++ b/src/api2/tape/drive.rs > @@ -484,11 +484,21 @@ pub async fn restore_key( > let (_media_id, key_config) =3D drive.read_label()?; > =20 > if let Some(key_config) =3D key_config { > - let hint =3D String::from("fixme: add hint"); > - // fixme: howto show restore hint > let password_fn =3D || { Ok(password.as_bytes().to_vec()) }; > - let (key, ..) =3D decrypt_key_config(&key_config, &password_= fn)?; > - config::tape_encryption_keys::insert_key(key, key_config, hi= nt)?; > + let key =3D match decrypt_key_config(&key_config, &password_= fn) { > + Ok((key, ..)) =3D> key, > + Err(_) =3D> { > + match key_config.hint { > + Some(hint) =3D> { > + bail!("decrypt key failed (password hint: {}= )", hint); > + } > + None =3D> { > + bail!("decrypt key failed (wrong password)")= ; > + } this could move into decrypt_key_config, then we have a single place to=20 handle it.. > + } > + } > + }; > + config::tape_encryption_keys::insert_key(key, key_config)?; > } else { > bail!("media does not contain any encryption key configurati= on"); > } > diff --git a/src/api2/types/mod.rs b/src/api2/types/mod.rs > index 71875324..07289ca1 100644 > --- a/src/api2/types/mod.rs > +++ b/src/api2/types/mod.rs > @@ -1249,3 +1249,10 @@ pub const DATASTORE_NOTIFY_STRING_SCHEMA: Schema = =3D StringSchema::new( > "Datastore notification setting") > .format(&ApiStringFormat::PropertyString(&DatastoreNotify::API_SCHEM= A)) > .schema(); > + > + > +pub const PASSWORD_HINT_SCHEMA: Schema =3D StringSchema::new("Password h= int.") > + .format(&SINGLE_LINE_COMMENT_FORMAT) > + .min_length(1) > + .max_length(64) > + .schema(); > diff --git a/src/backup/key_derivation.rs b/src/backup/key_derivation.rs > index b0647618..ec338166 100644 > --- a/src/backup/key_derivation.rs > +++ b/src/backup/key_derivation.rs > @@ -94,7 +94,10 @@ pub struct KeyConfig { > #[serde(skip_serializing_if =3D "Option::is_none")] > #[serde(default)] > pub fingerprint: Option, > - } > + /// Password hint > + #[serde(skip_serializing_if =3D "Option::is_none")] > + pub hint: Option, > +} > =20 > pub fn store_key_config( > path: &std::path::Path, > @@ -181,6 +184,7 @@ pub fn encrypt_key_with_passphrase( > modified: created, > data: enc_data, > fingerprint: None, > + hint: None, > }) > } > =20 > @@ -192,6 +196,15 @@ pub fn load_and_decrypt_key( > .with_context(|| format!("failed to load decryption key from {:?= }", path)) > } > =20 > +/// Loads a KeyConfig from path > +pub fn load_key_config( > + path: &std::path::Path, > +) -> Result { > + let keydata =3D file_get_contents(&path)?; > + let key_config: KeyConfig =3D serde_json::from_reader(&keydata[..])?= ; > + Ok(key_config) > +} > + > pub fn decrypt_key_config( > key_config: &KeyConfig, > passphrase: &dyn Fn() -> Result, Error>, > @@ -243,7 +256,7 @@ pub fn decrypt_key_config( > ); > } > } > - =20 > + > Ok((result, key_config.created, fingerprint)) > } > =20 > diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-cl= ient.rs > index 2cd00c2e..bb905cc3 100644 > --- a/src/bin/proxmox-backup-client.rs > +++ b/src/bin/proxmox-backup-client.rs > @@ -929,6 +929,7 @@ async fn create_backup( > modified: proxmox::tools::time::epoch_i64(), > data: key.to_vec(), > fingerprint: Some(fingerprint), > + hint: None, > }; > let enc_key =3D rsa_encrypt_key_config(rsa, &key_con= fig)?; > println!("Master key '{:?}'", path); > diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backu= p_client/key.rs > index 109f0384..8618a6ec 100644 > --- a/src/bin/proxmox_backup_client/key.rs > +++ b/src/bin/proxmox_backup_client/key.rs > @@ -19,19 +19,24 @@ use proxmox::api::router::ReturnType; > use proxmox::sys::linux::tty; > use proxmox::tools::fs::{file_get_contents, replace_file, CreateOptions}= ; > =20 > -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::{ > + api2::types::{ > + PASSWORD_HINT_SCHEMA, > + }, > + backup::{ > + encrypt_key_with_passphrase, > + load_key_config, > + decrypt_key_config, > + rsa_decrypt_key_config, > + store_key_config, > + CryptConfig, > + Kdf, > + KeyConfig, > + KeyDerivationConfig, > + }, > + tools, > }; > =20 > -use proxmox_backup::tools; > - > #[api()] > #[derive(Debug, Serialize, Deserialize)] > #[serde(rename_all =3D "lowercase")] > @@ -99,12 +104,20 @@ pub fn get_encryption_key_password() -> Result, Error> { > description: > "Output file. Without this the key will become the n= ew default encryption key.", > optional: true, > - } > + }, > + hint: { > + schema: PASSWORD_HINT_SCHEMA, > + optional: true, > + }, > }, > }, > )] > /// Create a new encryption key. > -fn create(kdf: Option, path: Option) -> Result<(), Error> { > +fn create( > + kdf: Option, > + path: Option, > + hint: Option > +) -> Result<(), Error> { > let path =3D match path { > Some(path) =3D> PathBuf::from(path), > None =3D> { > @@ -125,6 +138,10 @@ fn create(kdf: Option, path: Option) ->= Result<(), Error> { > Kdf::None =3D> { > let created =3D proxmox::tools::time::epoch_i64(); > =20 > + if hint.is_some() { > + bail!("password hint not allowed for Kdf::None"); > + } could also be just a warning that it is ignored? > + > store_key_config( > &path, > false, > @@ -134,6 +151,7 @@ fn create(kdf: Option, path: Option) -> = Result<(), Error> { > modified: created, > data: key, > fingerprint: Some(crypt_config.fingerprint()), > + hint: None, > }, > )?; > } > @@ -147,6 +165,7 @@ fn create(kdf: Option, path: Option) -> = Result<(), Error> { > =20 > let mut key_config =3D encrypt_key_with_passphrase(&key, &pa= ssword, kdf)?; > key_config.fingerprint =3D Some(crypt_config.fingerprint()); > + key_config.hint =3D hint; could be handled by encrypt_key_with_passphrase > =20 > store_key_config(&path, false, key_config)?; > } > @@ -172,7 +191,11 @@ fn create(kdf: Option, path: Option) ->= Result<(), Error> { > description: > "Output file. Without this the key will become the n= ew default encryption key.", > optional: true, > - } > + }, > + hint: { > + schema: PASSWORD_HINT_SCHEMA, > + optional: true, > + }, > }, > }, > )] > @@ -182,6 +205,7 @@ async fn import_with_master_key( > encrypted_keyfile: String, > kdf: Option, > path: Option, > + hint: Option, > ) -> Result<(), Error> { > let path =3D match path { > Some(path) =3D> PathBuf::from(path), > @@ -213,6 +237,10 @@ async fn import_with_master_key( > Kdf::None =3D> { > let modified =3D proxmox::tools::time::epoch_i64(); > =20 > + if hint.is_some() { > + bail!("password hint not allowed for Kdf::None"); > + } same as above in create > + > store_key_config( > &path, > true, > @@ -222,6 +250,7 @@ async fn import_with_master_key( > modified, > data: key.to_vec(), > fingerprint: Some(fingerprint), > + hint: None, > }, > )?; > } > @@ -231,6 +260,7 @@ async fn import_with_master_key( > let mut new_key_config =3D encrypt_key_with_passphrase(&key,= &password, kdf)?; > new_key_config.created =3D created; // keep original value > new_key_config.fingerprint =3D Some(fingerprint); > + new_key_config.hint =3D hint; same as above > =20 > store_key_config(&path, true, new_key_config)?; > } > @@ -249,12 +279,20 @@ async fn import_with_master_key( > path: { > description: "Key file. Without this the default key's p= assword will be changed.", > optional: true, > - } > + }, > + hint: { > + schema: PASSWORD_HINT_SCHEMA, > + optional: true, > + }, > }, > }, > )] > /// Change the encryption key's password. > -fn change_passphrase(kdf: Option, path: Option) -> Result<(= ), Error> { > +fn change_passphrase( > + kdf: Option, > + path: Option, > + hint: Option, > +) -> Result<(), Error> { > let path =3D match path { > Some(path) =3D> PathBuf::from(path), > None =3D> { > @@ -273,12 +311,17 @@ fn change_passphrase(kdf: Option, path: Option= ) -> Result<(), Error > bail!("unable to change passphrase - no tty"); > } > =20 > - let (key, created, fingerprint) =3D load_and_decrypt_key(&path, &get= _encryption_key_password)?; > + let key_config =3D load_key_config(&path)?; > + let (key, created, fingerprint) =3D decrypt_key_config(&key_config, = &get_encryption_key_password)?; > =20 > match kdf { > Kdf::None =3D> { > let modified =3D proxmox::tools::time::epoch_i64(); > =20 > + if hint.is_some() { > + bail!("password hint not allowed for Kdf::None"); > + } same as above > + > store_key_config( > &path, > true, > @@ -288,6 +331,7 @@ fn change_passphrase(kdf: Option, path: Option) -> Result<(), Error > modified, > data: key.to_vec(), > fingerprint: Some(fingerprint), > + hint: None, > }, > )?; > } > @@ -297,7 +341,7 @@ fn change_passphrase(kdf: Option, path: Option) -> Result<(), Error > let mut new_key_config =3D encrypt_key_with_passphrase(&key,= &password, kdf)?; > new_key_config.created =3D created; // keep original value > new_key_config.fingerprint =3D Some(fingerprint); > - > + new_key_config.hint =3D hint; same as above > store_key_config(&path, true, new_key_config)?; > } > } > @@ -323,7 +367,11 @@ struct KeyInfo { > /// Key modification time > pub modified: i64, > /// Key fingerprint > + #[serde(skip_serializing_if=3D"Option::is_none")] > pub fingerprint: Option, > + /// Password hint > + #[serde(skip_serializing_if=3D"Option::is_none")] > + pub hint: Option, > } > =20 > #[api( > @@ -374,6 +422,7 @@ fn show_key( > Some(ref fp) =3D> Some(format!("{}", fp)), > None =3D> None, > }, > + hint: config.hint, > }; > =20 > let options =3D proxmox::api::cli::default_table_format_options() > @@ -381,7 +430,8 @@ fn show_key( > .column(ColumnConfig::new("kdf")) > .column(ColumnConfig::new("created").renderer(tools::format::ren= der_epoch)) > .column(ColumnConfig::new("modified").renderer(tools::format::re= nder_epoch)) > - .column(ColumnConfig::new("fingerprint")); > + .column(ColumnConfig::new("fingerprint")) > + .column(ColumnConfig::new("hint")); > =20 > let return_type =3D ReturnType::new(false, &KeyInfo::API_SCHEMA); > =20 > diff --git a/src/config/tape_encryption_keys.rs b/src/config/tape_encrypt= ion_keys.rs > index ff565349..13f6961c 100644 > --- a/src/config/tape_encryption_keys.rs > +++ b/src/config/tape_encryption_keys.rs > @@ -1,4 +1,4 @@ > -use std::collections::{HashSet, HashMap}; > +use std::collections::HashMap; > =20 > use anyhow::{bail, Error}; > use serde::{Deserialize, Serialize}; > @@ -53,13 +53,6 @@ pub struct EncryptionKeyInfo { > pub key: [u8; 32], > } > =20 > -/// Store Hardware Encryption keys (public part) > -#[derive(Deserialize, Serialize)] > -pub struct EncryptionKeyConfig { > - pub hint: String, > - pub key_config: KeyConfig, > -} > - > pub fn compute_tape_key_fingerprint(key: &[u8; 32]) -> Result { > let crypt_config =3D CryptConfig::new(key.clone())?; > Ok(crypt_config.fingerprint()) > @@ -81,12 +74,6 @@ impl EncryptionKeyInfo { > } > } > =20 > -impl EncryptionKeyConfig { > - pub fn new(key_config: KeyConfig, hint: String) -> Self { > - Self { hint, key_config } > - } > -} > - > pub const TAPE_KEYS_FILENAME: &str =3D "/etc/proxmox-backup/tape-encrypt= ion-keys.json"; > pub const TAPE_KEY_CONFIG_FILENAME: &str =3D "/etc/proxmox-backup/tape-e= ncryption-key-config.json"; > pub const TAPE_KEYS_LOCKFILE: &str =3D "/etc/proxmox-backup/.tape-encryp= tion-keys.lck"; > @@ -122,25 +109,21 @@ pub fn load_keys() -> Result<(HashMap, [u8;32]) > } > =20 > /// Load tape encryption key configurations (public part) > -pub fn load_key_configs() -> Result<(HashMap, [u8;32]), Error> { > +pub fn load_key_configs() -> Result<(HashMap, [= u8;32]), Error> { > =20 > let content =3D file_read_optional_string(TAPE_KEY_CONFIG_FILENAME)?= ; > let content =3D content.unwrap_or_else(|| String::from("[]")); > =20 > let digest =3D openssl::sha::sha256(content.as_bytes()); > =20 > - let key_list: Vec =3D serde_json::from_str(&con= tent)?; > + let key_list: Vec =3D serde_json::from_str(&content)?; > =20 > let mut map =3D HashMap::new(); > - let mut hint_set =3D HashSet::new(); > =20 > - for item in key_list { > - match item.key_config.fingerprint { > + for key_config in key_list { > + match key_config.fingerprint { > Some(ref fingerprint) =3D> { > - if !hint_set.insert(item.hint.clone()) { > - bail!("found duplicate password hint '{}'", item.hin= t); > - } > - if map.insert(fingerprint.clone(), item).is_some() { > + if map.insert(fingerprint.clone(), key_config).is_some()= { > bail!("found duplicate fingerprint"); > } > } > @@ -174,16 +157,11 @@ pub fn save_keys(map: HashMap) -> Result<(), Err > Ok(()) > } > =20 > -pub fn save_key_configs(map: HashMap) = -> Result<(), Error> { > +pub fn save_key_configs(map: HashMap) -> Result<= (), Error> { > =20 > let mut list =3D Vec::new(); > =20 > - let mut hint_set =3D HashSet::new(); > - > for (_fp, item) in map { > - if !hint_set.insert(item.hint.clone()) { > - bail!("found duplicate password hint '{}'", item.hint); > - } > list.push(item); > } > =20 > @@ -203,7 +181,7 @@ pub fn save_key_configs(map: HashMap) -> Resul > Ok(()) > } > =20 > -pub fn insert_key(key: [u8;32], key_config: KeyConfig, hint: String) -> = Result<(), Error> { > +pub fn insert_key(key: [u8;32], key_config: KeyConfig) -> Result<(), Err= or> { > =20 > let _lock =3D open_file_locked( > TAPE_KEYS_LOCKFILE, > @@ -227,8 +205,7 @@ pub fn insert_key(key: [u8;32], key_config: KeyConfig= , hint: String) -> Result<( > key_map.insert(fingerprint.clone(), item); > save_keys(key_map)?; > =20 > - let item =3D EncryptionKeyConfig::new(key_config, hint); > - config_map.insert(fingerprint.clone(), item); > + config_map.insert(fingerprint.clone(), key_config); > save_key_configs(config_map)?; > =20 > Ok(()) > diff --git a/src/tape/pool_writer.rs b/src/tape/pool_writer.rs > index b04bfddb..c085b0b5 100644 > --- a/src/tape/pool_writer.rs > +++ b/src/tape/pool_writer.rs > @@ -456,10 +456,7 @@ fn update_media_set_label( > let key_config =3D if let Some(ref fingerprint) =3D new_set.encrypti= on_key_fingerprint { > let (config_map, _digest) =3D load_key_configs()?; > match config_map.get(fingerprint) { > - Some(item) =3D> { > - // fixme: store item.hint??? should be in key-config ins= tead > - Some(item.key_config.clone()) > - } > + Some(key_config) =3D> Some(key_config.clone()), > None =3D> { > bail!("unable to find tape encryption key config '{}'", = fingerprint); > } > --=20 > 2.20.1 >=20 >=20 > _______________________________________________ > pbs-devel mailing list > pbs-devel@lists.proxmox.com > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel >=20 >=20 >=20 =