Gentoo Websites Logo
Go to: Gentoo Home Documentation Forums Lists Bugs Planet Store Wiki Get Gentoo!
Bug 789306 - net-firewall/nftables: hardcoded "flush ruleset" prevents custom flush rules
Summary: net-firewall/nftables: hardcoded "flush ruleset" prevents custom flush rules
Status: IN_PROGRESS
Alias: None
Product: Gentoo Linux
Classification: Unclassified
Component: Current packages (show other bugs)
Hardware: All Linux
: Normal normal
Assignee: Matthew Thode ( prometheanfire )
URL:
Whiteboard:
Keywords: PullRequest
Depends on:
Blocks:
 
Reported: 2021-05-10 13:02 UTC by Ogelpre
Modified: 2023-03-22 05:17 UTC (History)
5 users (show)

See Also:
Package list:
Runtime testing required: ---


Attachments
Shell script (nftables-mk.sh-r1,1.64 KB, application/x-shellscript)
2021-05-13 06:31 UTC, Francisco Blas Izquierdo Riera (RETIRED)
Details
conf.d file (nftables-mk.confd-r1,2.60 KB, text/plain)
2021-05-13 06:32 UTC, Francisco Blas Izquierdo Riera (RETIRED)
Details
init.d file (nftables-mk.init-r2,1.98 KB, text/plain)
2021-05-13 06:32 UTC, Francisco Blas Izquierdo Riera (RETIRED)
Details
Shell script (nftables-mk.sh-r1,1.62 KB, application/x-shellscript)
2021-05-13 06:37 UTC, Francisco Blas Izquierdo Riera (RETIRED)
Details
Shell script (nftables-mk.sh-r1,1.62 KB, application/x-shellscript)
2021-05-13 06:44 UTC, Francisco Blas Izquierdo Riera (RETIRED)
Details

Note You need to log in before you can comment on or make changes to this bug.
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 (RETIRED) gentoo-dev 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 (RETIRED) gentoo-dev 2021-05-13 06:31:56 UTC
Created attachment 707781 [details]
Shell script
Comment 4 Francisco Blas Izquierdo Riera (RETIRED) gentoo-dev 2021-05-13 06:32:23 UTC
Created attachment 707784 [details]
conf.d file
Comment 5 Francisco Blas Izquierdo Riera (RETIRED) gentoo-dev 2021-05-13 06:32:42 UTC
Created attachment 707787 [details]
init.d file
Comment 6 Francisco Blas Izquierdo Riera (RETIRED) gentoo-dev 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 (RETIRED) gentoo-dev 2021-05-13 06:37:24 UTC
Created attachment 707790 [details]
Shell script

Sorry, messed a logic condition.
Comment 8 Francisco Blas Izquierdo Riera (RETIRED) gentoo-dev 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 (RETIRED) gentoo-dev 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.