Separate nft logging based on rule/policy action

[b333z]

Prior to this change, if --log was enabled in --allow mode, log statements would be added to the accept rules, logging traffic that was accepted rather than blocked.

This change inverts the logging in --allow mode by disabling the logging statement on the accept rules and appending a final drop rule to attach logging to, in order to log any traffic that makes it through the chain without a match.

If --counter was specified that is also attached to the final drop rule (however the counters on the accept rules have been retained).

[b333z]

Thanks for publishing this work @rpthms

[rpthms]

Hi @b333z

Thanks a lot for the pull request. While I agree in some cases it might make more sense to log denied packets than the allowed ones when using the --allow mode, there may be some use cases where people don't care about the denied packets and just want to log the allowed packets. I was thinking it might make more sense to add an extra flag to invert the logging behaviour when using the --allow mode. That way everyone can choose which packets should get logged when using the allow mode.

What do you say?

[rpthms]

Or maybe, we could just replace the --log flag with 2 new flags, --log-allow and --log-deny and you'll be able to choose which packets to log regardless of whether you're using the allow mode or not. Does that make sense?

[b333z]

I don't have a strong opinion either way, my ideal mode of operation would be to have some predefined empty sets in my nftables.conf that I'd put say a 24h timeout parameter on, referenced by some handwritten rules in and than just have ./nft-geo-filter load the sets every 12h or so, eg:

./nft-geo-filter --nft-binary "$(which nft)" --table-family inet --table filter --set-ipv4 filter-v4 --set-ipv6 filter-v6  MC

Which would run something like (and just let the removed entries age out from the set timeout):

nft add element inet filter filter-v4 { <ip41>, .. , <ip4N> }`
nft add element inet filter filter-v6 { <ip61>, .. , <ip6N> }`

As I'm happiest expressing the details via nftables.conf, but I get that the goal here is to have something more opinionated that sits under whatever rules you have, does it all for you but leaves the obvious things configurable which is great.

So I'd be up for adjusting this however you like if helpful to yourself or others, time permitting.

With what you proposed, to clarify your taking about the rules action, so --log-allow would cause all rules with an accept action have logging, and --log-deny would do the same for all rules with a drop action?

Would --log-allow and --log-deny start to get confused with --allow mode? Would it be better to go with --log-accept & --log-drop to make that clearer?

What about the prefixes, would the prefix flags be added to match, eg: --log-accept-prefix & --log-drop-prefix?

[rpthms]

With what you proposed, to clarify your taking about the rules action, so --log-allow would cause all rules with an accept action have logging, and --log-deny would do the same for all rules with a drop action?

Would --log-allow and --log-deny start to get confused with --allow mode? Would it be better to go with --log-accept & --log-drop to make that clearer?

--log-accept and --log-drop is a much better choice. You're also correct about my intention with the separate log flags. In short:

--log-accept:

• Add log statements to the accept rules when using the allow mode.
• Add a final log accept rule when NOT using the allow mode.

--log-drop:

• Add log statements to the drop rules when NOT using the allow mode.
• Add a final log drop rule when using the allow mode.

Separate prefixes would also be necessary in cases where we use both --log-accept and --log-drop at the same time, so yeah lets go with --log-accept-prefix and --log-drop-prefix.

Once again, thanks for your input @b333z

BTW: If you just want a script that updates nftables sets without adding any rules, the previous version of my script (called nft-blacklist at that time) did just that. So, you may want to take a look at that (https://github.com/rpthms/nft-geo-filter/tree/9dcd5c71ac3efbe1af6631de8a9e656f21fcd5f2). You would need to create an empty set in your nftables config and then run the script to populate the IPs in that set.

[b333z]

Those changes should be right to go @rpthms, I also split --log-level in the end as that seemed to make sense, I held off squashing till you've had a chance to review, thanks for the link, much appreciated.

[b333z]

Thanks for you review & suggestions @rpthms! I've done a squash

[rpthms]

Thanks for your PR @b333z. Really appreciate it.