diff --git a/changelog/46616.fixed.md b/changelog/46616.fixed.md new file mode 100644 index 00000000000..4976b85c520 --- /dev/null +++ b/changelog/46616.fixed.md @@ -0,0 +1 @@ +Fixed the iptables module rendering the SYNPROXY (mss, wscale, sack-perm, timestamp), CT (zone-orig, zone-reply), SET (map-set) and SNAT/MASQUERADE (random-fully) jump-target options before -j instead of after it, so the generated rules are now valid. diff --git a/salt/modules/iptables.py b/salt/modules/iptables.py index 986005b1f71..9a0011e0c85 100644 --- a/salt/modules/iptables.py +++ b/salt/modules/iptables.py @@ -431,7 +431,9 @@ def maybe_add_negation(arg): "log-tcp-options", "log-tcp-sequence", "log-uid", + "map-set", "mask", + "mss", "new", "nfmask", "nflog-group", @@ -449,12 +451,14 @@ def maybe_add_negation(arg): "queue-bypass", "queue-num", "random", + "random-fully", "rateest-ewmalog", "rateest-interval", "rateest-name", "reject-with", "restore", "restore-mark", + "sack-perm", #'save', # no arg, problematic name: How do we avoid collision with this? "save-mark", "selctx", @@ -467,6 +471,7 @@ def maybe_add_negation(arg): "set-xmark", "strip-options", "timeout", + "timestamp", "to", "to-destination", "to-ports", @@ -481,9 +486,12 @@ def maybe_add_negation(arg): "ulog-nlgroup", "ulog-prefix", "ulog-qthreshold", + "wscale", "xor-mark", "xor-tos", "zone", + "zone-orig", + "zone-reply", # IPTABLES-EXTENSIONS "dst-pfx", "hl-dec", diff --git a/tests/pytests/unit/modules/test_iptables.py b/tests/pytests/unit/modules/test_iptables.py index 27fc171c86c..edab7ebe8ca 100644 --- a/tests/pytests/unit/modules/test_iptables.py +++ b/tests/pytests/unit/modules/test_iptables.py @@ -242,6 +242,108 @@ def test_build_rule(): ) +def test_build_rule_after_jump_arguments(): + """ + Test that jump-target arguments for SYNPROXY, CT, SET and SNAT + (regression for issue #46616) are rendered after the --jump target + rather than before it. + """ + with patch.object(iptables, "_has_option", MagicMock(return_value=True)): + # SYNPROXY: --mss, --wscale, --sack-perm, --timestamp + assert ( + iptables.build_rule( + jump="SYNPROXY", + **{"sack-perm": "", "timestamp": "", "wscale": 7, "mss": 1460}, + ) + == "--jump SYNPROXY --mss 1460 --sack-perm --timestamp --wscale 7" + ) + + # CT: --zone-orig / --zone-reply + assert ( + iptables.build_rule(jump="CT", **{"zone-orig": 1}) + == "--jump CT --zone-orig 1" + ) + assert ( + iptables.build_rule(jump="CT", **{"zone-reply": 2}) + == "--jump CT --zone-reply 2" + ) + + # SET: --map-set + assert ( + iptables.build_rule(jump="SET", **{"map-set": "myset src"}) + == '--jump SET --map-set "myset src"' + ) + + # SNAT: --random-fully + assert ( + iptables.build_rule(jump="SNAT", **{"random-fully": None}) + == "--jump SNAT --random-fully" + ) + + +def test_build_rule_synproxy_state_append_46616(): + """ + Test issue #46616 through the exact argument shape used by the + iptables.append state (salt/states/iptables.py), which is the + production caller of build_rule. + """ + # The state passes full="True" (a string, not a bool) together with + # command="A" and family, plus name/table/chain kwargs that build_rule + # must strip; full="True" is the decisive flag because it exercises the + # complete command line the state hands to iptables.append/check. + kwargs = { + "name": "synproxy web traffic", + "table": "filter", + "chain": "INPUT", + "protocol": "tcp", + "dport": 443, + "match": "state", + "connstate": "INVALID,UNTRACKED", + "jump": "SYNPROXY", + "mss": 1460, + "wscale": 7, + "sack-perm": "", + "timestamp": "", + } + with patch.object(iptables, "_has_option", MagicMock(return_value=True)): + with patch.object( + iptables, "_iptables_cmd", MagicMock(return_value="/sbin/iptables") + ): + assert iptables.build_rule( + full="True", family="ipv4", command="A", **kwargs + ) == ( + "/sbin/iptables --wait -t filter -A INPUT " + "-p tcp -m state --state INVALID,UNTRACKED --dport 443 " + "--jump SYNPROXY --mss 1460 --sack-perm --timestamp --wscale 7" + ) + + +def test_build_rule_non_jump_options_unaffected_46616(): + """ + Guard against overcorrection of the #46616 fix: options that are not + on the after-jump whitelist must keep rendering before the --jump + target, and whitelist entries that predate the fix must render + exactly as before. This test passes with and without the fix. + """ + with patch.object(iptables, "_has_option", MagicMock(return_value=True)): + # "mark" (the mark match option) is a near-miss sibling of the + # newly whitelisted "mask"/"mss" names and must stay before the + # jump target. + assert ( + iptables.build_rule(match="mark", mark="0x64", jump="RETURN") + == "-m mark --mark 0x64 --jump RETURN" + ) + + # Pre-existing whitelist entries (SNAT --to-source/--random) must + # be rendered unchanged by the additions. + assert ( + iptables.build_rule( + jump="SNAT", **{"to-source": "192.168.0.1", "random": ""} + ) + == "--jump SNAT --random --to-source 192.168.0.1" + ) + + # 'get_saved_rules' function tests: 2