[pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[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

also improves #3554

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

diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
index e6d6802..25e2fd0 100644
--- a/src/PVE/Firewall.pm
+++ b/src/PVE/Firewall.pm
@@ -69,6 +69,14 @@ sub pve_verify_ip_or_cidr {
     if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
 	return $cidr if Net::IP->new($cidr);
 	return undef if $noerr;
+
+	# Error 171 in Net::IP comes up if the host part of the IP address isn't
+	# zero.
+	# for example:
+	#  error:   192.168.1.155/24
+	#  correct: 192.168.1.0/24
+	return $cidr if Net::IP::Errno() == 171;
+
 	die Net::IP::Error() . "\n";
     }
     return undef if $noerr;
-- 
2.30.2

Re: [pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[Thomas Lamprecht]

Some issue due to weird and unmentioned dependence on $noerr and 
while at it some small comment and commit message style nits that
I might have either ignored or "fixed" up myself other way. 

On 25/10/2022 16:31, 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:

nit: Would suggest:

... what it allows, for example:

> For example:
>   error:   192.168.1.155/24

    fails ...

>   correct: 192.168.1.0/24

    succeeds: ...

(as for us both are obviously correct, so we just want to show when
Net::IP fails or succeeds)

> 
> also improves #3554
> 
> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
> ---
>  src/PVE/Firewall.pm | 8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
> index e6d6802..25e2fd0 100644
> --- a/src/PVE/Firewall.pm
> +++ b/src/PVE/Firewall.pm
> @@ -69,6 +69,14 @@ sub pve_verify_ip_or_cidr {
>      if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
>  	return $cidr if Net::IP->new($cidr);
>  	return undef if $noerr;
> +
> +	# Error 171 in Net::IP comes up if the host part of the IP address isn't
> +	# zero.
> +	# for example:
> +	#  error:   192.168.1.155/24
> +	#  correct: 192.168.1.0/24

A comment for such a thing _is_ great, but it still should be somewhat concise
w.r.t. (line) space usage to avoid "bloat". E.g., the following would still
fit in the 100c upper limit

        # Net::IP sets Error `171` if the masked CIDR part isn't zero, e.g., `192.168.1.155/24`
        # fails but `192.168.1.0/24` succeeds. We allow non-zero though, so ignore.

> +	return $cidr if Net::IP::Errno() == 171;
> +

now for a actual non-nit: why only return the $cidr in that case if $noerr is falsy?

Seems odd to have that flag control the behavior.

Also, any details on that errno being restricted to really only that?
I only found some info in the actual code[0], and they don't seem to
have constant (or any management for assigner err#, meh), so just some
hint about that with a link to the source in the commit message.

Or did you find better sources?

It seems that we're also lucky that the check for this is basically the
last one in the `set` method the `new` constructor calls, so at least in
the current version we can assume that it'd be indeed a valid CIDR otherwise,
but still, feels a bit brittle.

Could another option be that we normalize CIDRs on entry, i.e., mask out
the end? I mean,. would not help existing setups, but at least future
proof it a bit for new systems if there's another call site that will
trip on this (maybe normalizing here in case of 171 could be an option
too). I don't want to shove you in that direction, just wondering if
that was considered.

[0]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L1811
[1]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L199

>  	die Net::IP::Error() . "\n";
>      }
>      return undef if $noerr;

Re: [pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[Stefan Hrdlicka]

On 10/28/22 11:28, Thomas Lamprecht wrote:> Some issue due to weird and 
unmentioned dependence on $noerr and
 > while at it some small comment and commit message style nits that
 > I might have either ignored or "fixed" up myself other way.
 >
 > On 25/10/2022 16:31, 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:
 >
 > nit: Would suggest:
 >
 > ... what it allows, for example:
 >
 >> For example:
 >>    error:   192.168.1.155/24
 >
 >      fails ...
 >
 >>    correct: 192.168.1.0/24
 >
 >      succeeds: ...
 >
 > (as for us both are obviously correct, so we just want to show when
 > Net::IP fails or succeeds)
 >
 >>
 >> also improves #3554
 >>
 >> Signed-off-by: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
 >> ---
 >>   src/PVE/Firewall.pm | 8 ++++++++
 >>   1 file changed, 8 insertions(+)
 >>
 >> diff --git a/src/PVE/Firewall.pm b/src/PVE/Firewall.pm
 >> index e6d6802..25e2fd0 100644
 >> --- a/src/PVE/Firewall.pm
 >> +++ b/src/PVE/Firewall.pm
 >> @@ -69,6 +69,14 @@ sub pve_verify_ip_or_cidr {
 >>       if ($cidr =~ m!^(?:$IPV6RE|$IPV4RE)(/(\d+))?$!) {
 >>   	return $cidr if Net::IP->new($cidr);
 >>   	return undef if $noerr;
 >> +
 >> +	# Error 171 in Net::IP comes up if the host part of the IP address 
isn't
 >> +	# zero.
 >> +	# for example:
 >> +	#  error:   192.168.1.155/24
 >> +	#  correct: 192.168.1.0/24
 >
 > A comment for such a thing _is_ great, but it still should be 
somewhat concise
 > w.r.t. (line) space usage to avoid "bloat". E.g., the following would 
still
 > fit in the 100c upper limit
 >
 >          # Net::IP sets Error `171` if the masked CIDR part isn't 
zero, e.g., `192.168.1.155/24`
 >          # fails but `192.168.1.0/24` succeeds. We allow non-zero 
though, so ignore.
 >
 >> +	return $cidr if Net::IP::Errno() == 171;
 >> +
 >
 > now for a actual non-nit: why only return the $cidr in that case if 
$noerr is falsy?

good point :) might need more cleanup. nothing I see is using this 
function that sets $noerr. Also as long as this function doesn't die it 
doesn't matter what it returns for the validation.

 >
 > Seems odd to have that flag control the behavior.
 >
 > Also, any details on that errno being restricted to really only that?
 > I only found some info in the actual code[0], and they don't seem to
 > have constant (or any management for assigner err#, meh), so just some
 > hint about that with a link to the source in the commit message.
 >
 > Or did you find better sources?
I just looked at the code.

 > It seems that we're also lucky that the check for this is basically the
 > last one in the `set` method the `new` constructor calls, so at least in
 > the current version we can assume that it'd be indeed a valid CIDR 
otherwise,
 > but still, feels a bit brittle.
I would have guessed somebody thought long and hard about the right 
order of tests :D.

Feels a bit brittle I agree. But also the last change of Net:IP was in 
2012 and the version before from 2006 so we could also call it rock solid^^.

 >
 > Could another option be that we normalize CIDRs on entry, i.e., mask out
 > the end? I mean,. would not help existing setups, but at least future
 > proof it a bit for new systems if there's another call site that will
 > trip on this (maybe normalizing here in case of 171 could be an option
 > too). I don't want to shove you in that direction, just wondering if
 > that was considered.

Yes that would be an option. Was more bit more faffing about when I 
tried it. Also would it then be a good idea to change config a user 
added to the the file, or should that be kept as it was entered?



 >
 > [0]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L1811
 > [1]: https://metacpan.org/release/MANU/Net-IP-1.26/source/IP.pm#L199
 >
 >>   	die Net::IP::Error() . "\n";
 >>       }
 >>       return undef if $noerr;
 >
 >

Re: [pve-devel] [PATCH pve-firewall] allow non zero ip address host bits

[Thomas Lamprecht]

Am 08/11/2022 um 15:15 schrieb Stefan Hrdlicka:
>> Could another option be that we normalize CIDRs on entry, i.e., mask out
>> the end? I mean,. would not help existing setups, but at least future
>> proof it a bit for new systems if there's another call site that will
>> trip on this (maybe normalizing here in case of 171 could be an option
>> too). I don't want to shove you in that direction, just wondering if
>> that was considered.
> 
> Yes that would be an option. Was more bit more faffing about when I tried it.
> Also would it then be a good idea to change config a user
> added to the the file, or should that be kept as it was entered?
> 

We normally don't auto-rewrite/update configs on package upgrade, as that can
be brittle and break immediate downgrades due to a, e.g., regression, but when
writing out a FW config anyway we could rewrite it indeed (at least if there's
no disagreement in that beeing a good idea in the first place)

So in any way, we would probably still want some silencing of the verifier,
if you cleanup the slightly confusing odd case with returning $cidr only
on $noerr I would go for that for now as stop gap.