Skip to content

Conversation

pr2502
Copy link

@pr2502 pr2502 commented Aug 10, 2025

hi, i'm not sure what the contribution flow is for core nixos module changes like this, CONTRIBUTING.md seems to be concerned primarily with package changes. i'm not sure if this deserves release note mention as it's a pure addition? should there be a test for this change?

this change introduces option networking.firewall.extraInputRulesEarly for the NFT based firewall configuration. the purpose of this option is to allow adding extra drop rules into the input chain while using the existing networking.firewall.allowed{TCP,UDP}Port{,Range}s. this is necessary because the accept rules generated by allowedPorts are terminal and rules added via the existing networking.firewall.extraInputRules cannot affect packet matching them as they appear after those accept rules.

personally i'm using the option to ban IP ranges which engage in inconsiderate scraping. i've been using this patch for about 10 months and think it's useful enough addition to nixos to be worth making a PR for.

Things done

  • Built on platform:
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • Tested, as applicable:
  • Ran nixpkgs-review on this PR. See nixpkgs-review usage.
  • Tested basic functionality of all binary files, usually in ./result/bin/.
  • Nixpkgs Release Notes
    • Package update: when the change is major or breaking.
  • NixOS Release Notes
    • Module addition: when adding a new NixOS module.
    • Module update: when the change is significant.
  • Fits CONTRIBUTING.md, pkgs/README.md, maintainers/README.md and other READMEs.

Add a 👍 reaction to pull requests you find important.

@nixpkgs-ci nixpkgs-ci bot added 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 12.first-time contribution This PR is the author's first one; please be gentle! 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 9.needs: reviewer This PR currently has no reviewers requested and needs attention. labels Aug 10, 2025
@nixpkgs-ci nixpkgs-ci bot removed the 9.needs: reviewer This PR currently has no reviewers requested and needs attention. label Aug 17, 2025
Copy link
Member

@MarcelCoding MarcelCoding left a comment

Choose a reason for hiding this comment

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

Seems fine to me. I've just wondered that we only assert that no iptables commands are configured when nftables is used, but not the other way around?

@nbdd0121
Copy link
Member

I feel that this could be better served with the mkOrder mechanism?

@MarcelCoding
Copy link
Member

MarcelCoding commented Aug 17, 2025

Please describe how you would image this be done? The chain input-allow chain contains as an expression the generation of allowed{TCP,UDP}Port{,Range}s, so there is no earlier injection point prior to that, not even a plain multiline string which you could modify with mkBefore or mkOrder. One could move the current body of the input-allow chain to extraInputRules, but then the final ordering would probably not be obvious? But then such rules could just be added using mkBefore.

@nbdd0121
Copy link
Member

You would have these default rules being added to the extraRules with a small order. For example, security.sudo.extraRules use this apporach:

security.sudo.extraRules =
let
defaultRule =
{
users ? [ ],
groups ? [ ],
opts ? [ ],
}:
[
{
inherit users groups;
commands = [
{
command = "ALL";
options = opts ++ cfg.defaultOptions;
}
];
}
];
in
lib.mkMerge [
# This is ordered before users' `mkBefore` rules,
# so as not to introduce unexpected changes.
(lib.mkOrder 400 (defaultRule {
users = [ "root" ];
}))
# This is ordered to show before (most) other rules, but
# late-enough for a user to `mkBefore` it.
(lib.mkOrder 600 (defaultRule {
groups = [ "wheel" ];
opts = (lib.optional (!cfg.wheelNeedsPassword) "NOPASSWD");
}))
];

which is introduced in 23.11:
- `security.sudo.extraRules` includes `root`'s default rule now, with ordering
priority 400. This is functionally identical for users not specifying rule
order, or relying on `mkBefore` and `mkAfter`, but may impact users calling
`mkOrder n` with n ≤ 400.

@nixpkgs-ci nixpkgs-ci bot added the 12.approvals: 1 This PR was reviewed and approved by one person. label Aug 17, 2025
@MarcelCoding
Copy link
Member

How would one prevent this from getting messy when we have to deal with the ordering numbers? In the sudo case, the is probably not the intention for users to add rules to the beginning?

@nbdd0121
Copy link
Member

I'm not sure what you mean by "getting messy"? For user or for the module? In the module, it should just be a single extraInputRules = mkOrder 400 [ all existing default rules ]; and then just use cfg.extraInputRules to generate the chain, and users can use mkOrder 300 [ their rules ] to inject before the default rules?

Commit message of sudo module does mention injecting rules being one of the use case for the change: 93011e3

@MarcelCoding
Copy link
Member

MarcelCoding commented Aug 17, 2025

With messy I meant the mkOrder 300 [ their rules ](the consumers of the module) thing, but if it's fine for others I am fine with it ;)

@nbdd0121
Copy link
Member

BTW: to achieve the desired outcome in the motivation, I would actually suggest to create a different table to handle this. You can just register mutliple hooks in different tables, and nftables will traverse all of them, and any hook can decide to drop the packet.

I would recommend just to do

networking.nftables.tables.ip_drop = {
  family = "inet";
  content = ''
    set blocked-ip4 {
      typeof ip saddr
      flags interval
      auto-merge
    }
    chain input {
      type filter hook input priority filter; policy accept;

      ip saddr @blocked-ip4 counter drop
    }
  '';
};

Now you also have the flexibility of being able to dynamically modify the set.

… rules

this change introduces option `networking.firewall.extraInputRulesEarly`
for the NFT based firewall configuration. the purpose of this function is
to allow adding `drop` rules into the input chain while using the existing
`networking.firewall.allowed{TCP,UDP}Port{,Range}s`. this is necessary
because the `accept` rules generated but allowedPorts are terminal and rules
added via the existing `networking.firewall.extraInputRules` cannot affect
packets matching them.
@pr2502 pr2502 force-pushed the nft-early-input-rules branch from 7dda3e4 to 60074b6 Compare August 17, 2025 16:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: module (update) This PR changes an existing module in `nixos/` 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 12.approvals: 1 This PR was reviewed and approved by one person. 12.first-time contribution This PR is the author's first one; please be gentle!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants