From: "Fabian Grünbichler" <f.gruenbichler@proxmox.com>
To: Proxmox Backup Server development discussion
<pbs-devel@lists.proxmox.com>
Subject: Re: [pbs-devel] [RFC proxmox-backup] add "password hint" to KeyConfig
Date: Wed, 20 Jan 2021 17:13:43 +0100 [thread overview]
Message-ID: <1611157977.5ji5ovitgo.astroid@nora.none> (raw)
In-Reply-To: <20210119120943.19363-1-dietmar@proxmox.com>
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
>
>
>
next prev parent reply other threads:[~2021-01-20 16:13 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-01-19 12:09 Dietmar Maurer
2021-01-20 16:13 ` Fabian Grünbichler [this message]
2021-01-20 16:59 ` Dietmar Maurer
2021-01-20 18:20 ` Fabian Grünbichler
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=1611157977.5ji5ovitgo.astroid@nora.none \
--to=f.gruenbichler@proxmox.com \
--cc=pbs-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.