pfctl: Split pool parsing into separate functions

[vegeta_tuxpowered.net]

Summary

The pf pools are used in NAT, route-to and af-to rules. Some parts of
code are duplicated between them. Create functions apply_redirspec(),
apply_nat_ports() and apply_rdr_ports() to handle the common tasks.

Simplify data structures used for pool parsing. Move the contents of
struct redirection to struct redirspec. Map all ways of parsing pools
directly onto struct redirspec. Name various forms of struct redirspect
to hint where they are used.

Remove struct redirspec *rroute from struct filter_opts, because
filter_opts is bzero()'ed after the route part of rule is parsed, and
thus can't be used.

Add tests to ensure that parsing and error messages behave as expected.
The tests have been written and tested with pfctl from before this
patch.

This is prerequisite for adding support for OpenBSD NAT syntax.

[kp]

This fails one of the new tests on my machine:

(kp@geb)  /usr/tests/sys/netpfil % sudo kyua debug pf/redirection_pool_syntax:af_to                                                                                                [11:14]
Testing rule:
pass in on vlan0 inet6 from any to 64:ff9b::/96 af-to inet from 192.0.2.102 to 198.51.100.102
Parsed ruleset difference:
--- /tmp/kyua.JWtsI2/2/work/tmp.aCH51evNTr      2025-03-04 10:14:41.041368000 +0000
+++ /tmp/kyua.JWtsI2/2/work/tmp.HYltlMftHQ      2025-03-04 10:14:41.060891000 +0000
@@ -1 +1 @@
-@0 pass in on vlan0 inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from 192.0.2.102 to 198.51.100.102
+@0 pass in on vlan0 inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from 192.0.2.102
nat cleared
rules cleared
No ALTQ support in kernel
ALTQ related functions disabled
No ALTQ support in kernel
ALTQ related functions disabled
nat cleared
rules cleared
Files left in work directory after failure: created_interfaces.lst, created_jails.lst, tmp.3cwsqfFkKC, tmp.RhFARssEeF, tmp.f2GuGZoVoI, tmp.y1MBnDkUtg
pf/redirection_pool_syntax:af_to  ->  failed: Parsed ruleset differs

Is this apply_redirspec() as found in OpenBSD? That's one of the differences I kept running into while doing the nat64 port. My approach of "la-la-la, I can't see you" for it was perhaps not the ultimate solution there.

Are you planning to bring the new nat syntax over? I've had a vague plan to look at that to make port forwarding for nat64 work, but I've done no serious investigation yet. In general I'm 100% in favour of other people doing the hard work, so I am motivated for this to land.

I'm going to need a fair bit of time to digest this, but I figured I'd mention the test failure first.

[vegeta_tuxpowered.net]

In D49218#1122711, @kp wrote:

This fails one of the new tests on my machine:

(kp@geb)  /usr/tests/sys/netpfil % sudo kyua debug pf/redirection_pool_syntax:af_to                                                                                                [11:14]
Testing rule:
pass in on vlan0 inet6 from any to 64:ff9b::/96 af-to inet from 192.0.2.102 to 198.51.100.102
Parsed ruleset difference:
--- /tmp/kyua.JWtsI2/2/work/tmp.aCH51evNTr      2025-03-04 10:14:41.041368000 +0000
+++ /tmp/kyua.JWtsI2/2/work/tmp.HYltlMftHQ      2025-03-04 10:14:41.060891000 +0000
@@ -1 +1 @@
-@0 pass in on vlan0 inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from 192.0.2.102 to 198.51.100.102
+@0 pass in on vlan0 inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from 192.0.2.102

I think you need https://reviews.freebsd.org/D49213 for that.

 
> Is this `apply_redirspec()` as found in OpenBSD? That's one of the differences I kept running into while doing the nat64 port. My approach of "la-la-la, I can't see you" for it was perhaps not the ultimate solution there.

OpenBSD's `apply_redirspec()` contains both pool and address parsing and it takes additional flag `int isrdr` to decide if ports are to be parsed for NAT or RDR rule. I took the liberty of splitting port parsing into separate functions. I like my functions short and with a single purpose :)

> Are you planning to bring the new nat syntax over?

Hopefully I can upload it today.

[kp]

