Fix #55262 (mixing of stderr and stdout in cmd.run return value leads to errors in iptables/nftables saved rules) (#58573)

* Fixing saltstack/salt#55262 by using cmd correctly

This commit attemts to fix saltstack/salt#55262 by ensuring that output
from stdout and stderr isn't mixed in `iptables.save`. It also changes
other invocations of `cmd.*` by using `cmd.run_stdout` and
`com.run_stderr` respectively to what the iptables function expects.

I found some other issues here, where for example `iptables.new_chain`
returns `True` when successful and `iptables.set_policy` returns `False`
when successful. But that's for a separate patch to fix.

* adding changelog entry

* run pre-commit hooks

* remove unused import

* update iptables.check to use stdout

* update tests

* explicitly use stdout for help output and version checking, too

* linting/blacken test_iptables

* add mocks for run_stdout and run_stderr where necessary

* blacken test_iptables again

* linting test_iptables

* empty commit to trigger PR rebuild to fix spurious error report

Co-authored-by: Megan Wilhite <megan.wilhite@gmail.com>

changelog/55262.fixed

@@ -0,0 +1 @@
+Fixed bug #55262 where `salt.modules.iptables` would call `cmd.run` and receive and interpret interspersed `stdout` and `stderr` output from subprocesses.

salt/modules/iptables.py

@@ -74,7 +74,7 @@ def _has_option(option, family="ipv4"):
         _has_option('--check', family='ipv6')
     """
     cmd = "{} --help".format(_iptables_cmd(family))
-    if option in __salt__["cmd.run"](cmd, output_loglevel="quiet"):
+    if option in __salt__["cmd.run_stdout"](cmd, output_loglevel="quiet"):
         return True
     return False
 
@@ -192,7 +192,7 @@ def version(family="ipv4"):
         salt '*' iptables.version family=ipv6
     """
     cmd = "{} --version".format(_iptables_cmd(family))
-    out = __salt__["cmd.run"](cmd).split()
+    out = __salt__["cmd.run_stdout"](cmd).split()
     return out[1]
 
 
@@ -678,7 +678,7 @@ def set_policy(table="filter", chain=None, policy=None, family="ipv4"):
     cmd = "{} {} -t {} -P {} {}".format(
         _iptables_cmd(family), wait, table, chain, policy
     )
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
     return out
 
 
@@ -704,7 +704,7 @@ def save(filename=None, family="ipv4"):
     if not os.path.isdir(parent_dir):
         os.makedirs(parent_dir)
     cmd = "{}-save".format(_iptables_cmd(family))
-    ipt = __salt__["cmd.run"](cmd)
+    ipt = __salt__["cmd.run_stdout"](cmd)
 
     # regex out the output if configured with filters
     if _conf_save_filters():
@@ -743,7 +743,7 @@ def check(table="filter", chain=None, rule=None, family="ipv4"):
 
     if _has_option("--check", family):
         cmd = "{} -t {} -C {} {}".format(ipt_cmd, table, chain, rule)
-        out = __salt__["cmd.run"](cmd, output_loglevel="quiet")
+        out = __salt__["cmd.run_stderr"](cmd, output_loglevel="quiet")
     else:
         _chain_name = hex(uuid.getnode())
 
@@ -753,7 +753,7 @@ def check(table="filter", chain=None, rule=None, family="ipv4"):
             "{} -t {} -A {} {}".format(ipt_cmd, table, _chain_name, rule)
         )
 
-        out = __salt__["cmd.run"]("{}-save".format(ipt_cmd))
+        out = __salt__["cmd.run_stdout"]("{}-save".format(ipt_cmd))
 
         # Clean up temporary table
         __salt__["cmd.run"]("{} -t {} -F {}".format(ipt_cmd, table, _chain_name))
@@ -791,7 +791,7 @@ def check_chain(table="filter", chain=None, family="ipv4"):
         return "Error: Chain needs to be specified"
 
     cmd = "{}-save -t {}".format(_iptables_cmd(family), table)
-    out = __salt__["cmd.run"](cmd).find(":{} ".format(chain))
+    out = __salt__["cmd.run_stdout"](cmd).find(":{} ".format(chain))
 
     if out != -1:
         out = True
