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

[Max Carrara <m.carrara@proxmox.com>]

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(())
>          })