Pass/Block test for three firewalls (pf, ipfw, ipf)

[ahsanb]

Summary

Added a framework to write tests for multiple firewalls, with a demonstration of the framework on the pass/block test.

[kp]

tests/sys/netpfil/Makefile

9

You'll have to check your editor settings. It's expanded tabs into spaces.

tests/sys/netpfil/common/Makefile

6

Same here. This is all indented with spaces, but should be using tabs.
This seems to apply to all files (Good, at least it's consistent.)

tests/sys/netpfil/common/pass_block.sh

40

Check for stray trailing spaces here. (In vim ':set list').

74

It looks a little odd to have ${firewall}_init and then firewall_cleanup.

I wonder if it'd be better to firewall_init $firewall and do the ${firewall}_init thing in a wrapper function in utils.subr. That would also let you do vnet_init in firewall_init, rather than three times in the different firewall init functions.

While I think about it, it'd probably also be good to pass the firewall type to the cleanup function (i.e. firewall_cleanup ${firewall}. We don't need it know, but might need it in the future, and then the required argument will be there.

128

There's no need for the '\' on the last line.

tests/sys/netpfil/common/runner.subr

33

I think the style wants there to be a space between ';' and 'do'.

34

The 'then' can go on the same line, expect it becomes an overly long line then.
This check also repeats in firewall_config. It's probably a good idea to factor it out into its own function.

38

It's probably a good idea to check that $testcase is set here, and it might also be good to check for duplicates (i.e. someone does setup_tests foo pf ipfw pf bar pf)

tests/sys/netpfil/common/utils.subr

62

This belongs with pf_init.

It's also not immediately clear to my what this is for.

75

That belongs with ipf_init.

123

It's probably better to elif [ ${fw} == "ipfnat" ] here, and then have a default case that throws an error.

[kp]

Also, I get 'install: /usr/tests/sys/netpfil/common/pass_block: No such file or directory' trying to install world.
This patch is missing this:

diff --git a/etc/mtree/BSD.tests.dist b/etc/mtree/BSD.tests.dist
index dfa35484c80..ef1b06935be 100644
--- a/etc/mtree/BSD.tests.dist
+++ b/etc/mtree/BSD.tests.dist
@@ -793,6 +793,8 @@
     netmap
     ..
         netpfil
+            common
+            ..
             pf
                 ioctl
                 ..

[ahsanb]

• Replaced spaces with tabs
• Created firewall_init function with firewall name as an argument
• Modified firewall_cleanup function to take name of the firewall as an argument
• Removed obsolete functions like ipf_init.

[kp]

I'd want Tom to have a look too, but I think we're pretty close to something ready to commit.

tests/sys/netpfil/common/utils.subr

54

Possibly 'case' / 'esac' instead of if/elif/elf/...?

101

I'd atf_fail here too.

[ahsanb]

• Add check for bad argument in setup_tests
• Add atf-fail in case of wrong firewall name in firewall_config

[thj]

tests/sys/netpfil/common/pass_block.sh

4

'All rights reserved' no longer has any legal meaning.

https://www.iusmentis.com/copyright/allrightsreserved/
https://abovethelaw.com/2018/10/all-rights-reserved-a-copyright-relic/

We have been going through the tree removing it in places, I'd prefer we not introduce more instances. This applies to all copyright declarations in this review.

For future the preferred license for new files in in the committers handbook:
https://www.freebsd.org/doc/en_US.ISO8859-1/articles/committers-guide/pref-license.html

88

Why are you disabling duplicate address detection?

2001::db8 (documentation) addresses are the wrong address space to use for tests. You should use unique local addresses (https://en.wikipedia.org/wiki/Unique_local_address) for the test addresses. I think this is also a problem in the v4 pftests too, I owe @kristof a diff

@bz Does using ULA addresses for tests seem correct to you?

tests/sys/netpfil/common/runner.subr

40

I think the error message here should be something like

"no testcase passed to setup_test"

[kp]

tests/sys/netpfil/common/pass_block.sh

88

The pf tests do that too. I seem to remember because that means the address is applied immediately, rather than waiting for DAD to be done. This speeds up the tests, means we don't need silly little 'sleep X' calls after address assignment, and makes things more robust.

Ahsan will have used 2001:db8 because I did that in pf. Your diff will be welcomed.

[ahsanb]

• Used ULA for v6 addresses
• Changed license according to preferred license
• For no_dad, I am taking kp's words regarding speed of the tests.

[kp]

I think I'm happy with this.
I'll give Tom a bit of time to add any more remarks he might have, but I think we can commit this soon.

I've also tested this with sysctl net.inet.ip.fw.default_to_accept=1, and that means we can just load all three firewalls and have the tests pass.

[ahsanb]

• Fix ipf check (using type ipf &> /dev/null)

[kp]

tests/sys/netpfil/common/utils.subr

65

service ipfilter start relies on ipfilter_enabled="YES" in /etc/rc.conf. We shouldn't assume that.
Start ipf using 'ipf -E'.

[ahsanb]

check if ipf module is loaded using "kldstat -q -m ipfilter " because "kldstat -q -m ipl" doesn't work.

[thj]

All the tests seem to work on r350568