From: Stefan Hrdlicka <s.hrdlicka@proxmox.com>
To: Proxmox VE development discussion <pve-devel@lists.proxmox.com>
Subject: Re: [pve-devel] [PATCH pve-firewall] allow non zero ip address host bits
Date: Tue, 8 Nov 2022 15:15:16 +0100 [thread overview]
Message-ID: <bc830a9f-0bc8-1417-f8df-61245189dd93@proxmox.com> (raw)
In-Reply-To: <510cda71-f621-826e-c00c-4bf25a5b91b0@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;
>
>
next prev parent reply other threads:[~2022-11-08 14:15 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-10-25 14:31 Stefan Hrdlicka
2022-10-28 9:28 ` Thomas Lamprecht
2022-11-08 14:15 ` Stefan Hrdlicka [this message]
2022-11-08 17:00 ` Thomas Lamprecht
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=bc830a9f-0bc8-1417-f8df-61245189dd93@proxmox.com \
--to=s.hrdlicka@proxmox.com \
--cc=pve-devel@lists.proxmox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox