Merge pull request #2692 from indrajitr/sagitta-ddclient-backports-20231226

ddclient: T5144,T5791: Consolidated backport for dynamic dns updates and fixes

5 files changed, 133 insertions, 31 deletions

diff --git a/interface-definitions/dns-dynamic.xml.in b/interface-definitions/dns-dynamic.xml.in
index f089f0e52..388e7c5d2 100644
--- a/interface-definitions/dns-dynamic.xml.in
+++ b/interface-definitions/dns-dynamic.xml.in
@@ -19,6 +19,10 @@
                     <format>txt</format>
                     <description>Dynamic DNS service name</description>
                   </valueHelp>
+                  <constraint>
+                    #include <include/constraint/alpha-numeric-hyphen-underscore.xml.i>
+                  </constraint>
+                  <constraintErrorMessage>Dynamic DNS service name must be alphanumeric and can contain hyphens and underscores</constraintErrorMessage>
                 </properties>
                 <children>
                   #include <include/generic-description.xml.i>
diff --git a/smoketest/scripts/cli/test_service_dns_dynamic.py b/smoketest/scripts/cli/test_service_dns_dynamic.py
index 3c7303f32..ae46b18ba 100755
--- a/smoketest/scripts/cli/test_service_dns_dynamic.py
+++ b/smoketest/scripts/cli/test_service_dns_dynamic.py
@@ -17,8 +17,6 @@
 import os
 import unittest
 import tempfile
-import random
-import string
 
 from base_vyostest_shim import VyOSUnitTestSHIM
 
@@ -67,14 +65,12 @@ class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
             self.cli_set(name_path + [svc, 'address', interface])
             self.cli_set(name_path + [svc, 'host-name', hostname])
             self.cli_set(name_path + [svc, 'password', password])
-            self.cli_set(name_path + [svc, 'zone', zone])
-            self.cli_set(name_path + [svc, 'ttl', ttl])
             for opt, value in details.items():
                 self.cli_set(name_path + [svc, opt, value])
 
-            # 'zone' option is supported and required by 'cloudfare', but not 'freedns' and 'zoneedit'
+            # 'zone' option is supported by 'cloudfare' and 'zoneedit1', but not 'freedns'
             self.cli_set(name_path + [svc, 'zone', zone])
-            if details['protocol'] == 'cloudflare':
+            if details['protocol'] in ['cloudflare', 'zoneedit1']:
                 pass
             else:
                 # exception is raised for unsupported ones
@@ -292,7 +288,36 @@ class TestServiceDDNS(VyOSUnitTestSHIM.TestCase):
         self.assertIn(f'password=\'{password}\'', ddclient_conf)
         self.assertIn(f'{hostname}', ddclient_conf)
 
-    def test_07_dyndns_vrf(self):
+    def test_07_dyndns_dynamic_interface(self):
+        # Check if DDNS service can be configured and runs
+        svc_path = name_path + ['namecheap']
+        proto = 'namecheap'
+        dyn_interface = 'pppoe587'
+
+        self.cli_set(svc_path + ['address', dyn_interface])
+        self.cli_set(svc_path + ['protocol', proto])
+        self.cli_set(svc_path + ['server', server])
+        self.cli_set(svc_path + ['username', username])
+        self.cli_set(svc_path + ['password', password])
+        self.cli_set(svc_path + ['host-name', hostname])
+
+        # Dynamic interface will raise a warning but still go through
+        # XXX: We should have idiomatic class "ConfigSessionWarning" wrapping
+        #      "Warning" similar to "ConfigSessionError".
+        # with self.assertWarns(Warning):
+        #     self.cli_commit()
+        self.cli_commit()
+
+        # Check the generating config parameters
+        ddclient_conf = cmd(f'sudo cat {DDCLIENT_CONF}')
+        self.assertIn(f'ifv4={dyn_interface}', ddclient_conf)
+        self.assertIn(f'protocol={proto}', ddclient_conf)
+        self.assertIn(f'server={server}', ddclient_conf)
+        self.assertIn(f'login={username}', ddclient_conf)
+        self.assertIn(f'password=\'{password}\'', ddclient_conf)
+        self.assertIn(f'{hostname}', ddclient_conf)
+
+    def test_08_dyndns_vrf(self):
         # Table number randomized, but should be within range 100-65535
         vrf_table = '58710'
         vrf_name = f'vyos-test-{vrf_table}'
