[pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
Fabian Grünbichler
f.gruenbichler at proxmox.com
Wed Jan 20 17:13:43 CET 2021
in general this looks okay, some nits inline, and the following
question:
does it make sense to split the "public" (misleading naming ;)) and
private part in tape_encryption_keys.rs? it seems to me that we
access/modify them together, so we might want to make it a single file
to avoid running out of sync? i.e., EncryptionKeyInfo could just get the
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(-)
>
> diff --git a/src/api2/config/tape_encryption_keys.rs b/src/api2/config/tape_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(
>
> 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<Fingerprint, Error> {
>
> - let (key, key_config) = generate_tape_encryption_key(password.as_bytes())?;
> + let (key, mut key_config) = generate_tape_encryption_key(password.as_bytes())?;
we could pass the hint to encrypt_key_with_passphrase (via generate_tape_encryption_key), since hints and KDF go hand-in-hand..
> + key_config.hint = Some(hint);
>
> let fingerprint = key_config.fingerprint.clone().unwrap();
>
> - insert_key(key, key_config, hint)?;
> + insert_key(key, key_config)?;
>
> 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) = drive.read_label()?;
>
> if let Some(key_config) = key_config {
> - let hint = String::from("fixme: add hint");
> - // fixme: howto show restore hint
> let password_fn = || { Ok(password.as_bytes().to_vec()) };
> - let (key, ..) = decrypt_key_config(&key_config, &password_fn)?;
> - config::tape_encryption_keys::insert_key(key, key_config, hint)?;
> + let key = match decrypt_key_config(&key_config, &password_fn) {
> + Ok((key, ..)) => key,
> + Err(_) => {
> + match key_config.hint {
> + Some(hint) => {
> + bail!("decrypt key failed (password hint: {})", hint);
> + }
> + None => {
> + bail!("decrypt key failed (wrong password)");
> + }
this could move into decrypt_key_config, then we have a single place to
handle it..
> + }
> + }
> + };
> + config::tape_encryption_keys::insert_key(key, key_config)?;
> } else {
> bail!("media does not contain any encryption key configuration");
> }
> 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 = StringSchema::new(
> "Datastore notification setting")
> .format(&ApiStringFormat::PropertyString(&DatastoreNotify::API_SCHEMA))
> .schema();
> +
> +
> +pub const PASSWORD_HINT_SCHEMA: Schema = StringSchema::new("Password hint.")
> + .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 = "Option::is_none")]
> #[serde(default)]
> pub fingerprint: Option<Fingerprint>,
> - }
> + /// Password hint
> + #[serde(skip_serializing_if = "Option::is_none")]
> + pub hint: Option<String>,
> +}
>
> 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,
> })
> }
>
> @@ -192,6 +196,15 @@ pub fn load_and_decrypt_key(
> .with_context(|| format!("failed to load decryption key from {:?}", path))
> }
>
> +/// Loads a KeyConfig from path
> +pub fn load_key_config(
> + path: &std::path::Path,
> +) -> Result<KeyConfig, Error> {
> + let keydata = file_get_contents(&path)?;
> + let key_config: KeyConfig = serde_json::from_reader(&keydata[..])?;
> + Ok(key_config)
> +}
> +
> pub fn decrypt_key_config(
> key_config: &KeyConfig,
> passphrase: &dyn Fn() -> Result<Vec<u8>, Error>,
> @@ -243,7 +256,7 @@ pub fn decrypt_key_config(
> );
> }
> }
> -
> +
> Ok((result, key_config.created, fingerprint))
> }
>
> diff --git a/src/bin/proxmox-backup-client.rs b/src/bin/proxmox-backup-client.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 = rsa_encrypt_key_config(rsa, &key_config)?;
> println!("Master key '{:?}'", path);
> diff --git a/src/bin/proxmox_backup_client/key.rs b/src/bin/proxmox_backup_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};
>
> -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,
> };
>
> -use proxmox_backup::tools;
> -
> #[api()]
> #[derive(Debug, Serialize, Deserialize)]
> #[serde(rename_all = "lowercase")]
> @@ -99,12 +104,20 @@ pub fn get_encryption_key_password() -> Result<Vec<u8>, Error> {
> description:
> "Output file. Without this the key will become the new default encryption key.",
> optional: true,
> - }
> + },
> + hint: {
> + schema: PASSWORD_HINT_SCHEMA,
> + optional: true,
> + },
> },
> },
> )]
> /// Create a new encryption key.
> -fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
> +fn create(
> + kdf: Option<Kdf>,
> + path: Option<String>,
> + hint: Option<String>
> +) -> Result<(), Error> {
> let path = match path {
> Some(path) => PathBuf::from(path),
> None => {
> @@ -125,6 +138,10 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
> Kdf::None => {
> let created = proxmox::tools::time::epoch_i64();
>
> + 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<Kdf>, path: Option<String>) -> Result<(), Error> {
> modified: created,
> data: key,
> fingerprint: Some(crypt_config.fingerprint()),
> + hint: None,
> },
> )?;
> }
> @@ -147,6 +165,7 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
>
> let mut key_config = encrypt_key_with_passphrase(&key, &password, kdf)?;
> key_config.fingerprint = Some(crypt_config.fingerprint());
> + key_config.hint = hint;
could be handled by encrypt_key_with_passphrase
>
> store_key_config(&path, false, key_config)?;
> }
> @@ -172,7 +191,11 @@ fn create(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
> description:
> "Output file. Without this the key will become the new 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<Kdf>,
> path: Option<String>,
> + hint: Option<String>,
> ) -> Result<(), Error> {
> let path = match path {
> Some(path) => PathBuf::from(path),
> @@ -213,6 +237,10 @@ async fn import_with_master_key(
> Kdf::None => {
> let modified = proxmox::tools::time::epoch_i64();
>
> + 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 = encrypt_key_with_passphrase(&key, &password, kdf)?;
> new_key_config.created = created; // keep original value
> new_key_config.fingerprint = Some(fingerprint);
> + new_key_config.hint = hint;
same as above
>
> 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 password will be changed.",
> optional: true,
> - }
> + },
> + hint: {
> + schema: PASSWORD_HINT_SCHEMA,
> + optional: true,
> + },
> },
> },
> )]
> /// Change the encryption key's password.
> -fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error> {
> +fn change_passphrase(
> + kdf: Option<Kdf>,
> + path: Option<String>,
> + hint: Option<String>,
> +) -> Result<(), Error> {
> let path = match path {
> Some(path) => PathBuf::from(path),
> None => {
> @@ -273,12 +311,17 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
> bail!("unable to change passphrase - no tty");
> }
>
> - let (key, created, fingerprint) = load_and_decrypt_key(&path, &get_encryption_key_password)?;
> + let key_config = load_key_config(&path)?;
> + let (key, created, fingerprint) = decrypt_key_config(&key_config, &get_encryption_key_password)?;
>
> match kdf {
> Kdf::None => {
> let modified = proxmox::tools::time::epoch_i64();
>
> + 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<Kdf>, path: Option<String>) -> Result<(), Error
> modified,
> data: key.to_vec(),
> fingerprint: Some(fingerprint),
> + hint: None,
> },
> )?;
> }
> @@ -297,7 +341,7 @@ fn change_passphrase(kdf: Option<Kdf>, path: Option<String>) -> Result<(), Error
> 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);
> -
> + new_key_config.hint = 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="Option::is_none")]
> pub fingerprint: Option<String>,
> + /// Password hint
> + #[serde(skip_serializing_if="Option::is_none")]
> + pub hint: Option<String>,
> }
>
> #[api(
> @@ -374,6 +422,7 @@ fn show_key(
> Some(ref fp) => Some(format!("{}", fp)),
> None => None,
> },
> + hint: config.hint,
> };
>
> let options = 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::render_epoch))
> .column(ColumnConfig::new("modified").renderer(tools::format::render_epoch))
> - .column(ColumnConfig::new("fingerprint"));
> + .column(ColumnConfig::new("fingerprint"))
> + .column(ColumnConfig::new("hint"));
>
> let return_type = ReturnType::new(false, &KeyInfo::API_SCHEMA);
>
> diff --git a/src/config/tape_encryption_keys.rs b/src/config/tape_encryption_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;
>
> use anyhow::{bail, Error};
> use serde::{Deserialize, Serialize};
> @@ -53,13 +53,6 @@ pub struct EncryptionKeyInfo {
> pub key: [u8; 32],
> }
>
> -/// 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<Fingerprint, Error> {
> let crypt_config = CryptConfig::new(key.clone())?;
> Ok(crypt_config.fingerprint())
> @@ -81,12 +74,6 @@ impl EncryptionKeyInfo {
> }
> }
>
> -impl EncryptionKeyConfig {
> - pub fn new(key_config: KeyConfig, hint: String) -> Self {
> - Self { hint, key_config }
> - }
> -}
> -
> pub const TAPE_KEYS_FILENAME: &str = "/etc/proxmox-backup/tape-encryption-keys.json";
> pub const TAPE_KEY_CONFIG_FILENAME: &str = "/etc/proxmox-backup/tape-encryption-key-config.json";
> pub const TAPE_KEYS_LOCKFILE: &str = "/etc/proxmox-backup/.tape-encryption-keys.lck";
> @@ -122,25 +109,21 @@ pub fn load_keys() -> Result<(HashMap<Fingerprint, EncryptionKeyInfo>, [u8;32])
> }
>
> /// Load tape encryption key configurations (public part)
> -pub fn load_key_configs() -> Result<(HashMap<Fingerprint, EncryptionKeyConfig>, [u8;32]), Error> {
> +pub fn load_key_configs() -> Result<(HashMap<Fingerprint, KeyConfig>, [u8;32]), Error> {
>
> let content = file_read_optional_string(TAPE_KEY_CONFIG_FILENAME)?;
> let content = content.unwrap_or_else(|| String::from("[]"));
>
> let digest = openssl::sha::sha256(content.as_bytes());
>
> - let key_list: Vec<EncryptionKeyConfig> = serde_json::from_str(&content)?;
> + let key_list: Vec<KeyConfig> = serde_json::from_str(&content)?;
>
> let mut map = HashMap::new();
> - let mut hint_set = HashSet::new();
>
> - for item in key_list {
> - match item.key_config.fingerprint {
> + for key_config in key_list {
> + match key_config.fingerprint {
> Some(ref fingerprint) => {
> - if !hint_set.insert(item.hint.clone()) {
> - bail!("found duplicate password hint '{}'", item.hint);
> - }
> - 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<Fingerprint, EncryptionKeyInfo>) -> Result<(), Err
> Ok(())
> }
>
> -pub fn save_key_configs(map: HashMap<Fingerprint, EncryptionKeyConfig>) -> Result<(), Error> {
> +pub fn save_key_configs(map: HashMap<Fingerprint, KeyConfig>) -> Result<(), Error> {
>
> let mut list = Vec::new();
>
> - let mut hint_set = HashSet::new();
> -
> for (_fp, item) in map {
> - if !hint_set.insert(item.hint.clone()) {
> - bail!("found duplicate password hint '{}'", item.hint);
> - }
> list.push(item);
> }
>
> @@ -203,7 +181,7 @@ pub fn save_key_configs(map: HashMap<Fingerprint, EncryptionKeyConfig>) -> Resul
> Ok(())
> }
>
> -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<(), Error> {
>
> let _lock = 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)?;
>
> - let item = EncryptionKeyConfig::new(key_config, hint);
> - config_map.insert(fingerprint.clone(), item);
> + config_map.insert(fingerprint.clone(), key_config);
> save_key_configs(config_map)?;
>
> 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 = if let Some(ref fingerprint) = new_set.encryption_key_fingerprint {
> let (config_map, _digest) = load_key_configs()?;
> match config_map.get(fingerprint) {
> - Some(item) => {
> - // fixme: store item.hint??? should be in key-config instead
> - Some(item.key_config.clone())
> - }
> + Some(key_config) => Some(key_config.clone()),
> None => {
> bail!("unable to find tape encryption key config '{}'", fingerprint);
> }
> --
> 2.20.1
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel at lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
More information about the pbs-devel
mailing list