Re: [PATCH] firewall-lib.pl: Populate GeoIP rules only if location is available.

[Michael Tremer <michael.tremer@ipfire.org>]

[-- Attachment #1: Type: text/plain, Size: 3963 bytes --]

Hi,

> On 16 Apr 2019, at 20:40, Peter Müller <peter.mueller(a)ipfire.org> wrote:
> 
> 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-lib.pl
>> index 118744fd6..59ae096b0 100644
>> --- a/config/firewall/firewall-lib.pl
>> +++ b/config/firewall/firewall-lib.pl
>> @@ -70,6 +70,9 @@ my $netsettings		= "${General::swroot}/ethernet/settings";
>> &General::readhasharray("$configsrvgrp", \%customservicegrp);
>> &General::get_aliases(\%aliases);
>> 
>> +# Get all available GeoIP locations.
>> +my @available_geoip_locations = &get_geoip_locations();
>> +
>> sub get_srv_prot
>> {
>> 	my $val=shift;
>> @@ -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 = &get_external_interface();
>> +		# Check if the given GeoIP location is available.
>> +		if(&geoip_location_is_available($value)) {
>> +			# Get external interface.
>> +			my $external_interface = &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).

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 raised.

>> 
>> 	# Handle rule options with GeoIP as target.
>> 	} elsif ($key eq "cust_geoip_tgt") {
>> -		# Get external interface.
>> -		my $external_interface = &get_external_interface();
>> +		# Check if the given GeoIP location is available.
>> +		if(&geoip_location_is_available($value)) {
>> +			# Get external interface.
>> +			my $external_interface = &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) = @_;
>> +
>> +	# 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 available
>> +	# zones. Return nothing.
>> +	return;
>> +}
>> +
>> return 1;
>> 
> 
> Thanks, and best regards,
> Peter Müller
> -- 
> The road to Hades is easy to travel.
> 	-- Bion of Borysthenes