Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

nixos-firewall-tool: add nftables support #275126

Closed

Conversation

duament
Copy link
Contributor

@duament duament commented Dec 18, 2023

Description of changes

Add nftables support in nixos-firewall-tool and enable it by default when NixOS firewall enables.

Also add some tests for nixos-firewall-tool in nixosTests.firewall.

Depends on #275371

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:
  • 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.

ping @Atemu @clerie


Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: documentation This PR adds or changes documentation 8.has: changelog 8.has: module (update) This PR changes an existing module in `nixos/` labels Dec 18, 2023
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you split cd362006aced83af980799675184b668ac7cdb04 into two; one that refactors and one that does the actual change? It's hard to tell what it does.

The part pertaining the firewall tool looks pretty good to me though. I can't really review the nftables firewall change; if you want this merged quickly, do those in a separate PR.

nixos/modules/services/networking/firewall-nftables.nix Outdated Show resolved Hide resolved
nixos/modules/services/networking/firewall-nftables.nix Outdated Show resolved Hide resolved
Comment on lines +9 to +10
backend = if config.networking.nftables.enable then "nftables" else "iptables";

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you make the iptables backend selection also explicit? There might be more than two firewall implementations in the future and I'd rather throw here than install the iptables firewall tool in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, we only have a boolean option networking.nftables.enable for firewall backends. Do you mean we add a new option to set the backend?

pkgs/by-name/ni/nixos-firewall-tool/package.nix Outdated Show resolved Hide resolved
nixos/tests/firewall.nix Show resolved Hide resolved
@duament duament force-pushed the nixos-firewall-nftables-named-sets branch from 1e4ad2c to 6d5402d Compare December 18, 2023 07:02
@duament
Copy link
Contributor Author

duament commented Dec 18, 2023

The part pertaining the firewall tool looks pretty good to me though. I can't really review the nftables firewall change; if you want this merged quickly, do those in a separate PR.

Thanks for your quick review. Unfortunately the firewall tool change depends on the nftables firewall module change. It would be better not to separate them.

@clerie
Copy link
Member

clerie commented Dec 19, 2023

Hi @duament, thank you for figuring out a solution to make the nixos-firewall-tool available for nftables.

I agree with #275126 (review) to split out the required changes in the firewall module to the changes in the nixos-firewall-tool. The changes in the nftables module look quite sensible without nixos-firewall-tool too and I would like give people the options to discuss this separately. We can merge both PRs one after another later.

Regarding your change at nixos-firewall-tool I'm not sure yet if it is a good idea to maintain two similarly looking programs as two different code bases.

Future changes in script can break CLI command compatibility with the other. Is this even a requirement?

Passing the backend as a package option causes nix-shell or nix shell commands or using it in ${pkgs.nixos-firewall-tool} to always bring up the iptables version. This can lead to problems when integrating it into scripts for example. Do we care about this or should users rely on having the script in their path?

@@ -79,6 +79,8 @@ The pre-existing [services.ankisyncd](#opt-services.ankisyncd.enable) has been m

- The `hardware.pulseaudio` module now sets permission of pulse user home directory to 755 when running in "systemWide" mode. It fixes [issue 114399](https://github.com/NixOS/nixpkgs/issues/114399).

- `nixos-firewall-tool` now supports nftables and is installed by default when NixOS firewall enables.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- `nixos-firewall-tool` now supports nftables and is installed by default when NixOS firewall enables.
- `nixos-firewall-tool` now supports nftables and is installed by default when NixOS firewall is enabled.


let

inherit (lib) mkIf mkOption types mdDoc optionalString;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do you not inherit concatStringsSep, mapAttrsToList, ... here too?

@duament duament force-pushed the nixos-firewall-nftables-named-sets branch from 6d5402d to 8da2338 Compare December 19, 2023 09:57
@duament duament marked this pull request as draft December 19, 2023 10:00
@duament
Copy link
Contributor Author

duament commented Dec 19, 2023

I agree with #275126 (review) to split out the required changes in the firewall module to the changes in the nixos-firewall-tool. The changes in the nftables module look quite sensible without nixos-firewall-tool too and I would like give people the options to discuss this separately. We can merge both PRs one after another later.

You're right. I created #275371 for the firewall module change.

Regarding your change at nixos-firewall-tool I'm not sure yet if it is a good idea to maintain two similarly looking programs as two different code bases.

Future changes in script can break CLI command compatibility with the other. Is this even a requirement?

Passing the backend as a package option causes nix-shell or nix shell commands or using it in ${pkgs.nixos-firewall-tool} to always bring up the iptables version. This can lead to problems when integrating it into scripts for example. Do we care about this or should users rely on having the script in their path?

I guess we can support both backends in one script and determine which one to use at runtime. Just check if iptables or nft command is in PATH? In this way, we don't need runtimeInputs any more. It also avoid the version mismatch problem.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 20, 2024
nft list table inet nixos-fw
;;
"reset")
systemctl reload nftables.service
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reloading nftables for undoing port changes is a bit excessive, see: #286584.

Instead of the current approach, I would suggest add a set of type inet_proto . inet_service, call it something like tool-ports, and let this command do a single flush of this set.

@Janik-Haag Janik-Haag requested review from Atemu and NetaliDev April 12, 2024 19:39
@Atemu Atemu removed their request for review April 13, 2024 20:28
@nbdd0121
Copy link
Contributor

nbdd0121 commented Jul 3, 2024

@duament do you still intend to proceed with this PR? I can work on this if you don't have time.

@duament
Copy link
Contributor Author

duament commented Jul 3, 2024

@duament do you still intend to proceed with this PR? I can work on this if you don't have time.

Feel free to continue this pr.

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 1-10 10.rebuild-linux: 1-10
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants