protocols: T3464: proper handling of routing policy configuration

The introduction of key_mangling=('-', '_') when working with get_config_dict()
caused more harm then good. This commit extends common helpers and adds new
helpers when verifying the existence of route-maps, access-lists or prefix-lists.

7 files changed, 107 insertions, 72 deletions

diff --git a/python/vyos/configverify.py b/python/vyos/configverify.py
index 718b7445d..99c472582 100644
--- a/python/vyos/configverify.py
+++ b/python/vyos/configverify.py
@@ -337,18 +337,16 @@ def verify_accel_ppp_base_service(config):
 def verify_diffie_hellman_length(file, min_keysize):
     """ Verify Diffie-Hellamn keypair length given via file. It must be greater
     then or equal to min_keysize """
+    import os
+    import re
+    from vyos.util import cmd
 
     try:
         keysize = str(min_keysize)
     except:
         return False
 
-    import os
-    import re
-    from vyos.util import cmd
-
     if os.path.exists(file):
-
         out = cmd(f'openssl dhparam -inform PEM -in {file} -text')
         prog = re.compile('\d+\s+bit')
         if prog.search(out):
@@ -358,26 +356,55 @@ def verify_diffie_hellman_length(file, min_keysize):
 
     return False
 
-def verify_route_maps(config):
+def verify_common_route_maps(config):
     """
     Common helper function used by routing protocol implementations to perform
     recurring validation if the specified route-map for either zebra to kernel
     installation exists (this is the top-level route_map key) or when a route
     is redistributed with a route-map that it exists!
     """
-    if 'route_map' in config:
-        route_map = config['route_map']
+    # XXX: This function is called in combination with a previous call to:
+    # tmp = conf.get_config_dict(['policy']) - see protocols_ospf.py as example.
+    # We should NOT call this with the key_mangling option as this would rename
+    # route-map hypens '-' to underscores '_' and one could no longer distinguish
+    # what should have been the "proper" route-map name, as foo-bar and foo_bar
+    # are two entire different route-map instances!
+    for route_map in ['route-map', 'route_map']:
+        if route_map not in config:
+            continue
+        tmp = config[route_map]
         # Check if the specified route-map exists, if not error out
-        if dict_search(f'policy.route_map.{route_map}', config) == None:
-            raise ConfigError(f'Specified route-map "{route_map}" does not exist!')
+        if dict_search(f'policy.route-map.{tmp}', config) == None:
+            raise ConfigError(f'Specified route-map "{tmp}" does not exist!')
 
     if 'redistribute' in config:
         for protocol, protocol_config in config['redistribute'].items():
             if 'route_map' in protocol_config:
-                # A hyphen in a route-map name will be converted to _, take care
-                # about this effect during validation
-                route_map = protocol_config['route_map'].replace('-','_')
-                # Check if the specified route-map exists, if not error out
-                if dict_search(f'policy.route_map.{route_map}', config) == None:
-                    raise ConfigError(f'Redistribution route-map "{route_map}" ' \
-                                      f'for "{protocol}" does not exist!')
+                verify_route_map(protocol_config['route_map'], config)
+
+def verify_route_map(route_map_name, config):
+    """
+    Common helper function used by routing protocol implementations to perform
+    recurring validation if a specified route-map exists!
+    """
+    # Check if the specified route-map exists, if not error out
+    if dict_search(f'policy.route-map.{route_map_name}', config) == None:
+        raise ConfigError(f'Specified route-map "{route_map_name}" does not exist!')
+
+def verify_prefix_list(prefix_list, config, version=''):
+    """
+    Common helper function used by routing protocol implementations to perform
+    recurring validation if a specified prefix-list exists!
+    """
+    # Check if the specified prefix-list exists, if not error out
+    if dict_search(f'policy.prefix-list{version}.{prefix_list}', config) == None:
+        raise ConfigError(f'Specified prefix-list{version} "{prefix_list}" does not exist!')
+
+def verify_access_list(access_list, config, version=''):
+    """
+    Common helper function used by routing protocol implementations to perform
+    recurring validation if a specified prefix-list exists!
+    """
+    # Check if the specified ACL exists, if not error out
+    if dict_search(f'policy.access-list{version}.{access_list}', config) == None:
+        raise ConfigError(f'Specified access-list{version} "{access_list}" does not exist!')
diff --git a/src/conf_mode/protocols_isis.py b/src/conf_mode/protocols_isis.py
index 571520cfe..8b75f9873 100755
--- a/src/conf_mode/protocols_isis.py
+++ b/src/conf_mode/protocols_isis.py
@@ -22,6 +22,7 @@ from sys import argv
 from vyos.config import Config
 from vyos.configdict import dict_merge
 from vyos.configdict import node_changed
