nixos/nat: Match iptables behavior with nftables, add externalIP check

[JustTNE]

Description of changes

Previously, the behavior of the iptables and nftables NAT implementations had some differences. This PR addresses these differences by matching iptable nat's functionality with that of nftables. Additionally, this PR implements updated tests based on #279629. Finally, a relatively minor change mentioned in #279629 section 3 was implemented as well.

1. Previously, all requests arriving from outside NAT would still have their "source address" adjusted on iptables.

The following diagram describes the setup I've experienced the issue in:

1.2.3.4 - Example client
101.101.101.101 - Server's public address
10.0.0.5 - nixos nspawn container (via containers.*) address behind NAT

+--------------+   +----------------+   +-----+   +------------+
|    Client    |   |     Server     |   |     |   | Container  |
|   1.2.3.4    +---> 101.101.101.101+---> NAT +--->  10.0.0.5  |
|              |   |                |   |     |   |            |
+--------------+   +----------------+   +-----+   +------------+

If we introduce this example configuration:

networking.nat.forwardPorts = [
  {
    destination = "10.0.0.5:443";
    loopbackIPs = [ "101.101.101.101" ];
    proto = "tcp";
    sourcePort = 443;
  }
];

Any request previously targeted at 10.0.0.5, even from outside the NAT (outside of where NAT reflection is supposed to act), would have its source address rewritten to 101.101.101.101.

An application, like nginx, behind this NAT would then see the following:

101.101.101.101 - - [27/Dec/2023:02:00:04 +0100] "GET / HTTP/2.0" 200 11 "-" "curl/7.58.0"

The changes in this PR prevent the outside client's IP address from being overwritten by limiting the source address change to behind the NAT, matching the behavior with nftables. Test

2. NAT now supports default forward drop iptables rules.

Behaviors allowed by the NAT are now explicitly whitelisted in the forward ruleset on iptables, which allows behavior similar to nftables with the firewall filterForward setting enabled. Test

3. Only connections made to the nat.externalIP will be port forwarded

This is the one new addition to both nftables and iptables. This means that an interface that has multiple IP addresses can now correctly use only one of those IP addresses as the IP address used for NAT port forwarding, allowing the rest of the IP addresses on the interface to be used however else desired without interference. Test

Things done

• Built on platform(s)
  • x86_64-linux
  • aarch64-linux
  • x86_64-darwin
  • aarch64-darwin
• For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
  • sandbox = relaxed
  • sandbox = true
• Tested, as applicable:
  • NixOS test(s) (look inside nixos/tests)
  • and/or package tests
  • or, for functions and "core" functionality, tests in lib/tests or pkgs/test
  • made sure NixOS tests are linked to the relevant packages
• Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
• Tested basic functionality of all binary files (usually in ./result/bin/)
• 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
  • (Package updates) Added a release notes entry if the change is major or breaking
  • (Module updates) Added a release notes entry if the change is significant
  • (Module addition) Added a release notes entry if adding a new NixOS module
• Fits CONTRIBUTING.md.

@ofborg test nat

---

Add a 👍 reaction to pull requests you find important.

[JustTNE]

The user that originally implemented nat reflection seems to have deleted their GitHub account or it's inaccessible for another reason. That's unfortunate.

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/3789

[Luflosi]

What about the nftables implementation at nixos/modules/services/networking/nat-nftables.nix? Does that have the same problem?
Also I think it would be nice to have a NixOS test which fails without the required changes and passes with your changes.

[JustTNE]

What about the nftables implementation

@Luflosi We do not use nftables in production (I believe we tried to in the past), so I can't speak for how well the nftables code works (and I do not have any experience with nftables either)

About writing tests, it doesn't seem like there are nat reflection tests as is and I am not sure how to write some as of right now either. One would have to emulate a network environment where an "outside" and "inside" network exists I suppose?

[JustTNE]

Is there still any interest in getting this pull request merged?

[fpletz]

Code looks good to me. The nftables implementation looks like its has the same issue. We have to fix the nftables implementation for feature parity since both use the same options. Having a test for this would be awesome.

[Luflosi]

I think #279629 is relevant.

[JustTNE]

I think #279629 is relevant.

Oh yes! I would be willing to port that test to this pull request and try my luck at implementing it for nftables as well. Excellent!

[Luflosi]

That would be awesome!

[JustTNE]

So... Trying to write these tests has me a little stuck with some unexpected behavior.

I wrote up a minimal test case where two machines in separate networks seem to be able to talk to each other completely without any iptables statements allowing them to do so.

This assumes a disabled firewall, but to me, it seems rather odd that the Linux kernel just goes "yeah that checks out"
when two unconnected networks try to talk to each other while forwarding is enabled? Is this expected?
This test is from #279629, so at least someone put some thought into it being there for what I assume is a good reason?

Test file here:
custom.txt
Run with: nix-build ./custom.txt

[JustTNE]

