[pve-devel] [PATCH V2 firewall 0/2] allow non zero ip address host bits to be entered

[pve-devel] [PATCH V2 firewall 0/2] allow non zero ip address host bits to be entered

[Stefan Hrdlicka]

V1 -> V2
* zero out host bits instead of ignoring error
* regex "cleanup"

Stefan Hrdlicka (2):
  allow non zero ip address host bits to be entered
  cleanup: don't capture "/xx" of CIDR

 src/PVE/API2/Firewall/IPSet.pm |  2 +-
 src/PVE/Firewall.pm            | 31 +++++++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 3 deletions(-)

-- 
2.30.2

[pve-devel] [PATCH V2 firewall 1/2] allow non zero ip address host bits to be entered

[Stefan Hrdlicka]

They can already be set directly via the cluster.fw file. Net::IP is just a
bit more picky with what it allows:
For example:
  error:   192.168.1.155/24
  correct: 192.168.1.0/24

This cleans the entered IP and removes the non zero host bits.

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/API2/Firewall/IPSet.pm |  2 +-
 src/PVE/Firewall.pm            | 29 ++++++++++++++++++++++++++++-
 2 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
index a5f69e9..14bcfcb 100644
--- a/src/PVE/API2/Firewall/IPSet.pm
+++ b/src/PVE/API2/Firewall/IPSet.pm
@@ -199,7 +199,7 @@ sub register_create_ip {
 
 		my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
 
-		my $cidr = $param->{cidr};
+		my $cidr = PVE::Firewall::clean_cidr($param->{cidr});
 		if ($cidr =~ m/^${PVE::Firewall::ip_alias_pattern}$/) {
 		    # make sure alias exists (if $cidr is an alias)
 		    PVE::Firewall::resolve_alias($cluster_conf, $fw_conf, $cidr);
diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index d40a9b1..3c35b44 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -69,8 +69,12 @@ sub pve_verify_ip_or_cidr {
     my ($cidr, $noerr) = @_;
 
     if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
-	return $cidr if Net::IP->new($cidr);
+        # Net::IP throws an error if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
+        # fails but `192.168.1.0/24` succeeds. clean_cidr removes the non zero bits from the CIDR.
+	my $clean_cidr = clean_cidr($cidr);
+	return $cidr if Net::IP->new($clean_cidr);
 	return undef if $noerr;
+
 	die Net::IP::Error() . "\n";
     }
     return undef if $noerr;
@@ -86,6 +90,29 @@ sub pve_verify_ip_or_cidr_or_alias {
     return pve_verify_ip_or_cidr($cidr, $noerr);
 }
 
+sub clean_cidr {
+    my ($cidr) = @_;
+    my ($ip, $len) = split('/', $cidr);
+    return $cidr if !$len;
+
+    my $clean_func = sub {
+        my ($ver) = @_;
+        my $bin_ip = Net::IP::ip_iptobin( Net::IP::ip_expand_address($ip, $ver), $ver);
+        my $bin_mask = Net::IP::ip_get_mask($len, $ver);
+        my $clean_ip = Net::IP::ip_compress_address( Net::IP::ip_bintoip($bin_ip & $bin_mask, $ver), $ver);
+
+        return "${clean_ip}/$len";
+    };
+
+    if ($ip =~ m!^$IPV4RE$!) {
+        return &$clean_func(4)
+    } elsif ($ip =~ m!^$IPV6RE$!) {
+        return &$clean_func(6);
+    }
+
+    return $cidr;
+}
+
 PVE::JSONSchema::register_standard_option('ipset-name', {
     description => "IP set name.",
     type => 'string',
-- 
2.30.2

Re: [pve-devel] [PATCH V2 firewall 1/2] allow non zero ip address host bits to be entered

[Wolfgang Bumiller]

On Tue, Nov 29, 2022 at 05:01:51PM +0100, Stefan Hrdlicka wrote:
> They can already be set directly via the cluster.fw file. Net::IP is just a
> bit more picky with what it allows:
> For example:
>   error:   192.168.1.155/24
>   correct: 192.168.1.0/24
> 
> This cleans the entered IP and removes the non zero host bits.
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/API2/Firewall/IPSet.pm |  2 +-
>  src/PVE/Firewall.pm            | 29 ++++++++++++++++++++++++++++-
>  2 files changed, 29 insertions(+), 2 deletions(-)
> 
> diff --git a/src/PVE/API2/Firewall/IPSet.pm b/src/PVE/API2/Firewall/IPSet.pm
> index a5f69e9..14bcfcb 100644
> --- a/src/PVE/API2/Firewall/IPSet.pm
> +++ b/src/PVE/API2/Firewall/IPSet.pm
> @@ -199,7 +199,7 @@ sub register_create_ip {
>  
>  		my ($cluster_conf, $fw_conf, $ipset) = $class->load_config($param);
>  
> -		my $cidr = $param->{cidr};
> +		my $cidr = PVE::Firewall::clean_cidr($param->{cidr});
>  		if ($cidr =~ m/^${PVE::Firewall::ip_alias_pattern}$/) {
>  		    # make sure alias exists (if $cidr is an alias)
>  		    PVE::Firewall::resolve_alias($cluster_conf, $fw_conf, $cidr);
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index d40a9b1..3c35b44 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -69,8 +69,12 @@ sub pve_verify_ip_or_cidr {
>      my ($cidr, $noerr) = @_;
>  
>      if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
> -	return $cidr if Net::IP->new($cidr);
> +        # Net::IP throws an error if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
> +        # fails but `192.168.1.0/24` succeeds. clean_cidr removes the non zero bits from the CIDR.
> +	my $clean_cidr = clean_cidr($cidr);
> +	return $cidr if Net::IP->new($clean_cidr);
>  	return undef if $noerr;
> +
>  	die Net::IP::Error() . "\n";
>      }
>      return undef if $noerr;
> @@ -86,6 +90,29 @@ sub pve_verify_ip_or_cidr_or_alias {
>      return pve_verify_ip_or_cidr($cidr, $noerr);
>  }
>  
> +sub clean_cidr {
> +    my ($cidr) = @_;
> +    my ($ip, $len) = split('/', $cidr);
> +    return $cidr if !$len;
> +
> +    my $clean_func = sub {
> +        my ($ver) = @_;
> +        my $bin_ip = Net::IP::ip_iptobin( Net::IP::ip_expand_address($ip, $ver), $ver);
> +        my $bin_mask = Net::IP::ip_get_mask($len, $ver);
> +        my $clean_ip = Net::IP::ip_compress_address( Net::IP::ip_bintoip($bin_ip & $bin_mask, $ver), $ver);
> +
> +        return "${clean_ip}/$len";
> +    };
> +
> +    if ($ip =~ m!^$IPV4RE$!) {
> +        return &$clean_func(4)
> +    } elsif ($ip =~ m!^$IPV6RE$!) {
> +        return &$clean_func(6);
> +    }

IMO we should either say "we know our input was validated already" and
check only one (then we don't need the closure and can just do

    my $ver = ($ip =~ v4re...) ? 4 : 6;

or we also handle the `else` case here with an error.

Otherwise, LGTM

> +
> +    return $cidr;
> +}
> +
>  PVE::JSONSchema::register_standard_option('ipset-name', {
>      description => "IP set name.",
>      type => 'string',
> -- 
> 2.30.2

[pve-devel] [PATCH V2 firewall 2/2] cleanup: don't capture "/xx" of CIDR

[Stefan Hrdlicka]

Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
---
 src/PVE/Firewall.pm | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index 3c35b44..73e940b 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -68,7 +68,7 @@ PVE::JSONSchema::register_format('IPorCIDR', \&pve_verify_ip_or_cidr);
 sub pve_verify_ip_or_cidr {
     my ($cidr, $noerr) = @_;
 
-    if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
+    if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(?:/\d+)?$!) {
         # Net::IP throws an error if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
         # fails but `192.168.1.0/24` succeeds. clean_cidr removes the non zero bits from the CIDR.
 	my $clean_cidr = clean_cidr($cidr);
-- 
2.30.2