conf/93815: Adds in the ability to save ipfw rules to rc.d/ipfw and rc.d/ip6fw.

[Vulpes Velox]

The following reply was made to PR conf/93815; it has been noted by GNATS.

From: Vulpes Velox <v.velox at vvelox.net>
To: Giorgos Keramidas <keramida at FreeBSD.org>
Cc: bug-followup at 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 at FreeBSD.org> wrote:
 
 > On 2006-02-25 04:19, Vulpes Velox <v.velox at 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.