+from vyos.configverify import verify_common_route_maps
 from vyos.configverify import verify_interface_exists
 from vyos.util import call
 from vyos.util import dict_search
@@ -70,10 +71,12 @@ def get_config(config=None):
         return isis
 
     # We also need some additional information from the config, prefix-lists
-    # and route-maps for instance. They will be used in verify()
-    base = ['policy']
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'))
-    # Merge policy dict into OSPF dict
+    # and route-maps for instance. They will be used in verify().
+    #
+    # XXX: one MUST always call this without the key_mangling() option! See
+    # vyos.configverify.verify_common_route_maps() for more information.
+    tmp = conf.get_config_dict(['policy'])
+    # Merge policy dict into "regular" config dict
     isis = dict_merge(tmp, isis)
 
     return isis
@@ -91,6 +94,8 @@ def verify(isis):
     if int(tmp[-1]) != 0:
         raise ConfigError('Last byte of IS-IS network entity title must always be 0!')
 
+    verify_common_route_maps(isis)
+
     # If interface not set
     if 'interface' not in isis:
         raise ConfigError('Interface used for routing updates is mandatory!')
@@ -141,12 +146,6 @@ def verify(isis):
                         raise ConfigError(f'"protocols isis {process} redistribute {afi} {proto} {redistr_level}" ' \
                                           f'can not be used with \"protocols isis {process} level {proc_level}\"')
 
-                    if 'route_map' in redistr_config:
-                        name = redistr_config['route_map']
-                        tmp = name.replace('-', '_')
-                        if dict_search(f'policy.route_map.{tmp}', isis) == None:
-                            raise ConfigError(f'Route-map {name} does not exist!')
-
     # Segment routing checks
     if dict_search('segment_routing.global_block', isis):
         high_label_value = dict_search('segment_routing.global_block.high_label_value', isis)
diff --git a/src/conf_mode/protocols_ospf.py b/src/conf_mode/protocols_ospf.py
index 30cc33dcf..a97d5b5ed 100755
--- a/src/conf_mode/protocols_ospf.py
+++ b/src/conf_mode/protocols_ospf.py
@@ -22,7 +22,8 @@ from sys import argv
 from vyos.config import Config
 from vyos.configdict import dict_merge
 from vyos.configdict import node_changed
-from vyos.configverify import verify_route_maps
+from vyos.configverify import verify_common_route_maps
+from vyos.configverify import verify_route_map
 from vyos.configverify import verify_interface_exists
 from vyos.template import render_to_string
 from vyos.util import call
@@ -130,10 +131,12 @@ def get_config(config=None):
                 ospf['interface'][interface])
 
     # We also need some additional information from the config, prefix-lists
-    # and route-maps for instance. They will be used in verify()
-    base = ['policy']
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'))
-    # Merge policy dict into OSPF dict
+    # and route-maps for instance. They will be used in verify().
+    #
+    # XXX: one MUST always call this without the key_mangling() option! See
+    # vyos.configverify.verify_common_route_maps() for more information.
+    tmp = conf.get_config_dict(['policy'])
+    # Merge policy dict into "regular" config dict
     ospf = dict_merge(tmp, ospf)
 
     return ospf
@@ -142,7 +145,11 @@ def verify(ospf):
     if not ospf:
         return None
 
-    verify_route_maps(ospf)
+    verify_common_route_maps(ospf)
+
+    # As we can have a default-information route-map, we need to validate it!
+    route_map_name = dict_search('default_information.originate.route_map', ospf)
+    if route_map_name: verify_route_map(route_map_name, ospf)
 
     if 'interface' in ospf:
         for interface in ospf['interface']:
diff --git a/src/conf_mode/protocols_ospfv3.py b/src/conf_mode/protocols_ospfv3.py
index 42b6462e3..4ab7b65a3 100755
--- a/src/conf_mode/protocols_ospfv3.py
+++ b/src/conf_mode/protocols_ospfv3.py
@@ -20,7 +20,7 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import dict_merge
-from vyos.configverify import verify_route_maps
+from vyos.configverify import verify_common_route_maps
 from vyos.template import render_to_string
 from vyos.util import call
 from vyos.ifconfig import Interface
