From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits) server-digest SHA256) (No client certificate requested) by lists.proxmox.com (Postfix) with ESMTPS id F0943B826D for ; Thu, 7 Mar 2024 11:11:54 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id D300A344B3 for ; Thu, 7 Mar 2024 11:11:24 +0100 (CET) Received: from proxmox-new.maurer-it.com (proxmox-new.maurer-it.com [94.136.29.106]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature RSA-PSS (2048 bits)) (No client certificate requested) by firstgate.proxmox.com (Proxmox) with ESMTPS for ; Thu, 7 Mar 2024 11:11:23 +0100 (CET) Received: from proxmox-new.maurer-it.com (localhost.localdomain [127.0.0.1]) by proxmox-new.maurer-it.com (Proxmox) with ESMTP id 87D3C48849 for ; Thu, 7 Mar 2024 11:11:23 +0100 (CET) Message-ID: Date: Thu, 7 Mar 2024 11:11:21 +0100 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Content-Language: en-US To: pbs-devel@lists.proxmox.com References: <20240306123609.164021-1-s.sterz@proxmox.com> <20240306123609.164021-4-s.sterz@proxmox.com> From: Max Carrara In-Reply-To: <20240306123609.164021-4-s.sterz@proxmox.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit X-SPAM-LEVEL: Spam detection results: 0 AWL 0.003 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record T_SCC_BODY_TEXT_LINE -0.01 - URIBL_BLOCKED 0.001 ADMINISTRATOR NOTICE: The query to URIBL was blocked. See http://wiki.apache.org/spamassassin/DnsBlocklists#dnsbl-block for more information. [criterion.rs, lib.rs, ticket.rs] Subject: Re: [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring X-BeenThere: pbs-devel@lists.proxmox.com X-Mailman-Version: 2.1.29 Precedence: list List-Id: Proxmox Backup Server development discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 07 Mar 2024 10:11:55 -0000 On 3/6/24 13:36, Stefan Sterz wrote: > previously we only used asymmetric cryptographic schemes to > authenticate tickets. this is fairly costly and not necessary in every > instance. imagine a service that runs as a single daemon. this daemon > is then the only party that needs to sign and verify tickets. this > makes hmac perfectly suitable for such usecases. hmac has some > advantages over asymmetric schemes: > > - much simpler and well reviewed construction > - much faster and better optimized crypto primitives (hash functions) > > this commit first introduces a new hmac key wrapper that uses openssl's > hmac implementation and can easily be reused by other parts of the > code. it also refactors the keyring code to make it easier to rotate > new hmac keys into place so switching to hmac keys is easier. > > hmac keys are symmetric, so the verification key is the same key as the > signing key. this breaks the previous assumption by the keyring that > these correspond to public and private keys. thus, this commit > introduces two wrapper enums to distinguish between hmac and asymmetric > signature schemes. > > the verification of hmac keys is also done via `openssl::memcmp::eq()` > to avoid potential timing side-channel attacks. > > below are some simple benchmarks done with criterion.rs to show how much > faster hmac is, no matter the actual hash function: > > rsa 4096 + sha256 time: [2.7825 ms 2.7907 ms 2.7995 ms] > ed25519 time: [94.411 µs 94.840 µs 95.324 µs] > hmac sha256 time: [5.7202 µs 5.7412 µs 5.7645 µs] > hmac sha384 time: [6.6577 µs 6.6780 µs 6.7006 µs] > hmac sha3_256 time: [5.6930 µs 5.7114 µs 5.7322 µs] > > rsa with 4096 bit keys and a sha256 digest is our current default. the > test itself consists of a single sign + verification cycle. criterion > repeats this test as it sees fit to arrive at the above numbers. > > Signed-off-by: Stefan Sterz > --- > proxmox-auth-api/src/auth_key.rs | 131 ++++++++++++++++++++++++++++--- > proxmox-auth-api/src/lib.rs | 2 +- > proxmox-auth-api/src/ticket.rs | 17 ++++ > 3 files changed, 136 insertions(+), 14 deletions(-) > > diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs > index f7a83bb..b0847a1 100644 > --- a/proxmox-auth-api/src/auth_key.rs > +++ b/proxmox-auth-api/src/auth_key.rs > @@ -140,13 +140,90 @@ impl From> for PublicKey { > } > } > > +#[derive(Clone)] > +pub struct HMACKey { > + key: PKey, > +} > + > +impl HMACKey { > + fn from_bytes(bytes: &[u8]) -> Result { > + Ok(Self { > + key: PKey::hmac(bytes) > + .map_err(|err| format_err!("failed to create hmac key from bytes - {err}"))?, > + }) > + } > + > + pub fn from_base64(string: &str) -> Result { > + let bytes = base64::decode_config(string, base64::STANDARD_NO_PAD) > + .map_err(|e| format_err!("could not decode base64 hmac key - {e}"))?; > + > + Self::from_bytes(&bytes) > + } > + > + pub fn generate() -> Result { > + // 8*64 = 512 bit security > + let mut bytes = [0u8; 64]; > + openssl::rand::rand_bytes(&mut bytes) > + .map_err(|err| format_err!("failed to generate random bytes for hmac key - {err}"))?; > + > + Self::from_bytes(&bytes) > + } > + > + pub fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result, Error> { > + let mut signer = Signer::new(digest, &self.key) > + .map_err(|e| format_err!("failed to create hmac signer - {e}"))?; > + > + signer > + .sign_oneshot_to_vec(data) > + .map_err(|e| format_err!("failed to sign to vec using hmac - {e}")) > + } > + > + pub fn verify( > + &self, > + digest: MessageDigest, > + signature: &[u8], > + data: &[u8], > + ) -> Result { > + let digest = self.sign(digest, data).map_err(|e| { > + format_err!("failed to verify, could not create comparison signature - {e}") > + })?; > + > + if signature.len() == digest.len() && openssl::memcmp::eq(signature, &digest) { > + return Ok(true); > + } > + > + Ok(false) > + } > + > + /// This outputs the hmac key *without* any encryption just encoded as base64. > + pub fn to_base64(&self) -> Result { > + let bytes = self > + .key > + .raw_private_key() > + .map_err(|e| format_err!("could not get key as raw bytes - {e}"))?; > + > + Ok(base64::encode_config(bytes, base64::STANDARD_NO_PAD)) > + } > +} > + > +enum SigningKey { > + Private(PrivateKey), > + Hmac(HMACKey), > +} > + > +enum VerificationKey { > + Public(PublicKey), > + Hmac(HMACKey), > +} > + > /// A key ring for authentication. > /// > -/// This holds one active signing key for new tickets, and optionally multiple public keys for > -/// verifying them in order to support key rollover. > +/// This can hold one active signing key for new tickets (either an HMAC secret or an assymmetric > +/// key), and optionally multiple public keys and HMAC secrets for verifying them in order to > +/// support key rollover. > pub struct Keyring { > - signing_key: Option, > - public_keys: Vec, > + signing_key: Option, > + public_keys: Vec, > } > > impl Keyring { > @@ -158,6 +235,10 @@ impl Keyring { > PrivateKey::generate_ec().map(Self::with_private_key) > } > > + pub fn generate_new_hmac() -> Result { > + HMACKey::generate().map(Self::with_hmac_key) > + } > + > pub fn new() -> Self { > Self { > signing_key: None, > @@ -168,19 +249,30 @@ impl Keyring { > pub fn with_public_key(key: PublicKey) -> Self { > Self { > signing_key: None, > - public_keys: vec![key], > + public_keys: vec![VerificationKey::Public(key)], > } > } > > pub fn with_private_key(key: PrivateKey) -> Self { > Self { > - signing_key: Some(key), > + signing_key: Some(SigningKey::Private(key)), > + public_keys: Vec::new(), > + } > + } > + > + pub fn with_hmac_key(key: HMACKey) -> Self { > + Self { > + signing_key: Some(SigningKey::Hmac(key)), > public_keys: Vec::new(), > } > } > > pub fn add_public_key(&mut self, key: PublicKey) { > - self.public_keys.push(key); > + self.public_keys.push(VerificationKey::Public(key)); > + } > + > + pub fn add_hmac_key(&mut self, key: HMACKey) { > + self.public_keys.push(VerificationKey::Hmac(key)); > } > > pub fn verify( > @@ -209,14 +301,24 @@ impl Keyring { > } > > if let Some(key) = &self.signing_key { > - if verify_with(&key.key, digest, signature, data)? { > - return Ok(true); > + match key { > + SigningKey::Private(key) if verify_with(&key.key, digest, signature, data)? => { > + return Ok(true) > + } > + SigningKey::Hmac(key) if key.verify(digest, signature, data)? => return Ok(true), > + _ => (), > } > } > > for key in &self.public_keys { > - if verify_with(&key.key, digest, signature, data)? { > - return Ok(true); > + match key { > + VerificationKey::Public(key) if verify_with(&key.key, digest, signature, data)? => { > + return Ok(true) > + } > + VerificationKey::Hmac(key) if key.verify(digest, signature, data)? => { > + return Ok(true) > + } > + _ => (), > } > } > > @@ -224,11 +326,14 @@ impl Keyring { > } > > pub(crate) fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result, Error> { > - let key = self > + let signing_key = self > .signing_key > .as_ref() > .ok_or_else(|| format_err!("no private key available for signing"))?; > > - key.sign(digest, data) > + match signing_key { > + SigningKey::Private(key) => key.sign(digest, data), > + SigningKey::Hmac(key) => key.sign(digest, data), > + } > } > } > diff --git a/proxmox-auth-api/src/lib.rs b/proxmox-auth-api/src/lib.rs > index d371b96..332dffc 100644 > --- a/proxmox-auth-api/src/lib.rs > +++ b/proxmox-auth-api/src/lib.rs > @@ -22,7 +22,7 @@ pub use api::set_auth_context; > mod auth_key; > > #[cfg(any(feature = "api", feature = "ticket"))] > -pub use auth_key::{Keyring, PrivateKey, PublicKey}; > +pub use auth_key::{Keyring, PrivateKey, PublicKey, HMACKey}; Should be: pub use auth_key::{HMACKey, Keyring, PrivateKey, PublicKey}; IMHO super minor thing, can be done when applying the series. > > #[cfg(feature = "ticket")] > pub mod ticket; > diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs > index c8fc667..ff08815 100644 > --- a/proxmox-auth-api/src/ticket.rs > +++ b/proxmox-auth-api/src/ticket.rs > @@ -314,4 +314,21 @@ mod test { > false > }); > } > + > + #[test] > + fn test_tickets_hmac() { > + let keyring = > + Keyring::generate_new_hmac().expect("failed to generate HMAC key for testing"); > + > + simple_test(&keyring, Some("secret aad data"), |_| true); > + simple_test(&keyring, None, |_| true); > + simple_test(&keyring, None, |t| { > + t.change_time(0); > + false > + }); > + simple_test(&keyring, None, |t| { > + t.change_time(crate::time::epoch_i64() + 0x1000_0000); > + false > + }); > + } > }