nixos/nftables: check ruleset syntax during build #48201
Conversation
| # Check whether the file is syntactically valid | ||
| checkedRulesetFile = pkgs.runCommand "nftables-rules-checked" | ||
| { src = cfg.rulesetFile; } | ||
| ''${pkgs.nftables}/bin/nft -c -f "$src" && cp $src $out''; |
There was a problem hiding this comment.
Does this check attempt to resolve hostnames or network interfaces? This will not work inside sandboxes.
There was a problem hiding this comment.
Also loading port/service names from /etc/protocol or /etc/services would be a problem.
There was a problem hiding this comment.
It also requires root because it open a netlink connection.
$ cat foo.nft
table inet filter {
chain input {
type filter hook input priority 0;
ct state established,related accept
iif lo accept
# ssh
tcp dport {22, 22022} accept
}
}
$ nft -c -f foo.nft
foo.nft:1:1-2: Error: Could not process rule: Operation not permitted
table inet filter {
^^
foo.nft:2:9-13: Error: Could not process rule: Operation not permitted
chain input {
^^^^^
foo.nft:7:15-25: Error: Could not process rule: Operation not permitted
tcp dport {22, 22022} accept
^^^^^^^^^^^
foo.nft:4:5-39: Error: Could not process rule: Operation not permitted
ct state established,related accept
^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
foo.nft:5:5-17: Error: Could not process rule: Operation not permitted
iif lo accept
^^^^^^^^^^^^^
foo.nft:7:5-32: Error: Could not process rule: Operation not permitted
tcp dport {22, 22022} accept
^^^^^^^^^^^^^^^^^^^^^^^^^^^^
There was a problem hiding this comment.
Okay, that really seems to be a deal-breaker. I'll check whether these problems can be avoided somehow. If not, we should do the check in the systemd start script at least, as trying to load a broken ruleset is not very elegant.
There was a problem hiding this comment.
I had a look at the source code: In the current state I don't how this can work without root, which we not have at build time.
There was a problem hiding this comment.
Does this check attempt to resolve hostnames or network interfaces? This will not work inside sandboxes.
Also loading port/service names from /etc/protocol or /etc/services would be a problem.
I'd say that this isn't really an issue, as long as we would provide an option to disable the checks. Many people probably don't use these features.
There was a problem hiding this comment.
Okay, that really seems to be a deal-breaker.
It is. I could look into creating some nft -c alternative, that doesn't resolve protocols, domains and require a netlink connection, but this will have to wait for a few months. I'll reopen this PR as soon as I've figured something out.
Please excuse the broken PR. :(
Motivation for this change
The nftables ruleset syntax should be verified during build to avoid misconfiguration.
If the file is not syntactically valid, currently, the ruleset would be flushed without the new (incorrect) rules being applied, leaving the firewall in its default configuration (accept everything).Apparently,
nfthas become clever enough to not overwrite the old ruleset in case of a syntax error. However, the system configuration is still built and applied.Things done
sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)nix path-info -Sbefore and after)There are now two files with similar names:
nftables-rules-checkedandnftables-check. In my opinion, thenftables-checkfile (which is actually the service start script) has a pretty bad name. This could be changed to something more meaningful to avoid confusion.