I was able to replicate this with the firewall enabled on the router as well:
custom.txt

This means a device from outside the network is indeed able to access any other network connected to the router it seems, unless I'm missing something?? I would appreciate some pointers for sure.

For now, for the purposes of this pull request, I'm going to solve the one issue this pull request is supposed to solve and put solving the other issues on the to-do list...

Edit:
I found this rather detailed discussion on the moby/docker GitHub discussing this issue, seems like it is only relevant if a malicious actor has L2 access to the machine used as a NAT router, which isn't ideal, but nowhere near as big of a deal as I thought it was. I'm going to ignore this issue in the tests.

[JustTNE]

This PR has been majorly updated with all the changes necessary to make this work. Iptables's behavior now matches the behavior of nftables in the new tests, iptables can now be configured for forward default drop and externalIP actually matters for port forwarding now.

This should also address all the concerns @gray-heron had in #279629 in general.

@Luflosi @fpletz @stalkerhumanoid

@ofborg test nat

[TRPB]

Following the discussion in #328812 I have some general comments on the design/implementation of the NAT module in regards to port forwarding. Please point me to the appropriate location if this isn't the right place for this discussion.

1. If loopbackIps contains 127.0.0.1 then it should add port forwarding to localhost. This already works with nftables and it can be done via:

iptables -t nat -A OUTPUT -m addrtype --src-type LOCAL --dst-type LOCAL -p tcp --dport {PORT} -j DNAT --to-destination {DESTINATION}

iptables -t nat -A POSTROUTING -m addrtype --src-type LOCAL --dst-type UNICAST -j MASQUERADE

sysctl -w net.ipv4.conf.all.route_localnet=1

2. loopbackInterfaces would be more robust and useful in most cases than loopbackIps as IPs can change. Though if we could loopback 127.0.0.1 there probably isn't much of a use case for that anyway.

3. As far as I can work out there is currently no way to forward the same port on different host interfaces. For example, eth0 with 1.1.1.1 and eth1 with 2.2.2.2. Please correct me if I'm wrong but I don't believe it's possible to bind the different host IPs e.g.

(eth0) 1.1.1.1:80 -> 192.168.100.1:80
(eth1) 2.2.2.2:80 -> 192.168.100.2:80

Example use cases:
a. I'm running a server with an internal and external IPs. I want to serve one website to people on the local network and another to the internet.
b. I have a vps running a game server that has to use a specific port. I have two servers with different mods/instances configured so I have two public IPs.
c. I'm running a server with different websites on different IPs

For this we'd need something more like:

nat.eth0.forwardPorts = [ 
  {
    destination = "192.168.100.1:80";
    proto = "tcp";
    sourcePort = 80;
  }
];

nat.eth1.forwardPorts = [ 
  {
    destination = "192.168.100.2:80";
    proto = "tcp";
    sourcePort = 80;
  }
];

Again, I might be misunderstanding the implementation but from the documentation I can't see how this is possible in the current state.

Thinking aloud, I wonder if port forwarding should be it's own module rather than part of NAT but I'm not really concerned where it fits. I actually quite like the option sitting in containers since it maps neatly onto existing docker workflows but we could have containers..portFoward configure the nat (or hypothetical portfoward module) rather than its own implementation but that is currently not feasible due to the limitations above

[JustTNE]

I think in order to limit the scope, anything but number 1 should be its own PR later on and might need a whole different approach. I'll add a test for it and match the behavior.

What do you think of that for now? I don't want to scope creep this PR.

Ps: I think leaving per Interface forwarding to containers is probably the smarter idea yeah. NAT should do NAT things, not worry about arbitrary interfaces and stuff like that.

[Luflosi]

+1 on avoiding scope creep in this PR.

[JustTNE]

I've rebased this pull request to fix the merge conflicts it had. It would be great if the reviewers could review this pull request again so that we can finally get this merged! (Thanks for the help by the way!)

I've decided against implementing @TRPB request number 1 because this neither works on nftables nor iptables as of right now as far as I can tell, so it should be a separate PR I'd say. I've made some progress on implementing that, however I don't plan on finishing it. A diff is attached to this comment:
WIP.diff.txt

@stalkerhumanoid @fpletz (mentioning previous reviewers)
@ofborg test nat

[nixos-discourse]

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

https://discourse.nixos.org/t/prs-ready-for-review/3032/4648

[gray-heron]

Great job! Thank you very much for picking this up.

I would suggest adding a note to networking.nat.enable that it assumes trusted L2 on all interfaces.

Other than that, I have checked the new behavior extensively and I found the issues preventing me from using the NixOS NAT resolved. I hope this gets reviewed and merged soon.

[JustTNE]

Rebased and addressed the review concerns! Thank you for the review and extensive testing. I've been running this in production since day 1 of this pull request existing, but the extra eyes and extra testing gives me some extra peace of mind for sure!

@ofborg test nat