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: 3588 bytes --]

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).
>  
>  	# 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