Re: [pbs-devel] [PATCH proxmox-backup 2/7] key: add fingerprint to key config

[Wolfgang Bumiller <w.bumiller@proxmox.com>]

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)
    }
}