diff --git a/src/conf_mode/dns_dynamic.py b/src/conf_mode/dns_dynamic.py
index c4dcb76ed..99fa8feee 100755
--- a/src/conf_mode/dns_dynamic.py
+++ b/src/conf_mode/dns_dynamic.py
@@ -15,7 +15,7 @@
 # along with this program.  If not, see <http://www.gnu.org/licenses/>.
 
 import os
-
+import re
 from sys import exit
 
 from vyos.base import Warning
@@ -30,6 +30,9 @@ airbag.enable()
 config_file = r'/run/ddclient/ddclient.conf'
 systemd_override = r'/run/systemd/system/ddclient.service.d/override.conf'
 
+# Dynamic interfaces that might not exist when the configuration is loaded
+dynamic_interfaces = ('pppoe', 'sstpc')
+
 # Protocols that require zone
 zone_necessary = ['cloudflare', 'digitalocean', 'godaddy', 'hetzner', 'gandi',
                   'nfsn', 'nsupdate']
@@ -86,17 +89,29 @@ def verify(dyndns):
             if field not in config:
                 raise ConfigError(f'"{field.replace("_", "-")}" {error_msg_req}')
 
-        # If dyndns address is an interface, ensure that it exists
+        # If dyndns address is an interface, ensure
+        # that the interface exists (or just warn if dynamic interface)
         # and that web-options are not set
         if config['address'] != 'web':
             # exclude check interface for dynamic interfaces
-            interface_filter = ('pppoe', 'sstpc')
-            if config['address'].startswith(interface_filter):
-                Warning(f'interface {config["address"]} does not exist!')
+            if config['address'].startswith(dynamic_interfaces):
+                Warning(f'Interface "{config["address"]}" does not exist yet and cannot '
+                        f'be used for Dynamic DNS service "{service}" until it is up!')
             else:
                 verify_interface_exists(config['address'])
             if 'web_options' in config:
-                raise ConfigError(f'"web-options" is applicable only when using HTTP(S) web request to obtain the IP address')
+                raise ConfigError(f'"web-options" is applicable only when using HTTP(S) '
+                                  f'web request to obtain the IP address')
+
+        # Warn if using checkip.dyndns.org, as it does not support HTTPS
+        # See: https://github.com/ddclient/ddclient/issues/597
+        if 'web_options' in config:
+            if 'url' not in config['web_options']:
+                raise ConfigError(f'"url" in "web-options" {error_msg_req} '
+                                  f'with protocol "{config["protocol"]}"')
+            elif re.search("^(https?://)?checkip\.dyndns\.org", config['web_options']['url']):
+                Warning(f'"checkip.dyndns.org" does not support HTTPS requests for IP address '
+                        f'lookup. Please use a different IP address lookup service.')
 
         # RFC2136 uses 'key' instead of 'password'
         if config['protocol'] != 'nsupdate' and 'password' not in config:
@@ -124,13 +139,16 @@ def verify(dyndns):
 
         if config['ip_version'] == 'both':
             if config['protocol'] not in dualstack_supported:
-                raise ConfigError(f'Both IPv4 and IPv6 at the same time {error_msg_uns} with protocol "{config["protocol"]}"')
+                raise ConfigError(f'Both IPv4 and IPv6 at the same time {error_msg_uns} '
+                                  f'with protocol "{config["protocol"]}"')
             # dyndns2 protocol in ddclient honors dual stack only for dyn.com (dyndns.org)
             if config['protocol'] == 'dyndns2' and 'server' in config and config['server'] not in dyndns_dualstack_servers:
-                raise ConfigError(f'Both IPv4 and IPv6 at the same time {error_msg_uns} for "{config["server"]}" with protocol "{config["protocol"]}"')
+                raise ConfigError(f'Both IPv4 and IPv6 at the same time {error_msg_uns} '
+                                  f'for "{config["server"]}" with protocol "{config["protocol"]}"')
 
         if {'wait_time', 'expiry_time'} <= config.keys() and int(config['expiry_time']) < int(config['wait_time']):
