From mboxrd@z Thu Jan  1 00:00:00 1970
From: Stefan Schantl <stefan.schantl@ipfire.org>
To: development@lists.ipfire.org
Subject: Re: [PATCH 04/12] rules.pl: Destroy all ipset lists on rule reload.
Date: Thu, 17 Feb 2022 05:56:42 +0100
Message-ID: <8dbf6929eee4887265035c8177012f908c8a7e86.camel@ipfire.org>
In-Reply-To: <56E22B21-4AF6-4FD9-BFAD-B4098B4BE646@ipfire.org>
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="===============3029319882758862753=="
List-Id: <development.lists.ipfire.org>

--===============3029319882758862753==
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: 8bit

Hello Michael, Hello Tim,

thanks for your feedback and discussion.
> Hello Tim,
> 
> > On 15 Feb 2022, at 19:28, Tim FitzGeorge <ipfr(a)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(a)ipfire.org> wrote:
> > > > 
> > > > Signed-off-by: Stefan Schantl <stefan.schantl(a)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
> > > > 
> > > 
> > 
> 



--===============3029319882758862753==--