[pve-devel] [PATCH installer 0/5] proxinstall, tui: improve hostname/FQDN validation

[pve-devel] [PATCH installer 0/5] proxinstall, tui: improve hostname/FQDN validation

[Christoph Heiss]

This series improves various aspects regarding FQDN handling and
validation across both the GUI and TUI installer.

It (partially) addresses issue #5230 [0] in patch #5, by fixing the
regex through which we validate FQDNs in the GUI installer.

It also refactors the FQDN validation/parsing in the GUI installer, by
moving it into its own subroutine - making the whole thing bit easier to
reason about, as well as adding loads of test cases.

[0] https://bugzilla.proxmox.com/show_bug.cgi?id=5230

Christoph Heiss (5):
  common: fqdn: do not allow overlong FQDNs as per Debian spec
  common: fqdn: implement case-insensitive comparison as per RFC 952
  proxinstall: avoid open-coding FQDN sanity check
  sys: net: do not allow overlong FQDNs as per RFCs and Debian spec
  fix #5230: sys: net: properly escape FQDN regex

 Proxmox/Sys/Net.pm                    | 29 +++++++++-
 proxinstall                           | 22 +++-----
 proxmox-installer-common/src/utils.rs | 79 ++++++++++++++++++++++++++-
 test/Makefile                         |  6 +-
 test/parse-fqdn.pl                    | 57 +++++++++++++++++++
 5 files changed, 172 insertions(+), 21 deletions(-)
 create mode 100755 test/parse-fqdn.pl

--
2.43.0

[pve-devel] [PATCH installer 1/5] common: fqdn: do not allow overlong FQDNs as per Debian spec

[Christoph Heiss]

Debian limits labels to 63 characters each and the total length to 253
characters [0].

While at it, reference all the RFCs that apply when parsing FQDNs.

[0] https://manpages.debian.org/stable/manpages/hostname.7.en.html

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/utils.rs | 45 ++++++++++++++++++++++++++-
 1 file changed, 44 insertions(+), 1 deletion(-)

diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index c038524..067e025 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -117,6 +117,7 @@ pub enum FqdnParseError {
     MissingHostname,
     NumericHostname,
     InvalidPart(String),
+    TooLong(usize),
 }

 impl fmt::Display for FqdnParseError {
@@ -129,17 +130,46 @@ impl fmt::Display for FqdnParseError {
                 f,
                 "FQDN must only consist of alphanumeric characters and dashes. Invalid part: '{part}'",
             ),
+            TooLong(len) => write!(f, "FQDN too long: {len} > {}", Fqdn::MAX_LENGTH),
         }
     }
 }

