[pve-devel] [PATCH installer v2 0/7] use email regex from HTML spec for validation

[pve-devel] [PATCH installer v2 0/7] use email regex from HTML spec for validation

[Christoph Heiss]

This uses the regex for <input type="email" /> elements as defined in
the HTML spec to validate emails in the installer. That regex should be
a lot more robust than our current on and cover basically all cases of
email addresses that might be encountered in the while.

Also, patch #6 adds validation for the auto-installer `global.mailto`
option too, while at it - which previously was not validated in any
capacity.

Patches #1 through #4 are just small cleanups/fixes and may be applied
independently.

[0] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address

history
=======

Notable changes v1 -> v2:
  * rebased on current master

diffstat
========

Christoph Heiss (7):
  cargo: convert `anyhow` to workspace dependency
  tui: fix new clippy lint
  auto-installer: drop some unneeded `pub` modifiers
  auto-installer: print full anyhow message on failure
  tui: use email regex from HTML specification for validation
  auto-installer: validate `global.mailto` answer option
  proxinstall: use email regex from HTML specification for validation

 Cargo.toml                                    |  2 ++
 Proxmox/Makefile                              |  1 +
 Proxmox/Sys.pm                                |  9 +++++++
 proxinstall                                   |  3 ++-
 proxmox-auto-install-assistant/Cargo.toml     |  2 +-
 proxmox-auto-installer/Cargo.toml             |  2 +-
 .../src/bin/proxmox-auto-installer.rs         |  2 +-
 proxmox-auto-installer/src/utils.rs           | 20 +++++++++------
 proxmox-chroot/Cargo.toml                     |  2 +-
 proxmox-fetch-answer/Cargo.toml               |  2 +-
 proxmox-installer-common/Cargo.toml           |  1 +
 proxmox-installer-common/src/lib.rs           |  3 +++
 proxmox-installer-common/src/options.rs       | 25 +++++++++++++++++++
 proxmox-tui-installer/Cargo.toml              |  1 -
 proxmox-tui-installer/src/main.rs             | 22 ++++++----------
 proxmox-tui-installer/src/options.rs          |  3 ++-
 16 files changed, 69 insertions(+), 31 deletions(-)
 create mode 100644 Proxmox/Sys.pm

-- 
2.44.0



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

[pve-devel] [PATCH installer v2 1/7] cargo: convert `anyhow` to workspace dependency

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 Cargo.toml                                | 2 ++
 proxmox-auto-install-assistant/Cargo.toml | 2 +-
 proxmox-auto-installer/Cargo.toml         | 2 +-
 proxmox-chroot/Cargo.toml                 | 2 +-
 proxmox-fetch-answer/Cargo.toml           | 2 +-
 5 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/Cargo.toml b/Cargo.toml
index 1e730ce..ec1deaf 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -9,3 +9,5 @@ members = [
     "proxmox-tui-installer",
 ]
 
+[workspace.dependencies]
+anyhow = "1.0"
diff --git a/proxmox-auto-install-assistant/Cargo.toml b/proxmox-auto-install-assistant/Cargo.toml
index 9473b3d..ab70d39 100644
--- a/proxmox-auto-install-assistant/Cargo.toml
+++ b/proxmox-auto-install-assistant/Cargo.toml
@@ -11,7 +11,7 @@ exclude = [ "build", "debian" ]
 homepage = "https://www.proxmox.com"
 
 [dependencies]
-anyhow = "1.0"
+anyhow.workspace = true
 clap = { version = "4.0", features = ["derive"] }
 glob = "0.3"
 log = "0.4.20"
diff --git a/proxmox-auto-installer/Cargo.toml b/proxmox-auto-installer/Cargo.toml
index 4663902..0353d6a 100644
--- a/proxmox-auto-installer/Cargo.toml
+++ b/proxmox-auto-installer/Cargo.toml
@@ -11,7 +11,7 @@ exclude = [ "build", "debian" ]
 homepage = "https://www.proxmox.com"
 
 [dependencies]
-anyhow = "1.0"
+anyhow.workspace = true
 clap = { version = "4.0", features = ["derive"] }
 glob = "0.3"
 log = "0.4.20"
