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

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

[Stefan Schantl]

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

In case a GeoIP related firewall rule should be created, the script
now will check if the given location is still available.

Fixes #12054.

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"]);
+		}
 
 	# 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"]);
+		}
 
 	# 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;
-- 
2.20.1

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

[Peter Müller]

[-- 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

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

[Michael Tremer]

[-- 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

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

[Peter Müller]

[-- 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