* [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-26 20:22 ` Esi Y
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
` (10 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key Stefan Sterz
@ 2024-02-26 20:22 ` Esi Y
2024-02-27 9:12 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Esi Y @ 2024-02-26 20:22 UTC (permalink / raw)
To: pbs-devel
This might be a bit nitpicky, but wouldn't this exactly be an opportunity to properly introduce traits, so that in the future on the same kind of swapout, it is more streamlined and less regression prone going forward? Just wondering ... if there was a reason not to, on such a rewrite.
On Thu, Feb 15, 2024 at 04:19:50PM +0100, Stefan Sterz wrote:
> this commit moves the current ticket signing code into the private key
> implementation. the upside is that the caller does not need to deal
> with openssl's `Signer` directly. it also simplifies and unifies the
> code by using the same helper for verifying a signature and creating it.
>
> also derive `Clone` on `PrivateKey` and `PublicKey`. as they are
> essentially thin wrappers around `openssl::pkey::PKey<Private>` and
> `openssl::pkey::PKey<Public>`, which can be cloned, deriving `Clone`
> just makes them easier to use.
>
> Signed-off-by: Stefan Sterz <s.sterz at proxmox.com>
> ---
> proxmox-auth-api/src/auth_key.rs | 26 ++++++++++++++++----------
> proxmox-auth-api/src/ticket.rs | 21 ++-------------------
> 2 files changed, 18 insertions(+), 29 deletions(-)
>
> diff --git a/proxmox-auth-api/src/auth_key.rs b/proxmox-auth-api/src/auth_key.rs
> index cec7360..32120a3 100644
> --- a/proxmox-auth-api/src/auth_key.rs
> +++ b/proxmox-auth-api/src/auth_key.rs
> @@ -9,11 +9,13 @@ use openssl::rsa::Rsa;
> use openssl::sign::{Signer, Verifier};
>
> /// A private auth key used for API ticket signing and verification.
> +#[derive(Clone)]
> pub struct PrivateKey {
> pub(crate) key: PKey<Private>,
> }
>
> /// A private auth key used for API ticket verification.
> +#[derive(Clone)]
> pub struct PublicKey {
> pub(crate) key: PKey<Public>,
> }
> @@ -88,6 +90,13 @@ impl PrivateKey {
> pub fn public_key(&self) -> Result<PublicKey, Error> {
> PublicKey::from_pem(&self.public_key_to_pem()?)
> }
> +
> + pub(self) fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result<Vec<u8>, Error> {
> + Signer::new(digest, &self.key)
> + .map_err(|e| format_err!("could not create private key signer - {e}"))?
> + .sign_oneshot_to_vec(data)
> + .map_err(|e| format_err!("could not sign with private key - {e}"))
> + }
> }
>
> impl From<PKey<Private>> for PrivateKey {
> @@ -204,15 +213,12 @@ impl Keyring {
> Ok(false)
> }
>
> - pub(crate) fn signer(&self, digest: MessageDigest) -> Result<Signer, Error> {
> - Signer::new(
> - digest,
> - &self
> - .signing_key
> - .as_ref()
> - .ok_or_else(|| format_err!("no private key available for signing"))?
> - .key,
> - )
> - .map_err(|err| format_err!("failed to create openssl signer - {err}"))
> + pub(crate) fn sign(&self, digest: MessageDigest, data: &[u8]) -> Result<Vec<u8>, Error> {
> + let key = self
> + .signing_key
> + .as_ref()
> + .ok_or_else(|| format_err!("no private key available for signing"))?;
> +
> + key.sign(digest, data)
> }
> }
> diff --git a/proxmox-auth-api/src/ticket.rs b/proxmox-auth-api/src/ticket.rs
> index 1737912..81054f8 100644
> --- a/proxmox-auth-api/src/ticket.rs
> +++ b/proxmox-auth-api/src/ticket.rs
> @@ -116,27 +116,10 @@ where
> /// Sign the ticket.
> pub fn sign(&mut self, keyring: &Keyring, aad: Option<&str>) -> Result<String, Error> {
> let mut output = self.ticket_data();
> - let mut signer = keyring.signer(MessageDigest::sha256())?;
> -
> - signer
> - .update(output.as_bytes())
> - .map_err(Error::from)
> - .and_then(|()| {
> - if let Some(aad) = aad {
> - signer
> - .update(b":")
> - .and_then(|()| signer.update(aad.as_bytes()))
> - .map_err(Error::from)
> - } else {
> - Ok::<_, Error>(())
> - }
> - })
> + let signature = keyring
> + .sign(MessageDigest::sha256(), &self.verification_data(aad))
> .map_err(|err| format_err!("error signing ticket: {}", err))?;
>
> - let signature = signer
> - .sign_to_vec()
> - .map_err(|err| format_err!("error finishing ticket signature: {}", err))?;
> -
> use std::fmt::Write;
> write!(
> &mut output,
> --
> 2.39.2
>
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
2024-02-26 20:22 ` Esi Y
@ 2024-02-27 9:12 ` Stefan Sterz
2024-02-27 18:13 ` Esi Y
0 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-27 9:12 UTC (permalink / raw)
To: Esi Y, pbs-devel
On Mon Feb 26, 2024 at 9:22 PM CET, Esi Y wrote:
> This might be a bit nitpicky, but wouldn't this exactly be an
> opportunity to properly introduce traits, so that in the future on the
> same kind of swapout, it is more streamlined and less regression prone
> going forward? Just wondering ... if there was a reason not to, on
> such a rewrite.
i am a bit uncertain as to what you mean here, as this series more or
less uses such wrapper traits around the openssl api. `PrivateKey` and
`PublicKey` wrap a `PKey<Private>` and `PKey<Public>` respectivelly.
`HMACKey` wraps a `PKey<Private>` that is treated as an hmac key. this
means that users of the auht-api crate don't have to use openssl to
generate keys or handle them. it also means that the implementation of
these traits is entirely private in the auth-api, so users don't need to
adapt if we extend them with support for more cryptographic schemes
(note that removing such schemes is always a breaking change).
in the future, if you needed to switch out a key you have the keyring in
the authcontext that can handle roll over (for the most part, but i am
working on the missing links here). new keys would be added as the
signing key (hence my introduction of the `SigningKey` and
`VerificationKey` enums in patch 3) the old keys would stay as
verification keys and be evicted after a given number of key rotations.
using enums here was just simple, and unless a third type of crypto
beside symmetric and asymmetric arises, this should be sufficient and
not overcomplicate the code.
what kind of traits would you propose to make such transitions even
easier?
ps: hard wrapping your emails would be nice :)
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
2024-02-27 9:12 ` Stefan Sterz
@ 2024-02-27 18:13 ` Esi Y
2024-02-29 16:07 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Esi Y @ 2024-02-27 18:13 UTC (permalink / raw)
To: Stefan Sterz; +Cc: pbs-devel
On Tue, Feb 27, 2024 at 10:12:10AM +0100, Stefan Sterz wrote:
>On Mon Feb 26, 2024 at 9:22 PM CET, Esi Y wrote:
>> This might be a bit nitpicky, but wouldn't this exactly be an
>> opportunity to properly introduce traits, so that in the future on the
>> same kind of swapout, it is more streamlined and less regression prone
>> going forward? Just wondering ... if there was a reason not to, on
>> such a rewrite.
>
>i am a bit uncertain as to what you mean here, as this series more or
>less uses such wrapper traits around the openssl api. `PrivateKey` and
>`PublicKey` wrap a `PKey<Private>` and `PKey<Public>` respectivelly.
>`HMACKey` wraps a `PKey<Private>` that is treated as an hmac key. this
>means that users of the auht-api crate don't have to use openssl to
>generate keys or handle them. it also means that the implementation of
>these traits is entirely private in the auth-api, so users don't need to
>adapt if we extend them with support for more cryptographic schemes
>(note that removing such schemes is always a breaking change).
This is a misundertanding indeed, what I was getting at was the
state you already came to, so be easy on me in return please (too).
Wrapping PKey<T> is not really doing much when in fact auth_key.rs does
not really encapsulate, an AuthKey. The whole thing still looks more
like openssl helper of some universal kind with a keyring vector
tossed in.
For instance there's no reason to follow the openssl in this even,
there's no need to be splitting public and private key based on what is
just a key processing tool logic. It could be AnyKey that can generate,
derive, etc. This could have seperate impls, e.g. V1Key, V2Key, etc.
The ticket.rs would ideally not even get to be passing on which
MessageDigest it wants, it would just handle the formatting.
The AnyKey would be able (or not) to do import, export, sign, verify,
(another trait could even do encrypt, decrypt). Unless I am missing
something, the Keyring should not allow mixing key types and would
generically handle AnyKeys' vector with a reference to current
signing key.
>in the future, if you needed to switch out a key you have the keyring in
>the authcontext that can handle roll over (for the most part, but i am
>working on the missing links here). new keys would be added as the
>signing key (hence my introduction of the `SigningKey` and
>`VerificationKey` enums in patch 3) the old keys would stay as
>verification keys and be evicted after a given number of key rotations.
This is nice as for rotation, I am more looking at upgrading API
situation, where I am surprised at duplicate original code elements
such as:
if let Ok(rsa) = self.key.rsa() { ...
if let Ok(ec) = self.key.ec_key() { ...
No love for polymorphism, but also convoluted:
pub fn generate_rsa()
pub fn generate_ec()
... all in one struct. I know this was all there originally, but does
it have to stay?
>using enums here was just simple, and unless a third type of crypto
>beside symmetric and asymmetric arises, this should be sufficient and
>not overcomplicate the code.
The beauty of not worrying about any of this, i.e. having it
encapsulated would mean that e.g. one does not need to know what
crypto or digest is in use, if it's (a)symmetric and one calls any
function, AneKey would know best which key to use for that operation.
And ticket.rs shouldn't really care.
>what kind of traits would you propose to make such transitions even
>easier?
I suspect I get not much sympathy for the above already. I do not
think it's making it more complicated though, more the other way
around. Not for someone who knows difference between one time pad
and prime fields' differences in Edwards curves.
>ps: hard wrapping your emails would be nice :)
I tried now, I hope aerc can handle format=flowed. I did set it to
66 LaTeX-ish syndrome, but could not discriminate any further. I
mean, I do like to uphold the good old of being conservative in
what I send out and liberal in what I receive. Unlike some others:
https://lkml.org/lkml/2020/5/29/1038
>-- >8 snip 8< --
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key
2024-02-27 18:13 ` Esi Y
@ 2024-02-29 16:07 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-29 16:07 UTC (permalink / raw)
To: Esi Y; +Cc: pbs-devel
On Tue Feb 27, 2024 at 7:13 PM CET, Esi Y wrote:
> On Tue, Feb 27, 2024 at 10:12:10AM +0100, Stefan Sterz wrote:
> >On Mon Feb 26, 2024 at 9:22 PM CET, Esi Y wrote:
> >> This might be a bit nitpicky, but wouldn't this exactly be an
> >> opportunity to properly introduce traits, so that in the future on the
> >> same kind of swapout, it is more streamlined and less regression prone
> >> going forward? Just wondering ... if there was a reason not to, on
> >> such a rewrite.
> >
> >i am a bit uncertain as to what you mean here, as this series more or
> >less uses such wrapper traits around the openssl api. `PrivateKey` and
> >`PublicKey` wrap a `PKey<Private>` and `PKey<Public>` respectivelly.
> >`HMACKey` wraps a `PKey<Private>` that is treated as an hmac key. this
> >means that users of the auht-api crate don't have to use openssl to
> >generate keys or handle them. it also means that the implementation of
> >these traits is entirely private in the auth-api, so users don't need to
> >adapt if we extend them with support for more cryptographic schemes
> >(note that removing such schemes is always a breaking change).
>
> This is a misundertanding indeed, what I was getting at was the
> state you already came to, so be easy on me in return please (too).
sorry if that came across as harsh, that wasn't intentional.
> Wrapping PKey<T> is not really doing much when in fact auth_key.rs does
> not really encapsulate, an AuthKey. The whole thing still looks more
> like openssl helper of some universal kind with a keyring vector
> tossed in.
i guess the file could be called `keyring.rs` instead, but that's bike
shedding about a decision that was already made. not sure this really
helps anyone as `auth_key` is not a public module so users don't really
see this detail anyway. in any case, a keyring handles the actual auth
key, which can be a asymmetric key with a public or private key or a
symmetric hmac key.
> For instance there's no reason to follow the openssl in this even,
> there's no need to be splitting public and private key based on what is
> just a key processing tool logic. It could be AnyKey that can generate,
> derive, etc. This could have seperate impls, e.g. V1Key, V2Key, etc.
> The ticket.rs would ideally not even get to be passing on which
> MessageDigest it wants, it would just handle the formatting.
before i get to the rest of your argument: yes, the digest could also be
hidden from the users of these types. one may argue, though, at least in
the case of hmac, users may want to be able to upgrade the hashing
function without considering all down-stream users of this crate.
also users that are part of the same crate, like ticket.rs, don't really
create much of a maintenance burden as changing function signatures will
be caught by rustc anyway and are easily adapted in this case.
> The AnyKey would be able (or not) to do import, export, sign, verify,
> (another trait could even do encrypt, decrypt). Unless I am missing
> something, the Keyring should not allow mixing key types and would
> generically handle AnyKeys' vector with a reference to current
> signing key.
i think i can see what you are getting at, but there is one problem with
completelly abstracting away public, private and hmac keys into one
trait: users of this create do need to know what kind of key they are
using and providing to the keyring. for example, in pbs we have two
deamons that need to be able to verify tickets, but only one is allowed
to sign them.
in this case, you can't use HMAC keys as that would break the security
parameters for such a setup (both would be able to verify *and* sign).
so here you need to be able to have a public key type, as the daemon
that only verifies tickets has nothing else available.
so you could have an `AnyKey` trait here that offers a generic `sign`
and `verify`. however, the `sign` would need to fail if `AnyKey` is
backed by a public key, which makes it useless for the `signing_key`
member, as we'd lose type safety there. so you'd need an additional
`SigningKey` trait at a minimum that offers a `sign` method.
this would make the code more convoluted with little benefit in my
opionion. if we need to switch to a different scheme such as, and this
is just an example, ElGamal, we could just extend the `PrivateKey` and
`PublicKey` implementation fairly easily. all we'd need to do is adapt
the `sign` and verify functions and maybe add another generation
function.
with the `AnyKey` approach we would then have a a struct `ElGamal` that
implements `AnyKey` to verify stuff, and a second one that is called
`ElGamalSign` that implements `SigningKey`. you can then have an `Into`
implementation, that turns `ElGamalSign` into an `AnyKey` or implement
`AnyKey` for `ElGamalSign` so you can verify with that to.
now you basically have two new types that implement two traits to have
more or less the same type safety and utility. i am not sure this
actually helps.
> >in the future, if you needed to switch out a key you have the keyring in
> >the authcontext that can handle roll over (for the most part, but i am
> >working on the missing links here). new keys would be added as the
> >signing key (hence my introduction of the `SigningKey` and
> >`VerificationKey` enums in patch 3) the old keys would stay as
> >verification keys and be evicted after a given number of key rotations.
>
> This is nice as for rotation, I am more looking at upgrading API
> situation, where I am surprised at duplicate original code elements
> such as:
>
> if let Ok(rsa) = self.key.rsa() { ...
> if let Ok(ec) = self.key.ec_key() { ...
well as the comment here states, rsa needs to be non-pkcs#8 for legacy
reasons (pbs used this format for rsa keys up until now). the commit
that itroduces support for ed25519 also removes the bit with
`self.key.ec_key()`. all of this is basically due to how openssl works.
which is why we wrap it and abstract it away. this isn't exposed to
users of `PublicKey` and `PrivateKey`, so while this may not look
pretty, it isn't a real issue.
> No love for polymorphism, but also convoluted:
>
> pub fn generate_rsa()
> pub fn generate_ec()
i'll grant you that from a "software engineering" perspective it might
be "cleaner" to use generics here or something similar. however, that
would break the public api at this point and the upside would be fairly
slim. where i agree with you is that we could have an additional generic
`generate` function that would always generate the currently up-to-date
assymmetric key of choice. maybe also have a `generate_symmetric` or
similar as an alternative.
however that would need to be in addition, as some users may still
require specific crypto schemes. note that overly abstract and
"over-engineered" code has a maintenance burden as well. it is harder to
understand where which code path is taken if everything is an abstract
`AnyKey`.
and none of this code makes it hard to move to a new crypto scheme, we
just need to adapt some parts of `PrivateKey` which can be completelly
transparent to the users and that's it. all the users then neeed to do
is call the "newer" `generate` funciton.
once users create a new key, they can update the keyring's `signing_key`
with the newly generate `PrivateKey` and leave the old key's `PublicKey`
in the `public_keys` vector. this effectively switches the encryption
scheme for new tickets while still being able to verify older tickets
until the old `PublicKey` is also removed. and users have to worry about
nothing other then "is it safe to use a symmetric scheme yes/no?"
> ... all in one struct. I know this was all there originally, but does
> it have to stay?
>
> >using enums here was just simple, and unless a third type of crypto
> >beside symmetric and asymmetric arises, this should be sufficient and
> >not overcomplicate the code.
>
> The beauty of not worrying about any of this, i.e. having it
> encapsulated would mean that e.g. one does not need to know what
> crypto or digest is in use, if it's (a)symmetric and one calls any
> function, AneKey would know best which key to use for that operation.
> And ticket.rs shouldn't really care.
as stated above, some if it needs to be worried about by the users of
the crate, though. symmetric and asymmetric keys are non-interchangeable
and their suitability depends on the scenario.
> >what kind of traits would you propose to make such transitions even
> >easier?
>
> I suspect I get not much sympathy for the above already. I do not
> think it's making it more complicated though, more the other way
> around. Not for someone who knows difference between one time pad
> and prime fields' differences in Edwards curves.
thanks, i guess :)
> >ps: hard wrapping your emails would be nice :)
>
> I tried now, I hope aerc can handle format=flowed. I did set it to
> 66 LaTeX-ish syndrome, but could not discriminate any further. I
> mean, I do like to uphold the good old of being conservative in
> what I send out and liberal in what I receive. Unlike some others:
> https://lkml.org/lkml/2020/5/29/1038
works well, not sure that 66 is ideal, but alright, you do you.
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox 02/12] auth-api: move to Ed25519 signatures
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
` (9 subsequent siblings)
11 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* [pbs-devel] [PATCH proxmox 03/12] auth-api: add ability to use hmac singing in keyring
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 01/12] auth-api: move signing into the private key Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 02/12] auth-api: move to Ed25519 signatures Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens Stefan Sterz
` (8 subsequent siblings)
11 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (2 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 03/12] auth-api: add ability to use hmac singing in keyring Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-19 16:02 ` Max Carrara
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 05/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
` (7 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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 | 88 ++++++++++++++++++++++++------
proxmox-auth-api/src/api/mod.rs | 6 +-
proxmox-auth-api/src/auth_key.rs | 10 ++++
3 files changed, 84 insertions(+), 20 deletions(-)
diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
index 428d22a..5ddf1c4 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,
@@ -287,14 +286,28 @@ fn verify_csrf_prevention_token_do(
let timestamp = parts.pop_front().unwrap();
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);
-
- if digest != sig {
- bail!("invalid signature.");
+ if !secret.verify(
+ MessageDigest::sha3_256(),
+ &csrf_token_data(ttime, userid),
+ &sig,
+ )? {
+ // 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();
@@ -310,3 +323,44 @@ 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] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens Stefan Sterz
@ 2024-02-19 16:02 ` Max Carrara
2024-02-20 12:54 ` Max Carrara
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 16:02 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:19, Stefan Sterz wrote:
> 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>
I like the overall approach of this series quite a lot so far! However,
I'm not entirely sure if introducing a breaking change here is what we
actually want, though I'm curious what others are thinking.
There are some more comments inline.
> ---
> proxmox-auth-api/src/api/access.rs | 88 ++++++++++++++++++++++++------
> proxmox-auth-api/src/api/mod.rs | 6 +-
> proxmox-auth-api/src/auth_key.rs | 10 ++++
> 3 files changed, 84 insertions(+), 20 deletions(-)
>
> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
> index 428d22a..5ddf1c4 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,
> @@ -287,14 +286,28 @@ fn verify_csrf_prevention_token_do(
>
> let timestamp = parts.pop_front().unwrap();
> 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);
> -
> - if digest != sig {
> - bail!("invalid signature.");
> + if !secret.verify(
> + MessageDigest::sha3_256(),
> + &csrf_token_data(ttime, userid),
> + &sig,
> + )? {
The check above bothers me somewhat in particular, since we just fall back to the
original verification code below. As you mentioned in your commit message:
> 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. [...]
> 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.
So, technically, it would still be possible for an attacker to forge a signature
during the transition period, because the condition above (most, most likely) fails
anyway.
(Also, you made a quick comment on the side about this off-list, but I fail to recall
it at the moment, so I apologize if you've already mentioned this!)
I feel like it would be more practical to separate the HMAC implementation out as a
separate API and mark the current one as #[deprecated] (or similar) and provide an
upgrade path for implementors of this crate.
> + // legacy token verification code
> + // TODO: remove once all dependent products had a major version release (PBS)
Somewhat off-topic, but I feel that we should have guards in place for things that need
to be removed in future versions so that we don't miss them, even if it ends up being
a bit of a chore in the end.
There are two ideas I had in mind:
1. Marker comments in a certain format that are scanned by a tool; tool emits
warnings / messages for those comments
--> not sure how "convenient" or adaptable to peoples' workflow this might be though
2. Automatic compile time checks for cargo env vars (etc.), for example:
> macro_rules! crate_version {
> () => {
> env!("CARGO_PKG_VERSION_MAJOR")
> .parse::<u32>()
> .expect("Failed to parse crate major version")
> };
> }
>
> #[test]
> fn check_major() {
> let v = crate_version!();
>
> if v > 3 {
> panic!("Encountered major version bump [...]")
> }
> }
Putting this into a separate test in PBS would cause PBS to fail when running `make build`
on a newer major version than 3 (the current one in this case). We could then refer to the
things that still need to be changed for a major version bump. A combination with 1. could
perhaps also work. Though, I realize that this could be quite annoying for some when working
on things unrelated to the checks for the next PBS major release.
> + 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.");
> + }
This check should IMO be split into two for some finer-grained error handling - meaning,
one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal.
Speaking of, should `old_digest` and `sig` be equal here..? Unless I'm mistaken, the
digest and signature must be of equal length *and* be equal in order to be correct, right?
Or am I misunderstanding? (Do we want to fail if an old hash is being used?)
It's great though that `openssl::memcmp::eq()` is used here like in patch 03, but these checks
could perhaps go into a separate patch specifically for the old `compute_csrf_secret_digest()`
function first, so that it also benefits from the usage of constant time comparison.
This patch could then also be applied separately, of course.
> }
>
> let now = crate::time::epoch_i64();
> @@ -310,3 +323,44 @@ 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 {
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-19 16:02 ` Max Carrara
@ 2024-02-20 12:54 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-20 12:54 UTC (permalink / raw)
To: pbs-devel
On 2/19/24 17:02, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
>> 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>
>
> I like the overall approach of this series quite a lot so far! However,
> I'm not entirely sure if introducing a breaking change here is what we
> actually want, though I'm curious what others are thinking.
>
> There are some more comments inline.
>
>> ---
>> proxmox-auth-api/src/api/access.rs | 88 ++++++++++++++++++++++++------
>> proxmox-auth-api/src/api/mod.rs | 6 +-
>> proxmox-auth-api/src/auth_key.rs | 10 ++++
>> 3 files changed, 84 insertions(+), 20 deletions(-)
>>
>> diff --git a/proxmox-auth-api/src/api/access.rs b/proxmox-auth-api/src/api/access.rs
>> index 428d22a..5ddf1c4 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,
>> @@ -287,14 +286,28 @@ fn verify_csrf_prevention_token_do(
>>
>> let timestamp = parts.pop_front().unwrap();
>> 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);
>> -
>> - if digest != sig {
>> - bail!("invalid signature.");
>> + if !secret.verify(
>> + MessageDigest::sha3_256(),
>> + &csrf_token_data(ttime, userid),
>> + &sig,
>> + )? {
>
> The check above bothers me somewhat in particular, since we just fall back to the
> original verification code below. As you mentioned in your commit message:
>
>> 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. [...]
>
>> 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.
>
> So, technically, it would still be possible for an attacker to forge a signature
> during the transition period, because the condition above (most, most likely) fails
> anyway.
>
> (Also, you made a quick comment on the side about this off-list, but I fail to recall
> it at the moment, so I apologize if you've already mentioned this!)
>
> I feel like it would be more practical to separate the HMAC implementation out as a
> separate API and mark the current one as #[deprecated] (or similar) and provide an
> upgrade path for implementors of this crate.
... regarding the fallback mechanism, I've found something [0] that might be of interest:
sub verify_csrf_prevention_token {
my ($secret, $username, $token, $min_age, $max_age, $noerr) = @_;
if ($token =~ m/^([A-Z0-9]{8}):(\S+)$/) {
my $sig = $2;
my $timestamp = $1;
my $ttime = hex($timestamp);
my $digest;
if (length($sig) == 27) {
# detected sha1 csrf token from older proxy, fallback. FIXME: remove with 7.0
$digest = Digest::SHA::sha1_base64("$timestamp:$username", $secret);
} else {
$digest = Digest::SHA::hmac_sha256_base64("$timestamp:$username", $secret);
}
my $age = time() - $ttime;
return 1 if ($digest eq $sig) && ($age > $min_age) &&
($age < $max_age);
}
raise("Permission denied - invalid csrf token\n", code => HTTP_UNAUTHORIZED)
if !$noerr;
return undef;
}
I think what you're doing here might be fine after all ;)
[0]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Ticket.pm;h=ce8d5c8c6c158ed8a9c0b8b89bd55a5bc7d01431;hb=HEAD#l29
>
>> + // legacy token verification code
>> + // TODO: remove once all dependent products had a major version release (PBS)
>
> Somewhat off-topic, but I feel that we should have guards in place for things that need
> to be removed in future versions so that we don't miss them, even if it ends up being
> a bit of a chore in the end.
>
> There are two ideas I had in mind:
>
> 1. Marker comments in a certain format that are scanned by a tool; tool emits
> warnings / messages for those comments
> --> not sure how "convenient" or adaptable to peoples' workflow this might be though
>
> 2. Automatic compile time checks for cargo env vars (etc.), for example:
>
>> macro_rules! crate_version {
>> () => {
>> env!("CARGO_PKG_VERSION_MAJOR")
>> .parse::<u32>()
>> .expect("Failed to parse crate major version")
>> };
>> }
>>
>> #[test]
>> fn check_major() {
>> let v = crate_version!();
>>
>> if v > 3 {
>> panic!("Encountered major version bump [...]")
>> }
>> }
>
> Putting this into a separate test in PBS would cause PBS to fail when running `make build`
> on a newer major version than 3 (the current one in this case). We could then refer to the
> things that still need to be changed for a major version bump. A combination with 1. could
> perhaps also work. Though, I realize that this could be quite annoying for some when working
> on things unrelated to the checks for the next PBS major release.
>
>> + 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.");
>> + }
>
> This check should IMO be split into two for some finer-grained error handling - meaning,
> one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal.
>
> Speaking of, should `old_digest` and `sig` be equal here..? Unless I'm mistaken, the
> digest and signature must be of equal length *and* be equal in order to be correct, right?
> Or am I misunderstanding? (Do we want to fail if an old hash is being used?)
>
> It's great though that `openssl::memcmp::eq()` is used here like in patch 03, but these checks
> could perhaps go into a separate patch specifically for the old `compute_csrf_secret_digest()`
> function first, so that it also benefits from the usage of constant time comparison.
> This patch could then also be applied separately, of course.
>
>> }
>>
>> let now = crate::time::epoch_i64();
>> @@ -310,3 +323,44 @@ 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 {
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-20 12:54 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
2024-02-23 10:48 ` Thomas Lamprecht
2024-02-23 13:06 ` Wolfgang Bumiller
0 siblings, 2 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Tue Feb 20, 2024 at 1:54 PM CET, Max Carrara wrote:
> On 2/19/24 17:02, Max Carrara wrote:
> > On 2/15/24 16:19, Stefan Sterz wrote:
> >> 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>
> >
> > I like the overall approach of this series quite a lot so far! However,
> > I'm not entirely sure if introducing a breaking change here is what we
> > actually want, though I'm curious what others are thinking.
> >
> > There are some more comments inline.
> >
> >> ---
-- >8 snip 8< --
> >> fn verify_csrf_prevention_token_do(
> >> - secret: &[u8],
> >> + secret: &HMACKey,
> >> userid: &Userid,
> >> token: &str,
> >> min_age: i64,
> >> @@ -287,14 +286,28 @@ fn verify_csrf_prevention_token_do(
> >>
> >> let timestamp = parts.pop_front().unwrap();
> >> 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);
> >> -
> >> - if digest != sig {
> >> - bail!("invalid signature.");
> >> + if !secret.verify(
> >> + MessageDigest::sha3_256(),
> >> + &csrf_token_data(ttime, userid),
> >> + &sig,
> >> + )? {
> >
> > The check above bothers me somewhat in particular, since we just fall back to the
> > original verification code below. As you mentioned in your commit message:
> >
> >> 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. [...]
> >
> >> 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.
> >
> > So, technically, it would still be possible for an attacker to forge a signature
> > during the transition period, because the condition above (most, most likely) fails
> > anyway.
> >
> > (Also, you made a quick comment on the side about this off-list, but I fail to recall
> > it at the moment, so I apologize if you've already mentioned this!)
> >
> > I feel like it would be more practical to separate the HMAC implementation out as a
> > separate API and mark the current one as #[deprecated] (or similar) and provide an
> > upgrade path for implementors of this crate.
>
> ... regarding the fallback mechanism, I've found something [0] that might be of interest:
>
> sub verify_csrf_prevention_token {
> my ($secret, $username, $token, $min_age, $max_age, $noerr) = @_;
>
> if ($token =~ m/^([A-Z0-9]{8}):(\S+)$/) {
> my $sig = $2;
> my $timestamp = $1;
> my $ttime = hex($timestamp);
>
> my $digest;
> if (length($sig) == 27) {
> # detected sha1 csrf token from older proxy, fallback. FIXME: remove with 7.0
> $digest = Digest::SHA::sha1_base64("$timestamp:$username", $secret);
> } else {
> $digest = Digest::SHA::hmac_sha256_base64("$timestamp:$username", $secret);
> }
>
> my $age = time() - $ttime;
> return 1 if ($digest eq $sig) && ($age > $min_age) &&
> ($age < $max_age);
> }
>
> raise("Permission denied - invalid csrf token\n", code => HTTP_UNAUTHORIZED)
> if !$noerr;
>
> return undef;
> }
>
>
> I think what you're doing here might be fine after all ;)
>
>
> [0]: https://git.proxmox.com/?p=pve-common.git;a=blob;f=src/PVE/Ticket.pm;h=ce8d5c8c6c158ed8a9c0b8b89bd55a5bc7d01431;hb=HEAD#l29
>
alright, then i'll leave it as is. i did think about whether some kind
of csrf token "versioning" would make sense, if only through the length
of the token. however, that's just security by obscurity as having the
right length comes along with using a broken old hashing method. so
yeah, i think this is fine.
the alternative is not having a fallback at all and breaking all open
session once on upgrade. but basically we should be able to remove this
check even between minor versions since we don't support version
skipping to my knowledge. sessions are only valid for two hours and
usually we don't release those versions *that* quickly ;)
> >
> >> + // legacy token verification code
> >> + // TODO: remove once all dependent products had a major version release (PBS)
> >
> > Somewhat off-topic, but I feel that we should have guards in place for things that need
> > to be removed in future versions so that we don't miss them, even if it ends up being
> > a bit of a chore in the end.
> >
> > There are two ideas I had in mind:
> >
> > 1. Marker comments in a certain format that are scanned by a tool; tool emits
> > warnings / messages for those comments
> > --> not sure how "convenient" or adaptable to peoples' workflow this might be though
> >
> > 2. Automatic compile time checks for cargo env vars (etc.), for example:
> >
> >> macro_rules! crate_version {
> >> () => {
> >> env!("CARGO_PKG_VERSION_MAJOR")
> >> .parse::<u32>()
> >> .expect("Failed to parse crate major version")
> >> };
> >> }
> >>
> >> #[test]
> >> fn check_major() {
> >> let v = crate_version!();
> >>
> >> if v > 3 {
> >> panic!("Encountered major version bump [...]")
> >> }
> >> }
> >
> > Putting this into a separate test in PBS would cause PBS to fail when running `make build`
> > on a newer major version than 3 (the current one in this case). We could then refer to the
> > things that still need to be changed for a major version bump. A combination with 1. could
> > perhaps also work. Though, I realize that this could be quite annoying for some when working
> > on things unrelated to the checks for the next PBS major release.
> >
something like that would be good yeah, your example of that fallback
that should have been removed with pve 7 is a great example. but until
we have a universal way of doing something like that, i think this is
off-topic for this series.
> >> + 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.");
> >> + }
> >
> > This check should IMO be split into two for some finer-grained error handling - meaning,
> > one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal.
> >
as discussed off-list: we should avoid very spcific error messages in
this case. usually that is good practice as it makes debugging easier.
however, here it just give more information to a potential attacker. i'm
not even sure we should return an "invalid signature" error message
here, rather a "csrf token is invalid" for all failure cases would
probably be best. but since we are already here, changing it would also
give more information to a potential attacker.
> > Speaking of, should `old_digest` and `sig` be equal here..? Unless I'm mistaken, the
> > digest and signature must be of equal length *and* be equal in order to be correct, right?
> > Or am I misunderstanding? (Do we want to fail if an old hash is being used?)
> >
argh yeah, that is my mistake sorry, will fix that up in a v2!
> > It's great though that `openssl::memcmp::eq()` is used here like in patch 03, but these checks
> > could perhaps go into a separate patch specifically for the old `compute_csrf_secret_digest()`
> > function first, so that it also benefits from the usage of constant time comparison.
> > This patch could then also be applied separately, of course.
> >
will do that to, thanks for the suggestion!
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-23 9:26 ` Stefan Sterz
@ 2024-02-23 10:48 ` Thomas Lamprecht
2024-02-23 10:52 ` Stefan Sterz
2024-02-23 13:06 ` Wolfgang Bumiller
1 sibling, 1 reply; 35+ messages in thread
From: Thomas Lamprecht @ 2024-02-23 10:48 UTC (permalink / raw)
To: Proxmox Backup Server development discussion, Stefan Sterz
Am 23/02/2024 um 10:26 schrieb Stefan Sterz:
> the alternative is not having a fallback at all and breaking all open
> session once on upgrade. but basically we should be able to remove this
> check even between minor versions since we don't support version
> skipping to my knowledge. sessions are only valid for two hours and
> usually we don't release those versions *that* quickly 😉
Not sure if I understood you correctly, but one can update from any
previous minor version to the newer one,independent of how many versions
there are in-between. Just like one can update from the latest previous
major version to the next major version and the latest of it's minor
version.
So no, this check cannot be removed between minor version.
E.g., if this would get rolled out for PBS 3, then PBS 4 would be the
first version where it would be 100% fine to remove it without any
realistic user impact. As while could update from 3.1 to 3.4 and then
to 4.x in a matter of two hours easily, our official upgrade how-to
then documents that a reboot of the host and a (force) refresh the
web UI is required, which then makes it 100% fine.
If we wouldn't require reboots and refreshes then, users could update
ancient installations over a few major releases in a row, and we could
basically never drop such backward-compatibility code.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-23 10:48 ` Thomas Lamprecht
@ 2024-02-23 10:52 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 10:52 UTC (permalink / raw)
To: Thomas Lamprecht, Proxmox Backup Server development discussion
On Fri Feb 23, 2024 at 11:48 AM CET, Thomas Lamprecht wrote:
> Am 23/02/2024 um 10:26 schrieb Stefan Sterz:
> > the alternative is not having a fallback at all and breaking all open
> > session once on upgrade. but basically we should be able to remove this
> > check even between minor versions since we don't support version
> > skipping to my knowledge. sessions are only valid for two hours and
> > usually we don't release those versions *that* quickly 😉
>
> Not sure if I understood you correctly, but one can update from any
> previous minor version to the newer one,independent of how many versions
> there are in-between. Just like one can update from the latest previous
> major version to the next major version and the latest of it's minor
> version.
>
> So no, this check cannot be removed between minor version.
> E.g., if this would get rolled out for PBS 3, then PBS 4 would be the
> first version where it would be 100% fine to remove it without any
> realistic user impact. As while could update from 3.1 to 3.4 and then
> to 4.x in a matter of two hours easily, our official upgrade how-to
> then documents that a reboot of the host and a (force) refresh the
> web UI is required, which then makes it 100% fine.
>
> If we wouldn't require reboots and refreshes then, users could update
> ancient installations over a few major releases in a row, and we could
> basically never drop such backward-compatibility code.
ahye, sorry for that than. in that case yeah, this fallback could only
be removed with the next major version. sorry for the misinformation.
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens
2024-02-23 9:26 ` Stefan Sterz
2024-02-23 10:48 ` Thomas Lamprecht
@ 2024-02-23 13:06 ` Wolfgang Bumiller
1 sibling, 0 replies; 35+ messages in thread
From: Wolfgang Bumiller @ 2024-02-23 13:06 UTC (permalink / raw)
To: Stefan Sterz; +Cc: Proxmox Backup Server development discussion
On Fri, Feb 23, 2024 at 10:26:15AM +0100, Stefan Sterz wrote:
> On Tue Feb 20, 2024 at 1:54 PM CET, Max Carrara wrote:
> > On 2/19/24 17:02, Max Carrara wrote:
> > > On 2/15/24 16:19, Stefan Sterz wrote:
> > >> + 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.");
> > >> + }
> > >
> > > This check should IMO be split into two for some finer-grained error handling - meaning,
> > > one `bail!()` for different `.len()`s and one if `old_digest` and `sig` are equal.
> > >
>
> as discussed off-list: we should avoid very spcific error messages in
> this case. usually that is good practice as it makes debugging easier.
> however, here it just give more information to a potential attacker. i'm
> not even sure we should return an "invalid signature" error message
> here, rather a "csrf token is invalid" for all failure cases would
> probably be best. but since we are already here, changing it would also
> give more information to a potential attacker.
I'd stick with *not* splitting up the error message here - but mostly
out of habit, because the security impact is rather limited, given that
there aren't that many valid lengths to choose from when the code is
open source, so that's not something security should depend on either
;-)
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox 05/12] sys: crypt: move to yescrypt for password hashing
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (3 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 04/12] auth-api: move to hmac signing for csrf tokens Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
` (6 subsequent siblings)
11 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (4 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 05/12] sys: crypt: move to yescrypt for password hashing Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-19 16:11 ` Max Carrara
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes Stefan Sterz
` (5 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
@ 2024-02-19 16:11 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 16:11 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:19, Stefan Sterz wrote:
> 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>
See my reply to patch 04 - the usage of `openssl::memcmp::eq()` in the legacy
code block there could be merged with this commit first before moving to / implementing
HMAC.
LGTM otherwise, but see the comment inline.
> ---
> 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())
Like in my comment on patch 04, would it make sense here to split these checks into two
for more fine-grained error messaging? Or are there any reasons why they should be together?
> + {
> bail!("invalid credentials");
> }
> +
> Ok(())
> }
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification
2024-02-19 16:11 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 5:11 PM CET, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
> > 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>
>
> See my reply to patch 04 - the usage of `openssl::memcmp::eq()` in the legacy
> code block there could be merged with this commit first before moving to / implementing
> HMAC.
>
i'd like to keep the `proxmox-sys` and `proxmox-auth-api` commits
seperate. imo this makes the git history a bit "cleaner".
> LGTM otherwise, but see the comment inline.
>
-- >8 snip 8< --
> > + // `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())
>
> Like in my comment on patch 04, would it make sense here to split these checks into two
> for more fine-grained error messaging? Or are there any reasons why they should be together?
>
see my response to your comment in patch 04. we don't want to give an
attacker more info than we have to imo.
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (5 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 06/12] sys: crypt: use constant time comparison for password verification Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-19 18:50 ` Max Carrara
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 08/12] auth-api: fix types `compilefail` test Stefan Sterz
` (4 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 UTC (permalink / raw)
To: pbs-devel
`upgrade_hash` function allows us to transparently upgrade a password
hash to a newer hash function. thus, we can get rid of insecure hashes
even without the user needing to log in or reset their password.
it works by retaining only the settings of the previous hash and not the
hash itself. the new hash is a salted hash of the previous hash,
including the settings.
the `verify_crypt_pw` function is also adapted to deal with the new
string format. it verifies hashes whether they've been upgraded or not.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
this is a far from ideal method of upgrading hashes. as the input to the
new hash function has several well-known parts, it may break the
security assumptions of a newer password hashing function. it could be
possible that finding collisions is made easier compared with re-hashing
the original password. hence, only do this as a stop-gap meassure.
also, my choice of `HASH_SEPARATOR` is possibly not ideal. i welcome
some bike-shedding there if we want to consider this approach at all.
proxmox-sys/src/crypt.rs | 131 ++++++++++++++++++++++++++++++++++++++-
1 file changed, 129 insertions(+), 2 deletions(-)
diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs
index 3313f66..ad715ff 100644
--- a/proxmox-sys/src/crypt.rs
+++ b/proxmox-sys/src/crypt.rs
@@ -5,7 +5,7 @@
use std::ffi::{CStr, CString};
-use anyhow::{bail, Error};
+use anyhow::{bail, format_err, Error};
// from libcrypt1, 'lib/crypt.h.in'
const CRYPT_OUTPUT_SIZE: usize = 384;
@@ -25,6 +25,14 @@ pub const HASH_PREFIX: &str = "$y$";
// see `YESCRYPT_COST_FACTOR` in `/etc/login.defs`
const HASH_COST: u64 = 5;
+// for password hash upgrading
+// chosen because we don't use it in our configs, nor should it ever be returned by crypt as the
+// man page states (`man crypt(3)`):
+//
+// > It will be entirely printable ASCII, and will not contain whitespace or the characters ‘:’,
+// > ‘;’, ‘*’, ‘!’, or ‘\’. See crypt(5) for more detail on the format of hashed passphrases.
+const HASH_SEPARATOR: char = ';';
+
#[repr(C)]
struct crypt_data {
output: [libc::c_char; CRYPT_OUTPUT_SIZE],
@@ -142,6 +150,48 @@ pub fn crypt_gensalt(prefix: &str, count: u64, rbytes: &[u8]) -> Result<String,
Ok(res.to_str()?.to_string())
}
+/// Upgrades an existing hash with the latest hash function
+///
+/// Upgraded hashes have the following format:
+///
+/// `{old_settings}{HASH_SEPARATOR}{old_settings}{HASH_SEPARATOR}{new_hash}`
+///
+/// This allows us to upgrade hashes in place while keeping the settings which are necessary for
+/// verifying passwords. By keeping the oldest settings first, we can also simply replace them once
+/// a user logs back in and we can re-hash the proper password.
+pub fn upgrade_hash(hash: &str) -> Result<String, Error> {
+ let mut hashes = hash.split(HASH_SEPARATOR).rev().peekable();
+ let current = hashes.next().ok_or(format_err!(
+ "upgrading hash failed, could not get current hash!"
+ ))?;
+
+ let old_setting = if current.starts_with("$5$") {
+ let till = current.rfind('$').ok_or_else(|| {
+ format_err!("upgrading hash failed, could not separate hash from settings!")
+ })?;
+ current.chars().take(till).collect::<String>()
+ } else if current.starts_with(HASH_PREFIX) {
+ // we already have an an upgraded hash, no need to process it
+ return Ok(hash.to_string());
+ } else {
+ bail!("upgrading hash failed, current hash function is unknown");
+ };
+
+ let new_hash = encrypt_pw(current)?;
+ let mut to_return = format!("{old_setting}{HASH_SEPARATOR}{new_hash}");
+
+ if hashes.peek().is_some() {
+ to_return = format!(
+ "{}{HASH_SEPARATOR}{to_return}",
+ hashes
+ .collect::<Vec<&str>>()
+ .join(&HASH_SEPARATOR.to_string())
+ );
+ }
+
+ Ok(to_return)
+}
+
/// Encrypt a pasword using sha256 hashing method
pub fn encrypt_pw(password: &str) -> Result<String, Error> {
// 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`)
@@ -154,7 +204,19 @@ 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())?;
+ let verify = enc_password
+ .split(HASH_SEPARATOR)
+ .try_fold(password.to_string(), |password, salt| {
+ crypt(password.as_bytes(), salt.as_bytes())
+ })
+ .map_err(|e| format_err!("could not verify password, hashing failed - {e}"))?;
+
+ let enc_password = enc_password
+ .split(HASH_SEPARATOR)
+ .last()
+ .ok_or(format_err!(
+ "could not verify password, current hash could not be retrieved!"
+ ))?;
// `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.
@@ -202,3 +264,68 @@ fn test_old_hash_wrong_passphrase_fails() {
verify_crypt_pw(phrase, hash).expect("could not verify test password");
}
+
+#[test]
+fn test_simple_upgraded_hash_verifies() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+ // `$5$` -> sha256crypt, our previous default implementation
+ let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC";
+ let new_hash = upgrade_hash(hash).expect("could not upgrade test hash");
+
+ assert!(new_hash
+ .split(HASH_SEPARATOR)
+ .last()
+ .expect("could not get last upgraded hash")
+ .starts_with(HASH_PREFIX));
+
+ verify_crypt_pw(phrase, &new_hash).expect("could not verify test password");
+}
+
+#[test]
+fn test_upgrading_passphrase_hash() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+ let hash = [
+ // scrypt hash settings of the initial password hash
+ "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(),
+ // sha256crypt hash of the previous scrypt hash
+ "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(),
+ ]
+ .join(&HASH_SEPARATOR.to_string());
+
+ let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
+ assert!(new_hash
+ .split(HASH_SEPARATOR)
+ .last()
+ .expect("could not get last upgraded hash")
+ .starts_with(HASH_PREFIX));
+ verify_crypt_pw(phrase, &new_hash).expect("could not verify test password");
+}
+
+#[test]
+#[should_panic]
+fn test_upgraded_hash_wrong_passphrase_fails() {
+ let hash = [
+ // scrypt hash settings of the initial password hash
+ "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(),
+ // sha256crypt hash of the previous scrypt hash
+ "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(),
+ ]
+ .join(&HASH_SEPARATOR.to_string());
+
+ let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
+ assert!(new_hash
+ .split(HASH_SEPARATOR)
+ .last()
+ .expect("could not get last upgraded hash")
+ .starts_with(HASH_PREFIX));
+ verify_crypt_pw("nope", &new_hash).expect("could not verify test password");
+}
+
+#[test]
+fn test_current_hash_is_not_upgraded() {
+ let phrase = "supersecretpassphrasenoonewillguess";
+ let hash = encrypt_pw(phrase).expect("could not create test password hash");
+
+ let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
+ assert_eq!(hash, new_hash);
+}
--
2.39.2
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes Stefan Sterz
@ 2024-02-19 18:50 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 18:50 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:19, Stefan Sterz wrote:
> `upgrade_hash` function allows us to transparently upgrade a password
> hash to a newer hash function. thus, we can get rid of insecure hashes
> even without the user needing to log in or reset their password.
>
> it works by retaining only the settings of the previous hash and not the
> hash itself. the new hash is a salted hash of the previous hash,
> including the settings.
>
> the `verify_crypt_pw` function is also adapted to deal with the new
> string format. it verifies hashes whether they've been upgraded or not.
>
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> this is a far from ideal method of upgrading hashes. as the input to the
> new hash function has several well-known parts, it may break the
> security assumptions of a newer password hashing function. it could be
> possible that finding collisions is made easier compared with re-hashing
> the original password. hence, only do this as a stop-gap meassure.
>
> also, my choice of `HASH_SEPARATOR` is possibly not ideal. i welcome
> some bike-shedding there if we want to consider this approach at all.
I know you've meticulously tested this, you gave me a demonstration of this
as well, but it still makes me feel uneasy nevertheless - IMHO it is long
overdue that we upgrade our hashes, but there must be a cleaner way to do
this that doesn't involve keeping those amalgams of crypt hashes around.
There are quite a few comments inline, but I do want to mention that I
realize that this took you a lot of work, so I highly commend your efforts
on this.
>
> proxmox-sys/src/crypt.rs | 131 ++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 129 insertions(+), 2 deletions(-)
>
> diff --git a/proxmox-sys/src/crypt.rs b/proxmox-sys/src/crypt.rs
> index 3313f66..ad715ff 100644
> --- a/proxmox-sys/src/crypt.rs
> +++ b/proxmox-sys/src/crypt.rs
> @@ -5,7 +5,7 @@
>
> use std::ffi::{CStr, CString};
>
> -use anyhow::{bail, Error};
> +use anyhow::{bail, format_err, Error};
>
> // from libcrypt1, 'lib/crypt.h.in'
> const CRYPT_OUTPUT_SIZE: usize = 384;
> @@ -25,6 +25,14 @@ pub const HASH_PREFIX: &str = "$y$";
> // see `YESCRYPT_COST_FACTOR` in `/etc/login.defs`
> const HASH_COST: u64 = 5;
>
> +// for password hash upgrading
> +// chosen because we don't use it in our configs, nor should it ever be returned by crypt as the
> +// man page states (`man crypt(3)`):
> +//
> +// > It will be entirely printable ASCII, and will not contain whitespace or the characters ‘:’,
> +// > ‘;’, ‘*’, ‘!’, or ‘\’. See crypt(5) for more detail on the format of hashed passphrases.
> +const HASH_SEPARATOR: char = ';';
> +
> #[repr(C)]
> struct crypt_data {
> output: [libc::c_char; CRYPT_OUTPUT_SIZE],
> @@ -142,6 +150,48 @@ pub fn crypt_gensalt(prefix: &str, count: u64, rbytes: &[u8]) -> Result<String,
> Ok(res.to_str()?.to_string())
> }
>
> +/// Upgrades an existing hash with the latest hash function
> +///
> +/// Upgraded hashes have the following format:
> +///
> +/// `{old_settings}{HASH_SEPARATOR}{old_settings}{HASH_SEPARATOR}{new_hash}`
> +///
> +/// This allows us to upgrade hashes in place while keeping the settings which are necessary for
> +/// verifying passwords. By keeping the oldest settings first, we can also simply replace them once
> +/// a user logs back in and we can re-hash the proper password.
While upgrading the hashes will look ugly no matter what, I think this could be
wrapped into a more maintainable API. Here's a rough draft of an alternative approach:
#[non_exhaustive] // technically not necessary
enum HashType<'a> {
#[deprecated = "support for legacy type hashes (sha256crypt) will be removed soon"]
Legacy(Cow<'a, str>),
Upgraded(Cow<'a, str>),
// possible other variants
}
impl<'a> HashType<'a> {
fn new(s: &'a str) -> Result<Self, Error> {
todo!("parse type of hash here")
}
fn try_upgrade(&self) -> Result<Self, Error> {
todo!("try to rehash here if necessary")
}
fn into_inner(self) -> Cow<'a, str> {
match self {
HashType::Legacy(s) => s,
HashType::Upgraded(s) => s,
}
}
// as_str(), to_bytes(), to_owned(), etc.
}
// for good measure ;P
impl<'a> TryFrom<&'a str> for HashType<'a> {
type Error = Error;
fn try_from(value: &'a str) -> Result<Self, Self::Error> {
Self::new(value)
}
}
This enum would allow us to represent different types of hashes as different variants,
which means we don't have to check whether the `&str` is prefixed with a certain magic
constant in different places. Only if `HashType::new()` returns `Ok()` we know that
our `&str` does in fact represent a valid hash for our uses. This also reduces the
amount of awkward error handling at usage sites.
Furthermore, this enum is supposed to remain *strictly* private in case we need to change
its implementation (e.g. add another hash type, if that ever happens; fix a bug, etc.).
However, since we want to expose its functionality, we need a wrapper type:
pub struct CryptHash<'a> {
inner: HashType<'a>,
}
impl<'a> CryptHash<'a> {
pub fn new(s: &'a str) -> Result<Self, Error> {
HashType::new(s).map(|inner| Self { inner })
}
pub fn try_upgrade(&self) -> Result<Self, Error> {
self.inner
.try_upgrade()
.map(|new_inner| Self { inner: new_inner })
}
// other operations
// also as_str(), to_bytes(), to_owned(), etc.
}
The `CryptHash` struct above just wraps the `HashType` enum and exposes its underlying
mechanisms via a small API.
All implementation details regarding (re-)hashing and other things can therefore hidden
behind a small abstraction.
This is, as mentioned before, really just a rough draft - the `Cow<'a, &str>`s can be
put elsewhere / omitted if desired, or we could decide to always allocate, etc.
Whatever it might look like in the end, I think it's important we pack this away neatly,
so that use sites can consistently rely on it without having to worry about implementation
details.
This function here would then be implemented on `CryptHash` - since we know whether a
hash is valid or not when we call `CryptHash::new()` ...
> +pub fn upgrade_hash(hash: &str) -> Result<String, Error> {
... this would be eliminated:
> + let mut hashes = hash.split(HASH_SEPARATOR).rev().peekable();
> + let current = hashes.next().ok_or(format_err!(
> + "upgrading hash failed, could not get current hash!"
> + ))?;
> +
... this here too:
> + let old_setting = if current.starts_with("$5$") {
... this here as well:
> + let till = current.rfind('$').ok_or_else(|| {
> + format_err!("upgrading hash failed, could not separate hash from settings!")
> + })?;
> + current.chars().take(till).collect::<String>()
... we also know here whether the hash is upgraded or of some other variant:
> + } else if current.starts_with(HASH_PREFIX) {
> + // we already have an an upgraded hash, no need to process it
> + return Ok(hash.to_string());
> + } else {
... this here could maybe (?) be eliminated as well (if it's possible to determine the
hash function being used when `CryptHash` is instantiated, that is):
> + bail!("upgrading hash failed, current hash function is unknown");
> + };
> +
... this (with the remaining things above) would then go into `CryptHash::try_upgrade()`:
> + let new_hash = encrypt_pw(current)?;
> + let mut to_return = format!("{old_setting}{HASH_SEPARATOR}{new_hash}");
> +
> + if hashes.peek().is_some() {
> + to_return = format!(
> + "{}{HASH_SEPARATOR}{to_return}",
> + hashes
> + .collect::<Vec<&str>>()
> + .join(&HASH_SEPARATOR.to_string())
> + );
> + }
> +
> + Ok(to_return)
> +}
> +
One thing I'm unsure of is whether we can avoid storing concatenated `crypt` settings (those
separated by `HASH_SEPARATOR`) that way - ideally we should avoid that IMO and just upgrade
from one hash type to another, but I'm not 100% if that's possible, so please let me know
what you think!
This here could maybe also be added as constructor-fn to that type (e.g.
`CryptHash::new_from_password()`)
> /// Encrypt a pasword using sha256 hashing method
> pub fn encrypt_pw(password: &str) -> Result<String, Error> {
> // 8*32 = 256 bits security (128+ recommended, see `man crypt(5)`)
> @@ -154,7 +204,19 @@ pub fn encrypt_pw(password: &str) -> Result<String, Error> {
>
Also a good candidate for a method:
> /// 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())?;
> + let verify = enc_password
> + .split(HASH_SEPARATOR)
> + .try_fold(password.to_string(), |password, salt| {
> + crypt(password.as_bytes(), salt.as_bytes())
> + })
> + .map_err(|e| format_err!("could not verify password, hashing failed - {e}"))?;
> +
> + let enc_password = enc_password
> + .split(HASH_SEPARATOR)
> + .last()
> + .ok_or(format_err!(
> + "could not verify password, current hash could not be retrieved!"
> + ))?;
>
> // `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.
> @@ -202,3 +264,68 @@ fn test_old_hash_wrong_passphrase_fails() {
>
> verify_crypt_pw(phrase, hash).expect("could not verify test password");
> }
> +
> +#[test]
> +fn test_simple_upgraded_hash_verifies() {
> + let phrase = "supersecretpassphrasenoonewillguess";
> + // `$5$` -> sha256crypt, our previous default implementation
> + let hash = "$5$bx7fjhlS8yMPM3Nc$yRgB5vyoTWeRcYn31RFTg5hAGyTInUq.l0HqLKzRuRC";
> + let new_hash = upgrade_hash(hash).expect("could not upgrade test hash");
> +
> + assert!(new_hash
> + .split(HASH_SEPARATOR)
> + .last()
> + .expect("could not get last upgraded hash")
> + .starts_with(HASH_PREFIX));
> +
> + verify_crypt_pw(phrase, &new_hash).expect("could not verify test password");
> +}
> +
> +#[test]
> +fn test_upgrading_passphrase_hash() {
> + let phrase = "supersecretpassphrasenoonewillguess";
> + let hash = [
> + // scrypt hash settings of the initial password hash
> + "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(),
> + // sha256crypt hash of the previous scrypt hash
> + "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(),
> + ]
> + .join(&HASH_SEPARATOR.to_string());
> +
> + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
> + assert!(new_hash
> + .split(HASH_SEPARATOR)
> + .last()
> + .expect("could not get last upgraded hash")
> + .starts_with(HASH_PREFIX));
> + verify_crypt_pw(phrase, &new_hash).expect("could not verify test password");
> +}
> +
> +#[test]
> +#[should_panic]
> +fn test_upgraded_hash_wrong_passphrase_fails() {
> + let hash = [
> + // scrypt hash settings of the initial password hash
> + "$7$CU..../....t5ffrYy0ejA2e4irwo2JM.".to_string(),
> + // sha256crypt hash of the previous scrypt hash
> + "$5$UFxVcRPYnVIvUw9r$6fMExxbmSklsO/XucEkRCh/ZMivQle0ssRp9mc.YKz2".to_string(),
> + ]
> + .join(&HASH_SEPARATOR.to_string());
> +
> + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
> + assert!(new_hash
> + .split(HASH_SEPARATOR)
> + .last()
> + .expect("could not get last upgraded hash")
> + .starts_with(HASH_PREFIX));
> + verify_crypt_pw("nope", &new_hash).expect("could not verify test password");
> +}
> +
> +#[test]
> +fn test_current_hash_is_not_upgraded() {
> + let phrase = "supersecretpassphrasenoonewillguess";
> + let hash = encrypt_pw(phrase).expect("could not create test password hash");
> +
> + let new_hash = upgrade_hash(&hash).expect("could not upgrade test password");
> + assert_eq!(hash, new_hash);
> +}
The tests otherwise look fine to me; though, I'll have do admit, the `.split()`ing and
`.join()`ing still feels quite off to me...
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes
2024-02-19 18:50 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 7:50 PM CET, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
> > `upgrade_hash` function allows us to transparently upgrade a password
> > hash to a newer hash function. thus, we can get rid of insecure hashes
> > even without the user needing to log in or reset their password.
> >
> > it works by retaining only the settings of the previous hash and not the
> > hash itself. the new hash is a salted hash of the previous hash,
> > including the settings.
> >
> > the `verify_crypt_pw` function is also adapted to deal with the new
> > string format. it verifies hashes whether they've been upgraded or not.
> >
> > Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> > ---
> > this is a far from ideal method of upgrading hashes. as the input to the
> > new hash function has several well-known parts, it may break the
> > security assumptions of a newer password hashing function. it could be
> > possible that finding collisions is made easier compared with re-hashing
> > the original password. hence, only do this as a stop-gap meassure.
> >
> > also, my choice of `HASH_SEPARATOR` is possibly not ideal. i welcome
> > some bike-shedding there if we want to consider this approach at all.
>
> I know you've meticulously tested this, you gave me a demonstration of this
> as well, but it still makes me feel uneasy nevertheless - IMHO it is long
> overdue that we upgrade our hashes, but there must be a cleaner way to do
> this that doesn't involve keeping those amalgams of crypt hashes around.
>
> There are quite a few comments inline, but I do want to mention that I
> realize that this took you a lot of work, so I highly commend your efforts
> on this.
>
just wanted to say, after off-list discussions, i'll retract the
hash-upgrading mechanism. it would only be useful if there is a
pre-image attack on sha2 while exposing our users to potential
known-prefix attacks. if a sha2 pre-image attack becomes publicly known
we can reconsider this.
thanks for the suggestions, though! i do think that they would make all
of this a bit cleaner!
(i'll trim the rest of this message, though, as imo it's not relevant to
further discussions until the attack mentioned above does become known)
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox 08/12] auth-api: fix types `compilefail` test
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (6 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 07/12] sys: crypt: add helper to allow upgrading hashes Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
` (3 subsequent siblings)
11 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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] 35+ messages in thread* [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (7 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox 08/12] auth-api: fix types `compilefail` test Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-19 18:55 ` Max Carrara
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in Stefan Sterz
` (2 subsequent siblings)
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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>
---
note that the fallbacks here and in `proxmox-auth-api` should be removed
with the next (major) version if possible. simply removing the csrf key
before restarting the services should be enough here. it is possible
that this will break some sessions, possibly forcing a user to log back
in. so only do this when we can assume that an admin would have a
reasonable expectation that something like this will happen (i.e.:
possibly document this in the upgrade notes etc.).
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] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
@ 2024-02-19 18:55 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 18:55 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:19, Stefan Sterz wrote:
> 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>
> ---
> note that the fallbacks here and in `proxmox-auth-api` should be removed
> with the next (major) version if possible. [...]
As mentioned in my reply to patch 04, we should somehow ensure that this
removed with some kind of compile time check or similar, so we *really*
don't miss it.
> [...] simply removing the csrf key
> before restarting the services should be enough here. it is possible
> that this will break some sessions, possibly forcing a user to log back
> in. so only do this when we can assume that an admin would have a
> reasonable expectation that something like this will happen (i.e.:
> possibly document this in the upgrade notes etc.).
>
> 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();
Nice that we can use a `OnceLock` here!
> +
> + 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
See my comment above for the TODO up there.
> + .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 mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens
2024-02-19 18:55 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 7:55 PM CET, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
> > 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>
> > ---
> > note that the fallbacks here and in `proxmox-auth-api` should be removed
> > with the next (major) version if possible. [...]
>
> As mentioned in my reply to patch 04, we should somehow ensure that this
> removed with some kind of compile time check or similar, so we *really*
> don't miss it.
>
yeah, as stated before, that makes sense but imo should be worked out
separatelly from this series.
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (8 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 09/12] auth: move to hmac keys for csrf tokens Stefan Sterz
@ 2024-02-15 15:19 ` Stefan Sterz
2024-02-19 18:58 ` Max Carrara
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes Stefan Sterz
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys Stefan Sterz
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:19 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 | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/src/auth.rs b/src/auth.rs
index c89314f5..3379577f 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -28,20 +28,30 @@ 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) {
+ // ignore errors here, we already authenticated the user, re-hashing the
+ // password should not prevent them from logging in.
+ let _ = self.store_password(username, password, client_ip);
+ }
+ }
}
Ok(())
})
--
2.39.2
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in Stefan Sterz
@ 2024-02-19 18:58 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 18:58 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:19, Stefan Sterz wrote:
> 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 | 18 ++++++++++++++----
> 1 file changed, 14 insertions(+), 4 deletions(-)
>
> diff --git a/src/auth.rs b/src/auth.rs
> index c89314f5..3379577f 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -28,20 +28,30 @@ 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) {
> + // ignore errors here, we already authenticated the user, re-hashing the
> + // password should not prevent them from logging in.
> + let _ = self.store_password(username, password, client_ip);
IMO this should be logged somewhere instead of just swallowing the
error silently, possibly even warning the user or admin that re-hashing
failed (while letting them log on anyways).
The point of this series is to move away from the old stuff, so we
should ensure that we actually do.
> + }
> + }
> }
> Ok(())
> })
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in
2024-02-19 18:58 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 7:58 PM CET, Max Carrara wrote:
> On 2/15/24 16:19, Stefan Sterz wrote:
-- >8 snip 8< --
> > + // 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) {
> > + // ignore errors here, we already authenticated the user, re-hashing the
> > + // password should not prevent them from logging in.
> > + let _ = self.store_password(username, password, client_ip);
>
> IMO this should be logged somewhere instead of just swallowing the
> error silently, possibly even warning the user or admin that re-hashing
> failed (while letting them log on anyways).
>
> The point of this series is to move away from the old stuff, so we
> should ensure that we actually do.
>
makes sense to log this, but warning the users is probably not all that
useful. most users won't be able to do anything about the warning (with
exception of admins who should watch the logs anyway) and it is probably
more confusing than helpful.
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (9 preceding siblings ...)
2024-02-15 15:19 ` [pbs-devel] [PATCH proxmox-backup 10/12] auth: upgrade hashes on user log in Stefan Sterz
@ 2024-02-15 15:20 ` Stefan Sterz
2024-02-19 19:06 ` Max Carrara
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys Stefan Sterz
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:20 UTC (permalink / raw)
To: pbs-devel
this uses proxmox-sys' `upgrade_hash` function to upgrade all stored
hashes that don't already use the latest hash. this allows admins to
upgrade their user's password hashes even if the user's don't log in.
however, as this requires a newer version of proxmox-sys'
`verify_crypt_pw` function to work, make this a command that admin's
can optional perform instead of automatically upgrading all hashes.
admins can reset their users password to avoid issues when
downgrading pbs or similar to work around potential issues here, but
doing this automatically may cause problems.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
note that once an admin has upgraded a hash, downgrading
proxmox-backup-server will break logging in for all users with upgraded
passwords. an admin would then need to manually reset the password via
`proxmox-backup-manager user update <user> --password <pw>`.
src/auth_helpers.rs | 38 +++++++++++++++++++++++++-
src/bin/proxmox_backup_manager/user.rs | 34 ++++++++++++++++++++++-
2 files changed, 70 insertions(+), 2 deletions(-)
diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
index 1a483d84..375ce190 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -1,7 +1,8 @@
+use std::collections::HashMap;
use std::path::PathBuf;
use std::sync::OnceLock;
-use anyhow::Error;
+use anyhow::{format_err, Error};
use lazy_static::lazy_static;
use openssl::pkey::{PKey, Private, Public};
use openssl::rsa::Rsa;
@@ -16,6 +17,41 @@ use serde_json::json;
pub use crate::auth::setup_auth_context;
pub use proxmox_auth_api::api::assemble_csrf_prevention_token;
+pub fn upgrade_password_hashes() -> Result<(), Error> {
+ let data =
+ proxmox_sys::fs::file_get_json(crate::auth::SHADOW_CONFIG_FILENAME, Some(json!({})))?;
+ let mut new = HashMap::new();
+
+ for (username, password) in data
+ .as_object()
+ .ok_or_else(|| format_err!("shadow file has an unexpected format!"))?
+ {
+ let upgraded_password = proxmox_sys::crypt::upgrade_hash(
+ password
+ .as_str()
+ .ok_or_else(|| format_err!("user without password found!"))?,
+ )?;
+
+ new.insert(username.as_str(), upgraded_password);
+ }
+
+ let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600);
+ let options = proxmox_sys::fs::CreateOptions::new()
+ .perm(mode)
+ .owner(nix::unistd::ROOT)
+ .group(nix::unistd::Gid::from_raw(0));
+
+ let new_data = serde_json::to_vec_pretty(&new)?;
+ proxmox_sys::fs::replace_file(
+ crate::auth::SHADOW_CONFIG_FILENAME,
+ &new_data,
+ options,
+ true,
+ )?;
+
+ Ok(())
+}
+
pub fn generate_csrf_key() -> Result<(), Error> {
let path = PathBuf::from(configdir!("/csrf.key"));
diff --git a/src/bin/proxmox_backup_manager/user.rs b/src/bin/proxmox_backup_manager/user.rs
index 743c5d16..2daf2db7 100644
--- a/src/bin/proxmox_backup_manager/user.rs
+++ b/src/bin/proxmox_backup_manager/user.rs
@@ -1,7 +1,9 @@
-use anyhow::Error;
+use anyhow::{bail, Error};
use serde_json::Value;
use std::collections::HashMap;
+use std::io::IsTerminal;
+use std::io::Write;
use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
use proxmox_schema::api;
@@ -65,6 +67,32 @@ fn list_users(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
Ok(Value::Null)
}
+#[api]
+/// Updates the hashes of all users that don't use the current hashing method to the current
+/// hashing method. This is a stop-gap meassure to avoid keeping insecure hashes when migrating
+/// between password hash function.
+fn update_password_hashes() -> Result<(), Error> {
+ // If we're on a TTY, query the user
+ if std::io::stdin().is_terminal() {
+ println!("You are about to update all password hashes that are managed by Proxmox Backup Server!");
+ print!("Are you sure you want to continue? (y/N): ");
+ let _ = std::io::stdout().flush();
+ use std::io::{BufRead, BufReader};
+ let mut line = String::new();
+ match BufReader::new(std::io::stdin()).read_line(&mut line) {
+ Ok(_) => match line.trim() {
+ "y" | "Y" => (), // continue
+ _ => bail!("Aborting."),
+ },
+ Err(err) => bail!("Failed to read line - {err}."),
+ }
+ }
+
+ proxmox_backup::auth_helpers::upgrade_password_hashes()?;
+
+ Ok(())
+}
+
#[api(
input: {
properties: {
@@ -194,6 +222,10 @@ fn list_user_tfa(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value,
pub fn user_commands() -> CommandLineInterface {
let cmd_def = CliCommandMap::new()
.insert("list", CliCommand::new(&API_METHOD_LIST_USERS))
+ .insert(
+ "update-hashes",
+ CliCommand::new(&API_METHOD_UPDATE_PASSWORD_HASHES),
+ )
.insert(
"create",
// fixme: howto handle password parameter?
--
2.39.2
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes Stefan Sterz
@ 2024-02-19 19:06 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 19:06 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:20, Stefan Sterz wrote:
> this uses proxmox-sys' `upgrade_hash` function to upgrade all stored
> hashes that don't already use the latest hash. this allows admins to
> upgrade their user's password hashes even if the user's don't log in.
Neat!
>
> however, as this requires a newer version of proxmox-sys'
> `verify_crypt_pw` function to work, make this a command that admin's
> can optional perform instead of automatically upgrading all hashes.
> admins can reset their users password to avoid issues when
> downgrading pbs or similar to work around potential issues here, but
> doing this automatically may cause problems.
>
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> note that once an admin has upgraded a hash, downgrading
> proxmox-backup-server will break logging in for all users with upgraded
> passwords. an admin would then need to manually reset the password via
> `proxmox-backup-manager user update <user> --password <pw>`.
I think this is why we should implement dealing with all hashes we desire
before the next major release, so that this doesn't happen. I can see this
potentially cause quite a stir for some users.
If we're able to differ between hash types (I think you mentioned we can)
then we should represent the variants we may use *now* and prefer using
the upgraded hash with the next major release. Or in other words, IMO we
should remain forward compatible, at least (and at most) for one major
version bump.
>
> src/auth_helpers.rs | 38 +++++++++++++++++++++++++-
> src/bin/proxmox_backup_manager/user.rs | 34 ++++++++++++++++++++++-
> 2 files changed, 70 insertions(+), 2 deletions(-)
>
> diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
> index 1a483d84..375ce190 100644
> --- a/src/auth_helpers.rs
> +++ b/src/auth_helpers.rs
> @@ -1,7 +1,8 @@
> +use std::collections::HashMap;
> use std::path::PathBuf;
> use std::sync::OnceLock;
>
> -use anyhow::Error;
> +use anyhow::{format_err, Error};
> use lazy_static::lazy_static;
> use openssl::pkey::{PKey, Private, Public};
> use openssl::rsa::Rsa;
> @@ -16,6 +17,41 @@ use serde_json::json;
> pub use crate::auth::setup_auth_context;
> pub use proxmox_auth_api::api::assemble_csrf_prevention_token;
>
> +pub fn upgrade_password_hashes() -> Result<(), Error> {
> + let data =
> + proxmox_sys::fs::file_get_json(crate::auth::SHADOW_CONFIG_FILENAME, Some(json!({})))?;
> + let mut new = HashMap::new();
> +
> + for (username, password) in data
> + .as_object()
> + .ok_or_else(|| format_err!("shadow file has an unexpected format!"))?
> + {
> + let upgraded_password = proxmox_sys::crypt::upgrade_hash(
> + password
> + .as_str()
> + .ok_or_else(|| format_err!("user without password found!"))?,
> + )?;
> +
> + new.insert(username.as_str(), upgraded_password);
> + }
> +
> + let mode = nix::sys::stat::Mode::from_bits_truncate(0o0600);
> + let options = proxmox_sys::fs::CreateOptions::new()
> + .perm(mode)
> + .owner(nix::unistd::ROOT)
> + .group(nix::unistd::Gid::from_raw(0));
> +
> + let new_data = serde_json::to_vec_pretty(&new)?;
> + proxmox_sys::fs::replace_file(
> + crate::auth::SHADOW_CONFIG_FILENAME,
> + &new_data,
> + options,
> + true,
> + )?;
> +
> + Ok(())
> +}
> +
> pub fn generate_csrf_key() -> Result<(), Error> {
> let path = PathBuf::from(configdir!("/csrf.key"));
>
> diff --git a/src/bin/proxmox_backup_manager/user.rs b/src/bin/proxmox_backup_manager/user.rs
> index 743c5d16..2daf2db7 100644
> --- a/src/bin/proxmox_backup_manager/user.rs
> +++ b/src/bin/proxmox_backup_manager/user.rs
> @@ -1,7 +1,9 @@
> -use anyhow::Error;
> +use anyhow::{bail, Error};
> use serde_json::Value;
>
> use std::collections::HashMap;
> +use std::io::IsTerminal;
> +use std::io::Write;
>
> use proxmox_router::{cli::*, ApiHandler, RpcEnvironment};
> use proxmox_schema::api;
> @@ -65,6 +67,32 @@ fn list_users(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value, Er
> Ok(Value::Null)
> }
>
> +#[api]
> +/// Updates the hashes of all users that don't use the current hashing method to the current
> +/// hashing method. This is a stop-gap meassure to avoid keeping insecure hashes when migrating
> +/// between password hash function.
> +fn update_password_hashes() -> Result<(), Error> {
> + // If we're on a TTY, query the user
> + if std::io::stdin().is_terminal() {
> + println!("You are about to update all password hashes that are managed by Proxmox Backup Server!");
> + print!("Are you sure you want to continue? (y/N): ");
> + let _ = std::io::stdout().flush();
> + use std::io::{BufRead, BufReader};
> + let mut line = String::new();
> + match BufReader::new(std::io::stdin()).read_line(&mut line) {
> + Ok(_) => match line.trim() {
> + "y" | "Y" => (), // continue
> + _ => bail!("Aborting."),
> + },
> + Err(err) => bail!("Failed to read line - {err}."),
> + }
> + }
> +
> + proxmox_backup::auth_helpers::upgrade_password_hashes()?;
> +
> + Ok(())
> +}
> +
> #[api(
> input: {
> properties: {
> @@ -194,6 +222,10 @@ fn list_user_tfa(param: Value, rpcenv: &mut dyn RpcEnvironment) -> Result<Value,
> pub fn user_commands() -> CommandLineInterface {
> let cmd_def = CliCommandMap::new()
> .insert("list", CliCommand::new(&API_METHOD_LIST_USERS))
> + .insert(
> + "update-hashes",
> + CliCommand::new(&API_METHOD_UPDATE_PASSWORD_HASHES),
> + )
> .insert(
> "create",
> // fixme: howto handle password parameter?
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes
2024-02-19 19:06 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 8:06 PM CET, Max Carrara wrote:
> On 2/15/24 16:20, Stefan Sterz wrote:
-- >8 snip 8< --
> > ---
> > note that once an admin has upgraded a hash, downgrading
> > proxmox-backup-server will break logging in for all users with upgraded
> > passwords. an admin would then need to manually reset the password via
> > `proxmox-backup-manager user update <user> --password <pw>`.
>
> I think this is why we should implement dealing with all hashes we desire
> before the next major release, so that this doesn't happen. I can see this
> potentially cause quite a stir for some users.
>
> If we're able to differ between hash types (I think you mentioned we can)
> then we should represent the variants we may use *now* and prefer using
> the upgraded hash with the next major release. Or in other words, IMO we
> should remain forward compatible, at least (and at most) for one major
> version bump.
>
as i'd retract this patch too (see my response to patch 07), i think
this is a non-issue for now.
but yeah, dealing with this in a cleaner manner might make sense. the
problem here is that we'd need the newer version of the verify function
in the version that you want to downgrade to (note that we do not
support downgrades). you'd get that with the patch to `proxmox-sys` and
a bumped dependency in pbs automatically though, so this is kind of
seperate to this patch.
-- >8 snip 8< ---
^ permalink raw reply [flat|nested] 35+ messages in thread
* [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys
2024-02-15 15:19 [pbs-devel] [PATCH proxmox{, -backup} 00/12] authentication cleanup and Stefan Sterz
` (10 preceding siblings ...)
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes Stefan Sterz
@ 2024-02-15 15:20 ` Stefan Sterz
2024-02-19 19:10 ` Max Carrara
11 siblings, 1 reply; 35+ messages in thread
From: Stefan Sterz @ 2024-02-15 15:20 UTC (permalink / raw)
To: pbs-devel
this commit moves new installations from our default rsa keys toward
smaller and more efficient ec keys. this uses the `PrivateKey` and
`PublicKey` structs from proxmox-auth-api to handle generating the
keys.
this means we can move aways from using openssl directly in the
auth_helpers and instead rely on the implementation in
`proxmox-auth-api`. thus, further unifying key handling in
`proxmox-auth-api`. this should make it easier to switch keys in the
future if necessary.
Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
---
note that this breaks the following scenario:
- a user installs pbs from a version after this patch was packaged
- proxmox-backup then creates a new ed25519 authkey
- the user manually forces a downgrade
proxmox-backup-api and proxmox-backup-proxy will now fail to start as
they cannot read the, from their perspective, malformed authkey.
src/auth.rs | 4 ++--
src/auth_helpers.rs | 53 ++++++++++++++-------------------------------
2 files changed, 18 insertions(+), 39 deletions(-)
diff --git a/src/auth.rs b/src/auth.rs
index 3379577f..20d2e39f 100644
--- a/src/auth.rs
+++ b/src/auth.rs
@@ -262,9 +262,9 @@ pub(crate) fn authenticate_user<'a>(
}
static PRIVATE_KEYRING: Lazy<Keyring> =
- Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone().into()));
+ Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone()));
static PUBLIC_KEYRING: Lazy<Keyring> =
- Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone().into()));
+ Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone()));
static AUTH_CONTEXT: OnceCell<PbsAuthContext> = OnceCell::new();
pub fn setup_auth_context(use_private_key: bool) {
diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
index 375ce190..f518c2ee 100644
--- a/src/auth_helpers.rs
+++ b/src/auth_helpers.rs
@@ -3,12 +3,9 @@ use std::path::PathBuf;
use std::sync::OnceLock;
use anyhow::{format_err, Error};
-use lazy_static::lazy_static;
-use openssl::pkey::{PKey, Private, Public};
-use openssl::rsa::Rsa;
use pbs_config::BackupLockGuard;
-use proxmox_auth_api::HMACKey;
+use proxmox_auth_api::{HMACKey, PrivateKey, PublicKey};
use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
use pbs_buildcfg::configdir;
@@ -87,26 +84,22 @@ pub fn generate_auth_key() -> Result<(), Error> {
return Ok(());
}
- let rsa = Rsa::generate(4096).unwrap();
-
- let priv_pem = rsa.private_key_to_pem()?;
+ let key = proxmox_auth_api::PrivateKey::generate_ec()?;
use nix::sys::stat::Mode;
replace_file(
&priv_path,
- &priv_pem,
+ &key.private_key_to_pem()?,
CreateOptions::new().perm(Mode::from_bits_truncate(0o0600)),
true,
)?;
- let public_pem = rsa.public_key_to_pem()?;
-
let backup_user = pbs_config::backup_user()?;
replace_file(
&public_path,
- &public_pem,
+ &key.public_key_to_pem()?,
CreateOptions::new()
.perm(Mode::from_bits_truncate(0o0640))
.owner(nix::unistd::ROOT)
@@ -134,36 +127,22 @@ pub fn csrf_secret() -> &'static HMACKey {
})
}
-fn load_public_auth_key() -> Result<PKey<Public>, Error> {
- let pem = file_get_contents(configdir!("/authkey.pub"))?;
- let rsa = Rsa::public_key_from_pem(&pem)?;
- let key = PKey::from_rsa(rsa)?;
-
- Ok(key)
-}
-
-pub fn public_auth_key() -> &'static PKey<Public> {
- lazy_static! {
- static ref KEY: PKey<Public> = load_public_auth_key().unwrap();
- }
-
- &KEY
-}
-
-fn load_private_auth_key() -> Result<PKey<Private>, Error> {
- let pem = file_get_contents(configdir!("/authkey.key"))?;
- let rsa = Rsa::private_key_from_pem(&pem)?;
- let key = PKey::from_rsa(rsa)?;
+pub fn public_auth_key() -> &'static PublicKey {
+ static KEY: OnceLock<PublicKey> = OnceLock::new();
- Ok(key)
+ KEY.get_or_init(|| {
+ let pem = file_get_contents(configdir!("/authkey.pub")).unwrap();
+ PublicKey::from_pem(&pem).unwrap()
+ })
}
-pub fn private_auth_key() -> &'static PKey<Private> {
- lazy_static! {
- static ref KEY: PKey<Private> = load_private_auth_key().unwrap();
- }
+pub fn private_auth_key() -> &'static PrivateKey {
+ static KEY: OnceLock<PrivateKey> = OnceLock::new();
- &KEY
+ KEY.get_or_init(|| {
+ let pem = file_get_contents(configdir!("/authkey.key")).unwrap();
+ PrivateKey::from_pem(&pem).unwrap()
+ })
}
const LDAP_PASSWORDS_FILENAME: &str = configdir!("/ldap_passwords.json");
--
2.39.2
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys
2024-02-15 15:20 ` [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys Stefan Sterz
@ 2024-02-19 19:10 ` Max Carrara
2024-02-23 9:26 ` Stefan Sterz
0 siblings, 1 reply; 35+ messages in thread
From: Max Carrara @ 2024-02-19 19:10 UTC (permalink / raw)
To: pbs-devel
On 2/15/24 16:20, Stefan Sterz wrote:
> this commit moves new installations from our default rsa keys toward
> smaller and more efficient ec keys. this uses the `PrivateKey` and
> `PublicKey` structs from proxmox-auth-api to handle generating the
> keys.
>
> this means we can move aways from using openssl directly in the
> auth_helpers and instead rely on the implementation in
> `proxmox-auth-api`. thus, further unifying key handling in
> `proxmox-auth-api`. this should make it easier to switch keys in the
> future if necessary.
>
> Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> ---
> note that this breaks the following scenario:
>
> - a user installs pbs from a version after this patch was packaged
> - proxmox-backup then creates a new ed25519 authkey
> - the user manually forces a downgrade
>
> proxmox-backup-api and proxmox-backup-proxy will now fail to start as
> they cannot read the, from their perspective, malformed authkey.
Can we not ensure we remain forward-compatible here, too? I acknowledge
that we probably shouldn't support this kind of scenario, but if we
were to add support for ed25519 keys now (while continuing to use RSA)
we wouldn't ever run into problems in that regard.
Not sure if it actually makes sense to remain forward-compatible here,
though.
>
> src/auth.rs | 4 ++--
> src/auth_helpers.rs | 53 ++++++++++++++-------------------------------
> 2 files changed, 18 insertions(+), 39 deletions(-)
>
> diff --git a/src/auth.rs b/src/auth.rs
> index 3379577f..20d2e39f 100644
> --- a/src/auth.rs
> +++ b/src/auth.rs
> @@ -262,9 +262,9 @@ pub(crate) fn authenticate_user<'a>(
> }
>
> static PRIVATE_KEYRING: Lazy<Keyring> =
> - Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone().into()));
> + Lazy::new(|| Keyring::with_private_key(crate::auth_helpers::private_auth_key().clone()));
> static PUBLIC_KEYRING: Lazy<Keyring> =
> - Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone().into()));
> + Lazy::new(|| Keyring::with_public_key(crate::auth_helpers::public_auth_key().clone()));
> static AUTH_CONTEXT: OnceCell<PbsAuthContext> = OnceCell::new();
>
> pub fn setup_auth_context(use_private_key: bool) {
> diff --git a/src/auth_helpers.rs b/src/auth_helpers.rs
> index 375ce190..f518c2ee 100644
> --- a/src/auth_helpers.rs
> +++ b/src/auth_helpers.rs
> @@ -3,12 +3,9 @@ use std::path::PathBuf;
> use std::sync::OnceLock;
>
> use anyhow::{format_err, Error};
> -use lazy_static::lazy_static;
> -use openssl::pkey::{PKey, Private, Public};
> -use openssl::rsa::Rsa;
>
> use pbs_config::BackupLockGuard;
> -use proxmox_auth_api::HMACKey;
> +use proxmox_auth_api::{HMACKey, PrivateKey, PublicKey};
> use proxmox_sys::fs::{file_get_contents, replace_file, CreateOptions};
>
> use pbs_buildcfg::configdir;
> @@ -87,26 +84,22 @@ pub fn generate_auth_key() -> Result<(), Error> {
> return Ok(());
> }
>
> - let rsa = Rsa::generate(4096).unwrap();
> -
> - let priv_pem = rsa.private_key_to_pem()?;
> + let key = proxmox_auth_api::PrivateKey::generate_ec()?;
>
> use nix::sys::stat::Mode;
>
> replace_file(
> &priv_path,
> - &priv_pem,
> + &key.private_key_to_pem()?,
> CreateOptions::new().perm(Mode::from_bits_truncate(0o0600)),
> true,
> )?;
>
> - let public_pem = rsa.public_key_to_pem()?;
> -
> let backup_user = pbs_config::backup_user()?;
>
> replace_file(
> &public_path,
> - &public_pem,
> + &key.public_key_to_pem()?,
> CreateOptions::new()
> .perm(Mode::from_bits_truncate(0o0640))
> .owner(nix::unistd::ROOT)
> @@ -134,36 +127,22 @@ pub fn csrf_secret() -> &'static HMACKey {
> })
> }
>
> -fn load_public_auth_key() -> Result<PKey<Public>, Error> {
> - let pem = file_get_contents(configdir!("/authkey.pub"))?;
> - let rsa = Rsa::public_key_from_pem(&pem)?;
> - let key = PKey::from_rsa(rsa)?;
> -
> - Ok(key)
> -}
> -
> -pub fn public_auth_key() -> &'static PKey<Public> {
> - lazy_static! {
> - static ref KEY: PKey<Public> = load_public_auth_key().unwrap();
> - }
> -
> - &KEY
> -}
> -
> -fn load_private_auth_key() -> Result<PKey<Private>, Error> {
> - let pem = file_get_contents(configdir!("/authkey.key"))?;
> - let rsa = Rsa::private_key_from_pem(&pem)?;
> - let key = PKey::from_rsa(rsa)?;
> +pub fn public_auth_key() -> &'static PublicKey {
> + static KEY: OnceLock<PublicKey> = OnceLock::new();
>
> - Ok(key)
> + KEY.get_or_init(|| {
> + let pem = file_get_contents(configdir!("/authkey.pub")).unwrap();
> + PublicKey::from_pem(&pem).unwrap()
> + })
> }
>
> -pub fn private_auth_key() -> &'static PKey<Private> {
> - lazy_static! {
> - static ref KEY: PKey<Private> = load_private_auth_key().unwrap();
> - }
> +pub fn private_auth_key() -> &'static PrivateKey {
> + static KEY: OnceLock<PrivateKey> = OnceLock::new();
>
> - &KEY
> + KEY.get_or_init(|| {
> + let pem = file_get_contents(configdir!("/authkey.key")).unwrap();
> + PrivateKey::from_pem(&pem).unwrap()
> + })
> }
>
> const LDAP_PASSWORDS_FILENAME: &str = configdir!("/ldap_passwords.json");
> --
> 2.39.2
>
>
>
> _______________________________________________
> pbs-devel mailing list
> pbs-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pbs-devel
>
>
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [pbs-devel] [PATCH proxmox-backup 12/12] auth: us ec keys as auth keys
2024-02-19 19:10 ` Max Carrara
@ 2024-02-23 9:26 ` Stefan Sterz
0 siblings, 0 replies; 35+ messages in thread
From: Stefan Sterz @ 2024-02-23 9:26 UTC (permalink / raw)
To: Proxmox Backup Server development discussion
On Mon Feb 19, 2024 at 8:10 PM CET, Max Carrara wrote:
> On 2/15/24 16:20, Stefan Sterz wrote:
> > this commit moves new installations from our default rsa keys toward
> > smaller and more efficient ec keys. this uses the `PrivateKey` and
> > `PublicKey` structs from proxmox-auth-api to handle generating the
> > keys.
> >
> > this means we can move aways from using openssl directly in the
> > auth_helpers and instead rely on the implementation in
> > `proxmox-auth-api`. thus, further unifying key handling in
> > `proxmox-auth-api`. this should make it easier to switch keys in the
> > future if necessary.
> >
> > Signed-off-by: Stefan Sterz <s.sterz@proxmox.com>
> > ---
> > note that this breaks the following scenario:
> >
> > - a user installs pbs from a version after this patch was packaged
> > - proxmox-backup then creates a new ed25519 authkey
> > - the user manually forces a downgrade
> >
> > proxmox-backup-api and proxmox-backup-proxy will now fail to start as
> > they cannot read the, from their perspective, malformed authkey.
>
> Can we not ensure we remain forward-compatible here, too? I acknowledge
> that we probably shouldn't support this kind of scenario, but if we
> were to add support for ed25519 keys now (while continuing to use RSA)
> we wouldn't ever run into problems in that regard.
>
> Not sure if it actually makes sense to remain forward-compatible here,
> though.
>
well yeah we could do something like this, but where to we draw the
boundary? we don't support downgrades so which version should introduce
being able to load and handle ed25519 keys and which version should then
start generating such keys?
i can separate out the loading and generating part in the next version
of this series though, than we can apply them as we see fit.
also two notes:
1) such a downgrade scenario can easily be fixed by the admin by removing
the authkey. proxmox-backup-api will then just generate a new one next
time it is started with the old rsa format.
2) existing installations aren't impacted at all, as no new authkey will
generated if one already exists. there is no mechanism that checks if
the key is old/insecure/in-efficient or should be upgrade for other
reasons. as long as a key exists, no new key is generated. pbs also
lacks key-roll-over/key-rotation afaict, so yeah, once and authkey is
generated, it is never modified by pbs itself.
-- >8 snip 8< --
^ permalink raw reply [flat|nested] 35+ messages in thread