Does my code create correct nftables rules?

For Developers
1

Hi all,

So I have some code which generates geoip rules for nftables. This is basically the nftables-facing part of the geoip project I'm developing.

I'm just getting to know nftables, so I'd like to show you guys some samples of resulting rules and see if you think that they do what they are supposed to do, and if anyone has ideas for improvement.

First, a short description, for the uninitiated.

The code works in either whitelist mode or blacklist mode.
Rules are created based on user-specified parameters.

One of these parameters is defining the device the code runs on as a router or as a host.
(the project aims to support both options and to work on OpenWRT and non-OpenWRT devices)

For a router, rules are applied to connections arriving from the WAN interfaces.
For a host, rules are applied to all incoming connections. For a host, when in whitelist mode, exceptions are added for trusted LAN subnets.

Here are the resulting rules:

router: whitelist

Summary 
table inet geoip-shell {
        set NL_ipv4_2024-02-04_geoip-shell {
                type ipv4_addr
                policy memory
                flags interval
                auto-merge
        }

        set NL_ipv6_2024-02-04_geoip-shell {
                type ipv6_addr
                policy memory
                flags interval
                auto-merge
        }

        chain GEOIP-BASE {
                type filter hook prerouting priority mangle; policy accept;
                jump GEOIP-SHELL comment "geoip-shell_main"
        }

        chain GEOIP-SHELL {
                iifname { "eth1", "eth2" } ip saddr @NL_ipv4_2024-02-04_geoip-shell accept
                iifname { "eth1", "eth2" } ip6 saddr @NL_ipv6_2024-02-04_geoip-shell accept
                iifname { "eth1", "eth2" } ct state established,related accept comment "geoip-shell_aux-rel-est"
                iifname { "eth1", "eth2" } drop comment "geoip-shell_whitelist_block"
        }
}

router: blacklist

Summary 
table inet geoip-shell {
        set NL_ipv4_2024-02-04_geoip-shell {
                type ipv4_addr
                policy memory
                flags interval
                auto-merge
        }

        set NL_ipv6_2024-02-04_geoip-shell {
                type ipv6_addr
                policy memory
                flags interval
                auto-merge
        }

        chain GEOIP-BASE {
                type filter hook prerouting priority mangle; policy accept;
                jump GEOIP-SHELL comment "geoip-shell_main"
        }

        chain GEOIP-SHELL {
                iifname { "eth1", "eth2" } ip saddr @NL_ipv4_2024-02-04_geoip-shell drop
                iifname { "eth1", "eth2" } ip6 saddr @NL_ipv6_2024-02-04_geoip-shell drop
                iifname { "eth1", "eth2" } ct state established,related accept comment "geoip-shell_aux-rel-est"
        }
}

host: whitelist

Summary 
table inet geoip-shell {
        set NL_ipv4_2024-02-04_geoip-shell {
                type ipv4_addr
                policy memory
                flags interval
                auto-merge
        }

        set NL_ipv6_2024-02-04_geoip-shell {
                type ipv6_addr
                policy memory
                flags interval
                auto-merge
        }

        set geoip-shell_lansubnets_ipv4 {
                type ipv4_addr
                flags interval
                auto-merge
        }

        set geoip-shell_lansubnets_ipv6 {
                type ipv6_addr
                flags interval
                auto-merge
        }

        chain GEOIP-BASE {
                type filter hook prerouting priority mangle; policy accept;
                jump GEOIP-SHELL comment "geoip-shell_main"
        }

        chain GEOIP-SHELL {
                iifname "lo" accept comment "geoip-shell_aux-loopback"
                ip saddr @geoip-shell_lansubnets_ipv4 accept comment "geoip-shell_aux-lansubnet"
                ip6 saddr @geoip-shell_lansubnets_ipv6 accept comment "geoip-shell_aux-lansubnet"
                ip saddr @NL_ipv4_2024-02-04_geoip-shell accept
                ip6 saddr @NL_ipv6_2024-02-04_geoip-shell accept
                ct state established,related accept comment "geoip-shell_aux-rel-est"
                drop comment "geoip-shell_whitelist_block"
        }
}

host: blacklist

Summary 
table inet geoip-shell {
        set NL_ipv4_2024-02-04_geoip-shell {
                type ipv4_addr
                policy memory
                flags interval
                auto-merge
        }

        set NL_ipv6_2024-02-04_geoip-shell {
                type ipv6_addr
                policy memory
                flags interval
                auto-merge
        }

        chain GEOIP-BASE {
                type filter hook prerouting priority mangle; policy accept;
                jump GEOIP-SHELL comment "geoip-shell_main"
        }

        chain GEOIP-SHELL {
                ip saddr @NL_ipv4_2024-02-04_geoip-shell drop
                ip6 saddr @NL_ipv6_2024-02-04_geoip-shell drop
                ct state established,related accept comment "geoip-shell_aux-rel-est"
        }
}

Bonus question: which rules do you think is worth having a counter on?

2

I'm much more confident in the rules for hosts, so if someone short of time but willing to help goes by, please focus your valuable attention on the rules for router and tell me whether I'm missing something there.

3

Hi Anton, looks pretty good to me. One thing that jumps out to me is the placement of

    ct state established,related accept

This almost always the first rule after the hook statement in the base chain, as it short circuits all the other checks that have previously decided whether this flow is ok or not. In other words, once you've evaluated that first packet in a flow, you don't need to keep checking all the subsequent packets from the same connection. Do nft list chain inet fw4 input for an example of how jow has the basic firewall structured...

Regarding counters, I just put them everywhere because I'm curious and like to look and see what's going on every now and then. I'm not sure there's much overhead involved in using them, never really dug into it.

1 Like
4

@efahl Thank you for looking into this, and for the useful comment. That rule used to be up top but at some point I moved it, not sure why now. It should be higher up as you suggest. For now, I'll make it the first rule in the chain, so basically only the 'jump' rule precedes it (which should have minimal performance impact). The reason it's not in the base chain is that my code provides an on-off switch so the user can temporarily turn geoip blocking on or off without rebuilding the chain and the sets. This is implemented by simply adding or removing that 'jump' rule. So my logic is that all geoip-related rules should be placed after it. If you have a better idea for on-off switch implementation then I'll be happy to hear it.

5

With something like this? Just having that single jump rule in the base chain indeed does make it about as simple as you can get, no messing around with rule handles and all that nonsense.

# Turn it off.
nft flush chain inet geoip-shell GEOIP-BASE

# Turn it on.
nft add rule inet geoip-shell GEOIP-BASE  jump GEOIP-SHELL comment "geoip-shell_main"
1 Like
6

Good idea and it does make it marginally easier. Not sure I'll implement this because I already have a function which handles the handle nonsense (pun not intended), so I might as well just reuse it for reasons of consistency and readability of the code.

# 1 - chain name
# 2 - current chain contents
# 3... tags list
mk_nft_rm_cmd() {
	chain="$1"; _chain_cont="$2"; shift 2
	[ ! "$chain" ] || [ ! "$*" ] && return 1
	for tag in "$@"; do
		printf '%s\n' "$_chain_cont" | sed -n "/$tag/"'s/^.* # handle/'"delete rule inet $geotable $chain handle"'/p' || return 1
	done
}
1 Like
7

Slightly off-topic: this is a sample status report my code generates (firewall rules and ip ranges count are included in verbose mode). Any other info you think is worth including?

image
image700×475 11.2 KB