diff --git a/proxmox-chroot/Cargo.toml b/proxmox-chroot/Cargo.toml
index 43b96ff..6247baa 100644
--- a/proxmox-chroot/Cargo.toml
+++ b/proxmox-chroot/Cargo.toml
@@ -8,7 +8,7 @@ exclude = [ "build", "debian" ]
 homepage = "https://www.proxmox.com"
 
 [dependencies]
-anyhow = "1.0"
+anyhow.workspace = true
 clap = { version = "4.0", features = ["derive"] }
 nix = "0.26.1"
 proxmox-installer-common = { path = "../proxmox-installer-common" }
diff --git a/proxmox-fetch-answer/Cargo.toml b/proxmox-fetch-answer/Cargo.toml
index fcdc1b6..9149176 100644
--- a/proxmox-fetch-answer/Cargo.toml
+++ b/proxmox-fetch-answer/Cargo.toml
@@ -13,7 +13,7 @@ homepage = "https://www.proxmox.com"
 # See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html
 
 [dependencies]
-anyhow = "1.0"
+anyhow.workspace = true
 hex = "0.4"
 log = "0.4.20"
 native-tls = "0.2"
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 2/7] tui: fix new clippy lint

[Christoph Heiss]

warning: unnecessary hashes around raw string literal
   = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#needless_raw_string_hashes
   = note: `#[warn(clippy::needless_raw_string_hashes)]` on by default

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 proxmox-tui-installer/src/main.rs | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index 3fb87a7..c36daa5 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -35,12 +35,12 @@ use views::{
 };
 
 // TextView::center() seems to garble the first two lines, so fix it manually here.
