Re: pfctl should allow administrator to flush _anchors
Hello, > > > > So how people feel about changing '-Fa' to kill all rules and tables, not > > just > > those, which are attached to main ruleset (root)? > > > > thanks and > > regards > > sashan > > > > IMHO this is a needed feature, but I agree with your hesitation about > using -Fa. This would be convenient to type, but the current documentation > for pfctl -a says: > > "In addition to the main ruleset, pfctl can load and manipulate >additional rulesets by name, called anchors. The main ruleset is the >default anchor." > > The wording is slightly awkward but I read this as saying the current > behaviour is intended. > > There's an obvious alternative user interface for this. Currently > -a '*' is only described in conjunction with -s, but it would feel > natural to allow this to be used with -F as well, e.g. > ># pfctl -a '*' -Fa > I like this idea to interpret "-a '*'" option in conjunction with '-F...' in the same way we do it for "-s" already. I also like tedu's idea to introduce a '-Freset'. I'll try to cook up some diffs. One diff will deal with "-a '*' -F..." the other will bring '-Freset'. thanks and regards sashan
Re: pfctl should allow administrator to flush _anchors
On 2019/03/26 09:38, Alexandr Nedvedicky wrote:
> On Mon, Mar 25, 2019 at 10:28:40PM -0400, Ted Unangst wrote:
> > Alexandr Nedvedicky wrote:
> > > it is, however -Fall operates on main ruleset only. -Fall also does
> > > not reset limits and timeouts. Hence my first idea was to introduce
> > > '-FNuke', which kills all rulesets and tables.
> > >
> > > I don't want to change behaviour of existing option ('-Fall'),
> > > therefore
> > > I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > > for me. I'm the most concerned about flushing all rulesets.
> >
> > Is the existing behavior intentional or an oversight? I don't know when I
> > would want to use -Fall, but keep the old timeouts, and depend on that. I'd
> > guess most people using -Fall are keeping old timeout only by happen stance,
> > and not because they desire that.
>
> I had similar question on my mind when I came to PF for the first time.
> my expectations about '-Fall' were the option removes all rules (and
> tables)
> from kernel module. But it is not the case it acts on main ruleset only.
> Given '-Fall' works like that for ages, I see changing '-Fall' to remove
> all rules as disturbing (hence I'm in favor to introduce a new option). On
> the other hand if there will be consensus to fix '-Fall' so it will remove
> all rules (not just main ruleset), then we can forget about '-U'.
>
> With '-Fall' changed, we can further fix pfctl. The proposed '-U', will
> be achieved by combination of various '-F' modifiers:
> pfctl -FA -FS -Fs -Freset
> command above should revert PF driver state back to initial.
>
> >
> > In any case, if you're seeking input on the name, something like -Freset
> > says
> > to me that it resets pf back to its initial state.
>
> I like the '-Fresst' to reset all PF settings (variables modified by
> 'set')
> back to defaults.
>
> So how people feel about changing '-Fa' to kill all rules and tables, not just
> those, which are attached to main ruleset (root)?
>
> thanks and
> regards
> sashan
>
IMHO this is a needed feature, but I agree with your hesitation about
using -Fa. This would be convenient to type, but the current documentation
for pfctl -a says:
"In addition to the main ruleset, pfctl can load and manipulate
additional rulesets by name, called anchors. The main ruleset is the
default anchor."
The wording is slightly awkward but I read this as saying the current
behaviour is intended.
There's an obvious alternative user interface for this. Currently
-a '*' is only described in conjunction with -s, but it would feel
natural to allow this to be used with -F as well, e.g.
# pfctl -a '*' -Fa
Re: pfctl should allow administrator to flush _anchors
On Mon, Mar 25, 2019 at 10:28:40PM -0400, Ted Unangst wrote:
> Alexandr Nedvedicky wrote:
> > it is, however -Fall operates on main ruleset only. -Fall also does
> > not reset limits and timeouts. Hence my first idea was to introduce
> > '-FNuke', which kills all rulesets and tables.
> >
> > I don't want to change behaviour of existing option ('-Fall'), therefore
> > I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > for me. I'm the most concerned about flushing all rulesets.
>
> Is the existing behavior intentional or an oversight? I don't know when I
> would want to use -Fall, but keep the old timeouts, and depend on that. I'd
> guess most people using -Fall are keeping old timeout only by happen stance,
> and not because they desire that.
I had similar question on my mind when I came to PF for the first time.
my expectations about '-Fall' were the option removes all rules (and tables)
from kernel module. But it is not the case it acts on main ruleset only.
Given '-Fall' works like that for ages, I see changing '-Fall' to remove
all rules as disturbing (hence I'm in favor to introduce a new option). On
the other hand if there will be consensus to fix '-Fall' so it will remove
all rules (not just main ruleset), then we can forget about '-U'.
With '-Fall' changed, we can further fix pfctl. The proposed '-U', will
be achieved by combination of various '-F' modifiers:
pfctl -FA -FS -Fs -Freset
command above should revert PF driver state back to initial.
>
> In any case, if you're seeking input on the name, something like -Freset says
> to me that it resets pf back to its initial state.
I like the '-Fresst' to reset all PF settings (variables modified by 'set')
back to defaults.
So how people feel about changing '-Fa' to kill all rules and tables, not just
those, which are attached to main ruleset (root)?
thanks and
regards
sashan
Re: pfctl should allow administrator to flush _anchors
Alexandr Nedvedicky wrote:
> it is, however -Fall operates on main ruleset only. -Fall also does
> not reset limits and timeouts. Hence my first idea was to introduce
> '-FNuke', which kills all rulesets and tables.
>
> I don't want to change behaviour of existing option ('-Fall'), therefore
> I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> for me. I'm the most concerned about flushing all rulesets.
Is the existing behavior intentional or an oversight? I don't know when I
would want to use -Fall, but keep the old timeouts, and depend on that. I'd
guess most people using -Fall are keeping old timeout only by happen stance,
and not because they desire that.
In any case, if you're seeking input on the name, something like -Freset says
to me that it resets pf back to its initial state.
Re: pfctl should allow administrator to flush _anchors
Alexandr Nedvedicky wrote: > how about making the '-U' (or whatever name we agree) undocumented. We can > also make the option available if pfctl will get compiled with 'DEBUG' > option (assuming we are doing regress on debug bits anyway). no, it should be documented. but few people will use it, and that's fine
Re: pfctl should allow administrator to flush _anchors
Hello,
> > > > Isn't -U pretty close to -Fall ?
> > > >
> > >
> > > it is, however -Fall operates on main ruleset only. -Fall also does
> > > not reset limits and timeouts. Hence my first idea was to introduce
> > > '-FNuke', which kills all rulesets and tables.
> > >
> > > I don't want to change behaviour of existing option ('-Fall'),
> > > therefore
> > > I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > > for me. I'm the most concerned about flushing all rulesets.
> > >
> > > Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets
> > > works
> > > for me. Actually this is the most important thing I'd like to achieve.
> >
> > whatever gets done here, the initial-raw-state-forcing should be 1
> > operation.
> > not multiple operations acting on aspects of pf.
> >
> > I think if it is multiple operations, people won't ever get comfortable
> > using it.
>
> Not sure about that: I wont be comfortable anyway, as it can cause all sorts
> of problems on a running system.
>
> When i reset things to the boot state, i would expect thats not a simple
> thing and not without issues.
>
> I consider this as a cleanup op, most useful for regress tests, developers
> testing stuff etc. In normal sysadmin work i never needed it.
I think this is a good point. I don't expect experienced admins, who
maintain production systems to use an unconfigure operation. It's indeed
more useful for regression tests and people who are configuring their PF
in sandbox/testing environment.
how about making the '-U' (or whatever name we agree) undocumented. We can
also make the option available if pfctl will get compiled with 'DEBUG'
option (assuming we are doing regress on debug bits anyway).
sashan
Re: pfctl should allow administrator to flush _anchors
Theo de Raadt(dera...@openbsd.org) on 2019.03.24 10:22:25 -0600:
> Alexandr Nedvedicky wrote:
>
> > On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:
> > > On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > > > I think all the above calls for a new standalone option, which I named
> > > > as
> > > > 'Unconfigure'. Patch below suggest unconfigure behavior for PF.
> > > > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right
> > > > before
> > > > pf.conf got processed during the system boot). In case of PF the
> > > > proposed -U
> > > > will do following:
> > > > - remove all rulesets and tables
> > > > - remove all states and source nodes
> > > > - remove all OS fingerprints
> > > > - set all limits, timeouts and options to their defaults
> > > >
> > >
> > > Isn't -U pretty close to -Fall ?
> > >
> >
> > it is, however -Fall operates on main ruleset only. -Fall also does
> > not reset limits and timeouts. Hence my first idea was to introduce
> > '-FNuke', which kills all rulesets and tables.
> >
> > I don't want to change behaviour of existing option ('-Fall'), therefore
> > I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> > for me. I'm the most concerned about flushing all rulesets.
> >
> > Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
> > for me. Actually this is the most important thing I'd like to achieve.
>
> whatever gets done here, the initial-raw-state-forcing should be 1 operation.
> not multiple operations acting on aspects of pf.
>
> I think if it is multiple operations, people won't ever get comfortable
> using it.
Not sure about that: I wont be comfortable anyway, as it can cause all sorts
of problems on a running system.
When i reset things to the boot state, i would expect thats not a simple
thing and not without issues.
I consider this as a cleanup op, most useful for regress tests, developers
testing stuff etc. In normal sysadmin work i never needed it.
Re: pfctl should allow administrator to flush _anchors
Alexandr Nedvedicky wrote:
> On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:
> > On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > > I think all the above calls for a new standalone option, which I named as
> > > 'Unconfigure'. Patch below suggest unconfigure behavior for PF.
> > > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right
> > > before
> > > pf.conf got processed during the system boot). In case of PF the proposed
> > > -U
> > > will do following:
> > > - remove all rulesets and tables
> > > - remove all states and source nodes
> > > - remove all OS fingerprints
> > > - set all limits, timeouts and options to their defaults
> > >
> >
> > Isn't -U pretty close to -Fall ?
> >
>
> it is, however -Fall operates on main ruleset only. -Fall also does
> not reset limits and timeouts. Hence my first idea was to introduce
> '-FNuke', which kills all rulesets and tables.
>
> I don't want to change behaviour of existing option ('-Fall'), therefore
> I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
> for me. I'm the most concerned about flushing all rulesets.
>
> Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
> for me. Actually this is the most important thing I'd like to achieve.
whatever gets done here, the initial-raw-state-forcing should be 1 operation.
not multiple operations acting on aspects of pf.
I think if it is multiple operations, people won't ever get comfortable
using it.
Re: pfctl should allow administrator to flush _anchors
On Sun, Mar 24, 2019 at 09:51:13AM +0100, Denis Fondras wrote:
> On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote:
> > I think all the above calls for a new standalone option, which I named as
> > 'Unconfigure'. Patch below suggest unconfigure behavior for PF.
> > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
> > pf.conf got processed during the system boot). In case of PF the proposed -U
> > will do following:
> > - remove all rulesets and tables
> > - remove all states and source nodes
> > - remove all OS fingerprints
> > - set all limits, timeouts and options to their defaults
> >
>
> Isn't -U pretty close to -Fall ?
>
it is, however -Fall operates on main ruleset only. -Fall also does
not reset limits and timeouts. Hence my first idea was to introduce
'-FNuke', which kills all rulesets and tables.
I don't want to change behaviour of existing option ('-Fall'), therefore
I'm in favor to introduce a new option. Either '-FNuke' or '-U' works
for me. I'm the most concerned about flushing all rulesets.
Also making "pfctl -a '_1/_2' -Fr" to remove PF 'private' rulesets works
for me. Actually this is the most important thing I'd like to achieve.
sashan
Re: pfctl should allow administrator to flush _anchors
On Sun, Mar 24, 2019 at 09:24:34AM +0100, Alexandr Nedvedicky wrote: > I think all the above calls for a new standalone option, which I named as > 'Unconfigure'. Patch below suggest unconfigure behavior for PF. > Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before > pf.conf got processed during the system boot). In case of PF the proposed -U > will do following: > - remove all rulesets and tables > - remove all states and source nodes > - remove all OS fingerprints > - set all limits, timeouts and options to their defaults > Isn't -U pretty close to -Fall ?
Re: pfctl should allow administrator to flush _anchors
Hello,
I received a feedback suggesting to come up with better name than 'Nuke'
> not sure about the name.
>
> i've often want a similar operation for network interfaces, which
> returns them to 'raw' state.
>
> and there's also been people who want the same for the routing
> table
>
> if we ever do such a thing for all of these things, i think we should
> use the same word
>
> 'nuke' sounds like the wrong word, it is a 'destroy' operation rather
> than 'return to unitialized'.
I think all the above calls for a new standalone option, which I named as
'Unconfigure'. Patch below suggest unconfigure behavior for PF.
Doing 'pfctl -U' will bring PF back to its initial state (e.g. right before
pf.conf got processed during the system boot). In case of PF the proposed -U
will do following:
- remove all rulesets and tables
- remove all states and source nodes
- remove all OS fingerprints
- set all limits, timeouts and options to their defaults
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..562e03b3477 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -644,6 +644,9 @@ This flag is set when per-address counters are enabled on
the table.
.El
.It Fl t Ar table
Specify the name of the table.
+.It Fl U
+Unconfigure firewall. All rules, states and tables are removed. All limits
+and timeouts are reset to default values.
.It Fl V Ar rdomain
Select the routing domain to be used to kill states by host or by label.
The rdomain of a state is displayed in parentheses before the host by
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..19f63db56f2 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -54,6 +54,8 @@
#include
+#include
+
#include "pfctl_parser.h"
#include "pfctl.h"
@@ -63,7 +65,7 @@ intpfctl_disable(int, int);
voidpfctl_clear_queues(struct pf_qihead *);
voidpfctl_clear_stats(int, const char *, int);
voidpfctl_clear_interface_flags(int, int);
-voidpfctl_clear_rules(int, int, char *);
+int pfctl_clear_rules(int, int, char *);
voidpfctl_clear_src_nodes(int, int);
voidpfctl_clear_states(int, const char *, int);
struct addrinfo *
@@ -105,6 +107,14 @@ int pfctl_load_rule(struct pfctl *, char *, struct
pf_rule *, int);
const char *pfctl_lookup_option(char *, const char **);
void pfctl_state_store(int, const char *);
void pfctl_state_load(int, const char *);
+intpfctl_walk_show(int, struct pfioc_ruleset *, void *);
+intpfctl_walk_get(int, struct pfioc_ruleset *, void *);
+intpfctl_walk_anchors(int, int, char *, void *,
+int(*)(int, struct pfioc_ruleset *, void *));
+struct pfr_anchors *
+ pfctl_get_anchors(int, int);
+intpfctl_nuke_anchors(int, int);
+void pfctl_set_defaults(int, int);
const char *clearopt;
char *rulesopt;
@@ -124,6 +134,7 @@ char*state_kill[2];
int dev = -1;
int first_title = 1;
int labels = 0;
+int exit_val = 0;
#define INDENT(d, o) do {\
if (o) {\
@@ -232,7 +243,6 @@ static const char *optiopt_list[] = {
struct pf_qihead qspecs = TAILQ_HEAD_INITIALIZER(qspecs);
struct pf_qihead rootqs = TAILQ_HEAD_INITIALIZER(rootqs);
-
__dead void
usage(void)
{
@@ -249,6 +259,40 @@ usage(void)
exit(1);
}
+void
+pfctl_err(int opts, int eval, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ verr(1, fmt, ap);
+ else
+ vwarn(fmt, ap);
+
+ va_end(ap);
+
+ exit_val = eval;
+}
+
+void
+pfctl_errx(int opts, int eval, const char *fmt, ...)
+{
+ va_list ap;
+
+ va_start(ap, fmt);
+
+ if ((opts & PF_OPT_IGNFAIL) == 0)
+ verrx(1, fmt, ap);
+ else
+ vwarnx(fmt, ap);
+
+ va_end(ap);
+
+ exit_val = eval;
+}
+
int
pfctl_enable(int dev, int opts)
{
@@ -287,10 +331,10 @@ pfctl_clear_stats(int dev, const char *iface, int opts)
memset(, 0, sizeof(pi));
if (iface != NULL && strlcpy(pi.pfiio_name, iface,
sizeof(pi.pfiio_name)) >= sizeof(pi.pfiio_name))
- errx(1, "invalid interface: %s", iface);
+ pfctl_errx(opts, 1, "invalid interface: %s", iface);
if (ioctl(dev, DIOCCLRSTATUS, ))
- err(1, "DIOCCLRSTATUS");
+ pfctl_err(opts, 1, "DIOCCLRSTATUS");
if ((opts & PF_OPT_QUIET) == 0) {
fprintf(stderr, "pf: statistics cleared");
if (iface != NULL)
@@ -309,32 +353,36 @@ pfctl_clear_interface_flags(int dev, int opts)
pi.pfiio_flags = PFI_IFLAG_SKIP;
if (ioctl(dev, DIOCCLRIFFLAG, ))
-
Re: pfctl should allow administrator to flush _anchors
Hello,
I'm giving up on identifying unreferenced/orphaned rulesets. It seems to me
like too much work/complexity, which invites more troubles. I prefer to keep
things simple, hence I'm proposing new modifier for Flush option:
-F Nuke Flush all rulesets and tables.
The option is meant to be used when things will get too messy. It removes all
tables and rulesets from kernel allowing admin to quickly start from scratch
without doing reboot.
so here is my first question:
does proposal for the 'Nuke' flush modifier sound good?
or is it completely off?
Proposed change below splits current pfctl_show_anchors() to two functions:
pfctl_walk_anchors(), which implements ruleset/anchor traversal
pfctl_walk_show(), which prints full path to every anchor, while
anchors are traversed. the pfctl_walk_show() is passed
as argument to pfctl_walk_anchors()
The implementation of Nuke modifier comes with its own callback
pfctl_walk_get(), which collects anchors to SLIST during anchor tree
traversal. As soon as all anchors/rulesets are collected
we effectively apply:
pfctl -a ... -FT
pfctl -a ... -Fr
to every anchor/ruleset found in the list.
If you think the change is too big to be done in one step, then
I can split the diff to two steps:
split pfctl_show_anchors() in the first step
then send an updated diff, which just adds 'Nuke' modifier to F
option.
thanks and
regards
sashan
8<---8<---8<--8<
diff --git a/sbin/pfctl/pfctl.8 b/sbin/pfctl/pfctl.8
index 48b2893cfcd..06e76e35b68 100644
--- a/sbin/pfctl/pfctl.8
+++ b/sbin/pfctl/pfctl.8
@@ -199,6 +199,8 @@ Flush the tables.
Flush the passive operating system fingerprints.
.It Fl F Cm all
Flush all of the above.
+.It Fl F Cm Nuke
+Flush all rulesets and tables.
.El
.It Fl f Ar file
Replace the current ruleset with
diff --git a/sbin/pfctl/pfctl.c b/sbin/pfctl/pfctl.c
index 493ff47af2f..fc909ade507 100644
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -63,7 +63,7 @@ intpfctl_disable(int, int);
voidpfctl_clear_queues(struct pf_qihead *);
voidpfctl_clear_stats(int, const char *, int);
voidpfctl_clear_interface_flags(int, int);
-voidpfctl_clear_rules(int, int, char *);
+int pfctl_clear_rules(int, int, char *);
voidpfctl_clear_src_nodes(int, int);
voidpfctl_clear_states(int, const char *, int);
struct addrinfo *
@@ -105,6 +105,13 @@ int pfctl_load_rule(struct pfctl *, char *, struct
pf_rule *, int);
const char *pfctl_lookup_option(char *, const char **);
void pfctl_state_store(int, const char *);
void pfctl_state_load(int, const char *);
+intpfctl_walk_show(int, struct pfioc_ruleset *, void *);
+intpfctl_walk_get(int, struct pfioc_ruleset *, void *);
+intpfctl_walk_anchors(int, int, char *, void *,
+int(*)(int, struct pfioc_ruleset *, void *));
+struct pfr_anchors *
+ pfctl_get_anchors(int, int);
+intpfctl_nuke_anchors(int, int);
const char *clearopt;
char *rulesopt;
@@ -205,7 +212,8 @@ static const struct {
};
static const char *clearopt_list[] = {
- "rules", "Sources", "states", "info", "Tables", "osfp", "all", NULL
+ "rules", "Sources", "states", "info", "Tables", "osfp", "all",
+ "Nuke", NULL
};
static const char *showopt_list[] = {
@@ -232,7 +240,6 @@ static const char *optiopt_list[] = {
struct pf_qihead qspecs = TAILQ_HEAD_INITIALIZER(qspecs);
struct pf_qihead rootqs = TAILQ_HEAD_INITIALIZER(rootqs);
-
__dead void
usage(void)
{
@@ -315,19 +322,29 @@ pfctl_clear_interface_flags(int dev, int opts)
}
}
-void
+int
pfctl_clear_rules(int dev, int opts, char *anchorname)
{
- struct pfr_buffer t;
+ struct pfr_buffer t;
+ int rv = 0;
memset(, 0, sizeof(t));
t.pfrb_type = PFRB_TRANS;
if (pfctl_add_trans(, PF_TRANS_RULESET, anchorname) ||
pfctl_trans(dev, , DIOCXBEGIN, 0) ||
- pfctl_trans(dev, , DIOCXCOMMIT, 0))
- err(1, "pfctl_clear_rules");
- if ((opts & PF_OPT_QUIET) == 0)
+ pfctl_trans(dev, , DIOCXCOMMIT, 0)) {
+ if (opts & PF_OPT_IGNFAIL) {
+ if ((opts & PF_OPT_QUIET) == 0) {
+ warn("%s", __func__);
+ }
+ rv = 1;
+ } else {
+ err(1, "%s", __func__);
+ }
+ } else if ((opts & PF_OPT_QUIET) == 0)
fprintf(stderr, "rules cleared\n");
+
+ return (rv);
}
void
@@ -2107,13 +2124,59 @@ pfctl_debug(int dev, u_int32_t level, int opts)
}
int
-pfctl_show_anchors(int dev, int opts, char *anchorname)
+pfctl_walk_show(int opts, struct pfioc_ruleset *pr, void *warg)
+{
+ if (pr->path[0]) {
+ if (pr->path[0] != '_' || (opts & PF_OPT_VERBOSE))
+ printf(" %s/%s\n",
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 03:02:07PM +0100, Alexandr Nedvedicky wrote: > so the option '-F Anchors' will also perform a '-Fr' on main ruleset, is > that correct? No, my `-f /etc/pf.conf' is the equivalent to your `-F rules' here. > And also one more thing, which comes to my mind. How '-F Anchors' should > treat tables attached to anchors? If an unreferenced anchor contains a table, either destroy that table as well since it cannot be referenced or abort garbage collecting anchor completely, possibly prompting the user to clean tables in besaid anchors as well. I tend towards the first option since anchors may contain tables that were automatically created due to ruleset optimization. That is to say, while enforcing manual removal of tables as well, this may become annoying for big optimized rulesets where administrators would have to wield `-F Tables' and or `-a aname -T kill -t tname' before having `-F Anchors' run cleanly. Does that make sense? > the firewall service (which is a kind of rc-script in fact) on Solaris, > kills (removes) all tables first, then it removes anchors. > > What shall we do in case of '-F Anchors'? do we want '-F Anchors' to > kill attached tables too? Or should it just report "anchor can't be > removed, because table is still attached?" See above. What this "service" roughly seems like what I have in mind. > It looks like '-F Anchors' shifts pfctl(8) from simple tool, which does > exactly what it's told to do, to advanced tool, which does more things at > one step. But isn't that a perfect job for pfctl? It manipulates everything, it's the single interface for firewall related and that's a good. Cleaning things up after work is done is a necessity; this is about adding the missing peaces, imho. > The simple tools just seem to be more friendly for scripts, while advanced > tool is easier to use by human. pfctl is well suited for both, and it always should be.
Re: pfctl should allow administrator to flush _anchors
Hello Klemens, I just need to clarify some details. > > the 'unreferenced' means the anchor is not reachable by any packet. > > like there is no path for packet between main ruleset and that > > particular > > anchor (and all its descendants). > Yes. With the regress suite for example, the following should leave no > trace of regress anchors or rules: > > make > pfctl -f /etc/pf.conf > pfctl -F Anchors so the option '-F Anchors' will also perform a '-Fr' on main ruleset, is that correct? And also one more thing, which comes to my mind. How '-F Anchors' should treat tables attached to anchors? the firewall service (which is a kind of rc-script in fact) on Solaris, kills (removes) all tables first, then it removes anchors. What shall we do in case of '-F Anchors'? do we want '-F Anchors' to kill attached tables too? Or should it just report "anchor can't be removed, because table is still attached?" It looks like '-F Anchors' shifts pfctl(8) from simple tool, which does exactly what it's told to do, to advanced tool, which does more things at one step. The simple tools just seem to be more friendly for scripts, while advanced tool is easier to use by human. thanks and regards sasha
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 12:42:02PM +0100, Alexandr Nedvedicky wrote: > yes, that's what I thought. We have a kind 'service' on Solaris, which > wraps pfctl to manage firewall. If firewall is being enabled, the service > cleans up all rules (anchors). We basically dump the rulesets (pfctl -vsA) > and then traverse from leaves to root to clean it up. That's probaly how I'd approach `-F Anchors'. > so if I understand you right, your scenario for ruleset from my > first email works as follows: > pfctl -Fr # makes the anchors _1 and _1/_2 unreferenced > # (they are not reachable from root any more) > > pfctl -FAnchors # purge all unreferenced anchors. > > the 'unreferenced' means the anchor is not reachable by any packet. > like there is no path for packet between main ruleset and that particular > anchor (and all its descendants). Yes. With the regress suite for example, the following should leave no trace of regress anchors or rules: make pfctl -f /etc/pf.conf pfctl -F Anchors > sure, I agree, adding -FAnchors options i more systemic approach, though > such change is more complex. I think I can give it a try to prototype it. Cool! I'm happy to help here.
Re: pfctl should allow administrator to flush _anchors
Hello, On Fri, Feb 22, 2019 at 10:55:17AM +0100, Klemens Nanni wrote: > On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote: > > > so far so good. Now let's flush the rules from kernel: > > > > lumpy# ./pfctl -Fr > > rules cleared > > lumpy# ./pfctl -sr > > lumpy# > > > > However the underscore anchors are still there: > Any unreferenced anchor will remain, not just internal ones. We have no > code/logic that clears them automatically. yes, that's what I thought. We have a kind 'service' on Solaris, which wraps pfctl to manage firewall. If firewall is being enabled, the service cleans up all rules (anchors). We basically dump the rulesets (pfctl -vsA) and then traverse from leaves to root to clean it up. > > > lumpy# ./pfctl -vsA > > _1 > > _1/_2 > > lumpy# > That's a known issue, easily visible if you run the pfctl regress tests. > I've been pondering this with different attempts but haven't come up > with a good/safe one yet. unlike named anchors, the Solaris service can't remove the 'underscore' anchors. my patch solves that glitch for Solaris. I was looking for the smallest change possible, which will fix Solaris without introducing too much risk/changes in upstream. > > Perhaps we can teach pfctl something like `-F Anchors`, such that it > looks at the ruleset as well as all anchors and removes those which are > not referenced. so if I understand you right, your scenario for ruleset from my first email works as follows: pfctl -Fr # makes the anchors _1 and _1/_2 unreferenced # (they are not reachable from root any more) pfctl -FAnchors # purge all unreferenced anchors. the 'unreferenced' means the anchor is not reachable by any packet. like there is no path for packet between main ruleset and that particular anchor (and all its descendants). > > The benefits are that we do not require any further > starts-with-underscore quirks and it remains a wilful action done by > root. sure, I agree, adding -FAnchors options i more systemic approach, though such change is more complex. I think I can give it a try to prototype it. > > > I could not figure out any existing way to remove them, hence I'm proposing > > small patch, which allows me to remove those 'underscore' anchors by doing > > this: > > > > lumpy# ./pfctl -a _1/_2 -Fr > > rules cleared > > lumpy# ./pfctl -a _1 -Fr > > rules cleared > Did you look at happens with _1/_2 when you flush _1 directly? yes, the ruleset becomes empty, but it still hanging around, because its descendant (_1/_2) contains the block rule. the terminal looks as follows: lumpy# pfctl -f /tmp/pf.conf lumpy# ./pfctl -Fr rules cleared lumpy# ./pfctl -a _1 -Fr rules cleared lumpy# ./pfctl -vsA _1 _1/_2 lumpy# ./pfctl -a _1 -sr lumpy# ./pfctl -a _1/_2 -sr block drop all lumpy# > > > Does patch below make sense? Or are there some pitfalls I'm not aware of? > Given your example, using named anchors seems like the right approach. > > > + /* > > +* we want enable administrator to flush anonymous anchors, > > +* thus '_' should be allowed for '-Fr' only. Also make sure > > +* we fail in case of option combination as follows: > > +* pfctl -a _1 -Fr -f /some/rules.conf > > +*/ > > if (mode == O_RDWR && tblcmdopt == NULL && > > + (clearopt == NULL || *clearopt != 'r' || > > + rulesopt != NULL) && > I'm not fond of adding an exception to what already is an exception. > The anchor handling code is tricky already and has big potential for > pitfalls and subtle bugs (as seen in the past). I completely agree here. > > > (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL)) > > errx(1, "anchor names beginning with '_' cannot " > > "be modified from the command line"); > > That said, I think the internal semantics should stay purely internal to > avoid another can of worms. I partially agree here. As I don't think the current semantics is purely internal. IMO the underscore just denotes a kind of namespace, which requires special handling. I read it as 'name created by PF'. And as such is too weak to constitute a 'purely internal', because the rules contained in _1 anchor are coming from administrator not from PF itself. I agree with 'can of worms'. So let's see what else I'm going to find when I'll try to prototype '-FAnchors'. thanks and regards sashan
Re: pfctl should allow administrator to flush _anchors
On Fri, Feb 22, 2019 at 01:52:24AM +0100, Alexandr Nedvedicky wrote: > so far so good. Now let's flush the rules from kernel: > > lumpy# ./pfctl -Fr > rules cleared > lumpy# ./pfctl -sr > lumpy# > > However the underscore anchors are still there: Any unreferenced anchor will remain, not just internal ones. We have no code/logic that clears them automatically. > lumpy# ./pfctl -vsA > _1 > _1/_2 > lumpy# That's a known issue, easily visible if you run the pfctl regress tests. I've been pondering this with different attempts but haven't come up with a good/safe one yet. Perhaps we can teach pfctl something like `-F Anchors`, such that it looks at the ruleset as well as all anchors and removes those which are not referenced. The benefits are that we do not require any further starts-with-underscore quirks and it remains a wilful action done by root. > I could not figure out any existing way to remove them, hence I'm proposing > small patch, which allows me to remove those 'underscore' anchors by doing > this: > > lumpy# ./pfctl -a _1/_2 -Fr > rules cleared > lumpy# ./pfctl -a _1 -Fr > rules cleared Did you look at happens with _1/_2 when you flush _1 directly? > Does patch below make sense? Or are there some pitfalls I'm not aware of? Given your example, using named anchors seems like the right approach. > + /* > + * we want enable administrator to flush anonymous anchors, > + * thus '_' should be allowed for '-Fr' only. Also make sure > + * we fail in case of option combination as follows: > + * pfctl -a _1 -Fr -f /some/rules.conf > + */ > if (mode == O_RDWR && tblcmdopt == NULL && > + (clearopt == NULL || *clearopt != 'r' || > + rulesopt != NULL) && I'm not fond of adding an exception to what already is an exception. The anchor handling code is tricky already and has big potential for pitfalls and subtle bugs (as seen in the past). > (anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL)) > errx(1, "anchor names beginning with '_' cannot " > "be modified from the command line"); That said, I think the internal semantics should stay purely internal to avoid another can of worms.
pfctl should allow administrator to flush _anchors
Hello,
This issue has been reported by one of our customers.
consider pf.conf comes with rules as follows:
anchor {
pass all
anchor {
block all
}
}
We load pf.conf to kernel and (pfctl -f pf.conf) and display what got loaded:
lumpy# ./pfctl -f /tmp/pf.conf
lumpy# ./pfctl -sr
anchor all {
pass all flags S/SA
anchor all {
block drop all
}
}
so far so good. Now let's flush the rules from kernel:
lumpy# ./pfctl -Fr
rules cleared
lumpy# ./pfctl -sr
lumpy#
However the underscore anchors are still there:
lumpy# ./pfctl -vsA
_1
_1/_2
lumpy#
I could not figure out any existing way to remove them, hence I'm proposing
small patch, which allows me to remove those 'underscore' anchors by doing
this:
lumpy# ./pfctl -a _1/_2 -Fr
rules cleared
lumpy# ./pfctl -a _1 -Fr
rules cleared
lumpy# ./pfctl -vsA
lumpy#
Does patch below make sense? Or are there some pitfalls I'm not aware of?
thanks and
regards
sashan
8<---8<---8<--8<
--- a/sbin/pfctl/pfctl.c
+++ b/sbin/pfctl/pfctl.c
@@ -2445,7 +2445,16 @@ main(int argc, char *argv[])
warnx("anchors apply to -f, -F, -s, and -T only");
usage();
}
+
+ /*
+* we want enable administrator to flush anonymous anchors,
+* thus '_' should be allowed for '-Fr' only. Also make sure
+* we fail in case of option combination as follows:
+* pfctl -a _1 -Fr -f /some/rules.conf
+*/
if (mode == O_RDWR && tblcmdopt == NULL &&
+ (clearopt == NULL || *clearopt != 'r' ||
+ rulesopt != NULL) &&
(anchoropt[0] == '_' || strstr(anchoropt, "/_") != NULL))
errx(1, "anchor names beginning with '_' cannot "
"be modified from the command line");
