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

[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

[JustTNE]

Rebased to fix the merge conflict in the docs.
@ofborg test nat

[JustTNE]

May I ask anyone with write access what could be done to unblock this pull request? This PR has been open since 2023 after all, and the 24.11 breaking changes freeze is coming up very soon according to #339153.

[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/4705

[JustTNE]

Rebased and release notes updated to 25.05! I'd still really like to get this merged some time! It's been almost a full year now!

[misuzu]

@ofborg test nat

[misuzu]

Unless there are any objections I'll merge this tomorrow.