Are the tests doing something we can't do in the sbin/pfctl/tests tests? Those don't spin up a jail, they just feed input to pfctl and compare the parsed result to an expected output.
It's significantly faster to run those than it is to mess with real jails and interfaces.

Ah, yeah, these tests also have expected failures, which I don't think the pfctl tests support. I wonder if it's worth adding that and migrating these tests, or if we just keep the ones you have.

The above is not blocking (and indeed, I'm not sure what the correct answer is. Both options are acceptable.), but the af-to pfctl tests should be fixed as part of this commit. Other than that I don't see any more issues.

sbin/pfctl/parse.y

6097

This used to be conditional on it being a route-to pool, or a nat rule.
that's no longer the case, and it breaks a couple of the pfctl tests for af-to:

/usr/tests/sbin/pfctl % sudo kyua debug pfctl_test:pf1026
Running pfctl -o none -nvf /usr/tests/sbin/pfctl/files/pf1026.in
---
pass in on epair2b route-to (epair0a 192.0.2.2) inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from (epair0a) round-robin
---
*** Check failed: /usr/src/sbin/pfctl/tests/pfctl_test.c:167: sbuf_data(expected_output) != sbuf_data(real_output) (pass in on epair2b route-to (epair0a 192.0.2.2) inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from (epair0a)
 != pass in on epair2b route-to (epair0a 192.0.2.2) inet6 from any to 64:ff9b::/96 flags S/SA keep state af-to inet from (epair0a) round-robin
)
pfctl_test:pf1026  ->  failed: 1 checks failed; see output for more details

That's probably something we can just change the test for though, not a critical problem.
In fact, I've just checked OpenBSD 7.6 and see your behaviour there, so we should fix the tests to match.

tests/sys/netpfil/pf/redirection_pool_syntax.sh

5 ↗

(On Diff #151795)

2024 or 2025 or both?

[vegeta_tuxpowered.net]

In D49218#1123087, @kp wrote:

Are the tests doing something we can't do in the sbin/pfctl/tests tests? Those don't spin up a jail, they just feed input to pfctl and compare the parsed result to an expected output.
It's significantly faster to run those than it is to mess with real jails and interfaces.

Ah, yeah, these tests also have expected failures, which I don't think the pfctl tests support. I wonder if it's worth adding that and migrating these tests, or if we just keep the ones you have.

I think it is. I will see how complicated would that be.

[kp]

In D49218#1123121, @vegeta_tuxpowered.net wrote:

I think it is. I will see how complicated would that be.

I figure that if we create a pf<foo>.fail (rather than a pf<foo>.ok) with the expected error message it ought to be fairly straightforward.
The pfctl test runner got rewritten in C (for performance on riscv iirc.) a while ago, but it still ought to be possible to convince it to check if the .ok or .fail version exists, and compare the relevant stdout or std error and check for a zero/nonzero exit code.

[vegeta_tuxpowered.net]

In D49218#1123087, @kp wrote:

Ah, yeah, these tests also have expected failures, which I don't think the pfctl tests support.

There is another thing those tests don't support - some tests use an interface in a redirection pool and that gets expanded into IP addresses, probably even wrongly, because IPv6 LL addresses are kept, but still - it's tested.

I will move to pfctl only those tests which don't depend on jails, interfaces and addresses.

tests/sys/netpfil/pf/redirection_pool_syntax.sh

5 ↗

(On Diff #151795)

I've started working last year but I'll update the patch to only contain the date it was uploaded for review.

[kp]

In D49218#1123144, @vegeta_tuxpowered.net wrote:

There is another thing those tests don't support - some tests use an interface in a redirection pool and that gets expanded into IP addresses, probably even wrongly, because IPv6 LL addresses are kept, but still - it's tested.

I'm pretty sure we have the :0 syntax for that. Again something that doesn't need to be done in this patch, but we should consider test cases for that on redirection pools (right now there's only one test for it, pass_block:noalias) to make sure it works.

I will move to pfctl only those tests which don't depend on jails, interfaces and addresses.

To be clear: I don't think that's a requirement to land this patch.

[vegeta_tuxpowered.net]

I've moved the tests back to pfctl.

[kp]

All these tests, how can I possibly say no? ;)