From mboxrd@z Thu Jan  1 00:00:00 1970
From: Michael Tremer <michael.tremer@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH] firewall-lib.pl: Populate GeoIP rules only if location is
 available.
Date: Tue, 16 Apr 2019 21:57:29 +0100
Message-ID: <CF16BB36-A70C-47D0-85D4-8907975BFD14@ipfire.org>
In-Reply-To: <7b92045a-2123-773a-d2c7-1da0fdfc3f75@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============1312605413924695313=="
List-Id: <development.lists.ipfire.org>

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

Hi,

> On 16 Apr 2019, at 20:40, Peter M=C3=BCller <peter.mueller(a)ipfire.org> wr=
ote:
>=20
> Hello Stefan,
>=20
> first: Thank you for developing this fix. :-)
>=20
>> In case a GeoIP related firewall rule should be created, the script
>> now will check if the given location is still available.
>>=20
>> Fixes #12054.
> Thanks again - however, there is one question left (see below).
>>=20
>> Signed-off-by: Stefan Schantl <stefan.schantl(a)ipfire.org>
>> ---
>> config/firewall/firewall-lib.pl | 40 ++++++++++++++++++++++++++++-----
>> 1 file changed, 34 insertions(+), 6 deletions(-)
>>=20
>> diff --git a/config/firewall/firewall-lib.pl b/config/firewall/firewall-li=
b.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/settin=
gs";
>> &General::readhasharray("$configsrvgrp", \%customservicegrp);
>> &General::get_aliases(\%aliases);
>>=20
>> +# 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
>>=20
>> 	# 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();
>>=20
>> -		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.
>=20
> 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).

I think this is a valid objection, although I think that the code here should=
 be merged as is.

In addition there should be a patch to the UI that fixes what Peter has raise=
d.

>>=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();
>>=20
>> -		push(@ret, ["-m geoip --dst-cc $value", "$external_interface"]);
>> +			push(@ret, ["-m geoip --dst-cc $value", "$external_interface"]);
>> +		}
> Same as above.
>>=20
>> 	# If nothing was selected, we assume "any".
>> 	} else {
>> @@ -610,4 +619,23 @@ sub get_geoip_locations() {
>> 	return &GeoIP::get_geoip_locations();
>> }
>>=20
>> +# 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 availab=
le
>> +	# zones. Return nothing.
>> +	return;
>> +}
>> +
>> return 1;
>>=20
>=20
> Thanks, and best regards,
> Peter M=C3=BCller
> --=20
> The road to Hades is easy to travel.
> 	-- Bion of Borysthenes


--===============1312605413924695313==--