@@ -45,10 +45,12 @@ def get_config(config=None):
         return ospfv3
 
     # We also need some additional information from the config, prefix-lists
-    # and route-maps for instance. They will be used in verify()
-    base = ['policy']
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'))
-    # Merge policy dict into OSPF dict
+    # and route-maps for instance. They will be used in verify().
+    #
+    # XXX: one MUST always call this without the key_mangling() option! See
+    # vyos.configverify.verify_common_route_maps() for more information.
+    tmp = conf.get_config_dict(['policy'])
+    # Merge policy dict into "regular" config dict
     ospfv3 = dict_merge(tmp, ospfv3)
 
     return ospfv3
@@ -57,7 +59,7 @@ def verify(ospfv3):
     if not ospfv3:
         return None
 
-    verify_route_maps(ospfv3)
+    verify_common_route_maps(ospfv3)
 
     if 'interface' in ospfv3:
         for ifname, if_config in ospfv3['interface'].items():
diff --git a/src/conf_mode/protocols_rip.py b/src/conf_mode/protocols_rip.py
index e7eafd059..b48289dec 100755
--- a/src/conf_mode/protocols_rip.py
+++ b/src/conf_mode/protocols_rip.py
@@ -20,7 +20,9 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import dict_merge
-from vyos.configverify import verify_route_maps
+from vyos.configverify import verify_common_route_maps
+from vyos.configverify import verify_access_list
+from vyos.configverify import verify_prefix_list
 from vyos.util import call
 from vyos.util import dict_search
 from vyos.xml import defaults
@@ -51,10 +53,12 @@ def get_config(config=None):
     rip = dict_merge(default_values, rip)
 
     # We also need some additional information from the config, prefix-lists
-    # and route-maps for instance. They will be used in verify()
-    base = ['policy']
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'))
-    # Merge policy dict into OSPF dict
+    # and route-maps for instance. They will be used in verify().
+    #
+    # XXX: one MUST always call this without the key_mangling() option! See
+    # vyos.configverify.verify_common_route_maps() for more information.
+    tmp = conf.get_config_dict(['policy'])
+    # Merge policy dict into "regular" config dict
     rip = dict_merge(tmp, rip)
 
     return rip
@@ -63,21 +67,19 @@ def verify(rip):
     if not rip:
         return None
 
+    verify_common_route_maps(rip)
+
     acl_in = dict_search('distribute_list.access_list.in', rip)
-    if acl_in and acl_in not in  (dict_search('policy.access_list', rip) or []):
-        raise ConfigError(f'Inbound ACL "{acl_in}" does not exist!')
+    if acl_in: verify_access_list(acl_in, rip)
 
     acl_out = dict_search('distribute_list.access_list.out', rip)
-    if acl_out and acl_out not in (dict_search('policy.access_list', rip) or []):
-        raise ConfigError(f'Outbound ACL "{acl_out}" does not exist!')
+    if acl_out: verify_access_list(acl_out, rip)
 
-    prefix_list_in = dict_search('distribute_list.prefix_list.in', rip)
-    if prefix_list_in and prefix_list_in.replace('-','_') not in (dict_search('policy.prefix_list', rip) or []):
-        raise ConfigError(f'Inbound prefix-list "{prefix_list_in}" does not exist!')
+    prefix_list_in = dict_search('distribute_list.prefix-list.in', rip)
+    if prefix_list_in: verify_prefix_list(prefix_list_in, rip)
 
     prefix_list_out = dict_search('distribute_list.prefix_list.out', rip)
-    if prefix_list_out and prefix_list_out.replace('-','_') not in (dict_search('policy.prefix_list', rip) or []):
-        raise ConfigError(f'Outbound prefix-list "{prefix_list_out}" does not exist!')
+    if prefix_list_out: verify_prefix_list(prefix_list_out, rip)
 
     if 'interface' in rip:
         for interface, interface_options in rip['interface'].items():
@@ -89,8 +91,6 @@ def verify(rip):
                     raise ConfigError(f'You can not have "split-horizon poison-reverse" enabled ' \
                                       f'with "split-horizon disable" for "{interface}"!')
 
-    verify_route_maps(rip)
-
 def generate(rip):
     if not rip:
         rip['new_frr_config'] = ''