@@ -822,7 +822,7 @@ def new_chain(table="filter", chain=None, family="ipv4"):
 
     wait = "--wait" if _has_option("--wait", family) else ""
     cmd = "{} {} -t {} -N {}".format(_iptables_cmd(family), wait, table, chain)
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
 
     if not out:
         out = True
@@ -850,7 +850,7 @@ def delete_chain(table="filter", chain=None, family="ipv4"):
 
     wait = "--wait" if _has_option("--wait", family) else ""
     cmd = "{} {} -t {} -X {}".format(_iptables_cmd(family), wait, table, chain)
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
 
     if not out:
         out = True
@@ -888,7 +888,7 @@ def append(table="filter", chain=None, rule=None, family="ipv4"):
     if isinstance(returnCheck, bool) and returnCheck:
         return False
     cmd = "{} {} -t {} -A {} {}".format(_iptables_cmd(family), wait, table, chain, rule)
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
     return not out
 
 
@@ -939,7 +939,7 @@ def insert(table="filter", chain=None, position=None, rule=None, family="ipv4"):
     cmd = "{} {} -t {} -I {} {} {}".format(
         _iptables_cmd(family), wait, table, chain, position, rule
     )
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
     return out
 
 
@@ -976,7 +976,7 @@ def delete(table, chain=None, position=None, rule=None, family="ipv4"):
 
     wait = "--wait" if _has_option("--wait", family) else ""
     cmd = "{} {} -t {} -D {} {}".format(_iptables_cmd(family), wait, table, chain, rule)
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
     return out
 
 
@@ -997,7 +997,7 @@ def flush(table="filter", chain="", family="ipv4"):
 
     wait = "--wait" if _has_option("--wait", family) else ""
     cmd = "{} {} -t {} -F {}".format(_iptables_cmd(family), wait, table, chain)
-    out = __salt__["cmd.run"](cmd)
+    out = __salt__["cmd.run_stderr"](cmd)
     return out
 
 
@@ -1015,7 +1015,7 @@ def _parse_conf(conf_file=None, in_mem=False, family="ipv4"):
             rules = ifile.read()
     elif in_mem:
         cmd = "{}-save".format(_iptables_cmd(family))
-        rules = __salt__["cmd.run"](cmd)
+        rules = __salt__["cmd.run_stdout"](cmd)
     else:
         raise SaltException("A file was not found to parse")
 

tests/unit/modules/test_iptables.py

