openvpn: T9363: multiply the site-to-site keepalive timeout

"failure-count" counts missed intervals, so the timeout it describes is
interval * failure-count. Server mode renders and verifies it that way;
site-to-site passed the bare count to "ping-restart", where it lands as
seconds - 60s instead of 600s with the defaults.

Render the product. That puts a timeout past the 86400 "ping-restart"
accepts within reach, so verify() rejects it and the 5-to-6 migration
clamps configurations already beyond it.

This changes behaviour on upgrade: site-to-site tunnels now tolerate a
longer silence, 600 seconds instead of 60 with the defaults.

5 files changed, 123 insertions, 10 deletions

diff --git a/data/templates/openvpn/server.conf.j2 b/data/templates/openvpn/server.conf.j2
index 6bd5291c8..999af5086 100644
--- a/data/templates/openvpn/server.conf.j2
+++ b/data/templates/openvpn/server.conf.j2
@@ -142,7 +142,7 @@ bind ipv6only
 {%     endif %}
 {%     if keep_alive.interval | int > 0 %}
 ping {{ keep_alive.interval }}
-ping-restart {{ keep_alive.failure_count }}
+ping-restart {{ keep_alive.interval | int * keep_alive.failure_count | int }}
 {%     endif %}
 
 {%     if device_type == 'tap' %}
diff --git a/smoketest/scripts/cli/test_interfaces_openvpn.py b/smoketest/scripts/cli/test_interfaces_openvpn.py
index 922695ff2..7c7c3f69e 100755
--- a/smoketest/scripts/cli/test_interfaces_openvpn.py
+++ b/smoketest/scripts/cli/test_interfaces_openvpn.py
@@ -1381,7 +1381,8 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase):
         self.assertNotIn('ping-restart', directives)
         self.assertServiceRunning(interface)
 
-        # an enabled keepalive still renders both
+        # an enabled keepalive renders both, and the timeout is the interval
+        # times the number of failures it tolerates
         self.cli_set(path + ['keep-alive', 'interval', '10'])
         self.cli_set(path + ['keep-alive', 'failure-count', '60'])
         self.cli_commit()
@@ -1391,9 +1392,34 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase):
         config = read_file(f'/run/openvpn/{interface}.conf')
         lines = config.splitlines()
         self.assertIn('ping 10', lines)
-        self.assertIn('ping-restart 60', lines)
+        self.assertIn('ping-restart 600', lines)
         self.assertServiceRunning(interface)
 
+        # check validate() - OpenVPN caps ping-restart at 24 hours, and
+        # nothing doubles the value here the way it does for the server
+        self.cli_set(path + ['keep-alive', 'interval', '600'])
+        self.cli_set(path + ['keep-alive', 'failure-count', '145'])
+        with self.assertRaisesRegex(ConfigSessionError, r'cannot\s+exceed\s+86400'):
+            self.cli_commit()
+
+        # 600 * 144 is exactly 24 hours, which OpenVPN still accepts
+        self.cli_set(path + ['keep-alive', 'failure-count', '144'])
+        self.cli_commit()
+
+        config = read_file(f'/run/openvpn/{interface}.conf')
+        self.assertIn('ping-restart 86400', config.splitlines())
+        self.assertServiceRunning(interface)
+
+        # a failure-count below 2 is deliberately allowed here, unlike the
+        # server: it renders "ping-restart 10", a working "restart after one
+        # missed ping"
+        self.cli_set(path + ['keep-alive', 'interval', '10'])
+        self.cli_set(path + ['keep-alive', 'failure-count', '1'])
+        self.cli_commit()
+
+        config = read_file(f'/run/openvpn/{interface}.conf')
+        self.assertIn('ping-restart 10', config.splitlines())
+
         self.cli_delete(base_path)
         self.cli_commit()
 
diff --git a/src/conf_mode/interfaces_openvpn.py b/src/conf_mode/interfaces_openvpn.py
index c8f4ebc17..130c027a4 100755
--- a/src/conf_mode/interfaces_openvpn.py
+++ b/src/conf_mode/interfaces_openvpn.py
@@ -449,6 +449,27 @@ def verify(openvpn):
     # OpenVPN site-to-site - VERIFY
     #
     elif openvpn['mode'] == 'site-to-site':
+        # The rendered "ping-restart" timeout is interval * failure-count.
+        # OpenVPN caps ping and ping-restart at 24 hours, and nothing doubles
+        # the value here the way the server-mode "keepalive" is doubled, so
+        # the whole day is available.
+        #
+        # There is deliberately no "failure-count must be at least 2" rule to
+        # match the server: that one exists because "keepalive" wants its
+        # timeout to be at least twice its interval, a check that lives in
+        # OpenVPN's helper_keepalive() and only runs for "keepalive". Raw
+        # "ping"/"ping-restart" carry no such rule, and a failure-count of 0
+        # renders "ping-restart 0", which is a working "ping, never restart".
+        keep_alive = openvpn['keep_alive']
+        interval = int(keep_alive['interval'])
+        timeout = interval * int(keep_alive['failure_count'])
+
+        # a zero interval turns the keepalive off and renders nothing at all
+        if interval > 0 and timeout > 86400:
+            raise ConfigError(
+                f'Keepalive timeout of {timeout} seconds cannot exceed 86400'
+            )
+
         if 'ip_version' in openvpn and openvpn['ip_version'] == 'dual-stack':
             raise ConfigError('"ip-version dual-stack" is not supported in site-to-site mode')
 
