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

[Christoph Heiss]

On Tue, Dec 27, 2022 at 07:21:35PM +0100, Stoiko Ivanov wrote:
> On Tue, 27 Dec 2022 13:29:15 +0100
> Christoph Heiss <c.heiss at 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.
> >
> > In the GUI, the entries are displayed as the user entered them (as
> > suggested my Stoiko). This is done by considering /etc/pmg/mynetworks
> > as the "source of truth" - all entries are saved there verbatim, but
> > when writing the postfix config the right ones are picked.
> This sounds good!
> Currently the code breaks the GET,PUT,DELETE api calls for mynetworks
> (mismatch between the key (which is the 'computed/long string' and the
> provided parameter (which is the the data from the file)
>
> (tested with an ipv6 prefix - creating works, getting/setting/deleting
> does not work)
Weird, I though I tested at least deleting extensively enough. Well,
back to the drawing board.

>
> maybe keep the user-entered value as key - and add the host-bit-clean cidr
> (address/mask string) as additional field - then get this field when
> serializing the data in get_template_vars
Had that same thought too already, but then a new problem arises -
duplicate (IPv6) entries can be created, since the check then just
compare the user-entered value. E.g. both 2001:db8::/32 and
2001:db8::0/32 could be created with issue. That's why I chose to use
the normalized prefixes as keys.
But this was also possible before .. I'll see what I can come up with.

>
>
> > [..]
> >
> > diff --git a/src/PMG/Config.pm b/src/PMG/Config.pm
> > index 9ba5c76..a29060b 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} = {
> > +	    if ($line =~ m!^((?:$IPV4RE|$IPV6RE)/\d+)\s*(?:#(.*)\s*)?$!) {
> > +		my ($cidr, $comment) = ($1, $2);
> > +		my $ip = PVE::Network::IP_from_cidr($cidr);
> > +		$mynetworks->{$ip->prefix()} = {
> >  		    cidr => $cidr,
> > -		    network_address => $network,
> > -		    prefix_size => $prefix_size,
> > +		    network_address => $ip->short(),
> the short() method yields IPv4 addresses in a somewhat uncommon format (at
> least for me -> 10.2.2.0/24 -> '10.2.2') - however at a quick glance it
> seems that the 'network_address' field is not really used anywhere - so we
> should probably just drop it.
Yeah, ->short() also abbreviates IPv4 addresses. At first I had a check
differentiating between v4 and v6 and only ->short()'ening v6 addresses.

I'll investigate if and where this field is actually used (and
prefix_size too, while at it).

Removing it would change the API though - is stability / backwards
compatibility something we have to be wary of or can I just drop it if
it's really not used anywhere?

>
>
>
> > +		    prefix_size => $ip->prefixlen(),
> >  		    comment => $comment // '',
> >  		};
> >  	    } else {
> > @@ -1032,7 +1033,7 @@ sub write_pmg_mynetworks {
> >      foreach my $cidr (sort keys %$mynetworks) {
> >  	my $data = $mynetworks->{$cidr};
> >  	my $comment = $data->{comment} // '*';
> > -	PVE::Tools::safe_print($filename, $fh, "$cidr #$comment\n");
> > +	PVE::Tools::safe_print($filename, $fh, "$data->{cidr} #$comment\n");
> >      }
> >  }
> >
> > --
> > 2.30.2
> >
> >
> >
> > _______________________________________________
> > pmg-devel mailing list
> > pmg-devel at lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> >
> >
>