networking/nftables: add .tables property and disable ruleset flushing by default by mkg20001 · Pull Request #207758 · NixOS/nixpkgs

mkg20001 · 2022-12-25T23:59:42Z

Description of changes

This allows for other unmanaged tables to co-exist peacefully on the os,
by having the nixos-managed tables be re-created atomically and the other
tables will simply be left untouched.

Things done

ncfavier

Can't really comment on the nftables changes, sorry.

duament · 2022-12-27T02:00:01Z

We may also disable flushing in execStop to fix #207058.

mkg20001 · 2022-12-28T01:53:10Z

We may also disable flushing in execStop to fix #207058.

There appears to have been a systemd bug systemd/systemd#11238 and it's no longer occuring

nftables.service loaded active exited nftables firewall (after failed reload)

What I've added is that if flushRuleset is set to false, then the service will only remove tables it created, but this may be undesired. Opinions?

mkg20001 · 2022-12-28T01:59:41Z

So I couldn't reproduce it or I did it wrong, but seems the issue is fresh. Not sure how to reproduce then. Added a table with invalid contents and had a failure recorded, nft list ruleset still had the same tables as before.

mkg20001 · 2022-12-28T02:01:09Z

Thinking about flushRuleset I don't know if it makes sense to have it always off by default. Perhaps the default could be dynamic based on whether or not .tables is being used? Or does that make things even more complicated?

duament · 2022-12-28T04:50:08Z

I can't reproduce that issue either. It seems he restarted the nftables.service and the rulesets got flushed. This PR may not help that.

duament · 2022-12-28T05:12:16Z

Thinking about flushRuleset I don't know if it makes sense to have it always off by default. Perhaps the default could be dynamic based on whether or not .tables is being used? Or does that make things even more complicated?

I prefer to keep it off by default. This prevents deleting tables maintained by other programs like systemd, wg-quick, etc.

There's another problem that when the user switches to a profile with firewall disabled, the nixos-fw table remains.
Similar to this issue https://web.archive.org/web/20180605181330/https://github.com/NixOS/nixpkgs/issues/27510.

mkg20001 · 2022-12-28T22:31:10Z

Thinking about flushRuleset I don't know if it makes sense to have it always off by default. Perhaps the default could be dynamic based on whether or not .tables is being used? Or does that make things even more complicated?

I prefer to keep it off by default. This prevents deleting tables maintained by other programs like systemd, wg-quick, etc.

Makes sense. I'll add a note to ruleset+rulesetFile that the user has to delete tables manually

There's another problem that when the user switches to a profile with firewall disabled, the nixos-fw table remains. Similar to this issue web.archive.org/web/20180605181330/https://github.com/NixOS/nixpkgs/issues/27510.