+/// A type for safely representing fully-qualified domain names (FQDNs).
+///
+/// It considers following RFCs:
+/// https://www.ietf.org/rfc/rfc952.txt (sec. "ASSUMPTIONS", 1.)
+/// https://www.ietf.org/rfc/rfc1035.txt (sec. 2.3. "Conventions")
+/// https://www.ietf.org/rfc/rfc1123.txt (sec. 2.1. "Host Names and Numbers")
+/// https://www.ietf.org/rfc/rfc3492.txt
+/// https://www.ietf.org/rfc/rfc4343.txt
+///
+/// .. and applies some restriction given by Debian, e.g. 253 instead of 255
+/// maximum total length and maximum 63 characters per label.
+/// https://manpages.debian.org/stable/manpages/hostname.7.en.html
+///
+/// Additionally:
+/// - It enforces the restriction as per Bugzilla #1054, in that
+///   purely numeric hostnames are not allowed - against RFC1123 sec. 2.1.
+///
+/// Some terminology:
+/// - "label" - a single part of a FQDN, e.g. <label>.<label>.<tld>
 #[derive(Clone, Debug, Eq, PartialEq)]
 pub struct Fqdn {
     parts: Vec<String>,
 }

 impl Fqdn {
+    /// Maximum length of a single label of the FQDN
+    const MAX_LABEL_LENGTH: usize = 63;
+    /// Maximum total length of the FQDN
+    const MAX_LENGTH: usize = 253;
+
     pub fn from(fqdn: &str) -> Result<Self, FqdnParseError> {
+        if fqdn.len() > Self::MAX_LENGTH {
+            return Err(FqdnParseError::TooLong(fqdn.len()));
+        }
+
         let parts = fqdn
             .split('.')
             .map(ToOwned::to_owned)
@@ -154,7 +184,8 @@ impl Fqdn {
         if parts.len() < 2 {
             Err(FqdnParseError::MissingHostname)
         } else if parts[0].chars().all(|c| c.is_ascii_digit()) {
-            // Not allowed/supported on Debian systems.
+            // Do not allow a purely numeric hostname, see:
+            // https://bugzilla.proxmox.com/show_bug.cgi?id=1054
             Err(FqdnParseError::NumericHostname)
         } else {
             Ok(Self { parts })
@@ -182,6 +213,7 @@ impl Fqdn {

     fn validate_single(s: &String) -> bool {
         !s.is_empty()
+            && s.len() <= Self::MAX_LABEL_LENGTH
             // First character must be alphanumeric
             && s.chars()
                 .next()
@@ -243,9 +275,20 @@ mod tests {
         assert_eq!(Fqdn::from("foo.com-"), Err(InvalidPart("com-".to_owned())));
         assert_eq!(Fqdn::from("-o-.com"), Err(InvalidPart("-o-".to_owned())));

+        // https://bugzilla.proxmox.com/show_bug.cgi?id=1054
         assert_eq!(Fqdn::from("123.com"), Err(NumericHostname));
         assert!(Fqdn::from("foo123.com").is_ok());
         assert!(Fqdn::from("123foo.com").is_ok());
+
+        assert!(Fqdn::from(&format!("{}.com", "a".repeat(63))).is_ok());
+        assert_eq!(
+            Fqdn::from(&format!("{}.com", "a".repeat(250))),
+            Err(TooLong(254)),
+        );
+        assert_eq!(
+            Fqdn::from(&format!("{}.com", "a".repeat(64))),
+            Err(InvalidPart("a".repeat(64))),
+        );
     }

     #[test]
--
2.43.0

[pve-devel] [PATCH installer 2/5] common: fqdn: implement case-insensitive comparison as per RFC 952

[Christoph Heiss]

Multiple DNS-related RFCs (notably RFC 952, RFC 1035 and RFC 4343)
reinforce that FQDN must not be case-sensitive.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 proxmox-installer-common/src/utils.rs | 28 ++++++++++++++++++++++++++-
 1 file changed, 27 insertions(+), 1 deletion(-)

diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 067e025..0ed7438 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -154,7 +154,7 @@ impl fmt::Display for FqdnParseError {
 ///
 /// Some terminology:
 /// - "label" - a single part of a FQDN, e.g. <label>.<label>.<tld>
-#[derive(Clone, Debug, Eq, PartialEq)]
+#[derive(Clone, Debug, Eq)]
 pub struct Fqdn {
     parts: Vec<String>,
 }
@@ -257,6 +257,26 @@ impl<'de> Deserialize<'de> for Fqdn {
     }
 }
 
+impl PartialEq for Fqdn {
+    fn eq(&self, other: &Self) -> bool {
+        // Case-insensitive comparison, as per RFC 952 "ASSUMPTIONS",
+        // RFC 1035 sec. 2.3.3. "Character Case" and RFC 4343 as a whole
+        let a = self
+            .parts
+            .iter()
+            .map(|s| s.to_lowercase())
+            .collect::<Vec<String>>();
+
+        let b = other
+            .parts
+            .iter()
+            .map(|s| s.to_lowercase())
+            .collect::<Vec<String>>();
+
+        a == b
+    }
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -309,4 +329,10 @@ mod tests {
             "foo.example.com"
         );
     }
+
+    #[test]
+    fn fqdn_compare() {
+        assert_eq!(Fqdn::from("example.com"), Fqdn::from("ExAmPle.Com"));
+        assert_eq!(Fqdn::from("ExAmPle.Com"), Fqdn::from("example.com"));
+    }
 }
-- 
2.43.0

[pve-devel] [PATCH installer 3/5] proxinstall: avoid open-coding FQDN sanity check

[Christoph Heiss]

.. by moving it into its own subroutine. Makes the whole thing quite a
bit neater and easier to maintain.

No functional changes.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
FWIW, might be a nice case for refactoring using perlmod and reusing the
(more fleshed-out) Rust implementation from the proxmox-installer-common
crate. Having to implementations of the same thing to keep on par is
kind of a PITA.

 Proxmox/Sys/Net.pm | 22 +++++++++++++++++++-
 proxinstall        | 22 +++++++-------------
 test/Makefile      |  6 +++++-
 test/parse-fqdn.pl | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 83 insertions(+), 17 deletions(-)
 create mode 100755 test/parse-fqdn.pl

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index ed83fb0..3e01d37 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -4,7 +4,7 @@ use strict;
 use warnings;

 use base qw(Exporter);
-our @EXPORT_OK = qw(parse_ip_address parse_ip_mask);
+our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);

 our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
 our $FQDN_RE = "(?:${HOSTNAME_RE}\.)*${HOSTNAME_RE}";
@@ -214,4 +214,24 @@ sub get_dhcp_fqdn : prototype() {
     return $name if defined($name) && $name =~ m/^([^\.]+)(?:\.(?:\S+))?$/;
 }

+# Follows the rules as laid out by proxmox_installer_common::utils::Fqdn
+sub parse_fqdn : prototype($) {
+    my ($text) = @_;
+
+    die "FQDN cannot be empty\n"
+	if !$text || length($text) == 0;
+
+    die "Purely numeric hostnames are not allowed\n"
+	if $text =~ /^[0-9]+(?:\.|$)/;
+
+    die "FQDN must only consist of alphanumeric characters and dashes\n"
+	if $text !~ m/^${Proxmox::Sys::Net::FQDN_RE}$/;
+
+    if ($text =~ m/^([^\.]+)\.(\S+)$/) {
+	return ($1, $2);
+    }
+
+    die "Hostname does not look like a fully qualified domain name\n";
+}
+
 1;
diff --git a/proxinstall b/proxinstall
index 81dd368..4265ba7 100755
--- a/proxinstall
+++ b/proxinstall
@@ -446,24 +446,16 @@ sub create_ipconf_view {
 	$text =~ s/^\s+//;
 	$text =~ s/\s+$//;

-	# Debian does not support purely numeric hostnames
-	if ($text && $text =~ /^[0-9]+(?:\.|$)/) {
-	    Proxmox::UI::message("Purely numeric hostnames are not allowed.");
-	    $hostentry->grab_focus();
-	    return;
-	}
+	my ($hostname, $domainname) = eval { Proxmox::Sys::Net::parse_fqdn($text) };
+	my $err = $@;

-	if ($text
-	    && $text =~ m/^${Proxmox::Sys::Net::FQDN_RE}$/
-	    && $text !~ m/.example.invalid$/
-	    && $text =~ m/^([^\.]+)\.(\S+)$/
-	) {
-	    Proxmox::Install::Config::set_hostname($1);
-	    Proxmox::Install::Config::set_domain($2);
-	} else {
-	    Proxmox::UI::message("Hostname does not look like a fully qualified domain name.");
+	if ($err || $text =~ m/.example.invalid$/) {
+	    Proxmox::UI::message($err // 'Hostname does not look like a valid fully qualified domain name');
 	    $hostentry->grab_focus();
 	    return;
+	} else {
+	    Proxmox::Install::Config::set_hostname($hostname);
+	    Proxmox::Install::Config::set_domain($domainname);
 	}

 	# verify ip address
diff --git a/test/Makefile b/test/Makefile
index fb80fc4..004bc1e 100644
--- a/test/Makefile
+++ b/test/Makefile
@@ -3,8 +3,12 @@ all:
 export PERLLIB=..

 .PHONY: check
-check: test-zfs-arc-max
+check: test-zfs-arc-max test-parse-fqdn

 .PHONY: test-zfs-arc-max
 test-zfs-arc-max:
 	./zfs-arc-max.pl
+
+.PHONY: test-parse-fqdn
+test-parse-fqdn:
+	./parse-fqdn.pl
diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
new file mode 100755
index 0000000..1ffb300
--- /dev/null
+++ b/test/parse-fqdn.pl
@@ -0,0 +1,50 @@
+#!/usr/bin/env perl
+
+use strict;
+use warnings;
+
+use Test::More;
+
+use Proxmox::Sys::Net qw(parse_fqdn);
+
+# some constants just to avoid repeating ourselves
+use constant ERR_INVALID => "Hostname does not look like a fully qualified domain name\n";
+use constant ERR_ALPHANUM => "FQDN must only consist of alphanumeric characters and dashes\n";
+use constant ERR_NUMERIC => "Purely numeric hostnames are not allowed\n";
+use constant ERR_TOOLONG => "FQDN too long\n";
+use constant ERR_EMPTY => "FQDN cannot be empty\n";
+
+sub is_parsed {
+    my ($fqdn, $expected) = @_;
+
+    my @parsed = parse_fqdn($fqdn);
+    is_deeply(\@parsed, $expected, "FQDN is valid and compared successfully: $fqdn");
+}
+
+sub is_invalid {
+    my ($fqdn, $expected_err) = @_;
+
+    my $parsed = eval { parse_fqdn($fqdn) };
+    is($parsed, undef, "invalid FQDN did fail parsing: $fqdn");
+    is($@, $expected_err, "invalid FQDN threw correct error: $fqdn");
+}
+
+is_invalid(undef, ERR_EMPTY);
+is_invalid('', ERR_EMPTY);
+
+is_parsed('foo.example.com', ['foo', 'example.com']);
+is_parsed('foo-bar.com', ['foo-bar', 'com']);
+is_parsed('a-b.com', ['a-b', 'com']);
+
+is_invalid('foo', ERR_INVALID);
+is_invalid('-foo.com', ERR_ALPHANUM);
+is_invalid('foo-.com', ERR_ALPHANUM);
+is_invalid('foo.com-', ERR_ALPHANUM);
+is_invalid('-o-.com', ERR_ALPHANUM);
+
+# https://bugzilla.proxmox.com/show_bug.cgi?id=1054
+is_invalid('123.com', ERR_NUMERIC);
+is_parsed('foo123.com', ['foo123', 'com']);
+is_parsed('123foo.com', ['123foo', 'com']);
+
+done_testing();
--
2.43.0

[pve-devel] [PATCH installer 4/5] sys: net: do not allow overlong FQDNs as per RFCs and Debian spec

[Christoph Heiss]

Debian limits labels to 63 characters each and the total length to 253
characters [0].

[0] https://manpages.debian.org/stable/manpages/hostname.7.en.html

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Net.pm | 5 ++++-
 test/parse-fqdn.pl | 4 ++++
 2 files changed, 8 insertions(+), 1 deletion(-)

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index 3e01d37..2d29116 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -6,7 +6,7 @@ use warnings;
 use base qw(Exporter);
 our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);
 
-our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]*[a-zA-Z0-9])?)";
+our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{,61}?[a-zA-Z0-9])?)";
 our $FQDN_RE = "(?:${HOSTNAME_RE}\.)*${HOSTNAME_RE}";
 
 my $IPV4OCTET = "(?:25[0-5]|(?:2[0-4]|1[0-9]|[1-9])?[0-9])";
@@ -221,6 +221,9 @@ sub parse_fqdn : prototype($) {
     die "FQDN cannot be empty\n"
 	if !$text || length($text) == 0;
 
+    die "FQDN too long\n"
+	if length($text) > 253;
+
     die "Purely numeric hostnames are not allowed\n"
 	if $text =~ /^[0-9]+(?:\.|$)/;
 
diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
index 1ffb300..3009984 100755
--- a/test/parse-fqdn.pl
+++ b/test/parse-fqdn.pl
@@ -47,4 +47,8 @@ is_invalid('123.com', ERR_NUMERIC);
 is_parsed('foo123.com', ['foo123', 'com']);
 is_parsed('123foo.com', ['123foo', 'com']);
 
+is_parsed('a' x 63 . '.com', ['a' x 63, 'com']);
+is_invalid('a' x 250 . '.com', ERR_TOOLONG);
+is_invalid('a' x 64 . '.com', ERR_ALPHANUM);
+
 done_testing();
-- 
2.43.0

[pve-devel] [PATCH installer 5/5] fix #5230: sys: net: properly escape FQDN regex

[Christoph Heiss]

Due to interpolation, the \. sequence must be double-escaped.
Previously, this would result in a non-escaped dot, thus matching much
more liberally than it should.

Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
 Proxmox/Sys/Net.pm                    | 2 +-
 proxmox-installer-common/src/utils.rs | 6 ++++++
 test/parse-fqdn.pl                    | 3 +++
 3 files changed, 10 insertions(+), 1 deletion(-)

diff --git a/Proxmox/Sys/Net.pm b/Proxmox/Sys/Net.pm
index 2d29116..c2f3e9c 100644
--- a/Proxmox/Sys/Net.pm
+++ b/Proxmox/Sys/Net.pm
@@ -7,7 +7,7 @@ use base qw(Exporter);
 our @EXPORT_OK = qw(parse_ip_address parse_ip_mask parse_fqdn);
 
 our $HOSTNAME_RE = "(?:[a-zA-Z0-9](?:[a-zA-Z0-9\-]{,61}?[a-zA-Z0-9])?)";
-our $FQDN_RE = "(?:${HOSTNAME_RE}\.)*${HOSTNAME_RE}";
+our $FQDN_RE = "(?:${HOSTNAME_RE}\\.)*${HOSTNAME_RE}";
 
 my $IPV4OCTET = "(?:25[0-5]|(?:2[0-4]|1[0-9]|[1-9])?[0-9])";
 my $IPV4RE = "(?:(?:$IPV4OCTET\\.){3}$IPV4OCTET)";
diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
index 0ed7438..03efdd7 100644
--- a/proxmox-installer-common/src/utils.rs
+++ b/proxmox-installer-common/src/utils.rs
@@ -309,6 +309,12 @@ mod tests {
             Fqdn::from(&format!("{}.com", "a".repeat(64))),
             Err(InvalidPart("a".repeat(64))),
         );
+
+        // https://bugzilla.proxmox.com/show_bug.cgi?id=3497
+        assert_eq!(
+            Fqdn::from("123@foo.com"),
+            Err(InvalidPart("123@foo".to_owned()))
+        );
     }
 
     #[test]
diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
index 3009984..df8739d 100755
--- a/test/parse-fqdn.pl
+++ b/test/parse-fqdn.pl
@@ -51,4 +51,7 @@ is_parsed('a' x 63 . '.com', ['a' x 63, 'com']);
 is_invalid('a' x 250 . '.com', ERR_TOOLONG);
 is_invalid('a' x 64 . '.com', ERR_ALPHANUM);
 
+# https://bugzilla.proxmox.com/show_bug.cgi?id=3497
+is_invalid('123@foo.com', ERR_ALPHANUM);
+
 done_testing();
-- 
2.43.0

Re: [pve-devel] [PATCH installer 2/5] common: fqdn: implement case-insensitive comparison as per RFC 952

[Thomas Lamprecht]

Am 15/02/2024 um 13:39 schrieb Christoph Heiss:
> +impl PartialEq for Fqdn {
> +    fn eq(&self, other: &Self) -> bool {
> +        // Case-insensitive comparison, as per RFC 952 "ASSUMPTIONS",
> +        // RFC 1035 sec. 2.3.3. "Character Case" and RFC 4343 as a whole
> +        let a = self
> +            .parts
> +            .iter()
> +            .map(|s| s.to_lowercase())
> +            .collect::<Vec<String>>();
> +
> +        let b = other
> +            .parts
> +            .iter()
> +            .map(|s| s.to_lowercase())
> +            .collect::<Vec<String>>();
> +
> +        a == b

could be probably more efficiently done by doing the comparison part wise, not
on the whole thing, and with an early abort for mismatching part count, e.g.,
something like:

if self.parts.len() != other.parts.len() {             
    return false;                                      
}                                                      
                                                       
self.parts                                             
    .iter()                                            
    .zip(other.parts.iter())                           
    .all(|(a, b)| a.to_lowercase() == b.to_lowercase())

> @@ -309,4 +329,10 @@ mod tests {
>              "foo.example.com"
>          );
>      }
> +
> +    #[test]
> +    fn fqdn_compare() {
> +        assert_eq!(Fqdn::from("example.com"), Fqdn::from("ExAmPle.Com"));
> +        assert_eq!(Fqdn::from("ExAmPle.Com"), Fqdn::from("example.com"));

I always like throwing in some assert_ne ones for sanity checking (e.g., such
optimization tries like above), as otherwise one won't notice if this is broken,
like by just always returning false..

Re: [pve-devel] [PATCH installer 5/5] fix #5230: sys: net: properly escape FQDN regex

[Thomas Lamprecht]

Am 15/02/2024 um 13:39 schrieb Christoph Heiss:
> Due to interpolation, the \. sequence must be double-escaped.
> Previously, this would result in a non-escaped dot, thus matching much
> more liberally than it should.
> 

OK, but...


> diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
> index 0ed7438..03efdd7 100644
> --- a/proxmox-installer-common/src/utils.rs
> +++ b/proxmox-installer-common/src/utils.rs
> @@ -309,6 +309,12 @@ mod tests {
>              Fqdn::from(&format!("{}.com", "a".repeat(64))),
>              Err(InvalidPart("a".repeat(64))),
>          );
> +
> +        // https://bugzilla.proxmox.com/show_bug.cgi?id=3497

...why link on a feature request for terraform support then?


> diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
> index 3009984..df8739d 100755
> --- a/test/parse-fqdn.pl
> +++ b/test/parse-fqdn.pl
> @@ -51,4 +51,7 @@ is_parsed('a' x 63 . '.com', ['a' x 63, 'com']);
>  is_invalid('a' x 250 . '.com', ERR_TOOLONG);
>  is_invalid('a' x 64 . '.com', ERR_ALPHANUM);
>  
> +# https://bugzilla.proxmox.com/show_bug.cgi?id=3497

same here

[pve-devel] applied-series: [PATCH installer 0/5] proxinstall, tui: improve hostname/FQDN validation

[Thomas Lamprecht]

Am 15/02/2024 um 13:39 schrieb Christoph Heiss:
> This series improves various aspects regarding FQDN handling and
> validation across both the GUI and TUI installer.
> 
> It (partially) addresses issue #5230 [0] in patch #5, by fixing the
> regex through which we validate FQDNs in the GUI installer.
> 
> It also refactors the FQDN validation/parsing in the GUI installer, by
> moving it into its own subroutine - making the whole thing bit easier to
> reason about, as well as adding loads of test cases.
> 
> [0] https://bugzilla.proxmox.com/show_bug.cgi?id=5230
> 
> Christoph Heiss (5):
>   common: fqdn: do not allow overlong FQDNs as per Debian spec
>   common: fqdn: implement case-insensitive comparison as per RFC 952
>   proxinstall: avoid open-coding FQDN sanity check
>   sys: net: do not allow overlong FQDNs as per RFCs and Debian spec
>   fix #5230: sys: net: properly escape FQDN regex
> 
>  Proxmox/Sys/Net.pm                    | 29 +++++++++-
>  proxinstall                           | 22 +++-----
>  proxmox-installer-common/src/utils.rs | 79 ++++++++++++++++++++++++++-
>  test/Makefile                         |  6 +-
>  test/parse-fqdn.pl                    | 57 +++++++++++++++++++
>  5 files changed, 172 insertions(+), 21 deletions(-)
>  create mode 100755 test/parse-fqdn.pl
> 
> --
> 2.43.0
> 
applied, with a fix for the bug # reference squashed into patch 5/5 and
two small follow-ups w.r.t. FQDN test scope and comparison efficiency (not
that it will matter much at those string sizes), thanks!

Re: [pve-devel] [PATCH installer 5/5] fix #5230: sys: net: properly escape FQDN regex

[Christoph Heiss]

On Fri, Feb 23, 2024 at 05:27:52PM +0100, Thomas Lamprecht wrote:
> Am 15/02/2024 um 13:39 schrieb Christoph Heiss:
> > Due to interpolation, the \. sequence must be double-escaped.
> > Previously, this would result in a non-escaped dot, thus matching much
> > more liberally than it should.
> >
>
> OK, but...
>
>
> > diff --git a/proxmox-installer-common/src/utils.rs b/proxmox-installer-common/src/utils.rs
> > index 0ed7438..03efdd7 100644
> > --- a/proxmox-installer-common/src/utils.rs
> > +++ b/proxmox-installer-common/src/utils.rs
> > @@ -309,6 +309,12 @@ mod tests {
> >              Fqdn::from(&format!("{}.com", "a".repeat(64))),
> >              Err(InvalidPart("a".repeat(64))),
> >          );
> > +
> > +        // https://bugzilla.proxmox.com/show_bug.cgi?id=3497
>
> ...why link on a feature request for terraform support then?

Whoop, sorry for that - apparently had the wrong link in the the
clipboard when writing this ..

>
>
> > diff --git a/test/parse-fqdn.pl b/test/parse-fqdn.pl
> > index 3009984..df8739d 100755
> > --- a/test/parse-fqdn.pl
> > +++ b/test/parse-fqdn.pl
> > @@ -51,4 +51,7 @@ is_parsed('a' x 63 . '.com', ['a' x 63, 'com']);
> >  is_invalid('a' x 250 . '.com', ERR_TOOLONG);
> >  is_invalid('a' x 64 . '.com', ERR_ALPHANUM);
> >
> > +# https://bugzilla.proxmox.com/show_bug.cgi?id=3497
>
> same here