Re: [pbs-devel] [PATCH proxmox-backup 11/12] auth/manager: add manager command to upgrade hashes

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

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