[BUG] nftables state module not idempotent

[lkubb]

Description nftables.append keeps appending rules that have connstate set.

Edit: This also applies for if/of with a different cause (nft list does not include meta before iifname/oifname).

Edit2: This also applies for jump: dnat with to-destination set (nft list prints dnat to <target>, not dnat <target>).

Setup Salt minion on Debian 11 VM

Steps to Reproduce the behavior

testtable:
  nftables.table_present: []

testchain:
  nftables.chain_present:
    - table: testtable

This is not idempotent:
  nftables.append:
    - table: testtable
    - chain: testchain
    - jump: accept
    - connstate: established,related

Expected behavior Be idempotent.

Screenshots

$ nft list ruleset
$ salt-call state.apply nft_idem
$ nft list ruleset
table ip testtable {
    chain testchain {
        ct state { established, related } accept
    }
}
$ salt-call state.apply nft_idem
$ nft list ruleset
table ip testtable {
    chain testchain {
        ct state { established, related } accept
        ct state { established, related } accept
    }
}

Versions Report

salt --versions-report

(Provided by running salt --versions-report. Please also mention any differences in master/minion versions.) ```yaml Salt Version: Salt: 3004.2 Dependency Versions: cffi: Not Installed cherrypy: Not Installed dateutil: 2.8.1 docker-py: Not Installed gitdb: Not Installed gitpython: Not Installed Jinja2: 2.11.3 libgit2: Not Installed M2Crypto: Not Installed Mako: Not Installed msgpack: 1.0.0 msgpack-pure: Not Installed mysql-python: Not Installed pycparser: Not Installed pycrypto: Not Installed pycryptodome: 3.9.7 pygit2: Not Installed Python: 3.9.2 (default, Feb 28 2021, 17:03:44) python-gnupg: Not Installed PyYAML: 5.3.1 PyZMQ: 20.0.0 smmap: Not Installed timelib: Not Installed Tornado: 4.5.3 ZMQ: 4.3.4 System Versions: dist: debian 11 bullseye locale: utf-8 machine: x86_64 release: 5.10.0-13-amd64 system: Linux version: Debian GNU/Linux 11 bullseye ```

Additional context This behavior is caused by nftables.check execution module not recognizing the rule correctly. It lists the rules with the --numeric switch, which translates the connection states to hex values, but searches for them using their string representation.

Example output:

$ nft --handle --numeric --numeric --numeric list chain ip testtable testchain # why 3x numeric btw? copied from source
table ip testtable {
    chain testchain { # handle 1
        ct state { 0x2, 0x4 } accept # handle 3
        ct state { 0x2, 0x4 } accept # handle 5
    }
}

Workaround possible by specifying the values in hex (which should break the analogy to iptables afaik):

testtable:
  nftables.table_present: []

testchain:
  nftables.chain_present:
    - table: testtable

This is idempotent:
  nftables.append:
    - table: testtable
    - chain: testchain
    - jump: accept
    - connstate: '0x2, 0x4'

[mlalpho]

Thank you for reporting. I noticed the hex weirdness in addition to https://github.com/saltstack/salt/pull/61975. following 👀

[redbaron4]

I ran into this today.

This happens because modules/nftables.py in check() uses --numeric argument to nft which converts many things (including connstate) to numeric.

However, removing the --numeric arguments to nft command does not solve this issue because when salt builds the command it encapsulates connstate and port in {}, e.g. { new } or {established,related}, but when salt runs nft to print the rules (so that it can search for exact match), the nftables removes the {} for single connstate or port.

Consider a state element

allow_dns_udp:
  nftables.append:
    - save: True
    - table: filter
    - chain: OUTPUT
    - jump: accept
    - match: state
    - connstate: new
    - proto: udp
    - dport: 53
    - destination: 10.1.1.1

Thus if salt built the following rule

ct state { new } ip daddr 10.1.1.1 udp dport { 53 } accept

when it echoes the existing rules (without --numeric), it gets

ct state new ip daddr 10.1.1.1 udp dport 53 accept

Hence it can never match the rule exactly. The rule matching logic needs to probably use regular expressions for this issue to be fixed.

[jdelic]

To add the one thing missing in here from the duplicate issue I had filed: Due to this strange check for string booleaness:

if full in ["True", "true"]:

It's almost certain that nftables.append in line 261 will not get the string it expects.

[jdelic]

I tried a few ways to solve this problem last night and I believe the correct way to do this is to abandon the rule matching that's currently implemented in salt.modules.nftables and instead rely on the JSON renderer (e.g. nft -j list ruleset) as that appears to produce a reliable representation of any rule.

Of course, nftables being nftables, there is no way to use nft's parser and renderer to convert an incoming new rule into JSON. nft does have the -e "echo" parameter which will output JSON, but it doesn't have a no-op "dry-run" parameter.

For example nft -ej add rule filter input tcp dport { 22 } accept will output:

{
  "nftables": [
    {
      "add": {
        "rule": {
          "family": "ip6",
          "table": "filter",
          "chain": "input",
          "handle": 18,
          "expr": [
            {
              "match": {
                "op": "==",
                "left": {
                  "payload": {
                    "protocol": "tcp",
                    "field": "dport"
                  }
                },
                "right": 22
              }
            },
            {
              "accept": null
            }
          ]
        }
      }
    }
  ]
}

So my current theory is that if we can create the above rendering of a rule reliably, then checking for existing rules can be done reliably. The current implementation is otherwise easily derailed, even when you remove the --numeric mismatch explained above. For example, already a simple typo like:

my-rule:
    nftables.append:
        - table: filter
        - family: inet
        - chain: output
        - jump: accept
        - proto: icmp
        - icmp-type: echo-reply,  destination-unreachable
        #                       ^^ mind the extra space
...

will never match in the current system. An alternative approach to solving this problem may be to sanitize all comma-separated lists in a rule to ensure that they have a single space etc. and are only wrapped in {} when they're longer than one element, but that feels brittle to me.

[lkubb]

Side note: There's also the python-nftables utility (https://github.com/aborrero/python-nftables-tutorial), but it does not help with abstracting away the difficulties in the above approach.

A couple of months ago I started implementing a utility for doing the above, first with dataclasses representing the different entities of the scheme (I took https://github.com/mullvad/nftnl-rs / https://github.com/alexforster/nftables-json as a reference). Afair the mentioned issue (conversion between a rule def to its json implementation) caused me to stop it because it was too much work.

Edit: My workaround for now is to render the complete configuration: https://github.com/lkubb/salt-nftables-formula/blob/master/nftables/files/default/nftables.conf.j2