* [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
@ 2022-12-27 12:29 Christoph Heiss
2022-12-27 18:21 ` Stoiko Ivanov
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Heiss @ 2022-12-27 12:29 UTC (permalink / raw)
To: pmg-devel
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.
Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
---
src/PMG/API2/MyNetworks.pm | 10 ++++++++--
src/PMG/Config.pm | 15 ++++++++-------
2 files changed, 16 insertions(+), 9 deletions(-)
diff --git a/src/PMG/API2/MyNetworks.pm b/src/PMG/API2/MyNetworks.pm
index 975ca2e..325e59b 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);
@@ -10,6 +11,7 @@ use HTTP::Status qw(:constants);
use PVE::JSONSchema qw(get_standard_option);
use PVE::RESTHandler;
use PVE::INotify;
+use PVE::Network;
use PMG::Config;
@@ -77,13 +79,17 @@ __PACKAGE__->register_method ({
my ($param) = @_;
my $code = sub {
+ die "invalid network adress '$param->{cidr}', host-bits must be null\n"
+ if !Net::IP::ip_normalize($param->{cidr});
my $mynetworks = PVE::INotify::read_file('mynetworks');
+ my $ip = PVE::Network::IP_from_cidr($param->{cidr});
die "trusted network '$param->{cidr}' already exists\n"
- if $mynetworks->{$param->{cidr}};
+ if $mynetworks->{$ip->prefix()};
- $mynetworks->{$param->{cidr}} = {
+ $mynetworks->{$ip->prefix()} = {
+ cidr => $param->{cidr},
comment => $param->{comment} // '',
};
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(),
+ 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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
2022-12-27 12:29 [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks` Christoph Heiss
@ 2022-12-27 18:21 ` Stoiko Ivanov
2022-12-28 9:31 ` Christoph Heiss
0 siblings, 1 reply; 5+ messages in thread
From: Stoiko Ivanov @ 2022-12-27 18:21 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
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)
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
>
> Signed-off-by: Christoph Heiss <c.heiss@proxmox.com>
> ---
> src/PMG/API2/MyNetworks.pm | 10 ++++++++--
> src/PMG/Config.pm | 15 ++++++++-------
> 2 files changed, 16 insertions(+), 9 deletions(-)
>
> diff --git a/src/PMG/API2/MyNetworks.pm b/src/PMG/API2/MyNetworks.pm
> index 975ca2e..325e59b 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);
> @@ -10,6 +11,7 @@ use HTTP::Status qw(:constants);
> use PVE::JSONSchema qw(get_standard_option);
> use PVE::RESTHandler;
> use PVE::INotify;
> +use PVE::Network;
>
> use PMG::Config;
>
> @@ -77,13 +79,17 @@ __PACKAGE__->register_method ({
> my ($param) = @_;
>
> my $code = sub {
> + die "invalid network adress '$param->{cidr}', host-bits must be null\n"
> + if !Net::IP::ip_normalize($param->{cidr});
>
> my $mynetworks = PVE::INotify::read_file('mynetworks');
> + my $ip = PVE::Network::IP_from_cidr($param->{cidr});
>
> die "trusted network '$param->{cidr}' already exists\n"
> - if $mynetworks->{$param->{cidr}};
> + if $mynetworks->{$ip->prefix()};
>
> - $mynetworks->{$param->{cidr}} = {
> + $mynetworks->{$ip->prefix()} = {
> + cidr => $param->{cidr},
> comment => $param->{comment} // '',
> };
>
> 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.
> + 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
>
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
2022-12-27 18:21 ` Stoiko Ivanov
@ 2022-12-28 9:31 ` Christoph Heiss
2022-12-28 9:51 ` Stoiko Ivanov
0 siblings, 1 reply; 5+ messages in thread
From: Christoph Heiss @ 2022-12-28 9:31 UTC (permalink / raw)
To: Stoiko Ivanov; +Cc: pmg-devel
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.
>
>
> > [..]
> >
> > 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@lists.proxmox.com
> > https://lists.proxmox.com/cgi-bin/mailman/listinfo/pmg-devel
> >
> >
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
2022-12-28 9:31 ` Christoph Heiss
@ 2022-12-28 9:51 ` Stoiko Ivanov
2022-12-28 10:17 ` Christoph Heiss
0 siblings, 1 reply; 5+ messages in thread
From: Stoiko Ivanov @ 2022-12-28 9:51 UTC (permalink / raw)
To: Christoph Heiss; +Cc: pmg-devel
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
> > >
> > >
> >
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks`
2022-12-28 9:51 ` Stoiko Ivanov
@ 2022-12-28 10:17 ` Christoph Heiss
0 siblings, 0 replies; 5+ messages in thread
From: Christoph Heiss @ 2022-12-28 10:17 UTC (permalink / raw)
To: Stoiko Ivanov; +Cc: pmg-devel
On Wed, Dec 28, 2022 at 10:51:21AM +0100, Stoiko Ivanov wrote:
> [..]
>
> 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?
One upside I can think of would be that users cannot _that_ easily typo
the prefix, since they need to match up the prefix length to the network
address. Then again, we can only do so much if someone puts in wrong
values.
But otherwise, the above suggestion seems very reasonable to me. I don't
think a whole lot of users will read the postfix config directly anyway
(why would they?), so IMHO that is a non-issue anyway if the values
differ.
I'll try it out and send a v3 with the above implemented, maybe it is
the better way in the end anyway.
> > [..]
> >
> > 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)
Ack, I'll just add a comment and leave it as it is for now.
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2022-12-28 10:17 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-12-27 12:29 [pmg-devel] [PATCH pmg-api v2] fix #4410: Remove non-null host-bits from CIDR when reading `mynetworks` Christoph Heiss
2022-12-27 18:21 ` Stoiko Ivanov
2022-12-28 9:31 ` Christoph Heiss
2022-12-28 9:51 ` Stoiko Ivanov
2022-12-28 10:17 ` Christoph Heiss
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox