From b6abe23ba2d2a5d5055afd203bc3e60ee20bf757 Mon Sep 17 00:00:00 2001 From: Antonio Quartulli Date: Thu, 24 Sep 2026 16:32:15 +0200 Subject: 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. --- data/templates/openvpn/server.conf.j2 | 2 +- smoketest/scripts/cli/test_interfaces_openvpn.py | 30 +++++++++++++- src/conf_mode/interfaces_openvpn.py | 21 ++++++++++ src/migration-scripts/openvpn/5-to-6 | 28 +++++++++---- src/tests/test_openvpn_migration.py | 52 ++++++++++++++++++++++++ 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 ", 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 -- cgit v1.2.3