* [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
@ 2021-01-19 12:09 Dietmar Maurer
2021-01-20 16:13 ` Fabian Grünbichler
0 siblings, 1 reply; 4+ messages in thread
From: Dietmar Maurer @ 2021-01-19 12:09 UTC (permalink / raw)
To: pbs-devel
---
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()),
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())?;
+ 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)");
+ }
+ }
+ }
+ };
+ 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");
+ }
+
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;
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");
+ }
+
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;
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");
+ }
+
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;
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
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
2021-01-19 12:09 [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig Dietmar Maurer
@ 2021-01-20 16:13 ` Fabian Grünbichler
2021-01-20 16:59 ` Dietmar Maurer
0 siblings, 1 reply; 4+ messages in thread
From: Fabian Grünbichler @ 2021-01-20 16:13 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
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@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
2021-01-20 16:13 ` Fabian Grünbichler
@ 2021-01-20 16:59 ` Dietmar Maurer
2021-01-20 18:20 ` Fabian Grünbichler
0 siblings, 1 reply; 4+ messages in thread
From: Dietmar Maurer @ 2021-01-20 16:59 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Fabian Grünbichler
> 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..
I have done the split to protect the private part (only accessible by root).
The public part can be accessed (and even modified (change password) by user backup).
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
2021-01-20 16:59 ` Dietmar Maurer
@ 2021-01-20 18:20 ` Fabian Grünbichler
0 siblings, 0 replies; 4+ messages in thread
From: Fabian Grünbichler @ 2021-01-20 18:20 UTC (permalink / raw)
To: Dietmar Maurer, Proxmox Backup Server development discussion
> Dietmar Maurer <dietmar@proxmox.com> hat am 20.01.2021 17:59 geschrieben:
>
>
> > 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..
>
> I have done the split to protect the private part (only accessible by root).
> The public part can be accessed (and even modified (change password) by user backup).
not sure whether that's appropriate - the KeyConfig part is just as well-protected as the user's password choice, so unless there is a reason why this *has* to be accessible by user backup I'd make it root only as well.. anything actually using the key needs to run as root anyhow, so the password change can as well..
there is no public (as in, I don't care who can access this) and private part. there is an unprotected and a semi-protected encoding of the same secret..
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2021-01-20 18:20 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-01-19 12:09 [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig Dietmar Maurer
2021-01-20 16:13 ` Fabian Grünbichler
2021-01-20 16:59 ` Dietmar Maurer
2021-01-20 18:20 ` 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