Enable local-to-anchors tables in PF rules

In a recent post to tech@ titled let's make pf(4) anchors and tables better friends (possibly originating at the ongoing hackathon) Alexandr Nedvedicky (sashan@) introduced code to enable creating local tables inside anchors in pf(4) rulesets:

Date: Sat, 13 Jul 2024 14:32:21 +0200
From: Alexandr Nedvedicky <sashan () fastmail ! net>
To: tech@openbsd.org
Subject: let's make pf(4) anchors and tables better friends

Hello,

the change presented in diff below allows user to define table
inside the anchor. Consider rules here:

--------8<---------------8<---------------8<------------------8<--------
match in all scrub (no-df random-id)
pass out log proto tcp from self to any port 12345
anchor "relayd/*"
anchor "test" {
    pass out log proto tcp from self to any port 12346
    anchor "foo" {
        table <allow> persist { 192.168.2.10 }
        pass out log proto tcp from <allow> to <foo> port 12348
    }
    pass out log proto tcp from self to any port 12349
}
pass out log proto tcp from self to any port 12347
--------8<---------------8<---------------8<------------------8<--------

using pfctl(8) in current to load rules above I get errors as
follows:

    lifty# pfctl -f /tmp/pf-anchors.conf
    /tmp/pf-anchors.conf:7: syntax error
    /tmp/pf-anchors.conf:9: syntax error
    /tmp/pf-anchors.conf:11: syntax error
    pfctl: Syntax error in config file: pf rules not loaded

This is pfctl's parser limitation which I believe most people
have never ever noticed. One can workaround limitation of
the parser by creating table allow@test/foo using a
pfctl(8).
The idea is as follows: remove definition of <allow> table
from config file so it loads. Load the file and then
run command as follows:
    pfctl -a test/foo -t allow -T add 192.168.2.10
Using 'pfctl -a "*" -g -v -sT' we can see the result in pf(4):

--------8<---------------8<---------------8<------------------8<--------
lifty# pfctl -f /tmp/pf-anchors-workaround.conf
lifty# pfctl -a test/foo -t allow -T add 192.168.2.10
1/1 addresses added.
lifty# pfctl -a "*" -g -v -sT
-pa---- allow@test/foo
----r-- foo@test/foo
-----h- allow
-----h- foo
--------8<---------------8<---------------8<------------------8<--------

Trying to load the rules where anchor <allow> is defined using
fixed pfctl(8) we see table allow@test/foo differs lacks flag p (persistent)
and flag a (active). On the other hand table got flag 'r' to
indicate it is referenced by rule.

--------8<---------------8<---------------8<------------------8<--------
lifty# ./pfctl -f /tmp/pf-anchors.conf
lifty# pfctl -a "*" -g -v -sT
----r-- allow@test/foo
----r-- foo@test/foo
-----h- allow
-----h- foo
--------8<---------------8<---------------8<------------------8<--------

Stop reading here if you are not interested in details around
tables and anchors.

What's also worth to note is the sample ruleset here creates  two
pairs of tables:
    allow and allow@test/foo
    foo and foo@test/foo
the pair of tables linked together via member ->pfrkt_root
in pf(4) code. Tables foo and allow which belong to main anchor
(root) have ->pfrkt_root set to NULL. In tables allow@test/foo
and foo@test/foo member ->pfrkt_root refers to tables allow and
foo found in main anchor. If there will be table foo@test
then ->pfrkt_root in such table will refer to table foo in
main anchor too.

So how tables are attached to tables? Well tables are not
attached to anchors at all. Both those objects (anchors and
tables) are kept in their own respective trees. This what happens
once table gets `attached` to non-root anchor. Consider we use
command:
    pfctl -a test -t allow -T add 172.16.1.10

In this case pfctl(8) tries to create two instances of allow table:
    allow `attached` to root anchor, this time the allow table is
    already there, so function spits a warning about conflict,
    the allow table found in tree already is left there.

    and table allow@test `attached` to test anchor

RB tree which keeps tables in pf(4) uses compare function as
follows:

int
pfr_ktable_compare(struct pfr_ktable *p, struct pfr_ktable *q)
{
        int d;

        if ((d = strncmp(p->pfrkt_name, q->pfrkt_name, PF_TABLE_NAME_SIZE)))
                return (d);
        return (strcmp(p->pfrkt_anchor, q->pfrkt_anchor));
}

If table names do match function compares the anchor. The
pfrkt_anchor holds the path from root to anchor. So in the case
of allow table those might be allow@test vs. allow@test/foo

Now you might be asking: so shall we just stop using tables
in non-root anchors and remove that semi-working code completely?
The answers is: that semi-working code is actually being used
by relayd(8), spamd(8) and perhaps some other daemons which manage
pf(4) at runtime. Those daemons keep their tables attached to non-root
anchors.

In my opinion code around tables deserves some love. The fix to
parser is the first step. The next step I'd like to do is
to get rid off global pfr_ktables tree. Instead of global
pfr_ktables tree each pf_anchor will get its own. This is far
more complex change, but I think it's worth the effort. Because
it makes reasoning about code and rules bit easier (I think, but
I might be biased). Another benefit of such change is that it
will allow us to let anchors to define a scope for each table in
the same fashion as we use scope for local variables in C.
The diff which moves pfr_ktables into pf_anchor is still work in
progress. Today I'm just sharing the fix to parser.

sorry for long email.

OK to commit?

thanks and
regards
sashan

--------8<---------------8<---------------8<------------------8<--------

The rest of the message contains the diff against -current, and the thread goes on with submissions from Alexander Bluhm (bluhm@) seeking to weed out some regressions, and sashan@ submitted a revised diff that hopefully addresses those bugs.

The next question is, will this code make it into the upcoming OpenBSD 7.6 release?

This is only one of the several interesting developments going on in the OpenBSD network stack at the moment, stay tuned for more!

If you feel up to it, you can help out by testing the code.

UPDATE: The code has now been committed, further work will be in-tree.