From mboxrd@z Thu Jan  1 00:00:00 1970
From: Peter =?utf-8?q?M=C3=BCller?= <peter.mueller@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] firewall-lib.pl: Populate GeoIP rules only if location is
 available.
Date: Thu, 04 Jul 2019 16:48:00 +0000
Message-ID: <e52e0f76-0b3c-fa2f-81ef-044d14dd2215@ipfire.org>
In-Reply-To: <CF16BB36-A70C-47D0-85D4-8907975BFD14@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============7256976215227730898=="
List-Id: <development.lists.ipfire.org>

--===============7256976215227730898==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable

Hello *,

I think we/I completely forgot about this patch. Sorry.

> Hi,
>=20
>> On 16 Apr 2019, at 20:40, Peter M=C3=BCller <peter.mueller(a)ipfire.org> w=
rote:
>>
>> Hello Stefan,
>>
>> first: Thank you for developing this fix. :-)
>>
>>> In case a GeoIP related firewall rule should be created, the script
>>> now will check if the given location is still available.
>>>
>>> Fixes #12054.
>> Thanks again - however, there is one question left (see below).
>>>
>>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>>> ---
>>> config/firewall/firewall-lib.pl | 40 ++++++++++++++++++++++++++++-----
>>> 1 file changed, 34 insertions(+), 6 deletions(-)
>>>
>>> diff --git a/config/firewall/firewall-lib.pl b/config/firewall/firewall-l=
ib.pl
>>> index 118744fd6..59ae096b0 100644
>>> --- a/config/firewall/firewall-lib.pl
>>> +++ b/config/firewall/firewall-lib.pl
>>> @@ -70,6 +70,9 @@ my $netsettings		=3D "${General::swroot}/ethernet/setti=
ngs";
>>> &General::readhasharray("$configsrvgrp", \%customservicegrp);
>>> &General::get_aliases(\%aliases);
>>>
>>> +# Get all available GeoIP locations.
>>> +my @available_geoip_locations =3D &get_geoip_locations();
>>> +
>>> sub get_srv_prot
>>> {
>>> 	my $val=3Dshift;
>>> @@ -456,17 +459,23 @@ sub get_address
>>>
>>> 	# Handle rule options with GeoIP as source.
>>> 	} elsif ($key eq "cust_geoip_src") {
>>> -		# Get external interface.
>>> -		my $external_interface =3D &get_external_interface();
>>> +		# Check if the given GeoIP location is available.
>>> +		if(&geoip_location_is_available($value)) {
>>> +			# Get external interface.
>>> +			my $external_interface =3D &get_external_interface();
>>>
>>> -		push(@ret, ["-m geoip --src-cc $value", "$external_interface"]);
>>> +			push(@ret, ["-m geoip --src-cc $value", "$external_interface"]);
>>> +		}
>> If I am getting this right, firewall rules with a GeoIP
>> source unavailable at the moment (or permanently) are
>> silently ignored. IMHO, this might confuse users as the
>> firewalls behaviour does not change even if WebUI suggests.
>>
>> Since I have no idea how to avoid this, there are no
>> subjective objections (!) against the patch from my point
>> of view. It would, however, be better if this situation
>> never happens - as described in #12054 ("A1/A2/AP/EU" currently
>> not working/not present).
>=20
> I think this is a valid objection, although I think that the code here shou=
ld be merged as is.
Yes...

Reviewed-by: Peter M=C3=BCller <peter.mueller(a)ipfire.org>
>=20
> In addition there should be a patch to the UI that fixes what Peter has rai=
sed.
>=20
>>>
>>> 	# Handle rule options with GeoIP as target.
>>> 	} elsif ($key eq "cust_geoip_tgt") {
>>> -		# Get external interface.
>>> -		my $external_interface =3D &get_external_interface();
>>> +		# Check if the given GeoIP location is available.
>>> +		if(&geoip_location_is_available($value)) {
>>> +			# Get external interface.
>>> +			my $external_interface =3D &get_external_interface();
>>>
>>> -		push(@ret, ["-m geoip --dst-cc $value", "$external_interface"]);
>>> +			push(@ret, ["-m geoip --dst-cc $value", "$external_interface"]);
>>> +		}
>> Same as above.
>>>
>>> 	# If nothing was selected, we assume "any".
>>> 	} else {
>>> @@ -610,4 +619,23 @@ sub get_geoip_locations() {
>>> 	return &GeoIP::get_geoip_locations();
>>> }
>>>
>>> +# Function to check if a database of a given GeoIP location is
>>> +# available.
>>> +sub geoip_location_is_available($) {
>>> +	my ($location) =3D @_;
>>> +
>>> +	# Loop through the global array of available GeoIP locations.
>>> +	foreach my $geoip_location (@available_geoip_locations) {
>>> +		# Check if the current processed location is the searched one.
>>> +		if($location eq $geoip_location) {
>>> +			# If it is part of the array, return "1" - True.
>>> +			return 1;
>>> +		}
>>> +	}
>>> +
>>> +	# If we got here, the given location is not part of the array of availa=
ble
>>> +	# zones. Return nothing.
>>> +	return;
>>> +}
>>> +
>>> return 1;
>>>

Thanks, and best regards,
Peter M=C3=BCller
--=20
The road to Hades is easy to travel.
	-- Bion of Borysthenes

--===============7256976215227730898==--