-const PROXMOX_LOGO: &str = r#"
+const PROXMOX_LOGO: &str = r"
  ____
 |  _ \ _ __ _____  ___ __ ___   _____  __
 | |_) | '__/ _ \ \/ / '_ ` _ \ / _ \ \/ /
 |  __/| | | (_) >  <| | | | | | (_) >  <
-|_|   |_|  \___/_/\_\_| |_| |_|\___/_/\_\ "#;
+|_|   |_|  \___/_/\_\_| |_| |_|\___/_/\_\ ";
 
 struct InstallerView {
     view: ResizedView<Dialog>,
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 3/7] auto-installer: drop some unneeded `pub` modifiers

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 proxmox-auto-installer/src/utils.rs | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index 45ad222..cc483c9 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -16,7 +16,7 @@ use proxmox_installer_common::{
 };
 use serde::{Deserialize, Serialize};
 
-pub fn get_network_settings(
+fn get_network_settings(
     answer: &Answer,
     udev_info: &UdevInfo,
     runtime_info: &RuntimeInfo,
@@ -145,7 +145,7 @@ pub fn get_matched_udev_indexes(
     Ok(matches)
 }
 
-pub fn set_disks(
+fn set_disks(
     answer: &Answer,
     udev_info: &UdevInfo,
     runtime_info: &RuntimeInfo,
@@ -263,7 +263,7 @@ fn set_selected_disks(
     Ok(())
 }
 
-pub fn get_first_selected_disk(config: &InstallConfig) -> usize {
+fn get_first_selected_disk(config: &InstallConfig) -> usize {
     config
         .disk_selection
         .iter()
@@ -274,7 +274,7 @@ pub fn get_first_selected_disk(config: &InstallConfig) -> usize {
         .expect("could not parse key to usize")
 }
 
-pub fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<()> {
+fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<()> {
     info!("Verifying locale settings");
     if !locales
         .countries
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 4/7] auto-installer: print full anyhow message on failure

[Christoph Heiss]

Otherwise, context info is simply lost -- which might contain valuable
information for the user.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 proxmox-auto-installer/src/bin/proxmox-auto-installer.rs | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
index bf6f8fb..f2197a8 100644
--- a/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
+++ b/proxmox-auto-installer/src/bin/proxmox-auto-installer.rs
@@ -86,7 +86,7 @@ fn main() -> ExitCode {
     match run_installation(&answer, &locales, &runtime_info, &udevadm_info, &setup_info) {
         Ok(_) => info!("Installation done."),
         Err(err) => {
-            error!("Installation failed: {err}");
+            error!("Installation failed: {err:#}");
             return exit_failure(answer.global.reboot_on_error);
         }
     }
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 5/7] tui: use email regex from HTML specification for validation

[Christoph Heiss]

That regex should be a lot more accurate in what it allows - if it's
good enough for the HTML spec, it should be for us too.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * move `EMAIL_DEFAULT_PLACEHOLDER` constant to
    proxmox-installer-common/lib.rs

 proxmox-installer-common/Cargo.toml     |  1 +
 proxmox-installer-common/src/lib.rs     |  3 +++
 proxmox-installer-common/src/options.rs | 25 +++++++++++++++++++++++++
 proxmox-tui-installer/Cargo.toml        |  1 -
 proxmox-tui-installer/src/main.rs       | 18 +++++-------------
 proxmox-tui-installer/src/options.rs    |  3 ++-
 6 files changed, 36 insertions(+), 15 deletions(-)

diff --git a/proxmox-installer-common/Cargo.toml b/proxmox-installer-common/Cargo.toml
index 70f828a..e151b0e 100644
--- a/proxmox-installer-common/Cargo.toml
+++ b/proxmox-installer-common/Cargo.toml
@@ -8,6 +8,7 @@ exclude = [ "build", "debian" ]
 homepage = "https://www.proxmox.com"
 
 [dependencies]
+anyhow.workspace = true
 regex = "1.7"
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
diff --git a/proxmox-installer-common/src/lib.rs b/proxmox-installer-common/src/lib.rs
index 850e825..028b43c 100644
--- a/proxmox-installer-common/src/lib.rs
+++ b/proxmox-installer-common/src/lib.rs
@@ -4,3 +4,6 @@ pub mod setup;
 pub mod utils;
 
 pub const RUNTIME_DIR: &str = "/run/proxmox-installer";
+
+/// Default placeholder value for the administrator email address.
+pub const EMAIL_DEFAULT_PLACEHOLDER: &str = "mail@example.invalid";
diff --git a/proxmox-installer-common/src/options.rs b/proxmox-installer-common/src/options.rs
index 9375ded..ea06eee 100644
--- a/proxmox-installer-common/src/options.rs
+++ b/proxmox-installer-common/src/options.rs
@@ -1,5 +1,8 @@
+use anyhow::{bail, Result};
+use regex::Regex;
 use serde::Deserialize;
 use std::net::{IpAddr, Ipv4Addr};
+use std::sync::OnceLock;
 use std::{cmp, fmt};
 
 use crate::setup::{
@@ -403,6 +406,28 @@ impl NetworkOptions {
     }
 }
 
+/// Validates an email address using the regex for <input type="email" /> elements
+/// as defined in the HTML specification [0].
+/// Using that /should/ cover all possible cases that are encountered in the wild.
+///
+/// It additionally checks whether the email our default email placeholder value.
+///
+/// [0] https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
+pub fn email_validate(email: &str) -> Result<()> {
+    static RE: OnceLock<Regex> = OnceLock::new();
+    let re = RE.get_or_init(|| {
+        Regex::new(r"^[a-zA-Z0-9.!#$%&'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$").unwrap()
+    });
+
+    if !re.is_match(email) {
+        bail!("Email does not look like a valid address (user@domain.tld)")
+    } else if email == crate::EMAIL_DEFAULT_PLACEHOLDER {
+        bail!("Invalid (default) email address")
+    }
+
+    Ok(())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
diff --git a/proxmox-tui-installer/Cargo.toml b/proxmox-tui-installer/Cargo.toml
index 2516468..618904c 100644
--- a/proxmox-tui-installer/Cargo.toml
+++ b/proxmox-tui-installer/Cargo.toml
@@ -11,5 +11,4 @@ homepage = "https://www.proxmox.com"
 cursive = { version = "0.21", default-features = false, features = ["crossterm-backend"] }
 serde = { version = "1.0", features = ["derive"] }
 serde_json = "1.0"
-regex = "1.7"
 proxmox-installer-common = { path = "../proxmox-installer-common" }
diff --git a/proxmox-tui-installer/src/main.rs b/proxmox-tui-installer/src/main.rs
index c36daa5..d306969 100644
--- a/proxmox-tui-installer/src/main.rs
+++ b/proxmox-tui-installer/src/main.rs
@@ -13,13 +13,11 @@ use cursive::{
     Cursive, CursiveRunnable, ScreenId, View, XY,
 };
 
-use regex::Regex;
-
 mod options;
 use options::{InstallerOptions, PasswordOptions};
 
 use proxmox_installer_common::{
-    options::{BootdiskOptions, NetworkOptions, TimezoneOptions},
+    options::{email_validate, BootdiskOptions, NetworkOptions, TimezoneOptions},
     setup::{installer_setup, LocaleInfo, ProxmoxProduct, RuntimeInfo, SetupInfo},
     utils::Fqdn,
 };
@@ -448,18 +446,12 @@ fn password_dialog(siv: &mut Cursive) -> InstallerView {
                     .get_value::<EditView, _>(2)
                     .ok_or("failed to retrieve email")?;
 
-                let email_regex =
-                    Regex::new(r"^[\w\+\-\~]+(\.[\w\+\-\~]+)*@[a-zA-Z0-9\-]+(\.[a-zA-Z0-9\-]+)*$")
-                        .unwrap();
-
                 if root_password.len() < 5 {
-                    Err("password too short, must be at least 5 characters long")
+                    Err("password too short, must be at least 5 characters long".to_owned())
                 } else if root_password != confirm_password {
-                    Err("passwords do not match")
-                } else if email == "mail@example.invalid" {
-                    Err("invalid email address")
-                } else if !email_regex.is_match(&email) {
-                    Err("Email does not look like a valid address (user@domain.tld)")
+                    Err("passwords do not match".to_owned())
+                } else if let Err(err) = email_validate(&email) {
+                    Err(err.to_string())
                 } else {
                     Ok(PasswordOptions {
                         root_password,
diff --git a/proxmox-tui-installer/src/options.rs b/proxmox-tui-installer/src/options.rs
index 19992ca..b14ecf7 100644
--- a/proxmox-tui-installer/src/options.rs
+++ b/proxmox-tui-installer/src/options.rs
@@ -5,6 +5,7 @@ use proxmox_installer_common::{
         BootdiskOptions, BtrfsRaidLevel, FsType, NetworkOptions, TimezoneOptions, ZfsRaidLevel,
     },
     setup::LocaleInfo,
+    EMAIL_DEFAULT_PLACEHOLDER,
 };
 
 pub const FS_TYPES: &[FsType] = {
@@ -33,7 +34,7 @@ pub struct PasswordOptions {
 impl Default for PasswordOptions {
     fn default() -> Self {
         Self {
-            email: "mail@example.invalid".to_string(),
+            email: EMAIL_DEFAULT_PLACEHOLDER.to_string(),
             root_password: String::new(),
         }
     }
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 6/7] auto-installer: validate `global.mailto` answer option

[Christoph Heiss]

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * instead of re-using/-naming the `verify_locale_settings()` for the
    email validation, combine it the with root password validation

 proxmox-auto-installer/src/utils.rs | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/proxmox-auto-installer/src/utils.rs b/proxmox-auto-installer/src/utils.rs
index cc483c9..afaad85 100644
--- a/proxmox-auto-installer/src/utils.rs
+++ b/proxmox-auto-installer/src/utils.rs
@@ -1,4 +1,4 @@
-use anyhow::{bail, Context as _, Result};
+use anyhow::{bail, Context, Result};
 use clap::ValueEnum;
 use glob::Pattern;
 use log::info;
@@ -9,7 +9,7 @@ use crate::{
     udevinfo::UdevInfo,
 };
 use proxmox_installer_common::{
-    options::{FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption},
+    options::{email_validate, FsType, NetworkOptions, ZfsChecksumOption, ZfsCompressOption},
     setup::{
         InstallConfig, InstallRootPassword, InstallZfsOption, LocaleInfo, RuntimeInfo, SetupInfo,
     },
@@ -303,7 +303,11 @@ fn verify_locale_settings(answer: &Answer, locales: &LocaleInfo) -> Result<()> {
     Ok(())
 }
 
-fn verify_root_password_settings(answer: &Answer) -> Result<()> {
+fn verify_email_and_root_password_settings(answer: &Answer) -> Result<()> {
+    info!("Verifying email and root password settings");
+
+    email_validate(&answer.global.mailto).with_context(|| answer.global.mailto.clone())?;
+
     if answer.global.root_password.is_some() && answer.global.root_password_hashed.is_some() {
         bail!("`global.root_password` and `global.root_password_hashed` cannot be set at the same time");
     } else if answer.global.root_password.is_none() && answer.global.root_password_hashed.is_none()
@@ -329,7 +333,7 @@ pub fn parse_answer(
     let network_settings = get_network_settings(answer, udev_info, runtime_info, setup_info)?;
 
     verify_locale_settings(answer, locales)?;
-    verify_root_password_settings(answer)?;
+    verify_email_and_root_password_settings(answer)?;
 
     let mut config = InstallConfig {
         autoreboot: 1_usize,
-- 
2.46.0



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

[pve-devel] [PATCH installer v2 7/7] proxinstall: use email regex from HTML specification for validation

[Christoph Heiss]

That regex should be a lot more accurate in what it allows - if it's
good enough for the HTML spec, it should be for us too.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
Changes v1 -> v2:
  * no changes

 Proxmox/Makefile | 1 +
 Proxmox/Sys.pm   | 9 +++++++++
 proxinstall      | 3 ++-
 3 files changed, 12 insertions(+), 1 deletion(-)
 create mode 100644 Proxmox/Sys.pm

diff --git a/Proxmox/Makefile b/Proxmox/Makefile
index 035626b..edcabc1 100644
--- a/Proxmox/Makefile
+++ b/Proxmox/Makefile
@@ -12,6 +12,7 @@ PERL_MODULES=\
     Install/RunEnv.pm \
     Install/StorageConfig.pm \
     Log.pm \
+    Sys.pm \
     Sys/Block.pm \
     Sys/Command.pm \
     Sys/File.pm \
diff --git a/Proxmox/Sys.pm b/Proxmox/Sys.pm
new file mode 100644
index 0000000..afc6780
--- /dev/null
+++ b/Proxmox/Sys.pm
@@ -0,0 +1,9 @@
+package Proxmox::Sys;
+
+use strict;
+use warnings;
+
+# The HTML specification actually gives a "blessed" regex for email addresses:
+# https://html.spec.whatwg.org/multipage/input.html#valid-e-mail-address
+# Using that /should/ cover all possible cases that are encountered in the wild.
+our $EMAIL_RE = '^[a-zA-Z0-9.!#$%&\'*+/=?^_`{|}~-]+@[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?(?:\.[a-zA-Z0-9](?:[a-zA-Z0-9-]{0,61}[a-zA-Z0-9])?)*$';
diff --git a/proxinstall b/proxinstall
index 12f3eaa..8b7f1c0 100755
--- a/proxinstall
+++ b/proxinstall
@@ -33,6 +33,7 @@ my $iso_env = Proxmox::Install::ISOEnv::get();
 use Proxmox::Install;
 use Proxmox::Install::Config;
 
+use Proxmox::Sys;
 use Proxmox::Sys::Block qw(get_cached_disks);
 use Proxmox::Sys::Command qw(syscmd);
 use Proxmox::Sys::File qw(file_read_all file_write_all);
@@ -733,7 +734,7 @@ sub create_password_view {
 	}
 
 	my $t3 = $eme->get_text;
-	if ($t3 !~ m/^[\w\+\-\~]+(\.[\w\+\-\~]+)*@[a-zA-Z0-9\-]+(\.[a-zA-Z0-9\-]+)*$/) {
+	if ($t3 !~ m/$Proxmox::Sys::EMAIL_RE/) {
 	    Proxmox::UI::message("Email does not look like a valid address (user\@domain.tld)");
 	    $eme->grab_focus();
 	    return;
-- 
2.46.0



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