nftables configuration design issues

[pdf]

In #111 a new configuration structure was introduced to supprt custom table/chain names, however I think this has some deficiencies, though these probably stem somewhat from the initial design.

I am investigating adding support for filter hooks other than input (primarily forward), however in attempting to do so I find that this is incompatible with the current structure, since adding support for multiple hooks requires adding multiple chains. The current structure:

nftables:
  ipv4:
    enabled: true
    blacklist: crowdsec-blacklist
    set-only: false
    table: crowdsec
    chain: crowdsec-chain
  ipv6:
    enabled: false
    blacklist: crowdsec6-blacklist
    set-only: false
    table: crowdsec6
    chain: crowdsec6-chain

is clearly predicated on operating on a single chain. My initial thought was to add a hooks as an array of strings, however this would require chain to be treated as a base name (ie for hooks: [input, filter], the chains might be crowdsec6-chain-input / crowdsec6-chain-forward), and I suspect this is undesirable.

For my use-case I thought I might be able to make use of the new set-only functionality, however this surfaces another issue: in my firewall deployments I implement blocklists on both netdev (for early filtering) and inet (rather than separate ip/ip6, to reduce duplication) address families (it's conceivable that users may also want to block on bridge), however the configuration structure does not allow specifying the family, nor does it allow specifying multiple tables/chains.

This leads me to believe we probably need an additional level of nesting, to allow enough flexibility to support all desired behaviour. I'd propose something like:

nftables:
  ipv4:
    enabled: true
    targets:
    - blacklist: crowdsec-blacklist
      set-only: false
      table: crowdsec
      chain: crowdsec-chain
      family: ip # TODO: new functionality
      hook: input # TODO: new functionality
  ipv6:
    enabled: false
    targets:
    - blacklist: crowdsec6-blacklist
      set-only: false
      table: crowdsec6
      chain: crowdsec6-chain
      family: ip6  # TODO: new functionality
      hook: input # TODO: new functionality

Then multiple blocklists in set-only mode might look like:

nftables:
  ipv4:
    enabled: true
    targets:
    - blacklist: crowdsec-blacklist-netdev
      set-only: true
      table: crowdsec-netdev
      family: netdev # TODO: new functionality
    - blacklist: crowdsec-blacklist-filter
      set-only: true
      table: crowdsec-inet
      family: inet # TODO: new functionality

And multiple hooks might look like:

nftables:
  ipv4:
    enabled: true
    targets:
    - blacklist: crowdsec-blacklist
      set-only: false
      table: crowdsec
      chain: crowdsec-input
      family: ip # TODO: new functionality
      hook: input # TODO: new functionality
    - blacklist: crowdsec-blacklist
      set-only: false
      table: crowdsec
      chain: crowdsec-forward
      family: ip # TODO: new functionality
      hook: forward # TODO: new functionality

This will introduce some complexity, as we'll need to dedup set operations based on family+table+blacklist, but this should be relatively easy to resolve. It also may require some additional config validation to avoid some obvious footguns for users.

I'd certainly be open to alternative proposals, if anyone has better ideas.

I realise there's a bit to digest here, but it would be ideal if we could examine this before the current config makes it out of pre-release to avoid breaking user configs or having to maintain backwards-compatibility (the latter would be quite ugly).

[buixor]

Hello @pdf !

First of all thanks for the detailed proposal. I think this shows that some of our choices were a bit naive and I overall agree with what you pointed out.

I'll have a look at it w/ the team and let you know, but I overall agree,

[sbs2001]

@pdf This looks much better than our current config. One minor improvement to your proposal IMHO would be to eliminate the extra nesting caused by ipv4 and ipv6 stuff. We can infer this info from the new family field

Something like this:

nftables:
    enabled: true
    targets:
    - blacklist: crowdsec-blacklist
      set-only: false
      table: crowdsec
      chain: crowdsec-input
      family: ip # TODO: new functionality
      hook: input # TODO: new functionality
    - blacklist: crowdsec-blacklist
      set-only: false
      table: crowdsec
      chain: crowdsec-forward
      family: ip # TODO: new functionality
      hook: forward # TODO: new functionality

[pdf]

@pdf This looks much better than our current config. One minor improvement to your proposal IMHO would be to eliminate the extra nesting caused by ipv4 and ipv6 stuff. We can infer this info from the new family field

Not quite - table families like inet and netdev allow mixing both ip and ip6 in their chains, but sets and rule statements are limited to specifying an explicit protocol. We could possibly add two keys, one for the table family and one for the set/rule protocol, not certain if that's better or not.

This is what that might look like for an inet table that supports both ip and ip6:

nftables:
    enabled: true
    targets:
    - blacklist: crowdsec-blacklist4
      set-only: false
      table: crowdsec-inet
      chain: crowdsec-input
      family: inet # TODO: new functionality
      protocol: ip # TODO: new functionality
      hook: input # TODO: new functionality
    - blacklist: crowdsec-blacklist6
      set-only: false
      table: crowdsec-inet
      chain: crowdsec-input
      family: inet # TODO: new functionality
      protocol: ip6 # TODO: new functionality
      hook: input # TODO: new functionality

[sbs2001]

@pdf yes this makes sense. I think your original proposal is better t\and cleaner then.

[AlteredCoder]

Hello @pdf,

We did some tests with your proposal and the current configuration file for the backward compatibility and this will not be an issue. We plan to release the v0.0.23 soon since there is a lot of bugfixes and we will then implement your suggestion in the next release.

Thanks for the great proposal!

[jarppiko]

@pdf @sbs2001 the main idea of #111 was the introduction of set-only parameter. Automatic firewall logics (rules managed by an application) and hooks in more complex firewall setups can get problematic. One system doing its automatic stuff can be made to work, but having multiple apps doing their own "automatic" logic can get difficult to manage.

The set-only parameter allows crowdsec to be used in more advanced setups where the admin defines the nftables structure and uses nftables sets for blocking access by IP. This way the admin can specify exactly where the crowdsec filtering should be applied.

I use crowdsec in set-only mode for forward, output and input hooks and it works great. When crowdsec is turned off, it just empties the set leaving rest of the table intact. But you are correct that the inet table is not supported. I think it was the fact that sets require exact type definition (ipv4_addr, ipv6_addr) - i.e. there is not a shared inet type for sets. This leads to the fact that separate sets are required for ipv4, ipv6. But separating family and protocol in configs is gets around it.

I think your proposal is a good improvement. 👍

[LaurenceJJones]

Wasnt this resolved with #231 ?

EDIT: well kind off