Merge pull request #1356 from sarthurdev/nested_groups

firewall: T478: Add support for nesting groups

5 files changed, 174 insertions, 13 deletions

diff --git a/data/templates/firewall/nftables-defines.j2 b/data/templates/firewall/nftables-defines.j2
index 4fa92f2e3..12146879d 100644
--- a/data/templates/firewall/nftables-defines.j2
+++ b/data/templates/firewall/nftables-defines.j2
@@ -1,32 +1,38 @@
 {% if group is vyos_defined %}
 {%     if group.address_group is vyos_defined %}
-{%         for group_name, group_conf in group.address_group.items() %}
-define A_{{ group_name }} = { {{ group_conf.address | join(",") }} }
+{%         for group_name, group_conf in group.address_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define A_{{ group_name }} = { {{ group_conf.address | nft_nested_group(includes, 'A_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
 {%     if group.ipv6_address_group is vyos_defined %}
-{%         for group_name, group_conf in group.ipv6_address_group.items() %}
-define A6_{{ group_name }} = { {{ group_conf.address | join(",") }} }
+{%         for group_name, group_conf in group.ipv6_address_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define A6_{{ group_name }} = { {{ group_conf.address | nft_nested_group(includes, 'A6_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
 {%     if group.mac_group is vyos_defined %}
-{%         for group_name, group_conf in group.mac_group.items() %}
-define M_{{ group_name }} = { {{ group_conf.mac_address | join(",") }} }
+{%         for group_name, group_conf in group.mac_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define M_{{ group_name }} = { {{ group_conf.mac_address | nft_nested_group(includes, 'M_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
 {%     if group.network_group is vyos_defined %}
-{%         for group_name, group_conf in group.network_group.items() %}
-define N_{{ group_name }} = { {{ group_conf.network | join(",") }} }
+{%         for group_name, group_conf in group.network_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define N_{{ group_name }} = { {{ group_conf.network | nft_nested_group(includes, 'N_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
 {%     if group.ipv6_network_group is vyos_defined %}
-{%         for group_name, group_conf in group.ipv6_network_group.items() %}
-define N6_{{ group_name }} = { {{ group_conf.network | join(",") }} }
+{%         for group_name, group_conf in group.ipv6_network_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define N6_{{ group_name }} = { {{ group_conf.network | nft_nested_group(includes, 'N6_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
 {%     if group.port_group is vyos_defined %}
-{%         for group_name, group_conf in group.port_group.items() %}
-define P_{{ group_name }} = { {{ group_conf.port | join(",") }} }
+{%         for group_name, group_conf in group.port_group | sort_nested_groups %}
+{%             set includes = group_conf.include if group_conf.include is vyos_defined else [] %}
+define P_{{ group_name }} = { {{ group_conf.port | nft_nested_group(includes, 'P_') | join(",") }} }
 {%         endfor %}
 {%     endif %}
-{% endif %}
\ No newline at end of file
+{% endif %}
diff --git a/interface-definitions/firewall.xml.in b/interface-definitions/firewall.xml.in
index dea6ae79e..719088d03 100644
--- a/interface-definitions/firewall.xml.in
+++ b/interface-definitions/firewall.xml.in
@@ -97,6 +97,15 @@
                   <multi/>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another address-group</help>
+                  <completionHelp>
+                    <path>firewall group address-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
               #include <include/generic-description.xml.i>
             </children>
           </tagNode>
@@ -151,6 +160,15 @@
                   <multi/>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another ipv6-address-group</help>
+                  <completionHelp>
+                    <path>firewall group ipv6-address-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
               #include <include/generic-description.xml.i>
             </children>
           </tagNode>
@@ -176,6 +194,15 @@
                   <multi/>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another ipv6-network-group</help>
+                  <completionHelp>
+                    <path>firewall group ipv6-network-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
             </children>
           </tagNode>
           <tagNode name="mac-group">
@@ -200,6 +227,15 @@
                   <multi/>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another mac-group</help>
+                  <completionHelp>
+                    <path>firewall group mac-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
             </children>
           </tagNode>
           <tagNode name="network-group">
@@ -224,6 +260,15 @@
                   <multi/>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another network-group</help>
+                  <completionHelp>
+                    <path>firewall group network-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
             </children>
           </tagNode>
           <tagNode name="port-group">
@@ -256,6 +301,15 @@
                   </constraint>
                 </properties>
               </leafNode>
+              <leafNode name="include">
+                <properties>
+                  <help>Include another port-group</help>
+                  <completionHelp>
+                    <path>firewall group port-group</path>
+                  </completionHelp>
+                  <multi/>
+                </properties>
+              </leafNode>
             </children>
           </tagNode>
         </children>
diff --git a/python/vyos/template.py b/python/vyos/template.py
index ee82f8f8f..3feda47c8 100644
--- a/python/vyos/template.py
+++ b/python/vyos/template.py
@@ -591,6 +591,39 @@ def nft_intra_zone_action(zone_conf, ipv6=False):
             return f'jump {name_prefix}{name}'
     return 'return'
 
+@register_filter('nft_nested_group')
+def nft_nested_group(out_list, includes, prefix):
+    if not vyos_defined(out_list):
+        out_list = []
+    for name in includes:
+        out_list.append(f'${prefix}{name}')
+    return out_list
+
+@register_filter('sort_nested_groups')
+def sort_nested_groups(groups):
+    seen = []
+    out = {}
+
+    def include_iterate(group_name):
+        group = groups[group_name]
+        if 'include' not in group:
+            if group_name not in out:
+                out[group_name] = groups[group_name]
+            return
+
+        for inc_group_name in group['include']:
+            if inc_group_name not in seen:
+                seen.append(inc_group_name)
+                include_iterate(inc_group_name)
+
+        if group_name not in out:
+            out[group_name] = groups[group_name]
+
+    for group_name in groups:
+        include_iterate(group_name)
+
+    return out.items()
+
 @register_test('vyos_defined')
 def vyos_defined(value, test_value=None, var_type=None):
     """
diff --git a/smoketest/scripts/cli/test_firewall.py b/smoketest/scripts/cli/test_firewall.py
index b78d6fffa..998f1b3f3 100755
--- a/smoketest/scripts/cli/test_firewall.py
+++ b/smoketest/scripts/cli/test_firewall.py
@@ -20,6 +20,7 @@ from glob import glob
 
 from base_vyostest_shim import VyOSUnitTestSHIM
 
+from vyos.configsession import ConfigSessionError
 from vyos.util import cmd
 
 sysfs_config = {
@@ -108,6 +109,44 @@ class TestFirewall(VyOSUnitTestSHIM.TestCase):
         self.cli_delete(['system', 'static-host-mapping'])
         self.cli_commit()
 
+    def test_nested_groups(self):
+        self.cli_set(['firewall', 'group', 'network-group', 'smoketest_network', 'network', '172.16.99.0/24'])
+        self.cli_set(['firewall', 'group', 'network-group', 'smoketest_network1', 'network', '172.16.101.0/24'])
+        self.cli_set(['firewall', 'group', 'network-group', 'smoketest_network1', 'include', 'smoketest_network'])
+        self.cli_set(['firewall', 'group', 'port-group', 'smoketest_port', 'port', '53'])
+        self.cli_set(['firewall', 'group', 'port-group', 'smoketest_port1', 'port', '123'])
+        self.cli_set(['firewall', 'group', 'port-group', 'smoketest_port1', 'include', 'smoketest_port'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'action', 'accept'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'source', 'group', 'network-group', 'smoketest_network1'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'destination', 'group', 'port-group', 'smoketest_port1'])
+        self.cli_set(['firewall', 'name', 'smoketest', 'rule', '1', 'protocol', 'tcp_udp'])
+
+        self.cli_set(['interfaces', 'ethernet', 'eth0', 'firewall', 'in', 'name', 'smoketest'])
+
+        self.cli_commit()
+
+        # Test circular includes
+        self.cli_set(['firewall', 'group', 'network-group', 'smoketest_network', 'include', 'smoketest_network1'])
+        with self.assertRaises(ConfigSessionError):
+            self.cli_commit()
+
+        self.cli_delete(['firewall', 'group', 'network-group', 'smoketest_network', 'include', 'smoketest_network1'])
+
+        nftables_search = [
+            ['iifname "eth0"', 'jump NAME_smoketest'],
+            ['ip saddr { 172.16.99.0/24, 172.16.101.0/24 }', 'th dport { 53, 123 }', 'return']
+        ]
+
+        nftables_output = cmd('sudo nft list table ip filter')
+
+        for search in nftables_search:
+            matched = False
+            for line in nftables_output.split("\n"):
+                if all(item in line for item in search):
+                    matched = True
+                    break
+            self.assertTrue(matched, msg=search)
+
     def test_basic_rules(self):
         self.cli_set(['firewall', 'name', 'smoketest', 'default-action', 'drop'])
         self.cli_set(['firewall', 'name', 'smoketest', 'enable-default-log'])
diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py
index fbe0a3a13..792e17b85 100755
--- a/src/conf_mode/firewall.py
+++ b/src/conf_mode/firewall.py
@@ -85,10 +85,16 @@ nft6_iface_chains = ['VYOS_FW6_FORWARD', 'VYOS_FW6_OUTPUT', 'VYOS_FW6_LOCAL']
 
 valid_groups = [
     'address_group',
+    'domain_group',
     'network_group',
     'port_group'
 ]
 
+group_types = [
+    'address_group', 'network_group', 'port_group',
+    'ipv6_address_group', 'ipv6_network_group'
+]
+
 snmp_change_type = {
     'unknown': 0,
     'add': 1,
@@ -241,11 +247,34 @@ def verify_rule(firewall, rule_conf, ipv6):
                 if rule_conf['protocol'] not in ['tcp', 'udp', 'tcp_udp']:
                     raise ConfigError('Protocol must be tcp, udp, or tcp_udp when specifying a port or port-group')
 
+def verify_nested_group(group_name, group, groups, seen):
+    if 'include' not in group:
+        return
+
+    for g in group['include']:
+        if g not in groups:
+            raise ConfigError(f'Nested group "{g}" does not exist')
+
+        if g in seen:
+            raise ConfigError(f'Group "{group_name}" has a circular reference')
+            
+        seen.append(g)
+
+        if 'include' in groups[g]:
+            verify_nested_group(g, groups[g], groups, seen)
+
 def verify(firewall):
     if 'config_trap' in firewall and firewall['config_trap'] == 'enable':
         if not firewall['trap_targets']:
             raise ConfigError(f'Firewall config-trap enabled but "service snmp trap-target" is not defined')
 
+    if 'group' in firewall:
+        for group_type in group_types:
+            if group_type in firewall['group']:
+                groups = firewall['group'][group_type]
+                for group_name, group in groups.items():
+                    verify_nested_group(group_name, group, groups, [])
+
     for name in ['name', 'ipv6_name']:
         if name in firewall:
             for name_id, name_conf in firewall[name].items():