Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key

[Esi Y <esiy0676+proxmox@gmail.com>]

This might be a bit nitpicky, but wouldn't this exactly be an opportunity to properly introduce traits, so that in the future on the same kind of swapout, it is more streamlined and less regression prone going forward? Just wondering ... if there was a reason not to, on such a rewrite.

On Thu, Feb 15, 2024 at 04:19:50PM +0100, Stefan Sterz wrote:
> this commit moves the current ticket signing code into the private key
> implementation. the upside is that the caller does not need to deal
> with openssl's `Signer` directly. it also simplifies and unifies the
> code by using the same helper for verifying a signature and creating it.
> 
> also derive `Clone` on `PrivateKey` and `PublicKey`. as they are
> essentially thin wrappers around `openssl::pkey::PKey<Private>` and
> `openssl::pkey::PKey<Public>`, which can be cloned, deriving `Clone`
> just makes them easier to use.
> 
> Signed-off-by: Stefan Sterz <s.sterz at proxmox.com>
> ---
>  proxmox-auth-api/src/auth_key.rs | 26 ++++++++++++++++----------
>  proxmox-auth-api/src/ticket.rs   | 21 ++-------------------
>  2 files changed, 18 insertions(+), 29 deletions(-)
> 
> diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs
> index cec7360..32120a3 100644
> --- a/proxmox-auth-api/src/auth_key.rs
> +++ b/proxmox-auth-api/src/auth_key.rs
> @@ -9,11 +9,13 @@ use openssl::rsa::Rsa;
>  use openssl::sign::{Signer, Verifier};
>  
>  /// A private auth key used for API ticket signing and verification.
> +#[derive(Clone)]
>  pub struct PrivateKey {
>      pub(crate) key: PKey<Private>,
>  }
>  
>  /// A private auth key used for API ticket verification.
> +#[derive(Clone)]
>  pub struct PublicKey {
>      pub(crate) key: PKey<Public>,
>  }
> @@ -88,6 +90,13 @@ impl PrivateKey {
>      pub fn public_key(&self) -> Result<PublicKey, Error> {
>          PublicKey::from_pem(&self.public_key_to_pem()?)
>      }
> +
> +    pub(self) fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result<Vec<u8>, Error> {
> +        Signer::new(digest, &self.key)
> +            .map_err(|e| format_err!("could not create private key signer - {e}"))?
> +            .sign_oneshot_to_vec(data)
> +            .map_err(|e| format_err!("could not sign with private key - {e}"))
> +    }
>  }
>  
>  impl From<PKey<Private>> for PrivateKey {
> @@ -204,15 +213,12 @@ impl Keyring {
>          Ok(false)
>      }
>  
> -    pub(crate) fn signer(&self, digest: MessageDigest) -> Result<Signer, Error> {
> -        Signer::new(
> -            digest,
> -            &self
> -                .signing_key
> -                .as_ref()
> -                .ok_or_else(|| format_err!("no private key available for signing"))?
> -                .key,
> -        )
> -        .map_err(|err| format_err!("failed to create openssl signer - {err}"))
> +    pub(crate) fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result<Vec<u8>, Error> {
> +        let key = self
> +            .signing_key
> +            .as_ref()
> +            .ok_or_else(|| format_err!("no private key available for signing"))?;
> +
> +        key.sign(digest, data)
>      }
>  }
> diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs
> index 1737912..81054f8 100644
> --- a/proxmox-auth-api/src/ticket.rs
> +++ b/proxmox-auth-api/src/ticket.rs
> @@ -116,27 +116,10 @@ where
>      /// Sign the ticket.
>      pub fn sign(&mut self, keyring: &Keyring, aad: Option<&str>) -> Result<String, Error> {
>          let mut output = self.ticket_data();
> -        let mut signer = keyring.signer(MessageDigest::sha256())?;
> -
> -        signer
> -            .update(output.as_bytes())
> -            .map_err(Error::from)
> -            .and_then(|()| {
> -                if let Some(aad) = aad {
> -                    signer
> -                        .update(b":")
> -                        .and_then(|()| signer.update(aad.as_bytes()))
> -                        .map_err(Error::from)
> -                } else {
> -                    Ok::<_, Error>(())
> -                }
> -            })
> +        let signature = keyring
> +            .sign(MessageDigest::sha256(), &self.verification_data(aad))
>              .map_err(|err| format_err!("error signing ticket: {}", err))?;
>  
> -        let signature = signer
> -            .sign_to_vec()
> -            .map_err(|err| format_err!("error finishing ticket signature: {}", err))?;
> -
>          use std::fmt::Write;
>          write!(
>              &mut output,
> -- 
> 2.39.2
> 
> 
>