@Jookia, thanks for your PR! By analyzing the annotation information on this pull request, we identified @edolstra, @bjornfor and @offlinehacker to be potential reviewers

If you have ideas for a better interface, I'd be happy to listen. I use rulesetFile, but it would be annoying to have to do pkgs.writeText if you're just writing inline rules. The wants change is a good idea.

I use this personally, but if it's not useful for NixOS I'm happy to close the PR

Actually the wants change isn't a good idea. This module does the same as firewall.

Then the firewall module is broken too...
About network-pre.target source:

It's a passive unit: you cannot start it directly and it is not pulled in by the the network management service, but by the service that wants to run before it. [...] Services that want to be run before the network is configured should place Before=network-pre.target and also set Wants=network-pre.target to pull it in.

Yeah, I guess I'll fix it here then. The nftables module isn't intended to replace the firewall, but it is incompatible with the firewall. nftables itself is probably a little too early to go all out on, but it's nice to have for now as a module with some examples.

Fixed?

I think we need some wantedBy.. Maybe basic.target?

Added wantedBy = multi-user.target, this is how another firewall package does it.

My 2¢ as someone who manages many complex Linux firewalls, and in the process of switching some of them to NixOS.

I would like to have an abstraction layer over tables, chains and rules, so I can split my firewall configuration over multiple Nix files, eg. have a vpn.nix define a VPN connection and add some iptables/nftables rules to the relevant chains. Then the NixOS module system would merge all the rules and chains and generate a single text file as input for iptables-restore/nftables, so the ruleset can be loaded atomically.

An example of what I'm thinking about:

networking.firewall = {
  ipRules = {

    filter.INPUT = [
      { inputInterface = "eth2"; chain = "dmz"; }
    ];

    filter.dmz = [
      { proto = "tcp"; destinationPort = [ 80 443 ]; target = "ACCEPT"; } # expands to 2 iptables rules
      { allowedTCPPorts = [ 80 443 ]; }  # shortcut for the rule above
    ];

    nat.POSTROUTING = [
      { source = "192.168.0.1/24"; outputInterface = "eth1"; target = "SNAT"; targetArgs = "--to 192.168.7.1"; }
    ];

    mangle.PREROUTING = [
      { source = "10.0.0.1/24"; inputInterface = "eth0"; mark = "0"; target = "MARK"; targetArgs = "--set-mark 0x20"; }
    ];
  };

  ip6Rules = {
    # ...
  };
};

The proposal in #12940 would be a step in the right direction for iptables IMO.

However, nftables has a much more expressive language than iptables, and abstracting all its features with Nix would be way overkill.
So let's thin the abstraction and generate the rulesets from text fragments; it would be simpler to implement as a module, and still be nice to the firewall builder.
Something like this:

networking.firewall.iptablesRules = {

  filter.INPUT = [
    "-i eth2 -j dmz"
  ];

  filter.dmz = [
    "-p tcp --dport 80 -j ACCEPT"
    "-p tcp --dport 443 -j ACCEPT"
  ];

};

Or, for nftables:

networking.firewall.nftables = {

  sets.private = ''
    { 192.168.0.0/16, 10.0.0.0/8 }
  '';

  ipRules.filter.input = ''
    ip saddr @private log drop
    iif eth2 jump dmz
  '';

  ipRules.filter.dmz = ''
    tcp dport { 80, 443 } accept
  '';

  ip6Rules.filter.input = "...";

};

The existing allowedTCPPorts options would generate rules like these so we can keep backwards compatibility and a simple option for basic firewalls.

Let me know what you think.

Should this be renamed to something like nftables-raw then?

Something I do with my nftables ruleset files is to put flush ruleset at the top of the file, so I can always just re-run nft on that file and it will automatically flush + reload all rules. I find this nicer than doing it explicitly in ExecReload.

On Fri, Sep 23, 2016 at 06:15:14PM -0700, Aneesh Agrawal wrote:

Something I do with my nftables ruleset files is to put flush ruleset at the
top of the file, so I can always just re-run nft on that file and it will
automatically flush + reload all rules. I find this nicer than doing it
explicitly in ExecReload.

Having it twice doesn't hurt, and having it in ExecReload allows it to ensure
that things are loaded atomically. However, I'm open to changing this given that
it's hard to use the rules outside of nft reliably.

Would it be better to update documentation to include "flush ruleset" and also
provide a way to avoid the automatic flushing in cases people don't want that?