diff --git a/src/conf_mode/protocols_ripng.py b/src/conf_mode/protocols_ripng.py
index 140133bd0..06a9e97df 100755
--- a/src/conf_mode/protocols_ripng.py
+++ b/src/conf_mode/protocols_ripng.py
@@ -20,7 +20,9 @@ from sys import exit
 
 from vyos.config import Config
 from vyos.configdict import dict_merge
-from vyos.configverify import verify_route_maps
+from vyos.configverify import verify_common_route_maps
+from vyos.configverify import verify_access_list
+from vyos.configverify import verify_prefix_list
 from vyos.util import call
 from vyos.util import dict_search
 from vyos.xml import defaults
@@ -51,10 +53,12 @@ def get_config(config=None):
     ripng = dict_merge(default_values, ripng)
 
     # We also need some additional information from the config, prefix-lists
-    # and route-maps for instance. They will be used in verify()
-    base = ['policy']
-    tmp = conf.get_config_dict(base, key_mangling=('-', '_'))
-    # Merge policy dict into OSPF dict
+    # and route-maps for instance. They will be used in verify().
+    #
+    # XXX: one MUST always call this without the key_mangling() option! See
+    # vyos.configverify.verify_common_route_maps() for more information.
+    tmp = conf.get_config_dict(['policy'])
+    # Merge policy dict into "regular" config dict
     ripng = dict_merge(tmp, ripng)
 
     return ripng
@@ -63,21 +67,19 @@ def verify(ripng):
     if not ripng:
         return None
 
+    verify_common_route_maps(ripng)
+
     acl_in = dict_search('distribute_list.access_list.in', ripng)
-    if acl_in and acl_in not in  (dict_search('policy.access_list6', ripng) or []):
-        raise ConfigError(f'Inbound access-list6 "{acl_in}" does not exist!')
+    if acl_in: verify_access_list(acl_in, ripng, version='6')
 
     acl_out = dict_search('distribute_list.access_list.out', ripng)
-    if acl_out and acl_out not in (dict_search('policy.access_list6', ripng) or []):
-        raise ConfigError(f'Outbound access-list6 "{acl_out}" does not exist!')
+    if acl_out: verify_access_list(acl_out, ripng, version='6')
 
     prefix_list_in = dict_search('distribute_list.prefix_list.in', ripng)
-    if prefix_list_in and prefix_list_in.replace('-','_') not in (dict_search('policy.prefix_list6', ripng) or []):
-        raise ConfigError(f'Inbound prefix-list6 "{prefix_list_in}" does not exist!')
+    if prefix_list_in: verify_prefix_list(prefix_list_in, ripng, version='6')
 
     prefix_list_out = dict_search('distribute_list.prefix_list.out', ripng)
-    if prefix_list_out and prefix_list_out.replace('-','_') not in (dict_search('policy.prefix_list6', ripng) or []):
-        raise ConfigError(f'Outbound prefix-list6 "{prefix_list_out}" does not exist!')
+    if prefix_list_out: verify_prefix_list(prefix_list_out, ripng, version='6')
 
     if 'interface' in ripng:
         for interface, interface_options in ripng['interface'].items():
@@ -89,8 +91,6 @@ def verify(ripng):
                     raise ConfigError(f'You can not have "split-horizon poison-reverse" enabled ' \
                                       f'with "split-horizon disable" for "{interface}"!')
 
-    verify_route_maps(ripng)
-
 def generate(ripng):
     if not ripng:
         ripng['new_frr_config'] = ''
diff --git a/src/conf_mode/protocols_static.py b/src/conf_mode/protocols_static.py
index 7ae952af8..b5b2d6641 100755
--- a/src/conf_mode/protocols_static.py
+++ b/src/conf_mode/protocols_static.py
@@ -20,7 +20,7 @@ from sys import exit
 from sys import argv
 
 from vyos.config import Config
-from vyos.configverify import verify_route_maps
+from vyos.configverify import verify_common_route_maps
 from vyos.configverify import verify_vrf
 from vyos.template import render_to_string
 from vyos.util import call
@@ -52,7 +52,7 @@ def get_config(config=None):
     return static
 
 def verify(static):
-    verify_route_maps(static)
+    verify_common_route_maps(static)
 
     for route in ['route', 'route6']:
         # if there is no route(6) key in the dictionary we can immediately