Re: [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys

[Max Carrara <m.carrara@proxmox.com>]

On 2/15/24 16:20, Stefan Sterz wrote:
> this commit moves new installations from our default rsa keys toward
> smaller and more efficient ec keys. this uses the `PrivateKey` and
> `PublicKey` structs from proxmox-auth-api to handle generating the
> keys.
> 
> this means we can move aways from using openssl directly in the
> auth_helpers and instead rely on the implementation in
> `proxmox-auth-api`. thus, further unifying key handling in
> `proxmox-auth-api`. this should make it easier to switch keys in the
> future if necessary.
> 
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> note that this breaks the following scenario:
> 
> - a user installs pbs from a version after this patch was packaged
> - proxmox-backup then creates a new ed25519 authkey
> - the user manually forces a downgrade
> 
> proxmox-backup-api and proxmox-backup-proxy will now fail to start as
> they cannot read the, from their perspective, malformed authkey.

Can we not ensure we remain forward-compatible here, too? I acknowledge
that we probably shouldn't support this kind of scenario, but if we
were to add support for ed25519 keys now (while continuing to use RSA)
we wouldn't ever run into problems in that regard.

Not sure if it actually makes sense to remain forward-compatible here,
though.

> 
>  src/auth.rs         |  4 ++--
>  src/auth_helpers.rs | 53 ++++++++++++++-------------------------------
>  2 files changed, 18 insertions(+), 39 deletions(-)
> 
> diff --git a/src/auth.rs b/src/auth.rs
> index 3379577f..20d2e39f 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -262,9 +262,9 @@ pub(crate) fn authenticate_user<'a>(
>  }
> 
>  static PRIVATE_KEYRING: Lazy<Keyring> =
> -    Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone().into()));
> +    Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone()));
>  static PUBLIC_KEYRING: Lazy<Keyring> =
> -    Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone().into()));
> +    Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone()));
>  static AUTH_CONTEXT: OnceCell<PbsAuthContext> = OnceCell::new();
> 
>  pub fn setup_auth_context(use_private_key: bool) {
> diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
> index 375ce190..f518c2ee 100644
> --- a/src/auth_helpers.rs
> +++ b/src/auth_helpers.rs
> @@ -3,12 +3,9 @@ use std::path::PathBuf;
>  use std::sync::OnceLock;
> 
>  use anyhow::{format_err, Error};
> -use lazy_static::lazy_static;
> -use openssl::pkey::{PKey, Private, Public};
> -use openssl::rsa::Rsa;
> 
>  use pbs_config::BackupLockGuard;
> -use proxmox_auth_api::HMACKey;
> +use proxmox_auth_api::{HMACKey, PrivateKey, PublicKey};
>  use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
> 
>  use pbs_buildcfg::configdir;
> @@ -87,26 +84,22 @@ pub fn generate_auth_key() -> Result<(), Error> {
>          return Ok(());
>      }
> 
> -    let rsa = Rsa::generate(4096).unwrap();
> -
> -    let priv_pem = rsa.private_key_to_pem()?;
> +    let key = proxmox_auth_api::PrivateKey::generate_ec()?;
> 
>      use nix::sys::stat::Mode;
> 
>      replace_file(
>          &priv_path,
> -        &priv_pem,
> +        &key.private_key_to_pem()?,
>          CreateOptions::new().perm(Mode::from_bits_truncate(0o0600)),
>          true,
>      )?;
> 
> -    let public_pem = rsa.public_key_to_pem()?;
> -
>      let backup_user = pbs_config::backup_user()?;
> 
>      replace_file(
>          &public_path,
> -        &public_pem,
> +        &key.public_key_to_pem()?,
>          CreateOptions::new()
>              .perm(Mode::from_bits_truncate(0o0640))
>              .owner(nix::unistd::ROOT)
> @@ -134,36 +127,22 @@ pub fn csrf_secret() -> &'static HMACKey {
>      })
>  }
> 
> -fn load_public_auth_key() -> Result<PKey<Public>, Error> {
> -    let pem = file_get_contents(configdir!("/authkey.pub"))?;
> -    let rsa = Rsa::public_key_from_pem(&pem)?;
> -    let key = PKey::from_rsa(rsa)?;
> -
> -    Ok(key)
> -}
> -
> -pub fn public_auth_key() -> &'static PKey<Public> {
> -    lazy_static! {
> -        static ref KEY: PKey<Public> = load_public_auth_key().unwrap();
> -    }
> -
> -    &KEY
> -}
> -
> -fn load_private_auth_key() -> Result<PKey<Private>, Error> {
> -    let pem = file_get_contents(configdir!("/authkey.key"))?;
> -    let rsa = Rsa::private_key_from_pem(&pem)?;
> -    let key = PKey::from_rsa(rsa)?;
> +pub fn public_auth_key() -> &'static PublicKey {
> +    static KEY: OnceLock<PublicKey> = OnceLock::new();
> 
> -    Ok(key)
> +    KEY.get_or_init(|| {
> +        let pem = file_get_contents(configdir!("/authkey.pub")).unwrap();
> +        PublicKey::from_pem(&pem).unwrap()
> +    })
>  }
> 
> -pub fn private_auth_key() -> &'static PKey<Private> {
> -    lazy_static! {
> -        static ref KEY: PKey<Private> = load_private_auth_key().unwrap();
> -    }
> +pub fn private_auth_key() -> &'static PrivateKey {
> +    static KEY: OnceLock<PrivateKey> = OnceLock::new();
> 
> -    &KEY
> +    KEY.get_or_init(|| {
> +        let pem = file_get_contents(configdir!("/authkey.key")).unwrap();
> +        PrivateKey::from_pem(&pem).unwrap()
> +    })
>  }
> 
>  const LDAP_PASSWORDS_FILENAME: &str = configdir!("/ldap_passwords.json");
> --
> 2.39.2
> 
> 
> 
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
> 
>