nat: T3186: fix negated addresses not applied from CLI

(cherry picked from commit 806b1cb6eebce4a11a5d2496b062a93d5899746e)

2 files changed, 58 insertions, 46 deletions

diff --git a/data/templates/firewall/nftables-nat.tmpl b/data/templates/firewall/nftables-nat.tmpl
index 8769c2384..770a24a95 100644
--- a/data/templates/firewall/nftables-nat.tmpl
+++ b/data/templates/firewall/nftables-nat.tmpl
@@ -1,87 +1,87 @@
 #!/usr/sbin/nft -f
 
 {% macro nat_rule(rule, config, chain) %}
-{%   set comment  = "" %}
-{%   set base_log = "" %}
-{%   set src_addr  = "ip saddr " + config.source.address if config.source is defined and config.source.address is defined and config.source.address is not none %}
-{%   set dst_addr  = "ip daddr " + config.destination.address if config.destination is defined and config.destination.address is defined and config.destination.address is not none %}
+{%   set comment  = '' %}
+{%   set base_log = '' %}
+{%   set src_addr  = 'ip saddr ' + config.source.address.replace('!','!= ') if config.source is defined and config.source.address is defined and config.source.address is not none %}
+{%   set dst_addr  = 'ip daddr ' + config.destination.address.replace('!','!= ') if config.destination is defined and config.destination.address is defined and config.destination.address is not none %}
 {#   negated port groups need special treatment, move != in front of { } group #}
 {%   if config.source is defined and config.source.port is defined and config.source.port is not none and config.source.port.startswith('!=') %}
-{%     set src_port  = "sport != { " + config.source.port.replace('!=','') +" }" %}
+{%     set src_port  = 'sport != { ' + config.source.port.replace('!=','') + ' }' %}
 {%   else %}
-{%     set src_port  = "sport { " + config.source.port +" }" if config.source is defined and config.source.port is defined and config.source.port is not none %}
+{%     set src_port  = 'sport { ' + config.source.port + ' }' if config.source is defined and config.source.port is defined and config.source.port is not none %}
 {%   endif %}
 {#   negated port groups need special treatment, move != in front of { } group #}
 {%   if config.destination is defined and config.destination.port is defined and config.destination.port is not none and config.destination.port.startswith('!=') %}
-{%     set dst_port  = "dport != { " + config.destination.port.replace('!=','') +" }" %}
+{%     set dst_port  = 'dport != { ' + config.destination.port.replace('!=','') + ' }' %}
 {%   else %}
-{%     set dst_port  = "dport { " + config.destination.port +" }" if config.destination is defined and config.destination.port is defined and config.destination.port is not none %}
-{%   endif %}
-{%   if chain == "PREROUTING" %}
-{%     set comment   = "DST-NAT-" + rule %}
-{%     set base_log  = "[NAT-DST-" + rule %}
-{%     set interface = " iifname \"" + config.inbound_interface + "\"" if config.inbound_interface is defined and config.inbound_interface != 'any' else '' %}
-{%     set trns_addr = "dnat to " + config.translation.address if config.translation is defined and config.translation.address is defined and config.translation.address is not none %}
-{%   elif chain == "POSTROUTING" %}
-{%     set comment   = "SRC-NAT-" + rule %}
-{%     set base_log  = "[NAT-SRC-" + rule %}
-{%     set interface = " oifname \"" + config.outbound_interface + "\"" if config.outbound_interface is defined and config.outbound_interface != 'any' else '' %}
+{%     set dst_port  = 'dport { ' + config.destination.port + ' }' if config.destination is defined and config.destination.port is defined and config.destination.port is not none %}
+{%   endif %}
+{%   if chain == 'PREROUTING' %}
+{%     set comment   = 'DST-NAT-' + rule %}
+{%     set base_log  = '[NAT-DST-' + rule %}
+{%     set interface = ' iifname "' + config.inbound_interface + '"' if config.inbound_interface is defined and config.inbound_interface != 'any' else '' %}
+{%     set trns_addr = 'dnat to ' + config.translation.address if config.translation is defined and config.translation.address is defined and config.translation.address is not none %}
+{%   elif chain == 'POSTROUTING' %}
+{%     set comment   = 'SRC-NAT-' + rule %}
+{%     set base_log  = '[NAT-SRC-' + rule %}
+{%     set interface = ' oifname "' + config.outbound_interface + '"' if config.outbound_interface is defined and config.outbound_interface != 'any' else '' %}
 {%     if config.translation is defined and config.translation.address is defined and config.translation.address == 'masquerade' %}
 {%       set trns_addr = config.translation.address %}
 {%       if config.translation.port is defined and config.translation.port is not none %}
-{%         set trns_addr = trns_addr + " to " %}
+{%         set trns_addr = trns_addr + ' to ' %}
 {%       endif %}
 {%     else %}
-{%       set trns_addr = "snat to " + config.translation.address if config.translation is defined and config.translation.address is defined and config.translation.address is not none %}
+{%       set trns_addr = 'snat to ' + config.translation.address if config.translation is defined and config.translation.address is defined and config.translation.address is not none %}
 {%     endif %}
 {%   endif %}
-{%   set trns_port = ":" + config.translation.port if config.translation is defined and config.translation.port is defined and config.translation.port is not none %}
+{%   set trns_port = ':' + config.translation.port if config.translation is defined and config.translation.port is defined and config.translation.port is not none %}
 {#   protocol has a default value thus it is always present #}
-{%   if config.protocol == "tcp_udp" %}
-{%     set protocol  = "tcp" %}
-{%     set comment   = comment + " tcp_udp" %}
+{%   if config.protocol == 'tcp_udp' %}
+{%     set protocol  = 'tcp' %}
+{%     set comment   = comment + ' tcp_udp' %}
 {%   else %}
 {%     set protocol  = config.protocol %}
 {%   endif %}
 {%   if config.log is defined %}
 {%     if config.exclude is defined %}
-{%       set log = base_log + "-EXCL]" %}
+{%       set log = base_log + '-EXCL]' %}
 {%     elif config.translation is defined and config.translation.address is defined and config.translation.address == 'masquerade' %}
-{%       set log = base_log + "-MASQ]" %}
+{%       set log = base_log +'-MASQ]' %}
 {%     else %}
-{%       set log = base_log + "]" %}
+{%       set log = base_log + ']' %}
 {%     endif %}
 {%   endif %}
 {%   if config.exclude is defined %}
-{#     rule has been marked as "exclude" thus we simply return here #}
-{%     set trns_addr = "return" %}
-{%     set trns_port = "" %}
+{#     rule has been marked as 'exclude' thus we simply return here #}
+{%     set trns_addr = 'return' %}
+{%     set trns_port = '' %}
 {%   endif %}
-{%   set output = "add rule ip nat " + chain + interface %}
-{%   if protocol != "all" %}
-{%     set output = output + " ip protocol " + protocol %}
+{%   set output = 'add rule ip nat ' + chain + interface %}
+{%   if protocol != 'all' %}
+{%     set output = output + ' ip protocol ' + protocol %}
 {%   endif %}
 {%   if src_addr %}
-{%     set output = output + " " + src_addr %}
+{%     set output = output + ' ' + src_addr %}
 {%   endif %}
 {%   if src_port %}
-{%     set output = output + " " + protocol + " " + src_port %}
+{%     set output = output + ' ' + protocol + ' ' + src_port %}
 {%   endif %}
 {%   if dst_addr %}
-{%     set output = output + " " + dst_addr %}
+{%     set output = output + ' ' + dst_addr %}
 {%   endif %}
 {%   if dst_port %}
-{%     set output = output + " " + protocol + " " + dst_port %}
+{%     set output = output + ' ' + protocol + ' ' + dst_port %}
 {%   endif %}
 {#   Count packets #}
-{%     set output = output + " counter" %}
+{%     set output = output + ' counter' %}
 {#   Special handling of log option, we must repeat the entire rule before the #}
 {#   NAT translation options are added, this is essential                      #}
 {%   if log %}
-{%     set log_output = output + " log prefix \"" + log + "\" comment \"" + comment + "\"" %}
+{%     set log_output = output + ' log prefix "' + log + '" comment "' + comment + '"' %}
 {%   endif %}
 {%   if trns_addr %}
-{%     set output = output + " " + trns_addr %}
+{%     set output = output + ' ' + trns_addr %}
 {%   endif %}
 {%   if trns_port %}
 {#     Do not add a whitespace here, translation port must be directly added after IP address #}
@@ -89,15 +89,15 @@
 {%     set output = output + trns_port %}
 {%   endif %}
 {%   if comment %}
-{%     set output = output + " comment \"" + comment + "\"" %}
+{%     set output = output + ' comment "' + comment + '"' %}
 {%   endif %}
 {{ log_output if log_output }}
 {{ output }}
 {#   Special handling if protocol is tcp_udp, we must repeat the entire rule with udp as protocol #}
-{%   if config.protocol == "tcp_udp" %}
+{%   if config.protocol == 'tcp_udp' %}
 {#     Beware of trailing whitespace, without it the comment tcp_udp will be changed to udp_udp   #}
-{{ log_output | replace("tcp ", "udp ") if log_output }}
-{{ output | replace("tcp ", "udp ") }}
+{{ log_output | replace('tcp ', 'udp ') if log_output }}
+{{ output | replace('tcp ', 'udp ') }}
 {%   endif %}
 {% endmacro %}
 
@@ -105,7 +105,7 @@
 flush table nat
 {% if helper_functions == 'remove' %}
 {# NAT if going to be disabled - remove rules and targets from nftables #}
-{%   set base_command = "delete rule ip raw" %}
+{%   set base_command = 'delete rule ip raw' %}
 {{ base_command }} PREROUTING handle {{ pre_ct_ignore }}
 {{ base_command }} OUTPUT     handle {{ out_ct_ignore }}
 {{ base_command }} PREROUTING handle {{ pre_ct_conntrack }}
@@ -117,7 +117,7 @@ delete chain ip raw NAT_CONNTRACK
 {# NAT if enabled - add targets to nftables #}
 add chain ip raw NAT_CONNTRACK
 add rule ip raw NAT_CONNTRACK counter accept
-{%   set base_command = "add rule ip raw" %}
+{%   set base_command = 'add rule ip raw' %}
 {{ base_command }} PREROUTING position {{ pre_ct_ignore }}    counter jump VYATTA_CT_HELPER
 {{ base_command }} OUTPUT     position {{ out_ct_ignore }}    counter jump VYATTA_CT_HELPER
 {{ base_command }} PREROUTING position {{ pre_ct_conntrack }} counter jump NAT_CONNTRACK
diff --git a/smoketest/scripts/cli/test_nat.py b/smoketest/scripts/cli/test_nat.py
index b82805661..b5702d691 100755
--- a/smoketest/scripts/cli/test_nat.py
+++ b/smoketest/scripts/cli/test_nat.py
@@ -155,6 +155,18 @@ class TestNAT(unittest.TestCase):
         self.session.set(src_path + ['rule', rule, 'translation', 'address', 'masquerade'])
         self.session.commit()
 
+    def test_dnat_negated_addresses(self):
+        # T3186: negated addresses are not accepted by nftables
+        rule = '1000'
+        self.session.set(dst_path + ['rule', rule, 'destination', 'address', '!192.0.2.1'])
+        self.session.set(dst_path + ['rule', rule, 'destination', 'port', '53'])
+        self.session.set(dst_path + ['rule', rule, 'inbound-interface', 'eth0'])
+        self.session.set(dst_path + ['rule', rule, 'protocol', 'tcp_udp'])
+        self.session.set(dst_path + ['rule', rule, 'source', 'address', '!192.0.2.1'])
+        self.session.set(dst_path + ['rule', rule, 'translation', 'address', '192.0.2.1'])
+        self.session.set(dst_path + ['rule', rule, 'translation', 'port', '53'])
+        self.session.commit()
+
     def test_nat_no_rules(self):
         # T3206: deleting all rules but keep the direction 'destination' or
         # 'source' resulteds in KeyError: 'rule'.