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

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

On Wed, 28 Dec 2022 10:31:52 +0100
Christoph Heiss <c.heiss@proxmox.com> wrote:

> 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@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.
One option that came to my mind was ... just keeping the API part and the
values in /etc/pmg/mynetworks as they are ..
only normalize the prefixes in get_template_vars for use in the postfix
config

It has the downside of users getting different values there than they
entered - but I personally never understood why programs forced me to
clear out the host-bits for their use - so I'm not sure if there's any
upside to requiring our users to clear that up?

> 
> >
> >
> > > [..]
> > >
> > > 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?
good point! - we do return it from the API - so let's keep it for now
(a 'FIXME: drop network_address and prefix_size with PMG 8.0' comment
might make sense though - then we can clear it up with the next major
release)


> 
> >
> >
> >
> > > +		    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@lists.proxmox.com
> > > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> > >
> > >
> >