@@ -27,7 +27,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         Test if it return version from iptables --version
         """
         mock = MagicMock(return_value="iptables v1.4.21")
-        with patch.dict(iptables.__salt__, {"cmd.run": mock}):
+        with patch.dict(iptables.__salt__, {"cmd.run_stdout": mock}):
             self.assertEqual(iptables.version(), "v1.4.21")
 
     # 'build_rule' function tests: 1
@@ -388,7 +388,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
             )
 
             mock = MagicMock(return_value=True)
-            with patch.dict(iptables.__salt__, {"cmd.run": mock}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock}):
                 self.assertTrue(
                     iptables.set_policy(
                         table="filter", chain="INPUT", policy="ACCEPT", family="ipv4"
@@ -408,7 +408,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
             with patch.dict(
                 iptables.__salt__,
                 {
-                    "cmd.run": mock,
+                    "cmd.run_stdout": mock,
                     "file.write": mock,
                     "config.option": MagicMock(return_value=[]),
                 },
@@ -434,15 +434,19 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         mock_rule = "m state --state RELATED,ESTABLISHED -j ACCEPT"
         mock_chain = "INPUT"
         mock_uuid = 31337
-        mock_cmd = MagicMock(
+        mock_cmd_rule = MagicMock(
             return_value="-A {}\n-A {}".format(mock_chain, hex(mock_uuid))
         )
+        mock_cmd_nooutput = MagicMock(return_value="")
         mock_has = MagicMock(return_value=True)
         mock_not = MagicMock(return_value=False)
 
         with patch.object(iptables, "_has_option", mock_not):
             with patch.object(uuid, "getnode", MagicMock(return_value=mock_uuid)):
-                with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+                with patch.dict(
+                    iptables.__salt__,
+                    {"cmd.run_stdout": mock_cmd_rule, "cmd.run": mock_cmd_nooutput},
+                ):
                     self.assertTrue(
                         iptables.check(
                             table="filter",
@@ -457,7 +461,11 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         with patch.object(iptables, "_has_option", mock_not):
             with patch.object(uuid, "getnode", MagicMock(return_value=mock_uuid)):
                 with patch.dict(
-                    iptables.__salt__, {"cmd.run": MagicMock(return_value="")}
+                    iptables.__salt__,
+                    {
+                        "cmd.run_stdout": mock_cmd_nooutput,
+                        "cmd.run": mock_cmd_nooutput,
+                    },
                 ):
                     self.assertFalse(
                         iptables.check(
@@ -469,7 +477,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
                     )
 
         with patch.object(iptables, "_has_option", mock_has):
-            with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock_cmd}):
                 self.assertTrue(
                     iptables.check(
                         table="filter", chain="INPUT", rule=mock_rule, family="ipv4"
@@ -481,7 +489,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
 
         with patch.object(iptables, "_has_option", mock_has):
             with patch.object(uuid, "getnode", mock_uuid):
-                with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+                with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock_cmd}):
                     self.assertTrue(
                         iptables.check(
                             table="filter", chain="0x4d2", rule=mock_rule, family="ipv4"
@@ -500,7 +508,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         )
 
         mock_cmd = MagicMock(return_value="")
-        with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+        with patch.dict(iptables.__salt__, {"cmd.run_stdout": mock_cmd}):
             self.assertFalse(
                 iptables.check_chain(table="filter", chain="INPUT", family="ipv4")
             )
@@ -517,7 +525,13 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         )
 
         mock_cmd = MagicMock(return_value="")
-        with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+        with patch.dict(
+            iptables.__salt__,
+            {
+                "cmd.run_stdout": mock_cmd,  # called by iptables._has_option
+                "cmd.run_stderr": mock_cmd,
+            },
+        ):
             self.assertTrue(
                 iptables.new_chain(table="filter", chain="INPUT", family="ipv4")
             )
@@ -534,7 +548,13 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         )
 
         mock_cmd = MagicMock(return_value="")
-        with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+        with patch.dict(
+            iptables.__salt__,
+            {
+                "cmd.run_stdout": mock_cmd,  # called by iptables._has_option
+                "cmd.run_stderr": mock_cmd,
+            },
+        ):
             self.assertTrue(
                 iptables.delete_chain(table="filter", chain="INPUT", family="ipv4")
             )
@@ -562,7 +582,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
 
             _rule = "m state --state RELATED,ESTABLISHED -j ACCEPT"
             mock = MagicMock(side_effect=["", "SALT"])
-            with patch.dict(iptables.__salt__, {"cmd.run": mock}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock}):
                 self.assertTrue(
                     iptables.append(
                         table="filter", chain="INPUT", rule=_rule, family="ipv4"
@@ -613,7 +633,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
 
             _rule = "m state --state RELATED,ESTABLISHED -j ACCEPT"
             mock = MagicMock(return_value=True)
-            with patch.dict(iptables.__salt__, {"cmd.run": mock}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock}):
                 self.assertTrue(
                     iptables.insert(
                         table="filter",
@@ -640,7 +660,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
             )
 
             mock = MagicMock(return_value=True)
-            with patch.dict(iptables.__salt__, {"cmd.run": mock}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock}):
                 self.assertTrue(
                     iptables.delete(
                         table="filter",
@@ -660,7 +680,7 @@ class IptablesTestCase(TestCase, LoaderModuleMockMixin):
         """
         with patch.object(iptables, "_has_option", MagicMock(return_value=True)):
             mock_cmd = MagicMock(return_value=True)
-            with patch.dict(iptables.__salt__, {"cmd.run": mock_cmd}):
+            with patch.dict(iptables.__salt__, {"cmd.run_stderr": mock_cmd}):
                 self.assertTrue(
                     iptables.flush(table="filter", chain="INPUT", family="ipv4")
                 )