[pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[Stefan Sterz]

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`.

proxmox:

Stefan Sterz (8):
  auth-api: move signing into the private key
  auth-api: move to Ed25519 signatures
  auth-api: add ability to use hmac singing in keyring
  auth-api: use constant time comparison for csrf tokens
  auth-api: move to hmac signing for csrf tokens
  sys: crypt: move to yescrypt for password hashing
  sys: crypt: use constant time comparison for password verification
  auth-api: fix types `compilefail` test

 proxmox-auth-api/src/api/access.rs |  91 ++++++++++---
 proxmox-auth-api/src/api/mod.rs    |   6 +-
 proxmox-auth-api/src/auth_key.rs   | 211 +++++++++++++++++++++++------
 proxmox-auth-api/src/lib.rs        |   2 +-
 proxmox-auth-api/src/ticket.rs     |  40 +++---
 proxmox-auth-api/src/types.rs      |  10 +-
 proxmox-sys/Cargo.toml             |   3 +-
 proxmox-sys/src/crypt.rs           | 140 ++++++++++++++++++-
 8 files changed, 414 insertions(+), 89 deletions(-)


proxmox-backup:

Stefan Sterz (4):
  auth: move to hmac keys for csrf tokens
  auth: upgrade hashes on user log in
  auth: move to auth-api's private and public keys when loading keys
  auth: use auth-api when generating keys and generate ec keys

 src/auth.rs         |  35 +++++++----
 src/auth_helpers.rs | 148 +++++++++++---------------------------------
 2 files changed, 60 insertions(+), 123 deletions(-)


Summary over all repositories:
  10 files changed, 474 insertions(+), 212 deletions(-)

--
Generated by git-murpp 0.5.0

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

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 02/12] auth-api: move to Ed25519 signatures

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 05/12] auth-api: move to hmac signing for csrf tokens

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 06/12] sys: crypt: move to yescrypt for password hashing

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 07/12] sys: crypt: use constant time comparison for password verification

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox v2 08/12] auth-api: fix types `compilefail` test

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox-backup v2 09/12] auth: move to hmac keys for csrf tokens

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox-backup v2 10/12] auth: upgrade hashes on user log in

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox-backup v2 11/12] auth: move to auth-api's private and public keys when loading keys

[Stefan Sterz]

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

[pbs-devel] [PATCH proxmox-backup v2 12/12] auth: use auth-api when generating keys and generate ec keys

[Stefan Sterz]

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

Re: [pbs-devel] [PATCH proxmox v2 03/12] auth-api: add ability to use hmac singing in keyring

[Max Carrara]

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

Re: [pbs-devel] [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[Max Carrara]

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>

Re: [pbs-devel] [PATCH proxmox v2 04/12] auth-api: use constant time comparison for csrf tokens

[Max Carrara]

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.");
>      }
>  

[pbs-devel] applied-series: [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[Wolfgang Bumiller]

applied series, thanks


_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel

Re: [pbs-devel] applied-series: [PATCH proxmox{, -backup} v2 00/12] auth-api clean up and improvements

[Max Carrara]

On Wed May 22, 2024 at 4:13 PM CEST, Wolfgang Bumiller wrote:
> applied series, thanks
>

Unfortunately without my T-b and R-b trailers :(

>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel



_______________________________________________
pbs-devel mailing list
pbs-devel@lists.proxmox.com
https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel