pfctl: tables: improve namespace collision warnings

classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

pfctl: tables: improve namespace collision warnings

Klemens Nanni-2
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 <t1> 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 <t1> 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 <t3> already defined in anchor "/"
        1 table created.
        no changes.
        # ./obj/pfctl -T replace -t t3 -a a2 -n
        pfctl: warning: table <t3> already defined in anchor "/"
        pfctl: warning: table <t3> 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 -0000 1.57
+++ pfctl.h 29 Dec 2018 15:59:11 -0000
@@ -79,7 +79,7 @@ void pfctl_clear_tables(const char *, i
 void pfctl_show_tables(const char *, int);
 int pfctl_command_tables(int, char *[], char *, const char *, char *,
     const char *, int);
-void warn_namespace_collision(const char *);
+void warn_duplicate_tables(const char *, const char *);
 void pfctl_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 -0000 1.361
+++ pfctl.c 29 Dec 2018 15:59:11 -0000
@@ -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 -0000 1.688
+++ parse.y 29 Dec 2018 15:59:28 -0000
@@ -4075,6 +4075,7 @@ process_tabledef(char *name, struct tabl
  if (pf->opts & PF_OPT_VERBOSE)
  print_tabledef(name, opts->flags, opts->init_addr,
     &opts->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, &ab, 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 -0000 1.78
+++ pfctl_table.c 29 Dec 2018 15:59:21 -0000
@@ -94,7 +94,8 @@ static const char *istats_text[2][2][2]
  goto _error; \
  } \
  if (nadd) { \
- warn_namespace_collision(table.pfrt_name); \
+ warn_duplicate_tables(table.pfrt_name, \
+    table.pfrt_anchor); \
  xprintf(opts, "%d table created", nadd); \
  if (opts & PF_OPT_NOACTION) \
  return (0); \
@@ -537,12 +538,10 @@ pfctl_define_table(char *name, int flags
 }
 
 void
-warn_namespace_collision(const char *filter)
+warn_duplicate_tables(const char *tablename, const char *anchorname)
 {
  struct pfr_buffer b;
  struct pfr_table *t;
- const char *name = NULL, *lastcoll;
- int coll = 0;
 
  bzero(&b, sizeof(b));
  b.pfrb_type = PFRB_TABLES;
@@ -558,22 +557,13 @@ warn_namespace_collision(const char *fil
  PFRB_FOREACH(t, &b) {
  if (!(t->pfrt_flags & PFR_TFLAG_ACTIVE))
  continue;
- if (filter != NULL && strcmp(filter, t->pfrt_name))
+ if (!strcmp(anchorname, t->pfrt_anchor))
  continue;
- if (!t->pfrt_anchor[0])
- name = t->pfrt_name;
- else if (name != NULL && !strcmp(name, t->pfrt_name)) {
- coll++;
- lastcoll = name;
- name = NULL;
- }
+ if (!strcmp(tablename, t->pfrt_name))
+ warnx("warning: table <%s> already defined"
+    " in anchor \"%s\"", tablename,
+    t->pfrt_anchor[0] ? t->pfrt_anchor : "/");
  }
- if (coll == 1)
- warnx("warning: namespace collision with <%s> global table.",
-    lastcoll);
- else if (coll > 1)
- warnx("warning: namespace collisions with %d global tables.",
-    coll);
  pfr_buf_clear(&b);
 }
 
===================================================================
Stats: --- 21 lines 671 chars
Stats: +++ 10 lines 470 chars
Stats: -11 lines
Stats: -201 chars

Reply | Threaded
Open this post in threaded view
|

Re: pfctl: tables: improve namespace collision warnings

Alexandr Nedvedicky
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 <t1> ....

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 </t1> ....

I agree going that way just puts more pain to users for kind of
little gain.

</snip>
> # ./obj/pfctl -T replace -t t3 -a a2 -n
> pfctl: warning: table <t3> already defined in anchor "/"
> pfctl: warning: table <t3> 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

Reply | Threaded
Open this post in threaded view
|

Re: pfctl: tables: improve namespace collision warnings

Klemens Nanni-2
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 <t1> ....
>
> 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 </t1> ....
>
> 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 <t3> already defined in anchor "/"
> > pfctl: warning: table <t3> 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.

Reply | Threaded
Open this post in threaded view
|

Re: pfctl: tables: improve namespace collision warnings

Alexandr Nedvedicky
Hello,

</snip>

>
> >     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