diff --git a/src/migration-scripts/openvpn/5-to-6 b/src/migration-scripts/openvpn/5-to-6
index 9a6de85cd..fc4dccb38 100644
--- a/src/migration-scripts/openvpn/5-to-6
+++ b/src/migration-scripts/openvpn/5-to-6
@@ -17,6 +17,11 @@
 # can not serve, because OpenVPN 2.6 silently ignored the module. Now that the
 # offload is real those combinations are rejected, so drop the request from
 # configurations that would otherwise fail to load.
+#
+# T9363: site-to-site rendered "ping-restart <failure-count>", so the count
+# landed as a number of seconds while the server has always rendered
+# interval * failure-count. Now that both agree, a timeout OpenVPN refuses is
+# reachable in site-to-site too, so the same clamp has to cover it.
 
 from vyos.configtree import ConfigTree
 
@@ -94,8 +99,9 @@ def _value(config: ConfigTree, path: list):
 
 
 def _clamp_keepalive(config: ConfigTree, path: list) -> None:
-    """Bring a server keepalive within what verify() accepts."""
-    if _value(config, path + ['mode']) != 'server':
+    """Bring a keepalive within what verify() accepts."""
+    mode = _value(config, path + ['mode'])
+    if mode not in ['server', 'site-to-site']:
         return
 
     # both nodes carry a CLI default, so they render even when absent
@@ -107,11 +113,19 @@ def _clamp_keepalive(config: ConfigTree, path: list) -> None:
     if interval < 1:
         return
 
-    # the timeout is interval * failure-count: it has to reach twice the
-    # interval and stay below 12 hours. Beyond an interval of 21600 no count
-    # satisfies both - the CLI range stops at 600, so only a hand-edited
-    # configuration gets there and its interval is rejected anyway.
-    wanted = min(max(count, 2), max(2, 43200 // interval))
+    if mode == 'site-to-site':
+        # site-to-site renders "ping-restart" itself, which OpenVPN caps at
+        # 24 hours and does not double. It carries no "at least twice the
+        # interval" rule either, since that one belongs to "keepalive".
+        wanted = min(count, 86400 // interval)
+    else:
+        # the timeout is interval * failure-count: it has to reach twice the
+        # interval and stay below 12 hours. Beyond an interval of 21600 no
+        # count satisfies both - the CLI range stops at 600, so only a
+        # hand-edited configuration gets there and its interval is rejected
+        # anyway.
+        wanted = min(max(count, 2), max(2, 43200 // interval))
+
     if wanted != count:
         config.set(path + ['keep-alive', 'failure-count'], value=str(wanted))
 
diff --git a/src/tests/test_openvpn_migration.py b/src/tests/test_openvpn_migration.py
index 53640a0a6..2784c4f50 100644
--- a/src/tests/test_openvpn_migration.py
+++ b/src/tests/test_openvpn_migration.py
@@ -437,6 +437,58 @@ class TestOpenVPNMigration(TestCase):
         tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count']
         self.assertEqual(config.return_value(tmp), '1000')
 
+    def test_site_to_site_keepalive_left_alone_when_valid(self):
+        config = config_tree(
+            '        mode site-to-site\n'
+            '        keep-alive {\n'
+            '            interval 10\n'
+            '            failure-count 60\n'
+            '        }\n'
+        )
+        self.migrate(config)
+        tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count']
+        self.assertEqual(config.return_value(tmp), '60')
+
+    def test_site_to_site_keepalive_over_a_day_is_clamped(self):
+        config = config_tree(
+            '        mode site-to-site\n'
+            '        keep-alive {\n'
+            '            interval 600\n'
+            '            failure-count 1000\n'
+            '        }\n'
+        )
+        self.migrate(config)
+        tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count']
+        # 600 * 144 is exactly 24 hours, and nothing doubles it here
+        self.assertEqual(config.return_value(tmp), '144')
+
+    def test_site_to_site_disabled_keepalive_survives(self):
+        config = config_tree(
+            '        mode site-to-site\n'
+            '        keep-alive {\n'
+            '            interval 0\n'
+            '            failure-count 1000\n'
+            '        }\n'
+        )
+        self.migrate(config)
+        tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive']
+        self.assertEqual(config.return_value(tmp + ['interval']), '0')
+        self.assertEqual(config.return_value(tmp + ['failure-count']), '1000')
+
+    def test_site_to_site_keeps_a_failure_count_below_two(self):
+        # only "keepalive" wants the timeout to be twice the interval, and
+        # site-to-site renders no "keepalive"
+        config = config_tree(
+            '        mode site-to-site\n'
+            '        keep-alive {\n'
+            '            interval 10\n'
+            '            failure-count 1\n'
+            '        }\n'
+        )
+        self.migrate(config)
+        tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count']
+        self.assertEqual(config.return_value(tmp), '1')
+
     def test_raw_cipher_drops_dco_in_site_to_site(self):
         # site-to-site renders neither "client" nor "server", so OpenVPN takes
         # a raw "--cipher" as the fallback cipher and gives up the offload