Date: Thu, 9 Mar 2006 04:20:07 GMT From: Vulpes Velox <v.velox@vvelox.net> To: freebsd-rc@FreeBSD.org Subject: Re: conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw and rc.d/ip6fw. Message-ID: <200603090420.k294K7Mn092773@freefall.freebsd.org>
next in thread | raw e-mail | index | archive | help
The following reply was made to PR conf/93815; it has been noted by GNATS.
From: Vulpes Velox <v.velox@vvelox.net>
To: Giorgos Keramidas <keramida@FreeBSD.org>
Cc: bug-followup@FreeBSD.org
Subject: Re: conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw
and rc.d/ip6fw.
Date: Wed, 8 Mar 2006 22:23:21 -0600
On Sun, 5 Mar 2006 04:54:55 +0200
Giorgos Keramidas <keramida@FreeBSD.org> wrote:
> On 2006-02-25 04:19, Vulpes Velox <v.velox@vvelox.net> wrote:
> > This allows ipfw rules to be saved. /var/db/ipfw is used for
> > that. If a name for the save is not specified, last will be used.
> >
> > They can be saved like this...
> > /etc/rc.d/ipfw save <name>
> >
> > They can be recalled like this...
> > /etc/rc.d/ipfw restart <name>
>
> I feel a bit worried about allowing unquoted user-supplied names to
> a shell script and then using them as filenames.
>
> > --- rc.d_ipfw.patch begins here ---
> > 18a19,29
> > > extra_commands="save"
> > > save_cmd="ipfw_save"
> > >
> > >
> > > #gets the name of the save to use
> > > if [ ! -z $2 ]; then
> > > savename="$2"
> > > usingsave="yes"
> > > else
> > > savename="last"
> > > fi
Cool. Fixed.
> Please don't. This should be written at least with a proper quote
> set around $2 like this:
>
> if [ -z "$2" ]; then
> savename="last"
> else
> savename="$2"
> usingsave="yes"
> fi
>
> > 31a43,49
> > > ipfw_save()
> > > {
> > > # Saves the firewall rules to /var/db/ipfw/$savename
> > > [ ! -d /var/db/ipfw ] && mkdir /var/db/ipfw && chmod
> > > go-rwx /var/db/ipfw ipfw list | awk '{print "${fwcmd} add "
> > > $0 }' > /var/db/ipfw/$savename }
>
> The style sucks a bit here, but it's mostly ok. I'd probably avoid
> constructs that have the potential to end up using really-very-long
> lines, like cmd && cmd && cmd a bit and make the directory of the
> saved firewalls tunable through rc.conf:
>
> ipfw_save()
> {
> # set the firewall save directory if none was specified
> [ -z "${firewall_savedir}" ] &&
> firewall_savedir=/var/db/ipfw
>
> if [ ! -d "${firewall_savedir}" ]; then
> mkdir -p "${firewall_savedir}" || return 1
> fi
>
> ipfw list | sed -e 's/^/add /' >
> "${firewall_savedir}/${savename}" }
Cool. I like the that idea for the savedir. I am some what mixed
about making it longer, but I see the point in making it more
readable though.
> Also, in my opinion, loading saved rulesets shouldn't be overloaded
> with the special 'last' savename, but supported by a similar
> ipfw_load() function. Then 'last' could be used as a valid
> savename too :)
>
True. It can just be thrown in /etc/defualts/rc.conf.
I will have the new patch set pr submitted tomorrow.
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200603090420.k294K7Mn092773>