A script that runs ExecStartPost, that has the nftables file as an argument and generates a list of deletions, stores it as /var/lib/nftables/deletions.nft by running over the nftables file with grep or something (cat nftables | grep -r "^ *table" | sed -e "s|^ *table|delete table|g" -e 's|{||g'
- This would be hacky, maybe cause unwanted behaviour
Make a script that greps for all delete statements, puts them into /var/lib/nftables/deletions.nft (this would be more explicit and this has the advantage of including delete statements from custom ruleset/rulesetFile)
Or instead just write the deletions generated from .tables into /var/lib/nftables/deletions.nft
ExecStop changed to "nft -f /var/lib/nftables/deletions.nft"

Edit: We need to store it in deletions.nft anyways, since the tables that get removed from config won't be deleted otherwise

duament · 2022-12-29T02:06:41Z

A simple solution is to set an empty table when the firewall is disabled. Your solution seems more generic.

mkg20001 · 2022-12-29T21:53:24Z

@duament .tables allows adding custom tables. We need to handle those aswell. So it must be generic.

mkg20001 · 2023-03-27T17:56:54Z

How about keeping the flushRulesets option and disable .tables options when it's true?

Hrm this means any module we have that uses .tables is automatically turning off flushRuleset, which may not be what users want. Another idea is to disable it if no user extraRules are set. (This is only affecting the option default value, so users can change it anyways)

mkg20001 · 2023-03-27T18:14:04Z

I made it so flushRuleset is enabled on rulset{,File} being used (plus 23.05 being used so we can backport)

Also made rulesetFile and ruleset non-exclusive (there's no assertion for it and there's no good reason I see for it to be exclusive)

Not sure if I'm over feature-freeze with this pr? Otherwise feel free to merge

sg-qwt · 2023-04-21T16:43:54Z

after applying this patch with a nixos-rebuild switch, nfttables.service failed with following error:

Apr 22 00:36:33 xx systemd[1]: Reloading nftables firewall... Apr 22 00:36:33 xx gfzcn50sarqv9s711n1zfy6yky1wa2rf-nftables-rules[1274297]: /nix/store/gfzcn50sarqv9s711n1zfy6yky1wa2rf-nftables-rules:3:1-42: Error: File not found: /var/lib/nftables/deletions.nft Apr 22 00:36:33 xx gfzcn50sarqv9s711n1zfy6yky1wa2rf-nftables-rules[1274297]: include "/var/lib/nftables/deletions.nft" Apr 22 00:36:33 xx gfzcn50sarqv9s711n1zfy6yky1wa2rf-nftables-rules[1274297]: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

I have to manually restart nftables.service again to work. Maybe we missed a check for deletionsScriptVar here?

yes, we missed an ensureDeletions in ExecReload

sg-qwt

I hit some error on a first nixos switch with this patch, check comments.
Second switch with some firewall changes works as expected, custom tables are not flushed, which is cool!

KiruyaMomochi · 2023-04-23T10:32:59Z

This PR is great. For me the conflict between networking.nftables.rulesetFile and the firewall is disturbing.

Considering the reload issue, can I assume that ensureDeletions always run before rulesScript?
If yes, we can make a single script that first ensure deletions, then run the rule script.

mkg20001 · 2023-04-24T09:44:40Z

Considering the reload issue, can I assume that ensureDeletions always run before rulesScript?
If yes, we can make a single script that first ensure deletions, then run the rule script.

We need to run the previous deletions to clean up the previous rules.

nixos-discourse · 2023-06-09T08:06:27Z

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/is-it-possible-to-write-custom-rules-to-the-nixos-firewall/27900/7

justinas · 2023-08-20T21:15:29Z

Any movement on this?

…g by default This allows for other unmanaged tables to co-exist peacefully on the os, by having the nixos-managed tables be re-created atomically and the other tables will simply be left untouched.

Co-authored-by: Naïm Favier <[email protected]>

… false

Co-authored-by: duament

…used

mkg20001 · 2023-08-27T22:48:04Z

Rebased.

It turns out the reload error occurs when the service was started before this patchset was applied, as ensureDeletions wasn't run on startup then.

So added ensureDeletions to execreload which should fix the error

ajs124 · 2023-11-23T17:06:27Z

+                ${table.content}
+              }
+            '') enabledTables)}
+            ${cfg.ruleset}


I have some defines in my config like this:

networking.nftables.ruleset = lib.mkBefore '' define NET_BLA = 128.0.0.0/19 '';

which break with this change, because the config from nftables.tables is inserted here before ruleset.

Not sure if and how we can fix this, just commenting here so you're aware.

We can either move cfg.ruleset up and/or introduce properly typed nftables.define.<key>

Not sure what the proper type would be for define tbh. Something like attrsOf str can work, but that doesn't allow for any merging, e.g. of lists/sets. Moving rulset up modifies the behavior from what's on master after this, but maybe it's closer to before? I'll see if I can find the time to run some tests.

github-actions Bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 26, 2022

mkg20001 force-pushed the nftex branch from 6d6e283 to 8cb4962 Compare December 26, 2022 00:09

github-actions Bot added 8.has: changelog This PR adds or changes release notes 8.has: documentation This PR adds or changes documentation labels Dec 26, 2022

mkg20001 mentioned this pull request Dec 26, 2022

nixos/{firewall, nat}: add a nftables based implementation #203011

Merged

19 tasks

ofborg Bot added 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Dec 26, 2022

mkg20001 force-pushed the nftex branch from 8cb4962 to 1be656c Compare December 26, 2022 00:18

mkg20001 requested a review from ncfavier December 26, 2022 00:20

mkg20001 force-pushed the nftex branch 3 times, most recently from 256fc47 to 49ccf93 Compare December 26, 2022 00:49

ncfavier reviewed Dec 26, 2022

View reviewed changes

Comment thread nixos/modules/services/networking/nftables.nix Outdated

duament mentioned this pull request Dec 27, 2022

nixos/nftables: add option to not drop the whole ruleset #203606

