| Message ID | 20220214184257.2406-4-stefan.schantl@ipfire.org |
|---|---|
| State | Accepted |
| Commit | 3d8868807506331a1c4fe160748fa0635bac2a95 |
| 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 4JyClt5Jznz3xgD for <patchwork@web04.haj.ipfire.org>; Mon, 14 Feb 2022 18:43:22 +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 4JyClp2X3Hz5Mr; Mon, 14 Feb 2022 18:43:18 +0000 (UTC) Received: from mail02.haj.ipfire.org (localhost [127.0.0.1]) by mail02.haj.ipfire.org (Postfix) with ESMTP id 4JyClp0lkzz32KK; Mon, 14 Feb 2022 18:43:18 +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 4JyCll3rVmz2y3N for <development@lists.ipfire.org>; Mon, 14 Feb 2022 18:43:15 +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 4JyCll1Wy6z3Yn; Mon, 14 Feb 2022 18:43:15 +0000 (UTC) DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003ed25519; t=1644864195; 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=sRM0ubS5DBws1wrupA1+uvOel70J8sbpq2CnfaCL+zw=; b=qJXpmzZOoPNkPEn9x32mkOCHTt05gwzm9uKrh5NTKa6wlbO8iFUXx59jcmpDx2WEptGST9 P+w+5l8lmkwP+OAg== DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ipfire.org; s=202003rsa; t=1644864195; 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=sRM0ubS5DBws1wrupA1+uvOel70J8sbpq2CnfaCL+zw=; b=wQZgsUopcWztNDyuj2wNYF3CxBu1vLCp7TGtPRJnp5ibJFeb7X6nTNObOBwS0qYRlVsz7O K0ZA7QWlsOM8R/lFsXf6O8m7rFmcaWM1h1LE89xKQJy2r11Q9JSx6RqAF8bTHV3roFtOji ODya7wn/rv5/UV23ghXPOglynoZnn17ZjbxmQtE3Sxpq7zB9vWtmLldvkNMs3Xh/NXQvNw GwMSpeOwoVcLB2sReQPzV0m4BLPNt6v/OQbYIbjrBJ4PnlLMlqkrJIZmVRl4NCHcee9hyv MhSNQJKO+OeXimMqQr9gEKmGhg+qPLf5WeE7a0I/Ept4SxL4vYu331knhQg07Q== From: Stefan Schantl <stefan.schantl@ipfire.org> To: development@lists.ipfire.org Subject: [PATCH 04/12] rules.pl: Destroy all ipset lists on rule reload. Date: Mon, 14 Feb 2022 19:42:48 +0100 Message-Id: <20220214184257.2406-4-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
Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org>
---
config/firewall/rules.pl | 4 ++++
1 file changed, 4 insertions(+)
Comments
Reviewed-by: Peter Müller <peter.mueller@ipfire.org> > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > --- > config/firewall/rules.pl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl > index f685d08a7..da01b8775 100644 > --- a/config/firewall/rules.pl > +++ b/config/firewall/rules.pl > @@ -31,6 +31,7 @@ require "${General::swroot}/location-functions.pl"; > my $DEBUG = 0; > > my $IPTABLES = "iptables --wait"; > +my $IPSET = "ipset"; > > # iptables chains > my $CHAIN_INPUT = "INPUTFW"; > @@ -114,6 +115,9 @@ sub main { > # Flush all chains. > &flush(); > > + # Destroy all existing ipsets. > + run("$IPSET destroy"); > + > # Prepare firewall rules. > if (! -z "${General::swroot}/firewall/input"){ > &buildrules(\%configinputfw);
Hello, Looking at the other patchset that implements IP blocklists, could this interfere with this in any way? -Michael > On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl@ipfire.org> wrote: > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > --- > config/firewall/rules.pl | 4 ++++ > 1 file changed, 4 insertions(+) > > diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl > index f685d08a7..da01b8775 100644 > --- a/config/firewall/rules.pl > +++ b/config/firewall/rules.pl > @@ -31,6 +31,7 @@ require "${General::swroot}/location-functions.pl"; > my $DEBUG = 0; > > my $IPTABLES = "iptables --wait"; > +my $IPSET = "ipset"; > > # iptables chains > my $CHAIN_INPUT = "INPUTFW"; > @@ -114,6 +115,9 @@ sub main { > # Flush all chains. > &flush(); > > + # Destroy all existing ipsets. > + run("$IPSET destroy"); > + > # Prepare firewall rules. > if (! -z "${General::swroot}/firewall/input"){ > &buildrules(\%configinputfw); > -- > 2.30.2 >
Hello, I'm concerned about this as well. Depending on when it does the ipset destroy it may be OK (for example as part of shutting down the system or prior to rebuilding the firewall from scratch, as in these cases it either won't matter or the OP blocklist ipsets will be reloaded), but in general I would consider it a bad idea to delete all the ipsets whether or not you 'own' them - each 'package' should only touch it's own 'property', while this just deletes all the ipsets regardless. Having said that, I think it will probably be alright as according to the documentation ipset destroy won't delete lists which have references to them, and the IP blocklist ipsets should always have references. Tim On 15/02/2022 12:41, Michael Tremer wrote: > Hello, > > Looking at the other patchset that implements IP blocklists, could this interfere with this in any way? > > -Michael > >> On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl@ipfire.org> wrote: >> >> Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> >> --- >> config/firewall/rules.pl | 4 ++++ >> 1 file changed, 4 insertions(+) >> >> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl >> index f685d08a7..da01b8775 100644 >> --- a/config/firewall/rules.pl >> +++ b/config/firewall/rules.pl >> @@ -31,6 +31,7 @@ require "${General::swroot}/location-functions.pl"; >> my $DEBUG = 0; >> >> my $IPTABLES = "iptables --wait"; >> +my $IPSET = "ipset"; >> >> # iptables chains >> my $CHAIN_INPUT = "INPUTFW"; >> @@ -114,6 +115,9 @@ sub main { >> # Flush all chains. >> &flush(); >> >> + # Destroy all existing ipsets. >> + run("$IPSET destroy"); >> + >> # Prepare firewall rules. >> if (! -z "${General::swroot}/firewall/input"){ >> &buildrules(\%configinputfw); >> -- >> 2.30.2 >> >
Hello Tim, > On 15 Feb 2022, at 19:28, Tim FitzGeorge <ipfr@tfitzgeorge.me.uk> wrote: > > Hello, > > I'm concerned about this as well. Depending on when it does the ipset destroy it may be OK (for example as part of shutting down the system or prior to rebuilding the firewall from scratch, as in these cases it either won't matter or the OP blocklist ipsets will be reloaded), but in general I would consider it a bad idea to delete all the ipsets whether or not you 'own' them - each 'package' should only touch it's own 'property', while this just deletes all the ipsets regardless. This is quite hard to implement though. We could in theory iterate over all possible country codes and try to delete all sets, but that seems to be a very slow and not elegant solution to the problem. > Having said that, I think it will probably be alright as according to the documentation ipset destroy won't delete lists which have references to them, and the IP blocklist ipsets should always have references. This is good for us though. If we can consider the “destroy” command to be more of a cleanup and it is safe to call it, then we should not run into any trouble here. @Stefan: Can you confirm that any sets that are still referenced elsewhere won’t be destroyed and that there is no ugly output that could alarm anyone? -Michael > > Tim > > On 15/02/2022 12:41, Michael Tremer wrote: >> Hello, >> >> Looking at the other patchset that implements IP blocklists, could this interfere with this in any way? >> >> -Michael >> >>> On 14 Feb 2022, at 18:42, Stefan Schantl <stefan.schantl@ipfire.org> wrote: >>> >>> Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> >>> --- >>> config/firewall/rules.pl | 4 ++++ >>> 1 file changed, 4 insertions(+) >>> >>> diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl >>> index f685d08a7..da01b8775 100644 >>> --- a/config/firewall/rules.pl >>> +++ b/config/firewall/rules.pl >>> @@ -31,6 +31,7 @@ require "${General::swroot}/location-functions.pl"; >>> my $DEBUG = 0; >>> >>> my $IPTABLES = "iptables --wait"; >>> +my $IPSET = "ipset"; >>> >>> # iptables chains >>> my $CHAIN_INPUT = "INPUTFW"; >>> @@ -114,6 +115,9 @@ sub main { >>> # Flush all chains. >>> &flush(); >>> >>> + # Destroy all existing ipsets. >>> + run("$IPSET destroy"); >>> + >>> # Prepare firewall rules. >>> if (! -z "${General::swroot}/firewall/input"){ >>> &buildrules(\%configinputfw); >>> -- >>> 2.30.2 >>> >> >
Hello Michael, Hello Tim, thanks for your feedback and discussion. > Hello Tim, > > > On 15 Feb 2022, at 19:28, Tim FitzGeorge <ipfr@tfitzgeorge.me.uk> > > wrote: > > > > Hello, > > > > I'm concerned about this as well. Depending on when it does the > > ipset destroy it may be OK (for example as part of shutting down > > the system or prior to rebuilding the firewall from scratch, as in > > these cases it either won't matter or the OP blocklist ipsets will > > be reloaded), but in general I would consider it a bad idea to > > delete all the ipsets whether or not you 'own' them - each > > 'package' should only touch it's own 'property', while this just > > deletes all the ipsets regardless. > > This is quite hard to implement though. We could in theory iterate > over all possible country codes and try to delete all sets, but that > seems to be a very slow and not elegant solution to the problem. > > > Having said that, I think it will probably be alright as according > > to the documentation ipset destroy won't delete lists which have > > references to them, and the IP blocklist ipsets should always have > > references. > > This is good for us though. If we can consider the “destroy” command > to be more of a cleanup and it is safe to call it, then we should not > run into any trouble here. > > @Stefan: Can you confirm that any sets that are still referenced > elsewhere won’t be destroyed and that there is no ugly output that > could alarm anyone? I did not have a look at Tim's code at the moment, nor some testing of his feature so I'm unable to say yes or no, for both of your questions. I'll dig into this at the weekend and phone back what I got. -Stefan > > -Michael > > > > > Tim > > > > On 15/02/2022 12:41, Michael Tremer wrote: > > > Hello, > > > > > > Looking at the other patchset that implements IP blocklists, > > > could this interfere with this in any way? > > > > > > -Michael > > > > > > > On 14 Feb 2022, at 18:42, Stefan Schantl < > > > > stefan.schantl@ipfire.org> wrote: > > > > > > > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > > > > --- > > > > config/firewall/rules.pl | 4 ++++ > > > > 1 file changed, 4 insertions(+) > > > > > > > > diff --git a/config/firewall/rules.pl > > > > b/config/firewall/rules.pl > > > > index f685d08a7..da01b8775 100644 > > > > --- a/config/firewall/rules.pl > > > > +++ b/config/firewall/rules.pl > > > > @@ -31,6 +31,7 @@ require "${General::swroot}/location- > > > > functions.pl"; > > > > my $DEBUG = 0; > > > > > > > > my $IPTABLES = "iptables --wait"; > > > > +my $IPSET = "ipset"; > > > > > > > > # iptables chains > > > > my $CHAIN_INPUT = "INPUTFW"; > > > > @@ -114,6 +115,9 @@ sub main { > > > > # Flush all chains. > > > > &flush(); > > > > > > > > + # Destroy all existing ipsets. > > > > + run("$IPSET destroy"); > > > > + > > > > # Prepare firewall rules. > > > > if (! -z "${General::swroot}/firewall/input"){ > > > > &buildrules(\%configinputfw); > > > > -- > > > > 2.30.2 > > > > > > > > > >
Hello List, as promised, I had a look at Tim's blacklisting code and talked on the phone with Michael and Peter how to handle all this "ipset load and destroy stuff" in a good way. So we talked about, how to deal with all the different scripts and places where ipset sets are involved. We agreed that it would be the best to handle them at a central place (script) and decided the easiest way would be the perl-based firewall script which is used to generate and create the firewall rules. (rules.pl) We also talked about how the IP blocklist feature become a core component of IPFire and to integrate it into this. I'll give more details about this in the related discussion on this list. While discussing about this we came across that the dynamic approach of loading and destroying sets which Tim is using in his code is very lovely and so decided to adopt it in a very similar way. This results in two first patches which have been sent to the development mailing list. The first one will allow the firewall engine to dynamically destroy (unload) ipset sets if they are not longer required. https://patchwork.ipfire.org/project/ipfire/patch/20220227134903.1828-1-stefan.schantl@ipfire.org/ The second patch is a fist step of moving all "ipset" related rules into the same script. https://patchwork.ipfire.org/project/ipfire/patch/20220227134903.1828-2-stefan.schantl@ipfire.org/ Best regards, -Stefan > Hello Michael, Hello Tim, > > thanks for your feedback and discussion. > > Hello Tim, > > > > > On 15 Feb 2022, at 19:28, Tim FitzGeorge <ipfr@tfitzgeorge.me.uk> > > > wrote: > > > > > > Hello, > > > > > > I'm concerned about this as well. Depending on when it does the > > > ipset destroy it may be OK (for example as part of shutting down > > > the system or prior to rebuilding the firewall from scratch, as > > > in > > > these cases it either won't matter or the OP blocklist ipsets > > > will > > > be reloaded), but in general I would consider it a bad idea to > > > delete all the ipsets whether or not you 'own' them - each > > > 'package' should only touch it's own 'property', while this just > > > deletes all the ipsets regardless. > > > > This is quite hard to implement though. We could in theory iterate > > over all possible country codes and try to delete all sets, but > > that > > seems to be a very slow and not elegant solution to the problem. > > > > > Having said that, I think it will probably be alright as > > > according > > > to the documentation ipset destroy won't delete lists which have > > > references to them, and the IP blocklist ipsets should always > > > have > > > references. > > > > This is good for us though. If we can consider the “destroy” > > command > > to be more of a cleanup and it is safe to call it, then we should > > not > > run into any trouble here. > > > > @Stefan: Can you confirm that any sets that are still referenced > > elsewhere won’t be destroyed and that there is no ugly output that > > could alarm anyone? > > I did not have a look at Tim's code at the moment, nor some testing > of > his feature so I'm unable to say yes or no, for both of your > questions. > > I'll dig into this at the weekend and phone back what I got. > > -Stefan > > > > > -Michael > > > > > > > > Tim > > > > > > On 15/02/2022 12:41, Michael Tremer wrote: > > > > Hello, > > > > > > > > Looking at the other patchset that implements IP blocklists, > > > > could this interfere with this in any way? > > > > > > > > -Michael > > > > > > > > > On 14 Feb 2022, at 18:42, Stefan Schantl < > > > > > stefan.schantl@ipfire.org> wrote: > > > > > > > > > > Signed-off-by: Stefan Schantl <stefan.schantl@ipfire.org> > > > > > --- > > > > > config/firewall/rules.pl | 4 ++++ > > > > > 1 file changed, 4 insertions(+) > > > > > > > > > > diff --git a/config/firewall/rules.pl > > > > > b/config/firewall/rules.pl > > > > > index f685d08a7..da01b8775 100644 > > > > > --- a/config/firewall/rules.pl > > > > > +++ b/config/firewall/rules.pl > > > > > @@ -31,6 +31,7 @@ require "${General::swroot}/location- > > > > > functions.pl"; > > > > > my $DEBUG = 0; > > > > > > > > > > my $IPTABLES = "iptables --wait"; > > > > > +my $IPSET = "ipset"; > > > > > > > > > > # iptables chains > > > > > my $CHAIN_INPUT = "INPUTFW"; > > > > > @@ -114,6 +115,9 @@ sub main { > > > > > # Flush all chains. > > > > > &flush(); > > > > > > > > > > + # Destroy all existing ipsets. > > > > > + run("$IPSET destroy"); > > > > > + > > > > > # Prepare firewall rules. > > > > > if (! -z "${General::swroot}/firewall/input"){ > > > > > &buildrules(\%configinputfw); > > > > > -- > > > > > 2.30.2 > > > > > > > > > > > > > > > >
diff --git a/config/firewall/rules.pl b/config/firewall/rules.pl index f685d08a7..da01b8775 100644 --- a/config/firewall/rules.pl +++ b/config/firewall/rules.pl @@ -31,6 +31,7 @@ require "${General::swroot}/location-functions.pl"; my $DEBUG = 0; my $IPTABLES = "iptables --wait"; +my $IPSET = "ipset"; # iptables chains my $CHAIN_INPUT = "INPUTFW"; @@ -114,6 +115,9 @@ sub main { # Flush all chains. &flush(); + # Destroy all existing ipsets. + run("$IPSET destroy"); + # Prepare firewall rules. if (! -z "${General::swroot}/firewall/input"){ &buildrules(\%configinputfw);