Re: pfctl: tables: improve namespace collision warnings
Hello, > > > As I've said I don't object your change. I agree it does, > > what you intend, however I'm not sure how much it buys. > My intention is to make warnings clear and unambiguous, such that > referred table and anchor names can be copied and pasted into successive > pfctl invocations to fix things right away. I agree the better warnings don't hurt. I'm OK with your changes. sashan
Re: pfctl: tables: improve namespace collision warnings
On Wed, Jan 02, 2019 at 11:27:18PM +0100, Alexandr Nedvedicky wrote: > I don't object your change. However I hesitate to give OK too. I hope PF > users, who have non-trivial rulesets will speak up here. Feedback is welcome. > IMO opinion we are hitting limitations of pfctl(8) here. Making warnings > more useful requires to introduce some additional hints to pfctl, to > better express, which table should be bound to rule. This does not seem like a limitation to me. > Currently pfctl(8) tries to use table, which is attached to anchor. > If there is no table found, it implicitly fall backs to main anchor > and uses table found in main anchor (ruleset). This implicit fallback > is source of our doubts: > is it intentional the table at anchor is not defined? Pretty sure that's by design. All tables used to be global, until cedric reworked lookups at 30.04.2003 to be hierarchically per anchor. > I would prefer pfctl(8) to always complain if particular table is not defined > in anchor. e.g. if rule refers particular table as: > > pass in from > > then parser should always expect `t1` to be defined in the same anchor > as the rule itself. If no table is found anchor, then parser should > exit with error. Yes, but referring/accepting non-existing tables is a different issue. Now I want to (naively) take care of better collision warnings only. > If user wants rule above to use `t1` from main > anchor then the rule should look like: > > pass in from > > I agree going that way just puts more pain to users for kind of > little gain. What *is* the gain? I don't see it. > > # ./obj/pfctl -T replace -t t3 -a a2 -n > > pfctl: warning: table already defined in anchor "/" > > pfctl: warning: table already defined in anchor "a1" > > 1 table created (dummy). > > > > I just see an use case above from different perspective: > > it's not a problem where particular table is defined, > the tricky question is how do we refer them in rules. Just by name as it's done now, leaving scope to anchors alone. How would the user interface look like? pfctl -a foo -t /t pfctl -a / -t /t pfctl -t t pfctl a foo -t ../bar/t pfctl -t bar/t How should these behave? Thinking about hierarchies in two places makes for a big mess. > As I've said I don't object your change. I agree it does, > what you intend, however I'm not sure how much it buys. My intention is to make warnings clear and unambiguous, such that referred table and anchor names can be copied and pasted into successive pfctl invocations to fix things right away.
Re: pfctl: tables: improve namespace collision warnings
Hello, I don't object your change. However I hesitate to give OK too. I hope PF users, who have non-trivial rulesets will speak up here. IMO opinion we are hitting limitations of pfctl(8) here. Making warnings more useful requires to introduce some additional hints to pfctl, to better express, which table should be bound to rule. Currently pfctl(8) tries to use table, which is attached to anchor. If there is no table found, it implicitly fall backs to main anchor and uses table found in main anchor (ruleset). This implicit fallback is source of our doubts: is it intentional the table at anchor is not defined? I would prefer pfctl(8) to always complain if particular table is not defined in anchor. e.g. if rule refers particular table as: pass in from then parser should always expect `t1` to be defined in the same anchor as the rule itself. If no table is found anchor, then parser should exit with error. If user wants rule above to use `t1` from main anchor then the rule should look like: pass in from I agree going that way just puts more pain to users for kind of little gain. > # ./obj/pfctl -T replace -t t3 -a a2 -n > pfctl: warning: table already defined in anchor "/" > pfctl: warning: table already defined in anchor "a1" > 1 table created (dummy). > I just see an use case above from different perspective: it's not a problem where particular table is defined, the tricky question is how do we refer them in rules. As I've said I don't object your change. I agree it does, what you intend, however I'm not sure how much it buys. thanks and regards sashan
pfctl: tables: improve namespace collision warnings
Tables under different anchors may have the same name, but pfctl warns
about such scenarios upon table creation to avoid mixups. Unique and
descriptive names are highly recommended (for sanity).
# pfctl -T replace -t t1
1 table created.
no changes.
# pfctl -T replace -t t1 -a a1
pfctl: warning: namespace collision with global table.
1 table created.
no changes.
Adding a table to yet another anchor will only ever warn about the one
in the main anchor:
# pfctl -T replace -t t1 -a a2
pfctl: warning: namespace collision with global table.
1 table created.
no changes.
Adding equally named tables in non-main anchors only will never produce
warnings:
# pfctl -T add -t t2 -a a1
1 table created.
0/0 addresses added.
# pfctl -T add -t t2 -a a2
1 table created.
0/0 addresses added.
Things to improve:
- spot all collisions
- warn on dry runs (`-n') as well
- name offending anchors
For this the anchor name must be passed in, but this is impossible to
do from pfctl's main() upon processing `-f file', where it's done now
since the file may contain table definitions.
This diff moves the call to process_tabledefs() inside the parser where
it's only called when tables actually occur. This way both table and
anchor name is known and warn_namespace_collision()'s `filter == NULL'
semantic can be dropped to further simplify the function.
Besides new warnings on standard error, there's no functional change as
ruleset production/manipulation is not effected; regress passes.
# ./obj/pfctl -T replace -t t3
1 table created.
no changes.
# ./obj/pfctl -T replace -t t3 -a a1
pfctl: warning: table already defined in anchor "/"
1 table created.
no changes.
# ./obj/pfctl -T replace -t t3 -a a2 -n
pfctl: warning: table already defined in anchor "/"
pfctl: warning: table already defined in anchor "a1"
1 table created (dummy).
The main anchor is denoted as "/" as it keeps the logic around warnx(3)
simple while still allowing to copy/paste it as is.
Since it's not just about collisions with the "global" tables (those in
the main anchor), I renamed the function to warn_duplicate_tables() at
no cost/blame churn since signature and callers are touched anyway.
Feedback? Objections? OK?
Index: pfctl.h
===
RCS file: /cvs/src/sbin/pfctl/pfctl.h,v
retrieving revision 1.57
diff -u -p -r1.57 pfctl.h
--- pfctl.h 6 Sep 2018 15:07:33 - 1.57
+++ pfctl.h 29 Dec 2018 15:59:11 -
@@ -79,7 +79,7 @@ void pfctl_clear_tables(const char *, i
voidpfctl_show_tables(const char *, int);
int pfctl_command_tables(int, char *[], char *, const char *, char *,
const char *, int);
-voidwarn_namespace_collision(const char *);
+voidwarn_duplicate_tables(const char *, const char *);
voidpfctl_show_ifaces(const char *, int);
FILE *pfctl_fopen(const char *, const char *);
Index: pfctl.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl.c,v
retrieving revision 1.361
diff -u -p -r1.361 pfctl.c
--- pfctl.c 27 Dec 2018 16:33:44 - 1.361
+++ pfctl.c 29 Dec 2018 15:59:11 -
@@ -2690,8 +2690,6 @@ main(int argc, char *argv[])
if (pfctl_rules(dev, rulesopt, opts, optimize,
anchorname, NULL))
error = 1;
- else if (!(opts & PF_OPT_NOACTION))
- warn_namespace_collision(NULL);
}
if (opts & PF_OPT_ENABLE)
Index: parse.y
===
RCS file: /cvs/src/sbin/pfctl/parse.y,v
retrieving revision 1.688
diff -u -p -r1.688 parse.y
--- parse.y 15 Nov 2018 03:22:01 - 1.688
+++ parse.y 29 Dec 2018 15:59:28 -
@@ -4075,6 +4075,7 @@ process_tabledef(char *name, struct tabl
if (pf->opts & PF_OPT_VERBOSE)
print_tabledef(name, opts->flags, opts->init_addr,
>init_nodes);
+ warn_duplicate_tables(name, pf->anchor->path);
if (!(pf->opts & PF_OPT_NOACTION) &&
pfctl_define_table(name, opts->flags, opts->init_addr,
pf->anchor->path, , pf->anchor->ruleset.tticket)) {
Index: pfctl_table.c
===
RCS file: /cvs/src/sbin/pfctl/pfctl_table.c,v
retrieving revision 1.78
diff -u -p -r1.78 pfctl_table.c
--- pfctl_table.c 15 Oct 2018 21:15:35 - 1.78
+++ pfctl_table.c 29 Dec 2018 15:59:21 -
@@ -94,7 +94,8 @@ static const char *istats_text[2][2][2]
goto _error;\
} \
if (nadd) {
