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@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;
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@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
Hi,
On 16 Apr 2019, at 20:40, Peter Müller peter.mueller@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@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
Hello *,
I think we/I completely forgot about this patch. Sorry.
Hi,
On 16 Apr 2019, at 20:40, Peter Müller peter.mueller@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@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@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
-
Michael Tremer -
Peter Müller -
Stefan Schantl