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

["Peter Müller" <peter.mueller@ipfire.org>]

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

Hello *,

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

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

Reviewed-by: Peter Müller <peter.mueller(a)ipfire.org>
> 
> 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