Salt: Invert iptables rules no longer work after 2017.7.0 upgrade

[andrewmiskell]

Invert iptables rules appear not to be working anymore after upgrading to 2017.7.0 from 2016.x

My iptables rule in salt stack is:

unmatched packets:
  iptables.append:
    - table: filter
    - chain: INPUT
    - jump: DROP
    - match:
      - tcp
      - state
    - tcp-flags: "! FIN,SYN,RST,ACK SYN"
    - proto: tcp
    - connstate: NEW
    - save: True

When it tries to apply the iptables rule, it fails with:

          ID: unmatched packets
    Function: iptables.append
      Result: False
     Comment: Failed to set iptables rule for unmatched packets.
              Attempted rule was /sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW ! --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP for ipv4
     Started: 16:24:30.614774
    Duration: 48.855 ms
     Changes:

The generated rule should look like:

/sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW ! --tcp-flags FIN,SYN,RST,ACK SYN --jump DROP

or

/sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP

although the first one is the more acceptable rules as the second one is technically deprecated syntax.

Both systems are running CentOS 6.x (although I see the same issues on CentOS 7.x) and all systems are running the following salt version.
salt 2017.7.0 (Nitrogen)

[Ch3LL]

@andrewmiskell which version was this previously working? I'm seeing this behavior in 2016.11.6 as well

[andrewmiskell]

I believe we were running 2016.11.3 at the time on both master and minion.

[andrewmiskell]

Here's the output from a highstate on a newly installed 6.x server with the latest minion.

[ERROR   ] Command '/sbin/iptables  -t filter -A INPUT -p tcp -m tcp -m state --state NEW ! --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP' failed with return code: 2
[ERROR   ] output: Using intrapositioned negation (`--option ! this`) is deprecated in favor of extrapositioned (`! --option this`).
iptables v1.4.7: Multiple `!' flags not allowed
Try `iptables -h' or 'iptables --help' for more information.
[ERROR   ] Failed to set iptables rule for unmatched packets.

and a newly installed 7.x server with the latest minion.

[ERROR   ] Command '/sbin/iptables --wait -t filter -A INPUT -p tcp -m tcp -m state --state NEW ! --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP' failed with return code: 2
[ERROR   ] output: iptables v1.4.21: Unknown TCP flag `!'
Try `iptables -h' or 'iptables --help' for more information.
[ERROR   ] Failed to set iptables rule for unmatched packets.
Attempted rule was /sbin/iptables --wait -t filter -A INPUT  -p tcp -m tcp -m state --state NEW ! --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP for ipv4

[andrewmiskell]

I just tested with 2016.11.5 and it worked fine, then I upgraded to 2016.11.6 and it worked. Upgraded from 2016.11.5 to 2017.7.0 and it failed. So it definitely appears to be an issue on 2017.7.0.

[root@centos6-saltstack ~]# salt-minion --version
salt-minion 2016.11.5 (Carbon)

----------
          ID: unmatched packets
    Function: iptables.append
      Result: True
     Comment: Set and saved iptables rule unmatched packets for ipv4
              /sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW !  --tcp-flags FIN,SYN,RST,ACK SYN --jump DROP
              Wrote 1 lines to "/etc/sysconfig/iptables"
     Started: 09:18:20.681988
    Duration: 20.575 ms
     Changes:   
              ----------
              locale:
                  unmatched packets
----------

[root@centos6-saltstack ~]# salt-minion --version
salt-minion 2016.11.6 (Carbon)

----------
          ID: unmatched packets
    Function: iptables.append
      Result: True
     Comment: Set and saved iptables rule unmatched packets for ipv4
              /sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW !  --tcp-flags FIN,SYN,RST,ACK SYN --jump DROP
              Wrote 1 lines to "/etc/sysconfig/iptables"
     Started: 07:14:08.500103
    Duration: 21.222 ms
     Changes:   
              ----------
              locale:
                  unmatched packets
----------

[root@centos6-saltstack ~]# salt-minion --version
salt-minion 2017.7.0 (Nitrogen)

----------
          ID: unmatched packets
    Function: iptables.append
      Result: False
     Comment: Failed to set iptables rule for unmatched packets.
              Attempted rule was /sbin/iptables  -t filter -A INPUT  -p tcp -m tcp -m state --state NEW ! --tcp-flags ! FIN,SYN,RST,ACK SYN --jump DROP for ipv4
     Started: 07:14:13.840309
    Duration: 20.257 ms
     Changes:   
----------

[jdelic]

I just upgraded some boxes from the version of Salt in the Debian Stretch main repos to the official repo.saltstack.com Stretch version 2017.7.0. I see the same problem. Incidentally I run a pretty similar rule.

# prevent tcp packets without a connection
drop-confused-tcp-packets:
    iptables.insert:
        - position: 3
        - table: filter
        - chain: INPUT
        - jump: DROP
        - proto: tcp
        - match: state
        - connstate: NEW
        - tcp-flags: '! FIN,SYN,RST,ACK SYN'
        - order: 3
        - save: True
        - require:
            - pkg: iptables

