* [pbs-devel] [PATCH proxmox v2 01/12] auth-api: move signing into the private key
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
@ 2024-03-06 12:35 ` Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
` (12 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:35 UTC (permalink / raw)
To: pbs-devel
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@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
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox v2 02/12] auth-api: move to Ed25519 signatures
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 01/12] auth-api: move signing into the private key Stefan Sterz
@ 2024-03-06 12:35 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
` (11 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:35 UTC (permalink / raw)
To: pbs-devel
previously we used P-256 as the curve of our choice for ec signatures.
however, in the meantime Ed25519 has become a lot more wide-spread.
this simplifies our ec generation code significantly while keeping the
same security level. Ed25519 was also specifically designed and
reviewed to avoid implementation errors likely making it a more secure
choice
note that Ed25519 as a signature scheme always uses sha512, so signing
or verifying with a chosen digest is not supported.
as this mostly affects newly generated keys, this should not break any
existing setups.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-auth-api/src/auth_key.rs | 52 +++++++++++++++++++-------------
proxmox-auth-api/src/ticket.rs | 2 +-
2 files changed, 32 insertions(+), 22 deletions(-)
diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs
index 32120a3..f7a83bb 100644
--- a/proxmox-auth-api/src/auth_key.rs
+++ b/proxmox-auth-api/src/auth_key.rs
@@ -1,10 +1,8 @@
//! Auth key handling.
use anyhow::{bail, format_err, Error};
-use openssl::ec::{EcGroup, EcKey};
use openssl::hash::MessageDigest;
-use openssl::nid::Nid;
-use openssl::pkey::{HasPublic, PKey, PKeyRef, Private, Public};
+use openssl::pkey::{HasPublic, Id, PKey, PKeyRef, Private, Public};
use openssl::rsa::Rsa;
use openssl::sign::{Signer, Verifier};
@@ -33,14 +31,9 @@ impl PrivateKey {
/// Generate a new EC auth key.
pub fn generate_ec() -> Result<Self, Error> {
- let nid = Nid::X9_62_PRIME256V1;
- let group = EcGroup::from_curve_name(nid)
- .map_err(|err| format_err!("failed to get P-256 group - {err}"))?;
- let ec = EcKey::generate(&group)
- .map_err(|err| format_err!("failed to generate EC key for testing - {err}"))?;
Ok(Self {
- key: PKey::from_ec_key(ec)
- .map_err(|err| format_err!("failed to get PKey for EC key - {err}"))?,
+ key: PKey::generate_ed25519()
+ .map_err(|err| format_err!("failed to generate EC PKey - {err}"))?,
})
}
@@ -59,9 +52,10 @@ impl PrivateKey {
.map_err(|err| format_err!("failed to encode rsa private key as PEM - {err}"));
}
- if let Ok(ec) = self.key.ec_key() {
- return ec
- .private_key_to_pem()
+ if self.key.id() == Id::ED25519 {
+ return self
+ .key
+ .private_key_to_pem_pkcs8()
.map_err(|err| format_err!("failed to encode ec private key as PEM - {err}"));
}
@@ -77,8 +71,9 @@ impl PrivateKey {
.map_err(|err| format_err!("failed to encode rsa public key as PEM - {err}"));
}
- if let Ok(ec) = self.key.ec_key() {
- return ec
+ if self.key.id() == Id::ED25519 {
+ return self
+ .key
.public_key_to_pem()
.map_err(|err| format_err!("failed to encode ec public key as PEM - {err}"));
}
@@ -92,8 +87,15 @@ impl PrivateKey {
}
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}"))?
+ let mut signer = if self.key.id() == Id::ED25519 {
+ // ed25519 does not support signing with digest
+ Signer::new_without_digest(&self.key)
+ } else {
+ Signer::new(digest, &self.key)
+ }
+ .map_err(|e| format_err!("could not create private key signer - {e}"))?;
+
+ signer
.sign_oneshot_to_vec(data)
.map_err(|e| format_err!("could not sign with private key - {e}"))
}
@@ -121,8 +123,9 @@ impl PublicKey {
.map_err(|err| format_err!("failed to encode rsa public key as PEM - {err}"));
}
- if let Ok(ec) = self.key.ec_key() {
- return ec
+ if self.key.id() == Id::ED25519 {
+ return self
+ .key
.public_key_to_pem()
.map_err(|err| format_err!("failed to encode ec public key as PEM - {err}"));
}
@@ -192,8 +195,15 @@ impl Keyring {
signature: &[u8],
data: &[u8],
) -> Result<bool, Error> {
- Verifier::new(digest, key)
- .map_err(|err| format_err!("failed to create openssl verifier - {err}"))?
+ let mut verifier = if key.id() == Id::ED25519 {
+ // ed25519 does not support digests
+ Verifier::new_without_digest(key)
+ } else {
+ Verifier::new(digest, key)
+ }
+ .map_err(|err| format_err!("failed to create openssl verifier - {err}"))?;
+
+ verifier
.verify_oneshot(signature, data)
.map_err(|err| format_err!("openssl error verifying data - {err}"))
}
diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs
index 81054f8..c8fc667 100644
--- a/proxmox-auth-api/src/ticket.rs
+++ b/proxmox-auth-api/src/ticket.rs
@@ -300,7 +300,7 @@ mod test {
}
#[test]
- fn test_tickets_ecdsa() {
+ fn test_tickets_ed25519() {
let keyring = Keyring::generate_new_ec().expect("failed to generate EC key for testing");
simple_test(&keyring, Some("secret aad data"), |_| true);
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 01/12] auth-api: move signing into the private key Stefan Sterz
2024-03-06 12:35 ` [pbs-devel] [PATCH proxmox v2 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-07 10:11 ` Max Carrara
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens Stefan Sterz
` (10 subsequent siblings)
13 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
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 <s.sterz@proxmox.com>
---
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<PKey<Public>> for PublicKey {
}
}
+#[derive(Clone)]
+pub struct HMACKey {
+ key: PKey<Private>,
+}
+
+impl HMACKey {
+ fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
+ 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<Self, Error> {
+ 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<Self, Error> {
+ // 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<Vec<u8>, 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<bool, Error> {
+ 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<String, Error> {
+ 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<PrivateKey>,
- public_keys: Vec<PublicKey>,
+ signing_key: Option<SigningKey>,
+ public_keys: Vec<VerificationKey>,
}
impl Keyring {
@@ -158,6 +235,10 @@ impl Keyring {
PrivateKey::generate_ec().map(Self::with_private_key)
}
+ pub fn generate_new_hmac() -> Result<Self, Error> {
+ 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<Vec<u8>, 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};
#[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
+ });
+ }
}
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
@ 2024-03-07 10:11 ` Max Carrara
0 siblings, 0 replies; 18+ messages in thread
From: Max Carrara @ 2024-03-07 10:11 UTC (permalink / raw)
To: pbs-devel
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 <s.sterz@proxmox.com>
> ---
> 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<PKey<Public>> for PublicKey {
> }
> }
>
> +#[derive(Clone)]
> +pub struct HMACKey {
> + key: PKey<Private>,
> +}
> +
> +impl HMACKey {
> + fn from_bytes(bytes: &[u8]) -> Result<Self, Error> {
> + 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<Self, Error> {
> + 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<Self, Error> {
> + // 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<Vec<u8>, 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<bool, Error> {
> + 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<String, Error> {
> + 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<PrivateKey>,
> - public_keys: Vec<PublicKey>,
> + signing_key: Option<SigningKey>,
> + public_keys: Vec<VerificationKey>,
> }
>
> impl Keyring {
> @@ -158,6 +235,10 @@ impl Keyring {
> PrivateKey::generate_ec().map(Self::with_private_key)
> }
>
> + pub fn generate_new_hmac() -> Result<Self, Error> {
> + 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<Vec<u8>, 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
> + });
> + }
> }
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (2 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-07 10:17 ` Max Carrara
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing " Stefan Sterz
` (9 subsequent siblings)
13 siblings, 1 reply; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
by using openssl's `memcmp::eq()` we can avoid potential side-channel
attack on the csrf token comparison. this comparison's runtime only
depends on the length of the two byte vectors, not their contents.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-auth-api/src/api/access.rs | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index 428d22a..e22eea2 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -286,14 +286,15 @@ fn verify_csrf_prevention_token_do(
}
let timestamp = parts.pop_front().unwrap();
- let sig = parts.pop_front().unwrap();
+ let sig = parts.pop_front().unwrap().as_bytes();
let ttime = i64::from_str_radix(timestamp, 16)
.map_err(|err| format_err!("timestamp format error - {}", err))?;
let digest = compute_csrf_secret_digest(ttime, secret, userid);
+ let digest = digest.as_bytes();
- if digest != sig {
+ if digest.len() != sig.len() || !openssl::memcmp::eq(digest, sig) {
bail!("invalid signature.");
}
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens Stefan Sterz
@ 2024-03-07 10:17 ` Max Carrara
0 siblings, 0 replies; 18+ messages in thread
From: Max Carrara @ 2024-03-07 10:17 UTC (permalink / raw)
To: pbs-devel
On 3/6/24 13:36, Stefan Sterz wrote:
> by using openssl's `memcmp::eq()` we can avoid potential side-channel
> attack on the csrf token comparison. this comparison's runtime only
> depends on the length of the two byte vectors, not their contents.
>
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> proxmox-auth-api/src/api/access.rs | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index 428d22a..e22eea2 100644
> --- a/proxmox-auth-api/src/api/access.rs
> +++ b/proxmox-auth-api/src/api/access.rs
> @@ -286,14 +286,15 @@ fn verify_csrf_prevention_token_do(
> }
>
> let timestamp = parts.pop_front().unwrap();
> - let sig = parts.pop_front().unwrap();
> + let sig = parts.pop_front().unwrap().as_bytes();
Thought these `unwrap()`s here seemed a bit spicy at first, but we do
check for the length of `parts` beforehand, so this is fine.
>
> let ttime = i64::from_str_radix(timestamp, 16)
> .map_err(|err| format_err!("timestamp format error - {}", err))?;
>
> let digest = compute_csrf_secret_digest(ttime, secret, userid);
> + let digest = digest.as_bytes();
>
> - if digest != sig {
> + if digest.len() != sig.len() || !openssl::memcmp::eq(digest, sig) {
> bail!("invalid signature.");
> }
>
^ permalink raw reply [flat|nested] 18+ messages in thread
* [pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing for csrf tokens
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (3 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 06/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
` (8 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
previously we used our own hmac-like implementation for csrf token
signing that simply appended the key to the message (csrf token).
however, this is possibly insecure as an attacker that finds a
collision in the hash function can easily forge a signature. after all,
two messages would then produce the same start conditions before
hashing the key. while this is probably a theoretic attack on our csrf
implementation, it does not hurt to move to the safer standard hmac
implementation that avoids such pitfalls.
this commit re-uses the hmac key wrapper used for the keyring. it also
keeps the old construction around so we can use it for a transition
period between old and new csrf token implementations.
this is a breaking change as it changes the signature of the
`csrf_secret` method of the `AuthContext` trait to return an hmac
key.
also exposes `assemble_csrf_prevention_toke` so we can re-use this
code here instead of duplicating it in e.g. proxmox-backup's
auth_helpers.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-auth-api/src/api/access.rs | 94 ++++++++++++++++++++++++------
proxmox-auth-api/src/api/mod.rs | 6 +-
proxmox-auth-api/src/auth_key.rs | 10 ++++
3 files changed, 88 insertions(+), 22 deletions(-)
diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index e22eea2..5c75094 100644
--- a/proxmox-auth-api/src/api/access.rs
+++ b/proxmox-auth-api/src/api/access.rs
@@ -1,6 +1,7 @@
//! Provides the "/access/ticket" API call.
use anyhow::{bail, format_err, Error};
+use openssl::hash::MessageDigest;
use serde_json::{json, Value};
use proxmox_rest_server::RestEnvironment;
@@ -8,8 +9,8 @@ use proxmox_router::{http_err, Permission, RpcEnvironment};
use proxmox_schema::{api, api_types::PASSWORD_SCHEMA};
use proxmox_tfa::api::TfaChallenge;
-use super::auth_context;
use super::ApiTicket;
+use super::{auth_context, HMACKey};
use crate::ticket::Ticket;
use crate::types::{Authid, Userid};
@@ -242,25 +243,23 @@ fn login_challenge(userid: &Userid) -> Result<Option<TfaChallenge>, Error> {
tfa_config.authentication_challenge(locked_config, userid.as_str(), None)
}
-fn assemble_csrf_prevention_token(secret: &[u8], userid: &Userid) -> String {
+pub fn assemble_csrf_prevention_token(secret: &HMACKey, userid: &Userid) -> String {
let epoch = crate::time::epoch_i64();
- let digest = compute_csrf_secret_digest(epoch, secret, userid);
-
+ let data = csrf_token_data(epoch, userid);
+ let digest = base64::encode_config(
+ secret.sign(MessageDigest::sha3_256(), &data).unwrap(),
+ base64::STANDARD_NO_PAD,
+ );
format!("{:08X}:{}", epoch, digest)
}
-fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String {
- let mut hasher = openssl::sha::Sha256::new();
- let data = format!("{:08X}:{}:", timestamp, userid);
- hasher.update(data.as_bytes());
- hasher.update(secret);
-
- base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD)
+fn csrf_token_data(timestamp: i64, userid: &Userid) -> Vec<u8> {
+ format!("{:08X}:{}:", timestamp, userid).as_bytes().to_vec()
}
pub(crate) fn verify_csrf_prevention_token(
- secret: &[u8],
+ secret: &HMACKey,
userid: &Userid,
token: &str,
min_age: i64,
@@ -271,7 +270,7 @@ pub(crate) fn verify_csrf_prevention_token(
}
fn verify_csrf_prevention_token_do(
- secret: &[u8],
+ secret: &HMACKey,
userid: &Userid,
token: &str,
min_age: i64,
@@ -286,16 +285,31 @@ fn verify_csrf_prevention_token_do(
}
let timestamp = parts.pop_front().unwrap();
- let sig = parts.pop_front().unwrap().as_bytes();
+ let sig = parts.pop_front().unwrap();
+ let sig = base64::decode_config(sig, base64::STANDARD_NO_PAD)
+ .map_err(|e| format_err!("could not base64 decode csrf signature - {e}"))?;
let ttime = i64::from_str_radix(timestamp, 16)
.map_err(|err| format_err!("timestamp format error - {}", err))?;
- let digest = compute_csrf_secret_digest(ttime, secret, userid);
- let digest = digest.as_bytes();
-
- if digest.len() != sig.len() || !openssl::memcmp::eq(digest, sig) {
- bail!("invalid signature.");
+ let verify = secret.verify(
+ MessageDigest::sha3_256(),
+ &sig,
+ &csrf_token_data(ttime, userid),
+ );
+
+ if verify.is_err() || !verify? {
+ // legacy token verification code
+ // TODO: remove once all dependent products had a major version release (PBS)
+ let mut hasher = openssl::sha::Sha256::new();
+ let data = format!("{:08X}:{}:", ttime, userid);
+ hasher.update(data.as_bytes());
+ hasher.update(&secret.as_bytes()?);
+ let old_digest = hasher.finish();
+
+ if old_digest.len() != sig.len() || !openssl::memcmp::eq(&old_digest, &sig) {
+ bail!("invalid signature.");
+ }
}
let now = crate::time::epoch_i64();
@@ -311,3 +325,45 @@ fn verify_csrf_prevention_token_do(
Ok(age)
}
+
+#[test]
+fn test_assemble_and_verify_csrf_token() {
+ let secret = HMACKey::generate().expect("failed to generate HMAC key for testing");
+
+ let userid: Userid = "name@realm"
+ .parse()
+ .expect("could not parse user id for HMAC testing");
+ let token = assemble_csrf_prevention_token(&secret, &userid);
+
+ verify_csrf_prevention_token(&secret, &userid, &token, -300, 300)
+ .expect("could not verify csrf for testing");
+}
+
+#[test]
+fn test_verify_legacy_csrf_tokens() {
+ use openssl::rsa::Rsa;
+
+ // assemble legacy key and token
+ let key = Rsa::generate(2048)
+ .expect("could not generate RSA key for testing")
+ .private_key_to_pem()
+ .expect("could not create private PEM for testing");
+ let userid: Userid = "name@realm"
+ .parse()
+ .expect("could not parse the user id for legacy csrf testing");
+ let epoch = crate::time::epoch_i64();
+
+ let mut hasher = openssl::sha::Sha256::new();
+ let data = format!("{:08X}:{}:", epoch, userid);
+ hasher.update(data.as_bytes());
+ hasher.update(&key);
+ let old_digest = base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD);
+
+ let token = format!("{:08X}:{}", epoch, old_digest);
+
+ // load key into new hmackey wrapper and verify
+ let string = base64::encode_config(key.clone(), base64::STANDARD_NO_PAD);
+ let secret =
+ HMACKey::from_base64(&string).expect("could not create HMAC key from base64 for testing");
+ verify_csrf_prevention_token(&secret, &userid, &token, -300, 300).unwrap();
+}
diff --git a/proxmox-auth-api/src/api/mod.rs b/proxmox-auth-api/src/api/mod.rs
index 129462f..c4e507c 100644
--- a/proxmox-auth-api/src/api/mod.rs
+++ b/proxmox-auth-api/src/api/mod.rs
@@ -9,7 +9,7 @@ use percent_encoding::percent_decode_str;
use proxmox_rest_server::{extract_cookie, AuthError};
use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
-use crate::auth_key::Keyring;
+use crate::auth_key::{HMACKey, Keyring};
use crate::types::{Authid, RealmRef, Userid, UsernameRef};
mod access;
@@ -18,7 +18,7 @@ mod ticket;
use crate::ticket::Ticket;
use access::verify_csrf_prevention_token;
-pub use access::{create_ticket, API_METHOD_CREATE_TICKET};
+pub use access::{assemble_csrf_prevention_token, create_ticket, API_METHOD_CREATE_TICKET};
pub use ticket::{ApiTicket, PartialTicket};
/// Authentication realms are used to manage users: authenticate, change password or remove.
@@ -67,7 +67,7 @@ pub trait AuthContext: Send + Sync {
fn auth_id_is_active(&self, auth_id: &Authid) -> Result<bool, Error>;
/// CSRF prevention token secret data.
- fn csrf_secret(&self) -> &[u8];
+ fn csrf_secret(&self) -> &'static HMACKey;
/// Verify a token secret.
fn verify_token_secret(&self, token_id: &Authid, token_secret: &str) -> Result<(), Error>;
diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs
index b0847a1..f42ed71 100644
--- a/proxmox-auth-api/src/auth_key.rs
+++ b/proxmox-auth-api/src/auth_key.rs
@@ -204,6 +204,16 @@ impl HMACKey {
Ok(base64::encode_config(bytes, base64::STANDARD_NO_PAD))
}
+
+ // This is needed for legacy CSRF token verifyication.
+ //
+ // TODO: remove once all dependent products had a major version release (PBS)
+ pub(crate) fn as_bytes(&self) -> Result<Vec<u8>, Error> {
+ // workaround to get access to the the bytes behind the key.
+ self.key
+ .raw_private_key()
+ .map_err(|e| format_err!("could not get raw bytes of HMAC key - {e}"))
+ }
}
enum SigningKey {
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox v2 06/12] sys: crypt: move to yescrypt for password hashing
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (4 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing " Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 07/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
` (7 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
previously we used `sha256scrypt` for password hashing. while this may
by safe if used with the correct parameters, we used the default
parameters which are considered unsafe. according to `man crypt(5)`:
> The default CPU time cost parameter is 5000, which is too low for
> modern hardware.
hence, we needed to adapt this code anyway. conveniently, verification
with crypt also works for older hashes as the parameters for the
hashing function are encoded in the output of crypt. so this is a drop
in replacement that will simply use yescrypt for new hashes while
old hashes will still verify properly.
this commit also adds a wrapper for `crypt_gensalt_rn` to more easily
generate correctly formatted salt strings. this is also useful for
switching the cpu time hardness parameter, as otherwise we'd need to
encode that ourselves.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-sys/src/crypt.rs | 132 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 129 insertions(+), 3 deletions(-)
diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs
index ab42685..fa10911 100644
--- a/proxmox-sys/src/crypt.rs
+++ b/proxmox-sys/src/crypt.rs
@@ -1,6 +1,9 @@
//! Rust bindings for libcrypt
+//!
+//! this may fail if we ever pull in dependencies that also link with libcrypt. we may eventually
+//! want to switch to pure rust re-implementations of libcrypt.
-use std::ffi::CStr;
+use std::ffi::{CStr, CString};
use anyhow::{bail, Error};
@@ -9,6 +12,18 @@ const CRYPT_OUTPUT_SIZE: usize = 384;
const CRYPT_MAX_PASSPHRASE_SIZE: usize = 512;
const CRYPT_DATA_RESERVED_SIZE: usize = 767;
const CRYPT_DATA_INTERNAL_SIZE: usize = 30720;
+const CRYPT_GENSALT_OUTPUT_SIZE: usize = 192;
+
+// the hash prefix selects the password hashing method, currently this is yescrypt. check `man
+// crypt(5)` for more info
+pub const HASH_PREFIX: &str = "$y$";
+
+// the cpu cost of the password hashing function. depends on the hashing function, see`man
+// crypt_gensalt(3)` and `man crypt(5) for more info
+//
+// `5` selects a good medium cpu time hardness that seems to be widely used by e.g. Debian
+// see `YESCRYPT_COST_FACTOR` in `/etc/login.defs`
+const HASH_COST: u64 = 5;
#[repr(C)]
struct crypt_data {
@@ -49,15 +64,90 @@ pub fn crypt(password: &[u8], salt: &[u8]) -> Result<String, Error> {
if status.is_null() {
bail!("internal error: crypt_r returned null pointer");
}
+
+ // according to man crypt(3):
+ //
+ // > Upon error, crypt_r, crypt_rn, and crypt_ra write an invalid hashed passphrase to the
+ // > output field of their data argument, and crypt writes an invalid hash to its static
+ // > storage area. This string will be shorter than 13 characters, will begin with a ‘*’,
+ // > and will not compare equal to setting.
+ if data.output.first().is_none() || Some(&('*' as i8)) == data.output.first() {
+ bail!("internal error: crypt_r returned invalid hash");
+ }
CStr::from_ptr(&data.output as *const _)
};
Ok(String::from(res.to_str()?))
}
+/// Rust wrapper around `crypt_gensalt_rn` from libcrypt. Useful to generate salts for crypt.
+///
+/// - `prefix`: The prefix that selects the hashing method to use (see `man crypt(5)`)
+/// - `count`: The CPU time cost parameter (e.g., for `yescrypt` between 1 and 11, see `man
+/// crypt(5)`)
+/// - `rbytes`: The byte slice that contains cryptographically random bytes for generating the salt
+pub fn crypt_gensalt(prefix: &str, count: u64, rbytes: &[u8]) -> Result<String, Error> {
+ #[link(name = "crypt")]
+ extern "C" {
+ #[link_name = "crypt_gensalt_rn"]
+ fn __crypt_gensalt_rn(
+ prefix: *const libc::c_char,
+ count: libc::c_ulong,
+ // `crypt_gensalt_rn`'s signature expects a char pointer here, which would be a pointer
+ // to an `i8` slice in rust. however, this is interpreted as raw bytes that are used as
+ // entropy, which in rust usually is a `u8` slice. so use this signature to avoid a
+ // pointless transmutation (random bytes are random, whether interpreted as `i8` or
+ // `u8`)
+ rbytes: *const u8,
+ nrbytes: libc::c_int,
+ output: *mut libc::c_char,
+ output_size: libc::c_int,
+ ) -> *mut libc::c_char;
+ }
+
+ let prefix = CString::new(prefix)?;
+
+ #[allow(clippy::useless_conversion)]
+ let mut output = [libc::c_char::from(0); CRYPT_GENSALT_OUTPUT_SIZE];
+
+ let status = unsafe {
+ __crypt_gensalt_rn(
+ prefix.as_ptr(),
+ count,
+ rbytes.as_ptr(),
+ rbytes.len().try_into()?,
+ output.as_mut_ptr(),
+ output.len().try_into()?,
+ )
+ };
+
+ if status.is_null() {
+ bail!("internal error: crypt_gensalt_rn returned a null pointer");
+ }
+
+ // according to man crypt_gensalt_rn(3):
+ //
+ // > Upon error, in addition to returning a null pointer, crypt_gensalt and crypt_gensalt_rn
+ // > will write an invalid setting string to their output buffer, if there is enough space;
+ // > this string will begin with a ‘*’ and will not be equal to prefix.
+ //
+ // while it states that this is "in addition" to returning a null pointer, this isn't how
+ // `crypt_r` seems to behave (sometimes only setting an invalid hash) so add this here too just
+ // in case.
+ if output.first().is_none() || Some(&('*' as i8)) == output.first() {
+ bail!("internal error: crypt_gensalt_rn could not create a valid salt");
+ }
+
+ let res = unsafe { CStr::from_ptr(output.as_ptr()) };
+
+ Ok(res.to_str()?.to_string())
+}
+
/// Encrypt a pasword using sha256 hashing method
pub fn encrypt_pw(password: &str) -> Result<String, Error> {
- let salt = crate::linux::random_data(8)?;
- let salt = format!("$5${}$", base64::encode_config(&salt, base64::CRYPT));
+ // 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`)
+ let salt = crate::linux::random_data(32)?;
+
+ let salt = crypt_gensalt(HASH_PREFIX, HASH_COST, &salt)?;
crypt(password.as_bytes(), salt.as_bytes())
}
@@ -70,3 +160,39 @@ pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error>
}
Ok(())
}
+
+#[test]
+fn test_hash_and_verify_passphrase() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+
+ let hash = encrypt_pw(phrase).expect("could not hash test password");
+ verify_crypt_pw(phrase, &hash).expect("could not verify test password");
+}
+
+#[test]
+#[should_panic]
+fn test_wrong_passphrase_fails() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+
+ let hash = encrypt_pw(phrase).expect("could not hash test password");
+ verify_crypt_pw("nope", &hash).expect("could not verify test password");
+}
+
+#[test]
+fn test_old_passphrase_hash() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+ // `$5$` -> sha256crypt, our previous default implementation
+ let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC";
+
+ verify_crypt_pw(phrase, hash).expect("could not verify test password");
+}
+
+#[test]
+#[should_panic]
+fn test_old_hash_wrong_passphrase_fails() {
+ let phrase = "nope";
+ // `$5$` -> sha256crypt, our previous default implementation
+ let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC";
+
+ verify_crypt_pw(phrase, hash).expect("could not verify test password");
+}
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox v2 07/12] sys: crypt: use constant time comparison for password verification
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (5 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 06/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 08/12] auth-api: fix types `compilefail` test Stefan Sterz
` (6 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
by using `openssl::memcmp::eq()` we can avoid potential timing side
channels as its runtime only depends on the length of the arrays, not
the contents. this requires the two arrays to have the same length, but
that should be a given since the hashes should always have the same
length.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-sys/Cargo.toml | 3 ++-
proxmox-sys/src/crypt.rs | 8 +++++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/proxmox-sys/Cargo.toml b/proxmox-sys/Cargo.toml
index 5ddbe21..1a44702 100644
--- a/proxmox-sys/Cargo.toml
+++ b/proxmox-sys/Cargo.toml
@@ -16,6 +16,7 @@ lazy_static.workspace = true
libc.workspace = true
log.workspace = true
nix.workspace = true
+openssl = { workspace = true, optional = true }
regex.workspace = true
serde_json.workspace = true
serde = { workspace = true, features = [ "derive" ] }
@@ -29,5 +30,5 @@ proxmox-time.workspace = true
default = []
logrotate = ["dep:zstd"]
acl = []
-crypt = []
+crypt = ["dep:openssl"]
timer = []
diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs
index fa10911..3313f66 100644
--- a/proxmox-sys/src/crypt.rs
+++ b/proxmox-sys/src/crypt.rs
@@ -155,9 +155,15 @@ pub fn encrypt_pw(password: &str) -> Result<String, Error> {
/// Verify if an encrypted password matches
pub fn verify_crypt_pw(password: &str, enc_password: &str) -> Result<(), Error> {
let verify = crypt(password.as_bytes(), enc_password.as_bytes())?;
- if verify != enc_password {
+
+ // `openssl::memcmp::eq()`'s runtime does not depend on the content of the arrays only the
+ // length, this makes it harder to exploit timing side-channels.
+ if verify.len() != enc_password.len()
+ || !openssl::memcmp::eq(verify.as_bytes(), enc_password.as_bytes())
+ {
bail!("invalid credentials");
}
+
Ok(())
}
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox v2 08/12] auth-api: fix types `compilefail` test
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (6 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 07/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
` (5 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
due to missing `use` statements they failed, as they should, but for
the wrong reasons. also adapt a test case that presumably was meant
to test whether `TokennameRef` can be compared, but instead
duplicated the `UsernameRef` test case.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
proxmox-auth-api/src/types.rs | 10 +++++++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/proxmox-auth-api/src/types.rs b/proxmox-auth-api/src/types.rs
index 319ac4b..18b1793 100644
--- a/proxmox-auth-api/src/types.rs
+++ b/proxmox-auth-api/src/types.rs
@@ -137,6 +137,7 @@ pub const PROXMOX_AUTH_REALM_SCHEMA: Schema = PROXMOX_AUTH_REALM_STRING_SCHEMA.s
/// `.as_str()`.
///
/// ```compile_fail
+/// # use proxmox_auth_api::types::Username;
/// fn test(a: Username, b: Username) -> bool {
/// a == b // illegal and does not compile
/// }
@@ -346,21 +347,24 @@ pub struct TokennameRef(str);
#[doc(hidden)]
/// ```compile_fail
+/// # use proxmox_auth_api::types::Username;
/// let a: Username = unsafe { std::mem::zeroed() };
/// let b: Username = unsafe { std::mem::zeroed() };
/// let _ = <Username as PartialEq>::eq(&a, &b);
/// ```
///
/// ```compile_fail
+/// # use proxmox_auth_api::types::UsernameRef;
/// let a: &UsernameRef = unsafe { std::mem::zeroed() };
/// let b: &UsernameRef = unsafe { std::mem::zeroed() };
/// let _ = <&UsernameRef as PartialEq>::eq(a, b);
/// ```
///
/// ```compile_fail
-/// let a: &UsernameRef = unsafe { std::mem::zeroed() };
-/// let b: &UsernameRef = unsafe { std::mem::zeroed() };
-/// let _ = <&UsernameRef as PartialEq>::eq(&a, &b);
+/// # use proxmox_auth_api::types::TokennameRef;
+/// let a: &TokennameRef = unsafe { std::mem::zeroed() };
+/// let b: &TokennameRef = unsafe { std::mem::zeroed() };
+/// let _ = <&TokennameRef as PartialEq>::eq(&a, &b);
/// ```
struct _AssertNoEqImpl;
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 09/12] auth: move to hmac keys for csrf tokens
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (7 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox v2 08/12] auth-api: fix types `compilefail` test Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] auth: upgrade hashes on user log in Stefan Sterz
` (4 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
previously we used a self-rolled implementation for csrf tokens. while
it's unlikely to cause issues in reality, as csrf tokens are only
valid for a given tickets lifetime, there are still theoretical
attacks on our implementation. so move all of this code into the
proxmox-auth-api crate and use hmac instead.
this change should not impact existing installations for now, as this
falls back to the old implementation if a key is already present. hmac
keys will only be used for new installations and if users manually
remove the old key and
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
src/auth.rs | 10 ++---
src/auth_helpers.rs | 97 ++++++++++-----------------------------------
2 files changed, 26 insertions(+), 81 deletions(-)
diff --git a/src/auth.rs b/src/auth.rs
index ec0bc41f..c89314f5 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -15,7 +15,7 @@ use serde_json::json;
use proxmox_auth_api::api::{Authenticator, LockedTfaConfig};
use proxmox_auth_api::ticket::{Empty, Ticket};
use proxmox_auth_api::types::Authid;
-use proxmox_auth_api::Keyring;
+use proxmox_auth_api::{HMACKey, Keyring};
use proxmox_ldap::{Config, Connection, ConnectionMode};
use proxmox_tfa::api::{OpenUserChallengeData, TfaConfig};
@@ -267,7 +267,7 @@ pub fn setup_auth_context(use_private_key: bool) {
AUTH_CONTEXT
.set(PbsAuthContext {
keyring,
- csrf_secret: crate::auth_helpers::csrf_secret().to_vec(),
+ csrf_secret: crate::auth_helpers::csrf_secret(),
})
.map_err(drop)
.expect("auth context setup twice");
@@ -285,7 +285,7 @@ pub(crate) fn public_auth_keyring() -> &'static Keyring {
struct PbsAuthContext {
keyring: &'static Keyring,
- csrf_secret: Vec<u8>,
+ csrf_secret: &'static HMACKey,
}
impl proxmox_auth_api::api::AuthContext for PbsAuthContext {
@@ -327,8 +327,8 @@ impl proxmox_auth_api::api::AuthContext for PbsAuthContext {
}
/// CSRF prevention token secret data.
- fn csrf_secret(&self) -> &[u8] {
- &self.csrf_secret
+ fn csrf_secret(&self) -> &'static HMACKey {
+ self.csrf_secret
}
/// Verify a token secret.
diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
index c2eaaef1..1a483d84 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -1,81 +1,20 @@
use std::path::PathBuf;
+use std::sync::OnceLock;
-use anyhow::{bail, format_err, Error};
+use anyhow::Error;
use lazy_static::lazy_static;
use openssl::pkey::{PKey, Private, Public};
use openssl::rsa::Rsa;
-use openssl::sha;
use pbs_config::BackupLockGuard;
-use proxmox_lang::try_block;
+use proxmox_auth_api::HMACKey;
use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
-use pbs_api_types::Userid;
use pbs_buildcfg::configdir;
use serde_json::json;
pub use crate::auth::setup_auth_context;
-
-fn compute_csrf_secret_digest(timestamp: i64, secret: &[u8], userid: &Userid) -> String {
- let mut hasher = sha::Sha256::new();
- let data = format!("{:08X}:{}:", timestamp, userid);
- hasher.update(data.as_bytes());
- hasher.update(secret);
-
- base64::encode_config(hasher.finish(), base64::STANDARD_NO_PAD)
-}
-
-pub fn assemble_csrf_prevention_token(secret: &[u8], userid: &Userid) -> String {
- let epoch = proxmox_time::epoch_i64();
-
- let digest = compute_csrf_secret_digest(epoch, secret, userid);
-
- format!("{:08X}:{}", epoch, digest)
-}
-
-pub fn verify_csrf_prevention_token(
- secret: &[u8],
- userid: &Userid,
- token: &str,
- min_age: i64,
- max_age: i64,
-) -> Result<i64, Error> {
- use std::collections::VecDeque;
-
- let mut parts: VecDeque<&str> = token.split(':').collect();
-
- try_block!({
- if parts.len() != 2 {
- bail!("format error - wrong number of parts.");
- }
-
- let timestamp = parts.pop_front().unwrap();
- let sig = parts.pop_front().unwrap();
-
- let ttime = i64::from_str_radix(timestamp, 16)
- .map_err(|err| format_err!("timestamp format error - {}", err))?;
-
- let digest = compute_csrf_secret_digest(ttime, secret, userid);
-
- if digest != sig {
- bail!("invalid signature.");
- }
-
- let now = proxmox_time::epoch_i64();
-
- let age = now - ttime;
- if age < min_age {
- bail!("timestamp newer than expected.");
- }
-
- if age > max_age {
- bail!("timestamp too old.");
- }
-
- Ok(age)
- })
- .map_err(|err| format_err!("invalid csrf token - {}", err))
-}
+pub use proxmox_auth_api::api::assemble_csrf_prevention_token;
pub fn generate_csrf_key() -> Result<(), Error> {
let path = PathBuf::from(configdir!("/csrf.key"));
@@ -84,17 +23,14 @@ pub fn generate_csrf_key() -> Result<(), Error> {
return Ok(());
}
- let rsa = Rsa::generate(2048).unwrap();
-
- let pem = rsa.private_key_to_pem()?;
+ let key = HMACKey::generate()?.to_base64()?;
use nix::sys::stat::Mode;
-
let backup_user = pbs_config::backup_user()?;
replace_file(
&path,
- &pem,
+ key.as_bytes(),
CreateOptions::new()
.perm(Mode::from_bits_truncate(0o0640))
.owner(nix::unistd::ROOT)
@@ -145,12 +81,21 @@ pub fn generate_auth_key() -> Result<(), Error> {
Ok(())
}
-pub fn csrf_secret() -> &'static [u8] {
- lazy_static! {
- static ref SECRET: Vec<u8> = file_get_contents(configdir!("/csrf.key")).unwrap();
- }
-
- &SECRET
+pub fn csrf_secret() -> &'static HMACKey {
+ static SECRET: OnceLock<HMACKey> = OnceLock::new();
+
+ SECRET.get_or_init(|| {
+ let bytes = file_get_contents(configdir!("/csrf.key")).unwrap();
+ std::str::from_utf8(&bytes)
+ .map_err(anyhow::Error::new)
+ .and_then(HMACKey::from_base64)
+ // legacy fall back to load legacy csrf secrets
+ // TODO: remove once we move away from legacy token verification
+ .unwrap_or_else(|_| {
+ let key_as_b64 = base64::encode_config(bytes, base64::STANDARD_NO_PAD);
+ HMACKey::from_base64(&key_as_b64).unwrap()
+ })
+ })
}
fn load_public_auth_key() -> Result<PKey<Public>, Error> {
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 10/12] auth: upgrade hashes on user log in
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (8 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] auth: move to auth-api's private and public keys when loading keys Stefan Sterz
` (3 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
if a users password is not hashed with the latest password hashing
function, re-hash the password with the newest hashing function. we
can only do this on login and after the password has been validated,
as this is the only point at which we have access to the plain text
password and also know that it matched the original password.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
src/auth.rs | 21 +++++++++++++++++----
1 file changed, 17 insertions(+), 4 deletions(-)
diff --git a/src/auth.rs b/src/auth.rs
index c89314f5..5c45c331 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -28,20 +28,33 @@ pub const TERM_PREFIX: &str = "PBSTERM";
struct PbsAuthenticator;
-const SHADOW_CONFIG_FILENAME: &str = configdir!("/shadow.json");
+pub(crate) const SHADOW_CONFIG_FILENAME: &str = configdir!("/shadow.json");
impl Authenticator for PbsAuthenticator {
fn authenticate_user<'a>(
- &self,
+ &'a self,
username: &'a UsernameRef,
password: &'a str,
- _client_ip: Option<&'a IpAddr>,
+ client_ip: Option<&'a IpAddr>,
) -> Pin<Box<dyn Future<Output = Result<(), Error>> + Send + 'a>> {
Box::pin(async move {
let data = proxmox_sys::fs::file_get_json(SHADOW_CONFIG_FILENAME, Some(json!({})))?;
match data[username.as_str()].as_str() {
None => bail!("no password set"),
- Some(enc_password) => proxmox_sys::crypt::verify_crypt_pw(password, enc_password)?,
+ Some(enc_password) => {
+ proxmox_sys::crypt::verify_crypt_pw(password, enc_password)?;
+
+ // if the password hash is not based on the current hashing function (as
+ // identified by its prefix), rehash the password.
+ if !enc_password.starts_with(proxmox_sys::crypt::HASH_PREFIX) {
+ // only log that we could not upgrade a password, we already know that the
+ // user has a valid password, no reason the deny to log in attempt.
+ if let Err(e) = self.store_password(username, password, client_ip) {
+ log::warn!("could not upgrade a users password! - {e}");
+ }
+ }
+
+ }
}
Ok(())
})
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 11/12] auth: move to auth-api's private and public keys when loading keys
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (9 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 10/12] auth: upgrade hashes on user log in Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] auth: use auth-api when generating keys and generate ec keys Stefan Sterz
` (2 subsequent siblings)
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
this commit moves away from using openssl's `PKey` and uses the
wrappers from proxmox-auth-api. this allows us to handle keys in a
more flexible way and enables as to move to ec based crypto for the
authkey in the future.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
src/auth.rs | 4 ++--
src/auth_helpers.rs | 42 +++++++++++++-----------------------------
2 files changed, 15 insertions(+), 31 deletions(-)
diff --git a/src/auth.rs b/src/auth.rs
index 5c45c331..35ed249c 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -265,9 +265,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 1a483d84..bbe3001d 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -2,12 +2,10 @@ use std::path::PathBuf;
use std::sync::OnceLock;
use anyhow::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;
@@ -98,36 +96,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)?;
+pub fn public_auth_key() -> &'static PublicKey {
+ static KEY: OnceLock<PublicKey> = OnceLock::new();
- 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)?;
-
- 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
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] [PATCH proxmox-backup v2 12/12] auth: use auth-api when generating keys and generate ec keys
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (10 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 11/12] auth: move to auth-api's private and public keys when loading keys Stefan Sterz
@ 2024-03-06 12:36 ` Stefan Sterz
2024-03-07 10:12 ` [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Max Carrara
2024-05-22 14:13 ` [pbs-devel] applied-series: " Wolfgang Bumiller
13 siblings, 0 replies; 18+ messages in thread
From: Stefan Sterz @ 2024-03-06 12:36 UTC (permalink / raw)
To: pbs-devel
this commit switches pbs over to generating ed25519 keys when
generating new auth api keys. this also removes the last direct
usages of openssl here and further unifies key handling in the auth
api.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
src/auth_helpers.rs | 11 +++--------
1 file changed, 3 insertions(+), 8 deletions(-)
diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
index bbe3001d..cb745eeb 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -2,7 +2,6 @@ use std::path::PathBuf;
use std::sync::OnceLock;
use anyhow::Error;
-use openssl::rsa::Rsa;
use pbs_config::BackupLockGuard;
use proxmox_auth_api::{HMACKey, PrivateKey, PublicKey};
@@ -49,26 +48,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)
--
2.39.2
^ permalink raw reply [flat|nested] 18+ messages in thread* Re: [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (11 preceding siblings ...)
2024-03-06 12:36 ` [pbs-devel] [PATCH proxmox-backup v2 12/12] auth: use auth-api when generating keys and generate ec keys Stefan Sterz
@ 2024-03-07 10:12 ` Max Carrara
2024-05-22 14:13 ` [pbs-devel] applied-series: " Wolfgang Bumiller
13 siblings, 0 replies; 18+ messages in thread
From: Max Carrara @ 2024-03-07 10:12 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Sterz
On 3/6/24 13:35, Stefan Sterz wrote:
> this series adds some more functionality to `proxmox-auth-api` and tries
> to clean it up a little. the first commit moves signing into the keyring
> itself, instead of exposing openssl's `Signer` further.
>
> the second commit replaces our old P-256 ec signatures with the Ed25519
> scheme which offers similar security but is a bit more modern and tries
> to avoid common implementation pitfalls.
>
> the third commit adds hmac signing to `proxmox-auth-api`'s `Keyring`
> which is useful for applications where only one daemon is issueing and
> verifying tickets. hmac uses symmetric keys and is much more efficient
> than asymetric signature schemes. the downside being that the verifier
> and the signer need to be the exact same party, as the verification key
> can also be used to issue new signatures.
>
> the fourth commit uses a constant time comparison for our csrf tokens to
> dimnish the chance of side-channel attack there. the fifth commit uses
> the hmac functionality to sign csrf tokens. here we previously used a
> self-rolled potentially insecure method of generating these tokens. hmac
> avoids common pitfalls here. the commit also provides a fallback to
> avoid compatability issues.
>
> the next two commits move our password hashing scheme to yescrypt and
> implement a constant time comparison for password hashes. the final
> commit for `proxmox-auth-api` cleans up some test cases that were
> failing for the wrong reasons.
>
> the four commits on the proxmox backup server side do the following:
>
> - use hmac keys when generating new csrf tokens
> - upgrade password hashes on log in if they are not using the latest
> password hash function already
> - use the auth-api's wrapper types to load authkeys
> - use Ed25519 keys when generating new auth keys
>
> the first and the last commit here will require a bump of
> `proxmox-auth-api`. the second commit also needs a bump to
> `proxmox-sys`.
>
I like all the changes you made since v1!
Regarding the Code
------------------
* All of the changes are easy to follow; IMO you explained your reasoning
very well
* Patches apply cleanly
* `cargo fmt` only changed a single line - honestly a non-issue, can just
be fixed when applying the patch IMO (see comment on patch)
* `cargo clippy` doesn't complain (about your changes)
My initial confusion regarding the hash upgrading on login was also
resolved (thanks for your answers off list).
Testing
-------
Note: I shamelessly commented out some lines in proxmox-schema while
the GUI thing is being worked on
* Made a snapshot of my testing VM and did a clean build of master
* Created a user called "test" - checked their hash in /etc/proxmox-backup/shadow.json
* Applied and built your patches, deployed everything
* Logged in and out as "test" user
* Checked hash again - hash was upgraded successfully
* New CSRF token also seems to work (didn't notice anything complaining)
Conclusion
----------
LGTM; everything just worked. Maybe somebody else can also chime in and
test things, just in case.
Reviewed-by: Max Carrara <m.carrara@proxmox.com>
Tested-by: Max Carrara <m.carrara@proxmox.com>
^ permalink raw reply [flat|nested] 18+ messages in thread* [pbs-devel] applied-series: [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements
2024-03-06 12:35 [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Stefan Sterz
` (12 preceding siblings ...)
2024-03-07 10:12 ` [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements Max Carrara
@ 2024-05-22 14:13 ` Wolfgang Bumiller
2024-05-24 8:45 ` Max Carrara
13 siblings, 1 reply; 18+ messages in thread
From: Wolfgang Bumiller @ 2024-05-22 14:13 UTC (permalink / raw)
To: Stefan Sterz; +Cc: pbs-devel
applied series, thanks
_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 18+ messages in thread