Message ID | 20220214184257.2406-9-stefan.schantl@ipfire.org |
---|---|
State | Accepted |
Commit | 278289690d50d6f28926742e29f5b005293132eb |
Headers |
Return-Path: <development-bounces@lists.ipfire.org> Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by web04.haj.ipfire.org (Postfix) with ESMTPS id 4JyCm35t1bz3xgN for <patchwork@web04.haj.ipfire.org>; Mon, 14 Feb 2022 18:43:31 +0000 (UTC) Received: from mail02.haj.ipfire.org (mail02.haj.ipfire.org [172.28.1.201]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail02.haj.ipfire.org", Issuer "R3" (verified OK)) by mail01.ipfire.org (Postfix) with ESMTPS id 4JyClr50pNz5QV; Mon, 14 Feb 2022 18:43:20 +0000 (UTC) Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4JyClr4hB9z2yxp; Mon, 14 Feb 2022 18:43:20 +0000 (UTC) Received: from mail01.ipfire.org (mail01.haj.ipfire.org [172.28.1.202]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) client-signature ECDSA (P-384)) (Client CN "mail01.haj.ipfire.org", Issuer "R3" (verified OK)) by mail02.haj.ipfire.org (Postfix) with ESMTPS id 4JyCln4VSJz2ywH for <development@lists.ipfire.org>; Mon, 14 Feb 2022 18:43:17 +0000 (UTC) Received: from [127.0.0.1] (localhost [127.0.0.1]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange X25519 server-signature ECDSA (P-384) server-digest SHA384) (No client certificate requested) by mail01.ipfire.org (Postfix) with ESMTPSA id 4JyCln1mH6z2Hr; Mon, 14 Feb 2022 18:43:17 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1644864197; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SZ/L3RirfIJvZvOkW0vdCIyA92+8Po4QIhRr1F+cPoE=; b=D6xUgIUriplAacn8zIJ2/renPvCmrh0GML8h+p8wQQD7Rt0Dou2xtqlKUVjFVlWQXm11Bc iMOnOo9YYTMzU2Dg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1644864197; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=SZ/L3RirfIJvZvOkW0vdCIyA92+8Po4QIhRr1F+cPoE=; b=YaZKE3HWxCVq1SQfEys2sg/+DB6EjBK8bJxHvlNO3+X11775F6iIGOzMkbSMUuXOS1SvS6 53cwR+ctkQFI3mS77Ok1ojD8n6E2OktyIyZ5Ax3VAebom6SWdxdSxC5eK5zVM02nEHZyvv Raj8b5PNOEPFG0u1lVTTMVPFJEJhKO8qK8MLzu9VjTWoRJDMWm5zTzw4cO+eDMncG3WbPL 18iOZqgRiFrQdb5VA+LqFffksIr1k49MkTlLmu4/eSZ9ILjKXvkolHZuBqbvvXLsAZUbsL ltQ1/wgfeB7Blc0ktia8/vaWrRYpUaJL1S5Zx/dkOVBKb9/bdIM+EHjXFa0cAA== From: Stefan Schantl <stefan.schantl@ipfire.org> To: development@lists.ipfire.org Subject: [PATCH 09/12] rules.pl: Do not try to restore the same ipset multiple times. Date: Mon, 14 Feb 2022 19:42:53 +0100 Message-Id: <20220214184257.2406-9-stefan.schantl@ipfire.org> In-Reply-To: <20220214184257.2406-1-stefan.schantl@ipfire.org> References: <20220214184257.2406-1-stefan.schantl@ipfire.org> MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-BeenThere: development@lists.ipfire.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: IPFire development talk <development.lists.ipfire.org> List-Unsubscribe: <https://lists.ipfire.org/mailman/options/development>, <mailto:development-request@lists.ipfire.org?subject=unsubscribe> List-Archive: <http://lists.ipfire.org/pipermail/development/> List-Post: <mailto:development@lists.ipfire.org> List-Help: <mailto:development-request@lists.ipfire.org?subject=help> List-Subscribe: <https://lists.ipfire.org/mailman/listinfo/development>, <mailto:development-request@lists.ipfire.org?subject=subscribe> Errors-To: development-bounces@lists.ipfire.org Sender: "Development" <development-bounces@lists.ipfire.org> |
Series |
[01/12] location-functions.pl: Rename and set the location for exported databases to "/var/lib/location/ipset/".
|
|
Commit Message
Stefan Schantl
Feb. 14, 2022, 6:42 p.m. UTC
When an ipset list get restored, this now will be documented in a hash
and this hash also will be checked before restoring a list if this has
not be done previously.
This will prevent from restoring the same list multiple times.
Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
---
config/firewall/rules.pl | 31 +++++++++++++++++++++++++------
1 file changed, 25 insertions(+), 6 deletions(-)
Comments
Reviewed-by: Peter Müller <peter.mueller@ipfire.org> > When an ipset list get restored, this now will be documented in a hash > and this hash also will be checked before restoring a list if this has > not be done previously. > > This will prevent from restoring the same list multiple times. > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > --- > config/firewall/rules.pl | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl > index d533ffb42..29990ee67 100644 > --- a/config/firewall/rules.pl > +++ b/config/firewall/rules.pl > @@ -70,6 +70,7 @@ my %confignatfw=(); > my %locationsettings = ( > "LOCATIONBLOCK_ENABLED" => "off" > ); > +my %loaded_ipset_lists=(); > > my @p2ps=(); > > @@ -405,8 +406,14 @@ sub buildrules { > # Grab location code from hash. > my $loc_src = $$hash{$key}[4]; > > - # Call function to load the networks list for this country. > - &ipset_restore($loc_src); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$loc_src}) { > + # Call function to load the networks list for this country. > + &ipset_restore($loc_src); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$loc_src} = "1"; > + } > > push(@source_options, $source); > } elsif($source) { > @@ -419,8 +426,14 @@ sub buildrules { > # Grab location code from hash. > my $loc_dst = $$hash{$key}[6]; > > - # Call function to load the networks list for this country. > - &ipset_restore($loc_dst); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$loc_dst}) { > + # Call function to load the networks list for this country. > + &ipset_restore($loc_dst); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$loc_dst} = "1"; > + } > > push(@destination_options, $destination); > } elsif ($destination) { > @@ -683,8 +696,14 @@ sub locationblock { > # is enabled. > foreach my $location (@locations) { > if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") { > - # Call function to load the networks list for this country. > - &ipset_restore($location); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$location}) { > + # Call function to load the networks list for this country. > + &ipset_restore($location); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$location} = "1"; > + } > > # Call iptables and create rule to use the loaded ipset list. > run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP");
Hello, I would have implemented this differently. Would it not be better to perform the check in ipset_restore() so that you won’t have to copy the code to everywhere you call ipset_restore? This solution bloats the code slightly. -Michael > On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl@ipfire.org> wrote: > > When an ipset list get restored, this now will be documented in a hash > and this hash also will be checked before restoring a list if this has > not be done previously. > > This will prevent from restoring the same list multiple times. > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > --- > config/firewall/rules.pl | 31 +++++++++++++++++++++++++------ > 1 file changed, 25 insertions(+), 6 deletions(-) > > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl > index d533ffb42..29990ee67 100644 > --- a/config/firewall/rules.pl > +++ b/config/firewall/rules.pl > @@ -70,6 +70,7 @@ my %confignatfw=(); > my %locationsettings = ( > "LOCATIONBLOCK_ENABLED" => "off" > ); > +my %loaded_ipset_lists=(); > > my @p2ps=(); > > @@ -405,8 +406,14 @@ sub buildrules { > # Grab location code from hash. > my $loc_src = $$hash{$key}[4]; > > - # Call function to load the networks list for this country. > - &ipset_restore($loc_src); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$loc_src}) { > + # Call function to load the networks list for this country. > + &ipset_restore($loc_src); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$loc_src} = "1"; > + } > > push(@source_options, $source); > } elsif($source) { > @@ -419,8 +426,14 @@ sub buildrules { > # Grab location code from hash. > my $loc_dst = $$hash{$key}[6]; > > - # Call function to load the networks list for this country. > - &ipset_restore($loc_dst); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$loc_dst}) { > + # Call function to load the networks list for this country. > + &ipset_restore($loc_dst); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$loc_dst} = "1"; > + } > > push(@destination_options, $destination); > } elsif ($destination) { > @@ -683,8 +696,14 @@ sub locationblock { > # is enabled. > foreach my $location (@locations) { > if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") { > - # Call function to load the networks list for this country. > - &ipset_restore($location); > + # Check if the network list for this country already has been loaded. > + unless($loaded_ipset_lists{$location}) { > + # Call function to load the networks list for this country. > + &ipset_restore($location); > + > + # Store to the hash that this list has been loaded. > + $loaded_ipset_lists{$location} = "1"; > + } > > # Call iptables and create rule to use the loaded ipset list. > run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP"); > -- > 2.30.2 >
Hello Michael, thanks for reviewing and your feedback. You are absolutely right, it would give us much cleaner code when moving this kind of check into the ipset_restore() function. I'll send a patch for this. Best regards, -Stefan > Hello, > > I would have implemented this differently. > > Would it not be better to perform the check in ipset_restore() so > that you won’t have to copy the code to everywhere you call > ipset_restore? > > This solution bloats the code slightly. > > -Michael > > > On 14 Feb 2022, at 18:42, Stefan Schantl > > <stefan.schantl@ipfire.org> wrote: > > > > When an ipset list get restored, this now will be documented in a > > hash > > and this hash also will be checked before restoring a list if this > > has > > not be done previously. > > > > This will prevent from restoring the same list multiple times. > > > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > > --- > > config/firewall/rules.pl | 31 +++++++++++++++++++++++++------ > > 1 file changed, 25 insertions(+), 6 deletions(-) > > > > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl > > index d533ffb42..29990ee67 100644 > > --- a/config/firewall/rules.pl > > +++ b/config/firewall/rules.pl > > @@ -70,6 +70,7 @@ my %confignatfw=(); > > my %locationsettings = ( > > "LOCATIONBLOCK_ENABLED" => "off" > > ); > > +my %loaded_ipset_lists=(); > > > > my @p2ps=(); > > > > @@ -405,8 +406,14 @@ sub buildrules { > > # Grab location > > code from hash. > > my $loc_src = > > $$hash{$key}[4]; > > > > - # Call function to > > load the networks list for this country. > > - > > &ipset_restore($loc_s > > rc); > > + # Check if the > > network list for this country already has been loaded. > > + unless($loaded_ipse > > t_lists{$loc_src}) { > > + # Call > > function to load the networks list for this country. > > + &ipset_rest > > ore($loc_src); > > + > > + # Store to > > the hash that this list has been loaded. > > + $loaded_ips > > et_lists{$loc_src} = "1"; > > + } > > > > push(@source_option > > s, $source); > > } elsif($source) { > > @@ -419,8 +426,14 @@ sub buildrules { > > # Grab location > > code from hash. > > my $loc_dst = > > $$hash{$key}[6]; > > > > - # Call function to > > load the networks list for this country. > > - > > &ipset_restore($loc_d > > st); > > + # Check if the > > network list for this country already has been loaded. > > + unless($loaded_ipse > > t_lists{$loc_dst}) { > > + # Call > > function to load the networks list for this country. > > + &ipset_rest > > ore($loc_dst); > > + > > + # Store to > > the hash that this list has been loaded. > > + $loaded_ips > > et_lists{$loc_dst} = "1"; > > + } > > > > push(@destination_o > > ptions, $destination); > > } elsif ($destination) { > > @@ -683,8 +696,14 @@ sub locationblock { > > # is enabled. > > foreach my $location (@locations) { > > if(exists $locationsettings{$location} && > > $locationsettings{$location} eq "on") { > > - # Call function to load the networks list > > for this country. > > - &ipset_restore($location); > > + # Check if the network list for this > > country already has been loaded. > > + unless($loaded_ipset_lists{$location}) { > > + # Call function to load the > > networks list for this country. > > + &ipset_restore($location); > > + > > + # Store to the hash that this list > > has been loaded. > > + $loaded_ipset_lists{$location} = > > "1"; > > + } > > > > # Call iptables and create rule to use the > > loaded ipset list. > > run("$IPTABLES -A LOCATIONBLOCK -m set -- > > match-set CC_$location src -j DROP"); > > -- > > 2.30.2 > > >
diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl index d533ffb42..29990ee67 100644 --- a/config/firewall/rules.pl +++ b/config/firewall/rules.pl @@ -70,6 +70,7 @@ my %confignatfw=(); my %locationsettings = ( "LOCATIONBLOCK_ENABLED" => "off" ); +my %loaded_ipset_lists=(); my @p2ps=(); @@ -405,8 +406,14 @@ sub buildrules { # Grab location code from hash. my $loc_src = $$hash{$key}[4]; - # Call function to load the networks list for this country. - &ipset_restore($loc_src); + # Check if the network list for this country already has been loaded. + unless($loaded_ipset_lists{$loc_src}) { + # Call function to load the networks list for this country. + &ipset_restore($loc_src); + + # Store to the hash that this list has been loaded. + $loaded_ipset_lists{$loc_src} = "1"; + } push(@source_options, $source); } elsif($source) { @@ -419,8 +426,14 @@ sub buildrules { # Grab location code from hash. my $loc_dst = $$hash{$key}[6]; - # Call function to load the networks list for this country. - &ipset_restore($loc_dst); + # Check if the network list for this country already has been loaded. + unless($loaded_ipset_lists{$loc_dst}) { + # Call function to load the networks list for this country. + &ipset_restore($loc_dst); + + # Store to the hash that this list has been loaded. + $loaded_ipset_lists{$loc_dst} = "1"; + } push(@destination_options, $destination); } elsif ($destination) { @@ -683,8 +696,14 @@ sub locationblock { # is enabled. foreach my $location (@locations) { if(exists $locationsettings{$location} && $locationsettings{$location} eq "on") { - # Call function to load the networks list for this country. - &ipset_restore($location); + # Check if the network list for this country already has been loaded. + unless($loaded_ipset_lists{$location}) { + # Call function to load the networks list for this country. + &ipset_restore($location); + + # Store to the hash that this list has been loaded. + $loaded_ipset_lists{$location} = "1"; + } # Call iptables and create rule to use the loaded ipset list. run("$IPTABLES -A LOCATIONBLOCK -m set --match-set CC_$location src -j DROP");