From 585c491b3ee5c02a0367bf2bd752fe4510200c6a Mon Sep 17 00:00:00 2001 From: Antonio Quartulli Date: Sun, 30 Aug 2026 19:43:51 +0200 Subject: openvpn: T8264: migrate away DCO the Kernel module cannot serve "offload dco" was accepted next to tap, static keys, compression, CBC ciphers and raw options OpenVPN refuses to offload, because 2.6 ignored the module altogether. Those combinations now fail verify(), which would break config load on upgrade, so drop the request instead. The migration has to mirror verify_dco() exactly: anything it keeps that verify() turns away leaves a router unable to commit its own configuration. --- src/migration-scripts/openvpn/5-to-6 | 95 ++++++++++++++++++++++++++++++++++++ 1 file changed, 95 insertions(+) create mode 100644 src/migration-scripts/openvpn/5-to-6 (limited to 'src/migration-scripts') diff --git a/src/migration-scripts/openvpn/5-to-6 b/src/migration-scripts/openvpn/5-to-6 new file mode 100644 index 000000000..46c9aaeb7 --- /dev/null +++ b/src/migration-scripts/openvpn/5-to-6 @@ -0,0 +1,95 @@ +# Copyright VyOS maintainers and contributors +# +# This library is free software; you can redistribute it and/or +# modify it under the terms of the GNU Lesser General Public +# License as published by the Free Software Foundation; either +# version 2.1 of the License, or (at your option) any later version. +# +# This library is distributed in the hope that it will be useful, +# but WITHOUT ANY WARRANTY; without even the implied warranty of +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU +# Lesser General Public License for more details. +# +# You should have received a copy of the GNU Lesser General Public License +# along with this library. If not, see . + +# T8264: "offload dco" used to be accepted next to settings the Kernel module +# 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. + +from vyos.configtree import ConfigTree + +base = ['interfaces', 'openvpn'] +dco_ciphers = ['aes128gcm', 'aes192gcm', 'aes256gcm'] +dco_incompatible_options = [ + 'comp-lzo', + 'disable-dco', + 'fragment', + 'http-proxy', + 'management-query-proxy', + 'socks-proxy', +] +dco_conditional_options = { + 'allow-compression': 'no', + 'compress': 'migrate', +} + + +def _offloadable(options: list) -> bool: + """Whether every raw option leaves the data path offloadable.""" + for option in options: + tmp = option.split() + if not tmp: + continue + keyword = tmp[0].lstrip('-') + if keyword in dco_incompatible_options: + return False + keep = dco_conditional_options.get(keyword) + if keep is not None and tmp[1:] != [keep]: + return False + return True + + +def _value(config: ConfigTree, path: list): + # return_value() raises on a missing path, and nodes carrying a CLI + # default are usually absent from a saved configuration + return config.return_value(path) if config.exists(path) else None + + +def migrate(config: ConfigTree) -> None: + if not config.exists(base): + return + + for interface in config.list_nodes(base): + path = base + [interface] + if not config.exists(path + ['offload', 'dco']): + continue + + ciphers = [] + if config.exists(path + ['encryption', 'data-ciphers']): + ciphers += config.return_values(path + ['encryption', 'data-ciphers']) + fallback = _value(config, path + ['encryption', 'data-ciphers-fallback']) + if fallback: + ciphers.append(fallback) + + options = [] + if config.exists(path + ['openvpn-option']): + options = config.return_values(path + ['openvpn-option']) + + topology = None + if _value(config, path + ['mode']) == 'server': + topology = _value(config, path + ['server', 'topology']) + + if ( + _value(config, path + ['device-type']) == 'tap' + or config.exists(path + ['shared-secret-key']) + or config.exists(path + ['use-lzo-compression']) + or (topology is not None and topology != 'subnet') + or any(cipher not in dco_ciphers for cipher in ciphers) + or not _offloadable(options) + ): + offload = path + ['offload'] + config.delete(offload + ['dco']) + if config.exists(offload) and not config.list_nodes(offload): + config.delete(offload) -- cgit v1.2.3 From 2985a69d02e01c2642ce70dda9bf81051ee99095 Mon Sep 17 00:00:00 2001 From: Antonio Quartulli Date: Tue, 1 Sep 2026 01:28:51 +0200 Subject: openvpn: T8264: keep the keepalive within what OpenVPN accepts The CLI renders "keepalive " while its two ranges are independent, so it can produce parameters the daemon refuses to start on: a timeout below twice the interval, or one beyond the 12 hours OpenVPN is about to cap ping and keepalive at. A zero interval renders "keepalive 0 0", on which OpenVPN skips these checks and simply runs without keepalive, so leave that combination alone. Migrate what stays rejected, or an upgrade would leave a configuration the router can no longer commit at boot. --- smoketest/scripts/cli/test_interfaces_openvpn.py | 61 ++++++++++++++++++++-- src/conf_mode/interfaces_openvpn.py | 22 ++++++++ src/migration-scripts/openvpn/5-to-6 | 27 ++++++++++ src/tests/test_openvpn_migration.py | 64 ++++++++++++++++++++++++ 4 files changed, 169 insertions(+), 5 deletions(-) (limited to 'src/migration-scripts') diff --git a/smoketest/scripts/cli/test_interfaces_openvpn.py b/smoketest/scripts/cli/test_interfaces_openvpn.py index abbf0bd86..6c480a526 100755 --- a/smoketest/scripts/cli/test_interfaces_openvpn.py +++ b/smoketest/scripts/cli/test_interfaces_openvpn.py @@ -26,6 +26,7 @@ from base_vyostest_shim import VyOSUnitTestSHIM from vyos.configsession import ConfigSessionError from vyos.utils.process import cmdl from vyos.utils.process import process_named_running +from vyos.utils.process import is_systemd_service_running from vyos.utils.file import read_file from vyos.template import address_from_cidr from vyos.template import inc_ip @@ -438,19 +439,19 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): # check validate() - DCO is tun only self.cli_set(path + ['device-type', 'tap']) - with self.assertRaisesRegex(ConfigSessionError, 'device-type tun'): + with self.assertRaisesRegex(ConfigSessionError, r'device-type\s+tun'): self.cli_commit() self.cli_set(path + ['device-type', 'tun']) # check validate() - DCO serves "topology subnet" only self.cli_set(path + ['server', 'topology', 'net30']) - with self.assertRaisesRegex(ConfigSessionError, 'topology subnet'): + with self.assertRaisesRegex(ConfigSessionError, r'topology\s+subnet'): self.cli_commit() self.cli_set(path + ['server', 'topology', 'subnet']) # check validate() - DCO implements AES-GCM only self.cli_set(path + ['encryption', 'data-ciphers', 'aes256']) - with self.assertRaisesRegex(ConfigSessionError, 'does not support cipher'): + with self.assertRaisesRegex(ConfigSessionError, r'support\s+cipher'): self.cli_commit() self.cli_delete(path + ['encryption', 'data-ciphers', 'aes256']) @@ -462,7 +463,7 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): # check validate() - a raw option OpenVPN refuses to offload self.cli_set(path + ['openvpn-option', '--fragment 1300']) - with self.assertRaisesRegex(ConfigSessionError, 'openvpn-option fragment'): + with self.assertRaisesRegex(ConfigSessionError, r'openvpn-option\s+fragment'): self.cli_commit() self.cli_delete(path + ['openvpn-option', '--fragment 1300']) @@ -742,7 +743,7 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): self.cli_set(path + ['offload', 'dco']) self.cli_set(path + ['encryption', 'cipher', '3des']) - with self.assertRaisesRegex(ConfigSessionError, 'deprecated for TLS mode'): + with self.assertRaisesRegex(ConfigSessionError, r'deprecated\s+for\s+TLS'): self.cli_commit() # without the cipher the same tunnel is accepted, so the rejection is @@ -1006,6 +1007,56 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): self.cli_delete(base_path) self.cli_commit() + def test_openvpn_server_keepalive_limit(self): + # The CLI ranges still allow a keepalive timeout beyond what OpenVPN + # accepts once the limit lands + interface = 'vtun5000' + path = base_path + [interface] + + self.cli_set(path + ['mode', 'server']) + self.cli_set(path + ['local-port', '2000']) + self.cli_set(path + ['server', 'subnet', '192.0.2.0/24']) + self.cli_set(path + ['tls', 'ca-certificate', 'ovpn_test']) + self.cli_set(path + ['tls', 'certificate', 'ovpn_test']) + self.cli_set(path + ['tls', 'dh-params', 'ovpn_test']) + + # check validate() - OpenVPN needs both parameters positive and the + # timeout at least twice the interval + self.cli_set(path + ['keep-alive', 'failure-count', '1']) + with self.assertRaisesRegex(ConfigSessionError, r'at\s+least\s+2'): + self.cli_commit() + + # check validate() - interval * failure-count exceeds 12 hours + self.cli_set(path + ['keep-alive', 'interval', '600']) + self.cli_set(path + ['keep-alive', 'failure-count', '1000']) + with self.assertRaisesRegex(ConfigSessionError, r'cannot\s+exceed\s+43200'): + self.cli_commit() + + # 600 * 73 is only just over, so the constant is pinned from both sides + self.cli_set(path + ['keep-alive', 'failure-count', '73']) + with self.assertRaisesRegex(ConfigSessionError, r'cannot\s+exceed\s+43200'): + self.cli_commit() + + # 600 * 72 is exactly 12 hours, which OpenVPN still accepts + self.cli_set(path + ['keep-alive', 'failure-count', '72']) + self.cli_commit() + + # a config the daemon refuses still commits, so check it came up + self.assertTrue(is_systemd_service_running(f'openvpn@{interface}.service')) + config = read_file(f'/run/openvpn/{interface}.conf') + self.assertIn('keepalive 600 43200', config) + + # "interval 0" renders "keepalive 0 0" - OpenVPN skips its own checks + # on that and runs without keepalive, so it has to keep committing + # whatever the failure-count says + self.cli_set(path + ['keep-alive', 'interval', '0']) + self.cli_set(path + ['keep-alive', 'failure-count', '1']) + self.cli_commit() + + self.assertTrue(is_systemd_service_running(f'openvpn@{interface}.service')) + config = read_file(f'/run/openvpn/{interface}.conf') + self.assertIn('keepalive 0 0', config) + def test_openvpn_server_reject_unconfigured_clients(self): # T8998: reject-unconfigured-clients without server client entries # must emit both client-config-dir and ccd-exclusive diff --git a/src/conf_mode/interfaces_openvpn.py b/src/conf_mode/interfaces_openvpn.py index 1f85d98f1..02b2cb6f5 100755 --- a/src/conf_mode/interfaces_openvpn.py +++ b/src/conf_mode/interfaces_openvpn.py @@ -483,6 +483,28 @@ def verify(openvpn): if openvpn['protocol'] == 'tcp-active': raise ConfigError('Protocol "tcp-active" is not valid in server mode') + # The rendered "keepalive" timeout is interval * failure-count. OpenVPN + # limits ping and keepalive to 24 hours and doubles the timeout on the + # server, so the rendered value can not exceed 12 hours. + keep_alive = openvpn['keep_alive'] + interval = int(keep_alive['interval']) + failure_count = int(keep_alive['failure_count']) + timeout = interval * failure_count + + # A zero interval renders "keepalive 0 0", on which OpenVPN skips its + # own sanity checks - that is how a configuration turns keepalive off, + # so only an enabled one has to satisfy them + if interval > 0: + # the timeout has to be at least twice the interval, which the CLI + # ranges do not enforce + if failure_count < 2: + raise ConfigError('Keepalive "failure-count" must be at least 2') + + if timeout > 43200: + raise ConfigError( + f'Keepalive timeout of {timeout} seconds cannot exceed 43200' + ) + if dict_search('authentication.username', openvpn) or dict_search('authentication.password', openvpn): raise ConfigError('Cannot specify "authentication" in server mode') diff --git a/src/migration-scripts/openvpn/5-to-6 b/src/migration-scripts/openvpn/5-to-6 index 46c9aaeb7..8f1abc99d 100644 --- a/src/migration-scripts/openvpn/5-to-6 +++ b/src/migration-scripts/openvpn/5-to-6 @@ -57,12 +57,39 @@ def _value(config: ConfigTree, path: list): return config.return_value(path) if config.exists(path) else None +def _clamp_keepalive(config: ConfigTree, path: list) -> None: + """Bring a server keepalive within what verify() accepts.""" + if _value(config, path + ['mode']) != 'server': + return + + # both nodes carry a CLI default, so they render even when absent + interval = int(_value(config, path + ['keep-alive', 'interval']) or 10) + count = int(_value(config, path + ['keep-alive', 'failure-count']) or 60) + + # a zero interval renders "keepalive 0 0" and turns keepalive off, which + # OpenVPN and verify() both leave alone + 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 wanted != count: + config.set(path + ['keep-alive', 'failure-count'], value=str(wanted)) + + def migrate(config: ConfigTree) -> None: if not config.exists(base): return for interface in config.list_nodes(base): path = base + [interface] + + # unrelated to the offload, so it runs for every interface + _clamp_keepalive(config, path) + if not config.exists(path + ['offload', 'dco']): continue diff --git a/src/tests/test_openvpn_migration.py b/src/tests/test_openvpn_migration.py index bd0bbdbbe..3cd50c36b 100644 --- a/src/tests/test_openvpn_migration.py +++ b/src/tests/test_openvpn_migration.py @@ -212,3 +212,67 @@ class TestOpenVPNMigration(TestCase): ) self.migrate(config) self.assertTrue(config.exists(dco)) + + def test_keepalive_left_alone_when_valid(self): + config = config_tree( + ' mode server\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_disabled_keepalive_survives(self): + # "interval 0" renders "keepalive 0 0", which turns keepalive off and + # loads fine - the migration must not turn it into something else + config = config_tree( + ' mode server\n' + ' keep-alive {\n' + ' interval 0\n' + ' failure-count 60\n' + ' }\n' + ) + self.migrate(config) + base_ka = ['interfaces', 'openvpn', 'vtun10', 'keep-alive'] + self.assertEqual(config.return_value(base_ka + ['interval']), '0') + self.assertEqual(config.return_value(base_ka + ['failure-count']), '60') + + def test_keepalive_below_twice_the_interval_is_clamped(self): + config = config_tree( + ' mode server\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), '2') + + def test_keepalive_over_twelve_hours_is_clamped(self): + config = config_tree( + ' mode server\n' + ' keep-alive {\n' + ' interval 600\n' + ' failure-count 1000\n' + ' }\n' + ) + self.migrate(config) + tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count'] + # 600 * 72 is exactly 12 hours + self.assertEqual(config.return_value(tmp), '72') + + def test_keepalive_ignored_outside_server_mode(self): + config = config_tree( + ' mode client\n' + ' keep-alive {\n' + ' interval 600\n' + ' failure-count 1000\n' + ' }\n' + ) + self.migrate(config) + tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count'] + self.assertEqual(config.return_value(tmp), '1000') -- cgit v1.2.3 From 043992126c638940d26ee075de84d8d457bd446d Mon Sep 17 00:00:00 2001 From: Antonio Quartulli Date: Thu, 3 Sep 2026 01:46:38 +0200 Subject: openvpn: T8264: reject the raw options DCO cannot serve Raw options are appended after --config and therefore override what the CLI rendered, so they have to face the same checks. A single cipher the Kernel module does not implement makes OpenVPN fall back to the userspace data path; "ncp-ciphers" is the 2.4 name for the negotiation list, and outside server and pull mode "cipher" becomes the fallback cipher. "dev-type" other than tun, an AF_UNIX "dev-node" and, in server mode, a "topology" other than subnet drop the offload just the same. "DEFAULT" stands for the built-in list, which OpenVPN expands to AEAD ciphers alone before it weighs the offload, and the migration drops the offload from whatever stays rejected. --- smoketest/scripts/cli/test_interfaces_openvpn.py | 43 ++++++ src/conf_mode/interfaces_openvpn.py | 48 +++++- src/migration-scripts/openvpn/5-to-6 | 45 +++++- src/tests/test_openvpn_migration.py | 187 +++++++++++++++++++++++ 4 files changed, 317 insertions(+), 6 deletions(-) (limited to 'src/migration-scripts') diff --git a/smoketest/scripts/cli/test_interfaces_openvpn.py b/smoketest/scripts/cli/test_interfaces_openvpn.py index 6c480a526..dcdacd81a 100755 --- a/smoketest/scripts/cli/test_interfaces_openvpn.py +++ b/smoketest/scripts/cli/test_interfaces_openvpn.py @@ -467,8 +467,39 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): self.cli_commit() self.cli_delete(path + ['openvpn-option', '--fragment 1300']) + # check validate() - a raw negotiation list overrides the CLI one, so a + # cipher DCO can not serve must be caught there as well + self.cli_set(path + ['openvpn-option', '--data-ciphers AES-256-CBC']) + with self.assertRaisesRegex(ConfigSessionError, r'support\s+cipher'): + self.cli_commit() + self.cli_delete(path + ['openvpn-option', '--data-ciphers AES-256-CBC']) + + # check validate() - OpenVPN still honours the 2.4 name for it + self.cli_set(path + ['openvpn-option', '--ncp-ciphers AES-256-CBC']) + with self.assertRaisesRegex(ConfigSessionError, r'support\s+cipher'): + self.cli_commit() + self.cli_delete(path + ['openvpn-option', '--ncp-ciphers AES-256-CBC']) + + # an offloadable raw list is fine, ChaCha20-Poly1305 included + self.cli_set( + path + ['openvpn-option', '--data-ciphers AES-256-GCM:CHACHA20-POLY1305'] + ) + self.cli_commit() + self.cli_delete( + path + ['openvpn-option', '--data-ciphers AES-256-GCM:CHACHA20-POLY1305'] + ) + + # so is the built-in list, which OpenVPN expands to AEAD ciphers alone + self.cli_set(path + ['openvpn-option', '--data-ciphers DEFAULT']) + self.cli_commit() + self.cli_delete(path + ['openvpn-option', '--data-ciphers DEFAULT']) + self.cli_commit() + # every rejection above stopped in verify(), so the daemon still has to + # be running on the configuration this test started from + self.assertTrue(is_systemd_service_running(f'openvpn@{interface}.service')) + def test_openvpn_server_subnet_topology(self): # Create OpenVPN server interfaces using different client subnets. # Validate configuration afterwards. @@ -756,6 +787,18 @@ class TestInterfacesOpenVPN(VyOSUnitTestSHIM.TestCase): self.cli_set(path + ['encryption', 'data-ciphers-fallback', 'aes256gcm']) self.cli_commit() + # a raw "--cipher" is the fallback cipher in this mode, so it decides + # the offload the very same way + self.cli_set(path + ['openvpn-option', '--cipher AES-256-CBC']) + with self.assertRaisesRegex(ConfigSessionError, r'support\s+cipher'): + self.cli_commit() + + self.cli_delete(path + ['openvpn-option', '--cipher AES-256-CBC']) + self.cli_set(path + ['openvpn-option', '--cipher AES-256-GCM']) + self.cli_commit() + + self.assertTrue(is_systemd_service_running(f'openvpn@{interface}.service')) + def test_openvpn_options(self): # Ensure OpenVPN process restart on openvpn-option CLI node change diff --git a/src/conf_mode/interfaces_openvpn.py b/src/conf_mode/interfaces_openvpn.py index 02b2cb6f5..f25e03ce7 100755 --- a/src/conf_mode/interfaces_openvpn.py +++ b/src/conf_mode/interfaces_openvpn.py @@ -95,7 +95,15 @@ dco_incompatible_options = [ dco_conditional_options = { 'allow-compression': 'no', 'compress': 'migrate', + 'dev-type': 'tun', } +# Cipher negotiation lists a raw option can override, "ncp-ciphers" being the +# 2.4 spelling OpenVPN still accepts. A raw list wins over the one rendered +# from "encryption data-ciphers", so it needs the very same check. +dco_cipher_options = ['data-ciphers', 'data-ciphers-fallback', 'ncp-ciphers'] +# dco_ciphers again, spelled the way OpenVPN reports them in +# dco_get_supported_ciphers() +dco_raw_ciphers = ['AES-128-GCM', 'AES-192-GCM', 'AES-256-GCM', 'CHACHA20-POLY1305'] otp_path = '/config/auth/openvpn' otp_file = '/config/auth/openvpn/{ifname}-otp-secrets' secret_chars = list('ABCDEFGHIJKLMNOPQRSTUVWXYZ234567') @@ -229,6 +237,23 @@ def verify_data_ciphers_fallback(openvpn): if dict_search('encryption.data_ciphers_fallback', openvpn): raise ConfigError('Cipher fallback is valid only in site-to-site mode') + +def unoffloadable_cipher(value): + """The first cipher of a raw negotiation list DCO can not serve, if any.""" + for cipher in value.split(':'): + # "DEFAULT" stands for the built-in list, which OpenVPN expands - case + # sensitively, and only as a bare token - to AEAD ciphers alone before + # it weighs the offload + if cipher == 'DEFAULT': + continue + # it strips exactly one "?", and drops such a cipher only when it does + # not know it at all, so an optional one still has to be offloadable + name = cipher[1:] if cipher.startswith('?') else cipher + if name.upper() not in dco_raw_ciphers: + return cipher + return None + + def verify_dco(openvpn): if dict_search('offload.dco', openvpn) is None: return @@ -258,6 +283,18 @@ def verify_dco(openvpn): if cipher not in dco_ciphers: raise ConfigError(f'DCO does not support cipher "{cipher}"') + # Outside server and pull mode OpenVPN takes a raw "--cipher" as the + # fallback cipher, which decides the offload just like a negotiation list + # does - and site-to-site is the mode that renders neither + cipher_options = dco_cipher_options + if openvpn['mode'] == 'site-to-site': + cipher_options = cipher_options + ['cipher'] + + # "topology" only reaches OpenVPN's decision in server mode + conditional_options = dco_conditional_options + if openvpn['mode'] == 'server': + conditional_options = {**conditional_options, 'topology': 'subnet'} + # A raw option OpenVPN refuses to offload leaves the daemon on the # userspace data path, where it can not use the interface it was given for option in dict_search('openvpn_option', openvpn) or []: @@ -267,10 +304,17 @@ def verify_dco(openvpn): keyword = tmp[0].lstrip('-') if keyword in dco_incompatible_options: raise ConfigError(f'DCO is incompatible with "openvpn-option {keyword}"') - if keyword in dco_conditional_options: - keep = dco_conditional_options[keyword] + if keyword in conditional_options: + keep = conditional_options[keyword] if tmp[1:] != [keep]: raise ConfigError(f'DCO requires "openvpn-option {keyword} {keep}"') + # only an AF_UNIX node rules out the offload, a real one is fine + if keyword == 'dev-node' and tmp[1:] and tmp[1].startswith('unix:'): + raise ConfigError('DCO is incompatible with an AF_UNIX "dev-node"') + if keyword in cipher_options and tmp[1:]: + cipher = unoffloadable_cipher(tmp[1]) + if cipher is not None: + raise ConfigError(f'DCO does not support cipher "{cipher}"') def verify_pki(openvpn): pki = openvpn['pki'] diff --git a/src/migration-scripts/openvpn/5-to-6 b/src/migration-scripts/openvpn/5-to-6 index 8f1abc99d..9a6de85cd 100644 --- a/src/migration-scripts/openvpn/5-to-6 +++ b/src/migration-scripts/openvpn/5-to-6 @@ -33,11 +33,41 @@ dco_incompatible_options = [ dco_conditional_options = { 'allow-compression': 'no', 'compress': 'migrate', + 'dev-type': 'tun', } +dco_cipher_options = ['data-ciphers', 'data-ciphers-fallback', 'ncp-ciphers'] +dco_raw_ciphers = ['AES-128-GCM', 'AES-192-GCM', 'AES-256-GCM', 'CHACHA20-POLY1305'] -def _offloadable(options: list) -> bool: +def _offloadable_ciphers(value: str) -> bool: + """Whether every cipher of a raw negotiation list can be offloaded.""" + for cipher in value.split(':'): + # "DEFAULT" stands for the built-in list, which OpenVPN expands - case + # sensitively, and only as a bare token - to AEAD ciphers alone before + # it weighs the offload + if cipher == 'DEFAULT': + continue + # it strips exactly one "?", and drops such a cipher only when it does + # not know it at all, so an optional one still has to be offloadable + name = cipher[1:] if cipher.startswith('?') else cipher + if name.upper() not in dco_raw_ciphers: + return False + return True + + +def _offloadable(options: list, mode: str) -> bool: """Whether every raw option leaves the data path offloadable.""" + # site-to-site renders neither "client" nor "server", and OpenVPN then + # takes a raw "--cipher" as the fallback cipher + cipher_options = dco_cipher_options + if mode == 'site-to-site': + cipher_options = cipher_options + ['cipher'] + + # "topology" only reaches OpenVPN's decision in server mode + conditional_options = dco_conditional_options + if mode == 'server': + conditional_options = {**conditional_options, 'topology': 'subnet'} + for option in options: tmp = option.split() if not tmp: @@ -45,9 +75,15 @@ def _offloadable(options: list) -> bool: keyword = tmp[0].lstrip('-') if keyword in dco_incompatible_options: return False - keep = dco_conditional_options.get(keyword) + keep = conditional_options.get(keyword) if keep is not None and tmp[1:] != [keep]: return False + # only an AF_UNIX node rules out the offload, a real one is fine + if keyword == 'dev-node' and tmp[1:] and tmp[1].startswith('unix:'): + return False + if keyword in cipher_options and tmp[1:]: + if not _offloadable_ciphers(tmp[1]): + return False return True @@ -104,8 +140,9 @@ def migrate(config: ConfigTree) -> None: if config.exists(path + ['openvpn-option']): options = config.return_values(path + ['openvpn-option']) + mode = _value(config, path + ['mode']) topology = None - if _value(config, path + ['mode']) == 'server': + if mode == 'server': topology = _value(config, path + ['server', 'topology']) if ( @@ -114,7 +151,7 @@ def migrate(config: ConfigTree) -> None: or config.exists(path + ['use-lzo-compression']) or (topology is not None and topology != 'subnet') or any(cipher not in dco_ciphers for cipher in ciphers) - or not _offloadable(options) + or not _offloadable(options, mode) ): offload = path + ['offload'] config.delete(offload + ['dco']) diff --git a/src/tests/test_openvpn_migration.py b/src/tests/test_openvpn_migration.py index 3cd50c36b..53640a0a6 100644 --- a/src/tests/test_openvpn_migration.py +++ b/src/tests/test_openvpn_migration.py @@ -213,6 +213,166 @@ class TestOpenVPNMigration(TestCase): self.migrate(config) self.assertTrue(config.exists(dco)) + def test_raw_cipher_list_drops_dco(self): + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_one_bad_cipher_in_a_raw_list_drops_dco(self): + # OpenVPN gives up the offload over a single cipher it can not offload + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers AES-256-GCM:AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_optional_raw_cipher_drops_dco(self): + # a "?" only keeps OpenVPN from erroring out on an unknown cipher, a + # known one still lands in the negotiation list + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers ?AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_cipher_fallback_drops_dco(self): + config = config_tree( + ' mode site-to-site\n' + ' openvpn-option "--data-ciphers-fallback AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_ncp_ciphers_drops_dco(self): + # OpenVPN still accepts the 2.4 name for "data-ciphers" + config = config_tree( + ' mode server\n' + ' openvpn-option "--ncp-ciphers AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_offloadable_raw_cipher_list_keeps_dco(self): + # spelled as OpenVPN normalises it, and ChaCha20-Poly1305 is offloaded + # too even though the CLI does not offer it + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers AES-256-GCM:CHACHA20-POLY1305"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_raw_cipher_case_is_normalised(self): + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers aes-256-gcm"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_valueless_raw_cipher_option_keeps_dco(self): + # OpenVPN rejects it itself, the migration must not trip over it + config = config_tree( + ' mode server\n openvpn-option "--data-ciphers"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_default_raw_cipher_alias_keeps_dco(self): + # "DEFAULT" is the built-in list, and OpenVPN expands it to AEAD + # ciphers alone - it can never cost the interface its offload + config = config_tree( + ' mode server\n openvpn-option "--data-ciphers DEFAULT"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_default_raw_cipher_alias_is_case_sensitive(self): + # OpenVPN matches the alias with strcmp(), so a lower case spelling is + # just an unknown cipher + config = config_tree( + ' mode server\n openvpn-option "--data-ciphers default"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_optional_default_raw_cipher_alias_drops_dco(self): + # OpenVPN expands only the bare token, so "?DEFAULT" is an unknown + # optional cipher to it and never the built-in list + config = config_tree( + ' mode server\n' ' openvpn-option "--data-ciphers ?DEFAULT"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_doubly_optional_raw_cipher_drops_dco(self): + # it strips exactly one "?", leaving a cipher it does not know + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers ??AES-256-GCM"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_optional_offloadable_raw_cipher_keeps_dco(self): + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers ?AES-256-GCM"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_default_raw_cipher_alias_with_a_bad_cipher_drops_dco(self): + config = config_tree( + ' mode server\n' + ' openvpn-option "--data-ciphers DEFAULT:BF-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_dev_type_drops_dco(self): + # a raw "dev-type" overrides the one rendered from the CLI + config = config_tree( + ' mode server\n openvpn-option "--dev-type tap"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_afunix_dev_node_drops_dco(self): + config = config_tree( + ' mode server\n' + ' openvpn-option "--dev-node unix:/run/ovpn.sock"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_real_dev_node_keeps_dco(self): + # only an AF_UNIX node rules out the offload + config = config_tree( + ' mode server\n' ' openvpn-option "--dev-node /dev/net/tun"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_raw_topology_drops_dco_in_server_mode(self): + config = config_tree( + ' mode server\n openvpn-option "--topology net30"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_topology_ignored_outside_server_mode(self): + config = config_tree( + ' mode client\n openvpn-option "--topology net30"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + def test_keepalive_left_alone_when_valid(self): config = config_tree( ' mode server\n' @@ -276,3 +436,30 @@ class TestOpenVPNMigration(TestCase): self.migrate(config) tmp = ['interfaces', 'openvpn', 'vtun10', 'keep-alive', 'failure-count'] self.assertEqual(config.return_value(tmp), '1000') + + 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 + config = config_tree( + ' mode site-to-site\n' + ' openvpn-option "--cipher AES-256-CBC"\n' + ) + self.migrate(config) + self.assertFalse(config.exists(dco)) + + def test_raw_cipher_keeps_dco_in_server_mode(self): + # with "server" OpenVPN only warns that --cipher is deprecated, the + # offload is decided by the negotiation list alone + config = config_tree( + ' mode server\n openvpn-option "--cipher AES-256-CBC"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) + + def test_offloadable_raw_cipher_keeps_dco_in_site_to_site(self): + config = config_tree( + ' mode site-to-site\n' + ' openvpn-option "--cipher AES-256-GCM"\n' + ) + self.migrate(config) + self.assertTrue(config.exists(dco)) -- cgit v1.2.3