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

[Stefan Hrdlicka <s.hrdlicka@proxmox.com>]

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;
 >
 >