Support ranges in nftables

[sbs2001]

Based on #84

[jarppiko]

Hello @sbs2001 this PR is in "changes requested" state. Are you still working on this?

[g00g1]

I am interested in intercepting this PR to implement this feature, however there is another PR pending which introduces breaking changes to cs-firewall-bouncer's nftables integration.

UPD: Based upon my another PR, I have implemented nftables' range feature here

[jarppiko]

@g00g1, I guess the best way forward is for you to open a new PR to the project? As a Crowdsec + nftables, I warmly welcome your contribution :-)

[gilbsgilbs]

I attempted to rebase this PR but found that it isn't sufficient due to overlaps, as nftables does not automatically merge overlapping ranges for you. So this would lead to errors when creating overlapping decisions in CrowdSec. The auto-merge feature in nftables may be able to merge elements "gracefully", but it complicates later deletions of specific rules, and still wouldn't work well with timeouts. Additionally, I couldn't get it to work with the Go nftables library (might be a bug or a skill issue on my part).

Currently, the implementation on master handles IP conflicts (i.e. overlaps) in a simplistic and incorrect manner: the first decision added to the set takes precedence. This approach is problematic because:

• Decisions may have different durations; ideally, we should apply the longest duration from conflicting decisions.
• If either decision is deleted (either due to expiration or manual removal), we should retain the rule in nftables and update the timeout to reflect the new maximum value.

To easily reproduce this bug in the current master, you can add two decisions with the same IP but different durations:

$ cscli decision delete --ip 10.1.2.42 --duration 1m
$ sleep 10
$ cscli decision delete --ip 10.1.2.42 --duration 1h

The bouncer will add the first IP to the set, then refuse to add it a second time (because it detects it's already applied in the set). Then the element will either be deleted when crowdsec notifies of the expiration of the first decision, or when nftables expires it automatically. At this point 10.1.2.42 is still banned in crowdsec, but the bouncer incorrectly removed the decision from the firewall.

(I suspect that iptables and potentially other implementations from this bouncer have similar issues.)

While this issue may seem minor with individual IPs, it starts to collapse with ranges. For example, having a decision for "10.1.2.0/24" alongside "10.1.2.42/32" doesn't seem completely unreasonable or "edge-case"-y. You don't want errors in such case, and you certainly don't want to ban only one IP when you also banned a whole range, or expire an IP when its whole range is not unbanned yet. So we somehow need to maintain a reflection of CrowdSec's state, and recalculate a non-overlapping set of rules accordingly for nftables.

Another concern with the current nftables implementation (that is remotely related to this issue) is its race condition when deleting decisions. It first updates a local "ban state" from the nftables set, and then deletes the element only if it exists in this state. If the rule expires between these two operations, an error will occur. This can't be resolved by simply checking for errors.Is(err, fs.ErrNotExist) when deleting because the error only happens at Flush time. So you don't actually know which element is causing the error.

---

TL;DR: I believe the proper way to implement ranges is to maintain a local state of all current decisions in CrowdSec (including overlapping ones). When adding or removing a decision, we should compute a diff of changes to apply to nftables, ensuring we keep only the highest timeout values and fix any overlap. However, this diff may not always apply correctly due to race conditions on expirations, so we need a strategy to manage such inconsistencies (though at this point, flushing the entire set and recreating it from scratch may be the simplest solution, as this situation shouldn't occur frequently anyway).

I might take a shot at implementing this because it seems interesting, but I'm concerned that the added complexity may prevent it from being accepted.