Bug 789306 – net-firewall/nftables: hardcoded "flush ruleset" prevents custom flush rules

• Product: Gentoo Linux
• Status: IN_PROGRESS ---

[Description Ogelpre 2021-05-10 13:02:31 UTC]

I am running a setup with LXD. LXD creates own dynamic nftables rules. I generate /var/lib/nftables/rules-save with custom flush rules for my tables which don't touch the LXD rules.

The init-helper script has a hardcoded flush of the whole ruleset in line 17.  [0]

> printf 'flush ruleset\ninclude "%s"\n' "${NFTABLES_SAVE}" | nft -f -

The is redundant to line 29 which writes the flush into the save file.[1]

> printf '#!/sbin/nft -f\nflush ruleset\n'

I suppose to remove the the flush in line 17 or add a switch to disable this behaviour.


[0] https://gitweb.gentoo.org/repo/gentoo.git/tree/net-firewall/nftables/files/libexec/nftables-mk.sh?id=52e344abd9cb435ff4bf7d276d576aa2cac1be13#n17
[1] https://gitweb.gentoo.org/repo/gentoo.git/tree/net-firewall/nftables/files/libexec/nftables-mk.sh?id=52e344abd9cb435ff4bf7d276d576aa2cac1be13#n29

Reproducible: Always

[Comment 1 Francisco Blas Izquierdo Riera 2021-05-11 19:44:04 UTC]

Hello,

The hardcoded flush has various reasons to be there:
1. There is no guarantee that the save file has a flush ruleset. This would result in multiple loads of the ruleset.
2. An empty rules files would not result in an empty firewall ruleset as expected.
3. Prior versions of the script did not add the flush, neither do many of the rulesets you will find online.

Also the script did flush the ruleset using its own flush from the beggining, the main reason why the approach on line 29 is used is to ensure the whole flush then load process happens atomically.

If you want I can write a patch to allow disabling the hardcoded flush, but I will not remove it because doing so may end up making other users' systems unreachable.

[Comment 2 Ogelpre 2021-05-11 22:44:09 UTC]

(In reply to Francisco Blas Izquierdo Riera from comment #1)

> If you want I can write a patch to allow disabling the hardcoded flush, but
> I will not remove it because doing so may end up making other users' systems
> unreachable.

Yeah, you are right. Maybe someone depends already on that behavior and changing it may break things.

Adding a switch to disable the hard coded flush allows people like me to write a custom rule set to the save file and use the standard distribution init scripts to load them.

I would be happy if you can implement it that way.

[Comment 3 Francisco Blas Izquierdo Riera 2021-05-13 06:31:56 UTC]

Created attachment 707781 [details]
Shell script

[Comment 4 Francisco Blas Izquierdo Riera 2021-05-13 06:32:23 UTC]

Created attachment 707784 [details]
conf.d file

[Comment 5 Francisco Blas Izquierdo Riera 2021-05-13 06:32:42 UTC]

Created attachment 707787 [details]
init.d file

[Comment 6 Francisco Blas Izquierdo Riera 2021-05-13 06:34:01 UTC]

I added some (still untested) changes to the nftables scripts to give more flexibility regarding flushes. Please give them a test and tell me if they work for you :)

[Comment 7 Francisco Blas Izquierdo Riera 2021-05-13 06:37:24 UTC]

Created attachment 707790 [details]
Shell script

Sorry, messed a logic condition.

[Comment 8 Francisco Blas Izquierdo Riera 2021-05-13 06:44:58 UTC]

Created attachment 707793 [details]
Shell script

(I shouldn't be writting code this early in the morning).

[Comment 9 Ogelpre 2021-05-13 19:35:39 UTC]

(In reply to Francisco Blas Izquierdo Riera from comment #6)
> I added some (still untested) changes to the nftables scripts to give more
> flexibility regarding flushes. Please give them a test and tell me if they
> work for you :)

I have teseted the scripts. They work for me. (:

During testing I found another small issue. See #790014.

[Comment 10 Francisco Blas Izquierdo Riera 2021-07-24 22:20:49 UTC]

Hey Matthew!

Hope all is well for you these days!

I'm assigning this to you as there isn't much more I can do for this bug.

Check the pull request when you have time and if you are okay with it please merge it. If you need to contact me e-mail will work better than IRC (we can chat on IRC but I don't check my IRC box that often nowadays so please send me an e-mail first so that I can see it).

[Comment 11 kfm 2023-03-21 22:44:06 UTC]

Regarding the PR, matching against (uppercase) "NO" is overly specific. For Gentoo conf.d material, it would be sensible to adhere to the conventions of the yesno function.

case ${NFTABLES_LOAD_FLUSH} in
	[Nn][Oo]|[Ff][Aa][Ll][Ss][Ee]|[Oo][Ff][Ff]|0)
		: do not flush
		;;
	*)
		: do flush
esac

Moreover, I'm concerned that it adds too many knobs. Why do we need three settings to cover the reporter's use case? It's too complicated.

I would suggest having a single boolean-like knob (it could still be named NFTABLES_FLUSH). If false, then both inhibit the injection of "flush ruleset" into the command stream conveyed to nft(8) and the insertion of "flush ruleset" into any saved ruleset. Otherwise, do neither. Also, if false, just make the clear action an error-raising no-op. I sincerely doubt that those for whom this use case applies would be particularly interested in running an OpenRC-specific "clear" action anyway, with all the action at a distance that is implied. I certainly would not be.

Besides, the clear action isn't all that useful to begin with. It was much more useful for iptables, where correctly, atomically, emptying a ruleset was anything but trivial. By contrast, manually running "nft flush ruleset" is easy and is something that any nftables user should be expected to know how to do.

[Comment 12 kfm 2023-03-22 05:17:25 UTC]

I've thought about this some more and have an idea which I think will not only address the reporter's use case but enable various others, while keeping the tooling relatively simple. I'll put it to the test first, then report back.