This leads to the same error mentioned by @andrewmiskell above. @Ch3LL I can definitely confirm that the above rule worked with 2016.11.3.

[jdelic]

Ok, I did some digging. The bug was introduced in commit 7c6ff77c988 by @svenauhagen. Specifically here in salt/modules/iptables.py:

@@ -413,12 +493,11 @@ def build_rule(table='filter', chain=None, command=None, position='', full=None,
                 after_jump.append('--{0} {1}'.format(after_jump_argument, value))
             del kwargs[after_jump_argument]

-    for item in kwargs:
-        rule.append(maybe_add_negation(item))
-        if len(item) == 1:
-            rule.append('-{0} {1}'.format(item, kwargs[item]))
-        else:
-            rule.append('--{0} {1}'.format(item, kwargs[item]))
+    for key, value in kwargs.items():
+        negation = maybe_add_negation(key)
+        flag = '-' if len(key) == 1 else '--'
+        value = '' if value in (None, '') else ' {0}'.format(value)
+        rule.append('{0}{1}{2}{3}'.format(negation, flag, key, value))

From a quick reading of the code:

• maybe_add_negation has side effects on the **kwargs dictionary of build_rule. This is documented in the function, but really bad design.
• Sven's patch, unlike the original code, does not reference kwargs[item], but value, which is untouched by the side-effects of maybe_add_negation and
• therefore still contains the additional ! which breaks the rules @andrewmiskell and I are using (and probably most other negations in iptables rules).

Quick fix: Change the code above to reference kwargs[key], or better yet, remove the key, value unpacking completely, go back to for item in and add a comment that makes it clear why the value always needs to be read from the kwargs dictionary. Write a regression test.

Real fix: Refactor this code to remove the icky side-effect from maybe_add_negation.

Unfortunately I don't have a salt development/test environment anymore, so I won't be able provide a PR right now.

(@Ch3ll: I guess this bug is now confirmed)

[svenauhagen]

Hi,

my original commit did not have that code and the fix is most likely to revert back to the original fix I submitted:
https://github.com/saltstack/salt/pull/37315/commits/73f8dedc1a66b9b56ef344d43a9120e0a4374903

This part was added per request on the discussion https://github.com/saltstack/salt/pull/37315.
Do you want me to resubmit and change it back to the original code?

Thanks for notifying me on this.

Best
Sven

[falzm]

Hi

I confirm having the same issue after upgrading from 2016.11.5 to 2017.7.0. The following rule settings:

   ...
    - iptables_nat_to_out:
        append:
        - comment: Masquerade local traffic to internet
        - table: nat
        - chain: POSTROUTING
        - jump: MASQUERADE
        - source: {{ base_subnet }}
        - destination: '!10.0.0.0/8'
        - out-interface: eth0
        - save: true

...generate an error at execution:

local:
----------
          ID: iptables_iptables_nat_to_out
    Function: iptables.append
      Result: False
     Comment: Failed to set iptables rule for iptables_iptables_nat_to_out.
              Attempted rule was /sbin/iptables --wait -t nat -A POSTROUTING  -m comment --comment "Masquerade local traffic to internet" --out-interface eth0 ! --destination !10.0.0.0/8 --source 10.0.0.0/16 --jump MASQUERADE for ipv4
     Started: 14:07:31.238730
    Duration: 468.946 ms
     Changes:

Version info:

Salt Version:
           Salt: 2017.7.0

Dependency Versions:
           cffi: 0.8.6
       cherrypy: Not Installed
       dateutil: 2.2
      docker-py: Not Installed
          gitdb: Not Installed
      gitpython: Not Installed
          ioflo: Not Installed
         Jinja2: 2.9.4
        libgit2: Not Installed
        libnacl: Not Installed
       M2Crypto: Not Installed
           Mako: Not Installed
   msgpack-pure: Not Installed
 msgpack-python: 0.4.2
   mysql-python: Not Installed
      pycparser: 2.10
       pycrypto: 2.6.1
   pycryptodome: Not Installed
         pygit2: Not Installed
         Python: 2.7.9 (default, Jun 29 2016, 13:08:31)
   python-gnupg: Not Installed
         PyYAML: 3.11
          PyZMQ: 14.4.0
           RAET: Not Installed
          smmap: Not Installed
        timelib: Not Installed
        Tornado: 4.2.1
            ZMQ: 4.0.5

System Versions:
           dist: debian 8.9
         locale: UTF-8
        machine: x86_64
        release: 3.16.0-4-amd64
         system: Linux
        version: debian 8.9

[vbatoufflet]

Hi @svenauhagen,

Is there any way to switch back the old behavior? At the moment, states that were valid before the upgrade are broken in the 2017.7 release.

[svenauhagen]

Hi @vbatoufflet

I can resubmit my original code.
Let me get on that and I will send a PR.

[svenauhagen]

PR is #42987

[vbatoufflet]

Great. Thanks for your work on this.

[thusoy]

This is also fixed by the PR I sent yesterday at #42988.

[thusoy]

@Ch3LL this can be closed now since #42988 has been merged.