Re: [pve-devel] [PATCH installer v2 1/1] assistant: validate: add verify-password option

["Christoph Heiss" <c.heiss@proxmox.com>]

Looks good overall, just some small nits inline :)

On Thu Sep 4, 2025 at 1:18 AM CEST, Peter wrote:
> [..]
> @@ -17,4 +17,5 @@ proxmox-installer-common = { workspace = true, features = [ "cli" ] }
>  serde_json.workspace = true
>  toml.workspace = true
>
> +proxmox-sys = { version = "1.0.0", features = [ "crypt" ] }

Forgot to mention on v1, but new dependencies must also be recorded in
debian/control.

You can use the command

  debcargo deb-dependencies proxmox-auto-install-assistant/Cargo.toml

to automatically generate that list (`debcargo` is available through the
normal Debian repositories) and afterwards

  wrap-and-sort -tkn

to sort that list.

>  glob = "0.3"
> diff --git a/proxmox-auto-install-assistant/src/main.rs b/proxmox-auto-install-assistant/src/main.rs
> index 5d6c1d5..98b4f23 100644
> --- a/proxmox-auto-install-assistant/src/main.rs
> +++ b/proxmox-auto-install-assistant/src/main.rs
> @@ -6,6 +6,9 @@
>
>  use anyhow::{Context, Result, bail, format_err};
>  use glob::Pattern;
> +use proxmox_sys::linux::tty::read_password;
> +use proxmox_sys::crypt::verify_crypt_pw;

These two lines should be alphabetically sorted - you can just run

  cargo fmt

before sending a patch, that will take care of all of that.

Personally I'd combine them:

use proxmox_sys::{linux::tty::read_password, crypt::verify_crypt_pw};

> [..]
>  impl cli::Subcommand for CommandValidateAnswerArgs {
>      fn parse(args: &mut cli::Arguments) -> Result<Self> {
>          Ok(Self {
>              debug: args.contains(["-d", "--debug"]),
> +            verify_password: args.contains("--verify-root-password"),
>              // Needs to be last
>              path: args.free_from_str()?,
>          })
> @@ -176,6 +182,7 @@ ARGUMENTS:
>
>  OPTIONS:
>    -d, --debug        Also show the full answer as parsed.
> +      --verify-root-password  Interactively verify the hashed root password.
>    -h, --help         Print this help
>    -V, --version      Print version

Please align all the descriptions.

> [..]
> @@ -545,6 +556,20 @@ fn validate_answer_file_keys(path: impl AsRef<Path> + fmt::Debug) -> Result<bool
>      }
>  }
>
> +fn verify_hashed_password_interactive(answer: &Answer) -> Result<()> {
> +    if let Some(hashed) = &answer.global.root_password_hashed {
> +        println!("Verifying hashed root password.");
> +
> +        let password = String::from_utf8(read_password("Enter root password to verify: ")?)?;
> +        verify_crypt_pw(&password, hashed)?;

verify_crypt_pw(&password, hashed).context("Failed to verify hashed root password")?;

Makes the output just a bit nicer when verification fails.

> +
> +        println!("Password matches hashed password.");

println!("Password matches hashed root password.");

For consistency with the other messages.

> +        Ok(())
> +    } else {
> +        bail!("'root-password-hashed' not set in answer file, cannot verify.");
> +    }
> +}
> +


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