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

[Peter via pve-devel <pve-devel@lists.proxmox.com>]

[-- Attachment #1: Type: message/rfc822, Size: 10438 bytes --]

From: Peter <pjcreath+proxmox@gmail.com>
To: pve-devel@lists.proxmox.com
Subject: Re: [PATCH installer 1/1] assistant: validate: add verify-password option
Date: Wed, 3 Sep 2025 09:41:28 -0400
Message-ID: <CADbnQj468=prtr7k3vsNjqdMwC4qybKOBV0XjBis2erq=r8m+w@mail.gmail.com>

Yes, I've signed the CLA.  Thank you for your feedback, I'll work on a
revised patch!

On Mon, Sep 1, 2025 at 6:09 AM Christoph Heiss <c.heiss@proxmox.com> wrote:

> On Fri Aug 29, 2025 at 8:26 PM CEST, Peter wrote:
> > Adds an option to interactively verify the hashed root password in
> > the answer file, so that mistakes can be caught before installation.
>
> Sounds like a useful option to me!
>
> >
> > Signed-off-by: Peter <pjcreath+proxmox@gmail.com>
> > ---
> >
> >  In preparing an answer file for auto-installation, I somehow mangled
> >  the hashed root password, which I only discovered after performing
> >  the automated installation.
> >
> >  This patch adds an option to the auto-install assistant that lets
> >  the user verify the hash in the answer file by interactively typing
> >  in the expected password and checking it against the hash.
> >
> >  I don't love that I had to add an unsafe call to crypt(), but there
> >  isn't a Rust implementation of yescrypt. To minimize the impact
> >  I wrapped the unsafe call in its own function.
> >
> >  This is my first submission to this mailing list. I've tried to
> >  follow all of the guidelines in the developer documentation, so
> >  please forgive any oversights and let me know if there's anything
> >  I should do differently.
>
> Did you sign our CLA [0]? Just to be sure we can accept it :)
>
> [0]
> https://pve.proxmox.com/wiki/Developer_Documentation#Software_License_and_Copyright
>
> >
> >  proxmox-auto-install-assistant/Cargo.toml  |  2 +
> >  proxmox-auto-install-assistant/src/main.rs | 51 +++++++++++++++++++++-
> >  2 files changed, 52 insertions(+), 1 deletion(-)
> >
> > diff --git a/proxmox-auto-install-assistant/Cargo.toml
> b/proxmox-auto-install-assistant/Cargo.toml
> > index 9b4a9c4..8af7d9d 100644
> > --- a/proxmox-auto-install-assistant/Cargo.toml
> > +++ b/proxmox-auto-install-assistant/Cargo.toml
> > @@ -17,4 +17,6 @@ proxmox-installer-common = { workspace = true,
> features = [ "cli" ] }
> >  serde_json.workspace = true
> >  toml.workspace = true
> >
> > +crypt-sys = "0.1"
>
> That crate is (currently) not packaged for Debian, so that would have to
> be done first.
>
> But fortunately we got that already properly & safely wrapped in our
> `proxmox-sys` [1] crate, including a function for verifying passwords.
> You can find it in proxmox-sys/src/crypt.rs, including some example
> usages in the unit tests.
>
> For the proxmox-sys crate you need to pull in our `devel` repository, as
> described in [2].
>
> [1] https://git.proxmox.com/?p=proxmox.git;a=tree;f=proxmox-sys
> [2]
> https://pve.proxmox.com/wiki/Developer_Documentation#Development_Package_Repository
>
> >  glob = "0.3"
> > +rpassword = "7.2"
>
> For this we also already got some functionality in our `proxmox-sys`
> crate, see below for an example.
>
> > diff --git a/proxmox-auto-install-assistant/src/main.rs
> b/proxmox-auto-install-assistant/src/main.rs
> > index 5d6c1d5..88d0032 100644
> > --- a/proxmox-auto-install-assistant/src/main.rs
> > +++ b/proxmox-auto-install-assistant/src/main.rs
> [..]
> >
> >  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-password"),
>
> This should probably throw an error if stdin (at least) is not connected
> to an interactive terminal, this can be checked with:
>
>   std::io::stdin().is_terminal()
>
> Also, IMO the option should be named `--verify-root-password`, to make
> its name a bit more precise.
>
> >              // 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-password  Interactively verify the hashed root password.
> >    -h, --help         Print this help
> >    -V, --version      Print version
> >      "#,
> > @@ -545,6 +552,42 @@ 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 = prompt_password("Enter root password to verify:
> ")
> > +            .context("Failed to read password")?;
>
> So this could be something like:
>
>   use proxmox_sys::linux::tty;
>   let password = tty::read_readpassword("Enter root password to verify: ")
>       .context("Failed to read password")?;
>
> > +
> > +        if system_crypt(&password, hashed)? {
> > +            println!("Password matches hashed password.");
> > +            Ok(())
> > +        } else {
> > +            bail!("Password does not match hashed password.");
> > +        }
> > +    } else {
> > +        bail!("'root-password-hashed' not set in answer file, cannot
> verify.");
> > +    }
> > +}
>
>

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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