-                raise ConfigError(f'"expiry-time" must be greater than "wait-time" for Dynamic DNS service "{service}"')
+                raise ConfigError(f'"expiry-time" must be greater than "wait-time" for '
+                                  f'Dynamic DNS service "{service}"')
 
     return None
 
diff --git a/src/migration-scripts/dns-dynamic/0-to-1 b/src/migration-scripts/dns-dynamic/0-to-1
index d80e8d44a..b7674a9c8 100755
--- a/src/migration-scripts/dns-dynamic/0-to-1
+++ b/src/migration-scripts/dns-dynamic/0-to-1
@@ -25,8 +25,10 @@
 #        to "service dns dynamic address <address> service <config> username ..."
 # - apply global 'ipv6-enable' to per <config> 'ip-version: ipv6'
 # - apply service protocol mapping upfront, they are not 'auto-detected' anymore
+# - migrate web-options url to stricter format
 
 import sys
+import re
 from vyos.configtree import ConfigTree
 
 service_protocol_mapping = {
@@ -81,20 +83,42 @@ for address in config.list_nodes(new_base_path):
                 config.rename(new_base_path + [address, 'service', svc_cfg, 'login'], 'username')
             # Apply global 'ipv6-enable' to per <config> 'ip-version: ipv6'
             if config.exists(new_base_path + [address, 'ipv6-enable']):
-                config.set(new_base_path + [address, 'service', svc_cfg, 'ip-version'],
-                           value='ipv6', replace=False)
+                config.set(new_base_path + [address, 'service', svc_cfg, 'ip-version'], 'ipv6')
                 config.delete(new_base_path + [address, 'ipv6-enable'])
             # Apply service protocol mapping upfront, they are not 'auto-detected' anymore
             if svc_cfg in service_protocol_mapping:
                 config.set(new_base_path + [address, 'service', svc_cfg, 'protocol'],
-                           value=service_protocol_mapping.get(svc_cfg), replace=False)
+                           service_protocol_mapping.get(svc_cfg))
 
-    # Migrate "service dns dynamic interface <interface> use-web"
-    #      to "service dns dynamic address <address> web-options"
-    # Also, rename <address> to 'web' literal for backward compatibility
+    # If use-web is set, then:
+    #   Move "service dns dynamic address <address> <service|rfc2136> <service> ..."
+    #     to "service dns dynamic address web <service|rfc2136> <service>-<address> ..."
+    #   Move "service dns dynamic address web use-web ..."
+    #     to "service dns dynamic address web web-options ..."
+    # Note: The config is named <service>-<address> to avoid name conflict with old entries
     if config.exists(new_base_path + [address, 'use-web']):
-        config.rename(new_base_path + [address], 'web')
-        config.rename(new_base_path + ['web', 'use-web'], 'web-options')
+        for svc_type in ['rfc2136', 'service']:
+            if config.exists(new_base_path + [address, svc_type]):
+                config.set(new_base_path + ['web', svc_type])
+                config.set_tag(new_base_path + ['web', svc_type])
+                for svc_cfg in config.list_nodes(new_base_path + [address, svc_type]):
+                    config.copy(new_base_path + [address, svc_type, svc_cfg],
+                                new_base_path + ['web', svc_type, f'{svc_cfg}-{address}'])
+
+        # Multiple web-options were not supported, so copy only the first one
+        # Also, migrate web-options url to stricter format and transition
+        # checkip.dyndns.org to https://domains.google.com/checkip for better
+        # TLS support (see: https://github.com/ddclient/ddclient/issues/597)
+        if not config.exists(new_base_path + ['web', 'web-options']):
+            config.copy(new_base_path + [address, 'use-web'], new_base_path + ['web', 'web-options'])
+            if config.exists(new_base_path + ['web', 'web-options', 'url']):
+                url = config.return_value(new_base_path + ['web', 'web-options', 'url'])
+                if re.search("^(https?://)?checkip\.dyndns\.org", url):
+                    config.set(new_base_path + ['web', 'web-options', 'url'], 'https://domains.google.com/checkip')
+                if not url.startswith(('http://', 'https://')):
+                    config.set(new_base_path + ['web', 'web-options', 'url'], f'https://{url}')
+
+        config.delete(new_base_path + [address])
 
 try:
     with open(file_name, 'w') as f:
diff --git a/src/migration-scripts/dns-dynamic/2-to-3 b/src/migration-scripts/dns-dynamic/2-to-3
index 187c2a895..4e0aa37d5 100755
--- a/src/migration-scripts/dns-dynamic/2-to-3
+++ b/src/migration-scripts/dns-dynamic/2-to-3
@@ -21,10 +21,27 @@
 #        to "service dns dynamic name <service> address <interface> protocol 'nsupdate'"
 # - migrate "service dns dynamic address <interface> service <service> ..."
 #        to "service dns dynamic name <service> address <interface> ..."
+# - normalize the all service names to conform with name constraints
 
 import sys
+import re
+from unicodedata import normalize
 from vyos.configtree import ConfigTree
 
+def normalize_name(name):
+    """Normalize service names to conform with name constraints.
+
+    This is necessary as part of migration because there were no constraints in
+    the old name format.
+    """
+    # Normalize unicode characters to ASCII (NFKD)
+    # Replace all separators with hypens, strip leading and trailing hyphens
+    name = normalize('NFKD', name).encode('ascii', 'ignore').decode()
+    name = re.sub(r'(\s|_|\W)+', '-', name).strip('-')
+
+    return name
+
+
 if len(sys.argv) < 2:
     print("Must specify file name!")
     sys.exit(1)
@@ -64,22 +81,36 @@ for address in config.list_nodes(address_path):
 
     for svc_type in ['service', 'rfc2136']:
         if config.exists(address_path_tag + [svc_type]):
-            # Move RFC2136 as service configuration, rename to avoid name conflict and set protocol to 'nsupdate'
+            # Set protocol to 'nsupdate' for RFC2136 configuration
             if svc_type == 'rfc2136':
-                for rfc_cfg_old in config.list_nodes(address_path_tag + ['rfc2136']):
-                    rfc_cfg_new = f'{rfc_cfg_old}-rfc2136'
-                    config.rename(address_path_tag + ['rfc2136', rfc_cfg_old], rfc_cfg_new)
-                    config.set(address_path_tag + ['rfc2136', rfc_cfg_new, 'protocol'], 'nsupdate')
+                for rfc_cfg in config.list_nodes(address_path_tag + ['rfc2136']):
+                    config.set(address_path_tag + ['rfc2136', rfc_cfg, 'protocol'], 'nsupdate')
 
             # Add address as config value in each service before moving the service path
-            # And then copy the services from 'address <interface> service <service>' to 'name <service>'
+            # And then copy the services from 'address <interface> service <service>'
+            #                              to 'name (service|rfc2136)-<service>-<address>'
+            # Note: The new service is named (service|rfc2136)-<service>-<address>
+            #       to avoid name conflict with old entries
             for svc_cfg in config.list_nodes(address_path_tag + [svc_type]):
                 config.set(address_path_tag + [svc_type, svc_cfg, 'address'], address)
-                config.copy(address_path_tag + [svc_type, svc_cfg], name_path + [svc_cfg])
+                config.copy(address_path_tag + [svc_type, svc_cfg],
+                            name_path + ['-'.join([svc_type, svc_cfg, address])])
 
 # Finally cleanup the old address path
 config.delete(address_path)
 
+# Normalize the all service names to conform with name constraints
+index = 1
+for name in config.list_nodes(name_path):
+    new_name = normalize_name(name)
+    if new_name != name:
+        # Append index if there is still a name conflicts after normalization
+        # For example, "foo-?(" and "foo-!)" both normalize to "foo-"
+        if config.exists(name_path + [new_name]):
+            new_name = f'{new_name}-{index}'
+            index += 1
+        config.rename(name_path + [name], new_name)
+
 try:
     with open(file_name, 'w') as f:
         f.write(config.to_string())