Closed

13 tasks

mkg20001 force-pushed the nftex branch from 3f7fd53 to facc731 Compare December 28, 2022 01:28

ofborg Bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. and removed 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. labels Dec 28, 2022

mkg20001 force-pushed the nftex branch from facc731 to a9e9cef Compare December 28, 2022 01:52

SuperSandro2000 reviewed Mar 19, 2023

View reviewed changes

Comment thread nixos/doc/manual/from_md/release-notes/rl-2305.section.xml Outdated

mkg20001 force-pushed the nftex branch 2 times, most recently from 61a95bc to 50d7ec2 Compare March 27, 2023 17:52

sg-qwt reviewed Apr 21, 2023

View reviewed changes

justinas mentioned this pull request Aug 20, 2023

miniupnpd-nftables: init at 2.3.3 #239028

Merged

12 tasks

mkg20001 and others added 3 commits August 28, 2023 00:30

networking/nftables: add .tables property and disable ruleset flushin…

048ef0d

…g by default This allows for other unmanaged tables to co-exist peacefully on the os, by having the nixos-managed tables be re-created atomically and the other tables will simply be left untouched.

*: migrate to using nftables.tables instead of ruleset directly

311d2fa

networking/nftables: enable flushing ruleset for older versions

cd3af25

Co-authored-by: Naïm Favier <[email protected]>

mkg20001 force-pushed the nftex branch from 495c47f to 689040f Compare August 27, 2023 22:38

mkg20001 added 6 commits August 28, 2023 00:40

release-notes: add networking.nftables.tables news

dc3f872

networking/nftables: remove no longer relevant conflict warnings

d5a0826

networking/nftables: only delete our tables if flushRuleset is set to…

5f300ad

… false

nixos/nftables: save deletions to file and run them afterwards

55213b5

Co-authored-by: duament

networking/nftables: enable flushRuleset by default if rulset{,File} …

a1dd69d

…used

networking/nftables: make ruleset+rulesetFile non-exclusive

6658b3f

mkg20001 force-pushed the nftex branch from 689040f to 6658b3f Compare August 27, 2023 22:40

networking/nftables: ensure deletions

369e18f

mkg20001 merged commit 18748c1 into NixOS:master Aug 27, 2023

mkg20001 deleted the nftex branch August 27, 2023 23:29

jian-lin mentioned this pull request Sep 18, 2023

nixos/nftables: Make ruleset.conf be rulesetFile to fix preCheckRuleset #230745

Closed

12 tasks

ajs124 reviewed Nov 23, 2023

View reviewed changes

nbdd0121 mentioned this pull request Jun 21, 2025

nixos/{firewall,nat}: Standardize around an iptables-restore / nftables solution #4155

Open

Uh oh!

Conversation

mkg20001 commented Dec 25, 2022

Description of changes

Things done

Uh oh!

ncfavier left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

duament commented Dec 27, 2022

Uh oh!

mkg20001 commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkg20001 commented Dec 28, 2022

Uh oh!

mkg20001 commented Dec 28, 2022

Uh oh!

duament commented Dec 28, 2022

Uh oh!

duament commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mkg20001 commented Dec 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

duament commented Dec 29, 2022

Uh oh!

mkg20001 commented Dec 29, 2022

Uh oh!

Uh oh!

mkg20001 commented Mar 27, 2023

Uh oh!

mkg20001 commented Mar 27, 2023

Uh oh!

sg-qwt Apr 21, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

duament Apr 21, 2023

Choose a reason for hiding this comment

Uh oh!

sg-qwt left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

KiruyaMomochi commented Apr 23, 2023

Uh oh!

mkg20001 commented Apr 24, 2023

Uh oh!

nixos-discourse commented Jun 9, 2023

Uh oh!

justinas commented Aug 20, 2023

Uh oh!

mkg20001 commented Aug 27, 2023

Uh oh!

ajs124 Nov 23, 2023

Choose a reason for hiding this comment

Uh oh!

mkg20001 Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

ajs124 Nov 24, 2023

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

10 participants

mkg20001 commented Dec 28, 2022 •

edited

Loading

duament commented Dec 28, 2022 •

edited

Loading

mkg20001 commented Dec 28, 2022 •

edited

Loading

sg-qwt Apr 21, 2023 •

edited

Loading

sg-qwt left a comment •

edited

Loading