Perhaps rulesetFile could not include it.

I can't imagine a scenario where you wouldn't want atomic flushing. IMO, always automatically adding flush ruleset to the top of the file is the simplest and thus best solution - no special ExecReload rule, impossible to forget it using nft, don't need to document how to enable it because it's always on.

If there is a good reason not to use atomic flushing, I would like to hear about it.

On Fri, Sep 23, 2016 at 08:48:13PM -0700, Aneesh Agrawal wrote:

I can't imagine a scenario where you wouldn't want atomic flushing. IMO,
always automatically adding flush ruleset to the top of the file is the
simplest and thus best solution - no special ExecReload rule, impossible to
forget it using nft, don't need to document how to enable it because it's
always on.

What's the difference between adding it at build-time versus runtime?

If there is a good reason not to use atomic flushing, I would like to hear
about it.

Load fast crash hard?

@aneeshusa even if ExecStart is atomic, one still want to have an ExecReload, because otherwise it will leave the system unprotected for a short amount of time between ExecStop and ExecStart on restart.

@grahamc security

Thanks!

@Mic92 Why did you merge this without any regard to #23181? (Don't want to flame her but I'm honestly a bit upset...)

A also see multiple issues with this PR (not that familiar with nftables though, so I might be wrong...). I'll probably post some comments anyway.

@primeos I saw your rfc, but I thought this pull request orthogonal. A generic firewall can still apply its rules using this module. Also my guess was that it would also take some time, while this module is already useful. I address your concerns regarding the default rule set.

How about leaving it ruleset null at the moment by default and moving default to example? Then user can set rules as they like.

Also my guess was that it would also take some time, while this module is already useful.

Agreed, I just hope we don't have to make any changes later that could break many setups.

How about leaving it ruleset null at the moment by default and moving default to example? Then user can set rules as they like.

Would probably be a good idea. Generally that obviously wouldn't be desirable but it would hopefully prevent SSH breakage (e.g. with services.openssh.ports).

But we should probably at least keep "# Check out https://wiki.nftables.org/ for better documentation." and fix the "# Allow anything in." comment.

I saw your rfc, but I thought this pull request orthogonal.

It probably is (I initially feared this would cause too many problems later but I guess I was wrong since the module is pretty minimal and we can probably easily extend it).

Suggestion for the example:

          # Check out https://wiki.nftables.org/ for better documentation.

          # Table for both IPv4 and IPv6.
          table inet filter {
            # Block all IPv4/IPv6 input traffic except SSH.
            chain input {
              type filter hook input priority 0;

              # accept any localhost traffic
              iifname lo accept

              # accept traffic originated from us
              ct state {established, related} accept

              # accept neighbour discovery otherwise IPv6 connectivity breaks
              ip6 nexthdr icmpv6 icmpv6 type { nd-neighbor-solicit, echo-request, nd-router-advert, nd-neighbor-advert } accept

              # accept SSH connections (required for a server)
              tcp dport 22 accept
              
              # count and drop any other traffic
              counter drop
            }
            
            # Allow anything out.
            chain output {
              type filter hook output priority 0;

              oifname lo accept

              accept
            }
            
            chain forward {
              type filter hook forward priority 0;

              accept
            }
          }

By far not a perfect example (I mainly copied the one from https://wiki.nftables.org/wiki-nftables/index.php/Simple_ruleset_for_a_workstation) but it should work (requires testing, unfortunately I can't do that ATM) but it should e.g. work with IPv6. But note that this can potentially still cause a lot of problems (it probably doesn't accept any ICMP packets -> outgoing ping, traceroute, etc. won't work).

A potential fix (from the Arch-Wiki) for the ICMP packets (but they might already be covered by the conntrack state related):

# ICMP
# routers may also want: mld-listener-query, nd-router-solicit
ip6 nexthdr icmpv6 icmpv6 type { destination-unreachable, packet-too-big, time-exceeded, parameter-problem, nd-router-advert, nd-neighbor-solicit, nd-neighbor-advert } accept
ip protocol icmp icmp type { destination-unreachable, router-advertisement, time-exceeded, parameter-problem } accept

I'd probably add fragmentation-required for ICMP(v4) as well (just to be extra save - but since it's only an example it probably doesn't really matter anyway). And sorry for the delayed update... :o (doesn't really matter if it's too late).

changed in 6c36d9f

@Mic92 Great, thanks 😄