Re: [pmg-devel] [PATCH pmg-api] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`

[Stoiko Ivanov <s.ivanov@proxmox.com>]

Huge thanks for addressing this!

I like the approach in general - two comments inline:

On Thu, 22 Dec 2022 11:19:40 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:

> This will simply drop non-null host bits when reading the config file,
> thus preserving backwards-compatibility.
> When creating new entries, invalid CIDRs are now rejected to prevent
> creation of such entries in the future.
> 
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
>  src/PMG/API2/MyNetworks.pm |  4 ++++
>  src/PMG/Config.pm          | 21 +++++++++++----------
>  2 files changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/src/PMG/API2/MyNetworks.pm b/src/PMG/API2/MyNetworks.pm
> index 975ca2e..aff4041 100644
> --- a/src/PMG/API2/MyNetworks.pm
> +++ b/src/PMG/API2/MyNetworks.pm
> @@ -3,6 +3,7 @@ package PMG::API2::MyNetworks;
>  use strict;
>  use warnings;
>  use Data::Dumper;
> +use Net::IP;
> 
>  use PVE::SafeSyslog;
>  use PVE::Tools qw(extract_param);
> @@ -83,6 +84,9 @@ __PACKAGE__->register_method ({
>  	    die "trusted network '$param->{cidr}' already exists\n"
>  		if $mynetworks->{$param->{cidr}};
> 
> +	    die "invalid network adress '$param->{cidr}', host-bits must be null\n"
> +		if !Net::IP::ip_normalize($param->{cidr});
> +
>  	    $mynetworks->{$param->{cidr}} = {
>  		comment => $param->{comment} // '',
>  	    };
> diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
> index 9ba5c76..f03f102 100755
> --- a/src/PMG/Config.pm
> +++ b/src/PMG/Config.pm
> @@ -730,6 +730,7 @@ use PVE::SafeSyslog;
>  use PVE::Tools qw($IPV4RE $IPV6RE);
>  use PVE::INotify;
>  use PVE::JSONSchema;
> +use PVE::Network;
> 
>  use PMG::Cluster;
>  use PMG::Utils;
> @@ -1008,13 +1009,13 @@ sub read_pmg_mynetworks {
>  	while (defined(my $line = <$fh>)) {
>  	    chomp $line;
>  	    next if $line =~ m/^\s*$/;
> -	    if ($line =~ m!^((?:$IPV4RE|$IPV6RE))/(\d+)\s*(?:#(.*)\s*)?$!) {
> -		my ($network, $prefix_size, $comment) = ($1, $2, $3);
> -		my $cidr = "$network/${prefix_size}";
> -		$mynetworks->{$cidr} = {
> -		    cidr => $cidr,
> -		    network_address => $network,
> -		    prefix_size => $prefix_size,
> +	    if ($line =~ m!^((?:$IPV4RE|$IPV6RE)/\d+)\s*(?:#(.*)\s*)?$!) {
> +		my ($cidr, $comment) = ($1, $2);
> +		my $ip = PVE::Network::IP_from_cidr($cidr);
this call expands the prefix to full-length - which I wouldn't have
noticed for ipv4 - but is quite visible with ipv6:
entering `2001:db8::/32` results in
`2001:0db8:0000:0000:0000:0000:0000:0000/32`
IIUC - Net::IP::ip_compress_prefix($ip->prefix(), $ip->version()) might
be an approach - but even that adds the last quad of zeros...

If at all possible it would be great to keep the data as the user entered it.
(In this case - in all situations where it's actually a valid prefix w/o
host-bits set)



> +		$mynetworks->{$ip->prefix()} = {
> +		    cidr => $ip->prefix(),
> +		    network_address => $ip->ip(),
> +		    prefix_size => $ip->prefixlen(),
>  		    comment => $comment // '',
>  		};
>  	    } else {
> @@ -1336,11 +1337,11 @@ sub get_template_vars {
>      }
> 
>      my $netlist = PVE::INotify::read_file('mynetworks');
> -    foreach my $cidr (keys %$netlist) {
> -	if ($cidr =~ m/^($IPV6RE)\/(\d+)$/) {
> +    foreach my $ip (values %$netlist) {
why switch here to iterating over the values - and then accessing the cidr
field twice, if it's by construction above the same as the key?

> +	if ($ip->{cidr} =~ m/^($IPV6RE)\/(\d+)$/) {
>  	    $mynetworks->{"[$1]/$2"} = 1;
>  	} else {
> -	    $mynetworks->{$cidr} = 1;
> +	    $mynetworks->{$ip->{cidr}} = 1;
>  	}
>      }
> 
> --
> 2.30.2
> 
> 
> 
> _______________________________________________
> pmg-devel mailing list
> pmg-devel@lists.proxmox.com
> https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> 
>