[pve-devel] [PATCH pve-firewall] allow non zero ip address host bits
Stefan Hrdlicka
s.hrdlicka at proxmox.com
Tue Nov 8 15:15:16 CET 2022
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 at 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;
>
>
More information about the pve-devel
mailing list