Page MenuHomeFreeBSD

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

Authored by ahsanb on Jul 25 2019, 10:16 AM.



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

Diff Detail

rS FreeBSD src repository
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

ahsanb created this revision.Jul 25 2019, 10:16 AM
ahsanb updated this revision to Diff 60145.Jul 25 2019, 6:26 PM
kristof added inline comments.Jul 25 2019, 7:59 PM
9 ↗(On Diff #60145)

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

5 ↗(On Diff #60145)

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

39 ↗(On Diff #60145)

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

73 ↗(On Diff #60145)

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.

127 ↗(On Diff #60145)

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

32 ↗(On Diff #60145)

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

33 ↗(On Diff #60145)

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.

37 ↗(On Diff #60145)

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)

61 ↗(On Diff #60145)

This belongs with pf_init.

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

74 ↗(On Diff #60145)

That belongs with ipf_init.

122 ↗(On Diff #60145)

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

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 @@
+            common
+            ..
ahsanb updated this revision to Diff 60163.Jul 26 2019, 3:37 PM
  • 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.

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

53 ↗(On Diff #60163)

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

100 ↗(On Diff #60163)

I'd atf_fail here too.

ahsanb updated this revision to Diff 60175.Jul 26 2019, 7:39 PM
  • Add check for bad argument in setup_tests
  • Add atf-fail in case of wrong firewall name in firewall_config
thj added a subscriber: bz.Jul 27 2019, 6:53 PM
thj added inline comments.
3 ↗(On Diff #60175)

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

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:

87 ↗(On Diff #60175)

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

39 ↗(On Diff #60175)

I think the error message here should be something like

"no testcase passed to setup_test"

kristof added inline comments.Jul 27 2019, 8:52 PM
87 ↗(On Diff #60175)

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 updated this revision to Diff 60238.Jul 29 2019, 6:04 PM
  • 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.
kristof accepted this revision.Jul 29 2019, 8:39 PM

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.

This revision is now accepted and ready to land.Jul 29 2019, 8:39 PM
ahsanb updated this revision to Diff 60288.Jul 30 2019, 3:37 PM
  • Fix ipf check (using type ipf &> /dev/null)
This revision now requires review to proceed.Jul 30 2019, 3:37 PM
kristof added inline comments.Jul 31 2019, 8:37 AM
65 ↗(On Diff #60288)

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

ahsanb updated this revision to Diff 60417.Aug 3 2019, 7:01 AM

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

cy added a subscriber: cy.Aug 3 2019, 1:40 PM
thj accepted this revision.Aug 4 2019, 8:33 PM

All the tests seem to work on r350568

This revision is now accepted and ready to land.Aug 4 2019, 8:33 PM
This revision was automatically updated to reflect the committed changes.