From 8381edc12a116eee3a524ee5d060c057cb039f47 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Mon, 3 Aug 2026 23:14:08 +0200 Subject: ntp: T9159: add source-address option for client requests chrony (the daemon backing VyOS' NTP service) has no way to pin the local address used for outgoing client requests to upstream servers. "interface"/"listen-address" only configure the server/listening role (binddevice/bindaddress); nothing controls the source address chosen for packets chronyd itself originates. On a router with more than one usable egress path to a given NTP server (e.g. multiple transit/peering sessions), the kernel's default source-address selection depends on whichever route wins at query time. If that route happens to egress through an interface whose own address is not globally reachable (a private peering-fabric segment, for example), the request goes out with an address the reply can never route back to - an intermittent, path-dependent failure with no existing workaround at the NTP layer. BGP and friends already solve the equivalent problem via "update-source" bound to the loopback; NTP had no analogous option. Add "service ntp source-address
", reusing the existing source-address-ipv4-ipv6-multi include (same pattern already used for e.g. RADIUS servers), and render it as chrony's "bindacqaddress" directive - the client-side counterpart to "bindaddress" that chrony already supports natively. --- src/conf_mode/service_ntp.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) (limited to 'src') diff --git a/src/conf_mode/service_ntp.py b/src/conf_mode/service_ntp.py index e734eeb76..3cb883ae1 100755 --- a/src/conf_mode/service_ntp.py +++ b/src/conf_mode/service_ntp.py @@ -94,6 +94,19 @@ def verify(ntp): if ipv6_addresses > 1: raise ConfigError(f'NTP Only admits one ipv6 value for listen-address parameter ') + if 'source_address' in ntp: + ipv4_addresses = 0 + ipv6_addresses = 0 + for address in ntp['source_address']: + if is_ipv4(address): + ipv4_addresses += 1 + else: + ipv6_addresses += 1 + if ipv4_addresses > 1: + raise ConfigError(f'NTP Only admits one ipv4 value for source-address parameter ') + if ipv6_addresses > 1: + raise ConfigError(f'NTP Only admits one ipv6 value for source-address parameter ') + if 'server' in ntp: for host, server in ntp['server'].items(): if 'ptp' in server: -- cgit v1.2.3 From 21a55fd494ef9c73744da718560ad540dd3fc0bf Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Wed, 5 Aug 2026 08:38:05 +0200 Subject: ntp: T9159: add source-interface option for client requests Complements source-address (bindacqaddress) with a device-based bind (chrony's bindacqdevice) for outgoing NTP client requests. Addresses feedback on T9159 that a source IP alone cannot unambiguously select the egress path in multi-VRF setups with overlapping address ranges. Both options bind different socket properties and can be combined. If "vrf" is set, source-interface must belong to that VRF, mirroring the existing check for "interface". --- data/templates/chrony/chrony.conf.j2 | 15 ++++++--- interface-definitions/service_ntp.xml.in | 1 + smoketest/scripts/cli/test_service_ntp.py | 56 +++++++++++++++++++++++++++++++ src/conf_mode/service_ntp.py | 14 ++++++++ 4 files changed, 81 insertions(+), 5 deletions(-) (limited to 'src') diff --git a/data/templates/chrony/chrony.conf.j2 b/data/templates/chrony/chrony.conf.j2 index 9de3e9518..271c6de87 100644 --- a/data/templates/chrony/chrony.conf.j2 +++ b/data/templates/chrony/chrony.conf.j2 @@ -72,12 +72,17 @@ binddevice {{ interface }} {% endif %} {% endif %} -{% if source_address is vyos_defined %} -# Source address used for outgoing NTP client requests, independent of -# whichever interface the kernel's routing table selects for a given server -{% for address in source_address %} +{% if source_address is vyos_defined or source_interface is vyos_defined %} +# Source used for outgoing NTP client requests, independent of whichever +# interface the kernel's routing table selects for a given server +{% if source_address is vyos_defined %} +{% for address in source_address %} bindacqaddress {{ address }} -{% endfor %} +{% endfor %} +{% endif %} +{% if source_interface is vyos_defined %} +bindacqdevice {{ source_interface }} +{% endif %} {% endif %} {% if timestamp.interface is vyos_defined %} diff --git a/interface-definitions/service_ntp.xml.in b/interface-definitions/service_ntp.xml.in index 5b481bd6e..40e4a5d15 100644 --- a/interface-definitions/service_ntp.xml.in +++ b/interface-definitions/service_ntp.xml.in @@ -13,6 +13,7 @@ #include #include #include + #include #include diff --git a/smoketest/scripts/cli/test_service_ntp.py b/smoketest/scripts/cli/test_service_ntp.py index 054cbeda9..6f1bb82e2 100755 --- a/smoketest/scripts/cli/test_service_ntp.py +++ b/smoketest/scripts/cli/test_service_ntp.py @@ -151,6 +151,62 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): self.cli_delete(base_path + ['source-address']) self.cli_commit() + def test_source_interface(self): + interface = 'eth0' + self.cli_set(base_path + ['source-interface', interface]) + + servers = ['time1.vyos.net', 'time2.vyos.net'] + for server in servers: + self.cli_set(base_path + ['server', server]) + + self.cli_commit() + + # Check generated client source-interface configuration + config = read_file(NTP_CONF, sudo=True) + self.assertIn(f'bindacqdevice {interface}', config) + + def test_source_address_and_source_interface(self): + # bindacqaddress (IP) and bindacqdevice (interface) bind different + # socket properties and can be configured together + source_addresses = ['127.0.0.1', '::1'] + interface = 'eth0' + for address in source_addresses: + self.cli_set(base_path + ['source-address', address]) + self.cli_set(base_path + ['source-interface', interface]) + + servers = ['time1.vyos.net', 'time2.vyos.net'] + for server in servers: + self.cli_set(base_path + ['server', server]) + + self.cli_commit() + + config = read_file(NTP_CONF, sudo=True) + for address in source_addresses: + self.assertIn(f'bindacqaddress {address}', config) + self.assertIn(f'bindacqdevice {interface}', config) + + def test_source_interface_vrf_mismatch(self): + vrf_name = 'vyos-mgmt' + interface = 'eth0' + + self.cli_set(['vrf', 'name', vrf_name, 'table', '12345']) + self.cli_set(base_path + ['vrf', vrf_name]) + self.cli_set(base_path + ['source-interface', interface]) + + servers = ['time1.vyos.net', 'time2.vyos.net'] + for server in servers: + self.cli_set(base_path + ['server', server]) + + # eth0 does not belong to the VRF - commit must be rejected + with self.assertRaises(ConfigSessionError): + self.cli_commit() + + # fix the invalid combination so chronyd is left running for tearDown + self.cli_delete(base_path + ['source-interface']) + self.cli_delete(base_path + ['vrf']) + self.cli_delete(['vrf', 'name', vrf_name]) + self.cli_commit() + def test_interface(self): interfaces = ['eth0'] for interface in interfaces: diff --git a/src/conf_mode/service_ntp.py b/src/conf_mode/service_ntp.py index 3cb883ae1..adc161a9d 100755 --- a/src/conf_mode/service_ntp.py +++ b/src/conf_mode/service_ntp.py @@ -81,6 +81,20 @@ def verify(ntp): raise ConfigError(f'NTP runs in VRF "{vrf_name}" - "{interface}" '\ f'does not belong to this VRF!') + if 'source_interface' in ntp: + # If outgoing NTP client requests should be bound to a given + # interface (device), ensure it exists + source_interface = ntp['source_interface'] + verify_interface_exists(ntp, source_interface) + + # If we run in a VRF, our source interface must belong to this VRF, too + if 'vrf' in ntp: + tmp = get_interface_config(source_interface) + vrf_name = ntp['vrf'] + if 'master' not in tmp or tmp['master'] != vrf_name: + raise ConfigError(f'NTP runs in VRF "{vrf_name}" - "{source_interface}" '\ + f'does not belong to this VRF!') + if 'listen_address' in ntp: ipv4_addresses = 0 ipv6_addresses = 0 -- cgit v1.2.3 From d798592061b9c5063ae849227d09a3f0c0af78cc Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Wed, 5 Aug 2026 14:42:26 +0200 Subject: ntp: T9159: verify source-address is locally assigned, fix ruff findings - reject non-local source-address, matching the is_addr_assigned pattern already used by system_syslog.py/system_login.py/etc, addressing CodeRabbit review feedback on PR #5371 - fix ruff findings in the touched file: extraneous f-string prefixes, E701 multi-statement line, and the same in the smoketest file - update the two duplicate-address negative tests to use locally assigned addresses (via a dummy interface) so they actually exercise the per-family duplicate check instead of incidentally tripping the new local-assignment check; add a dedicated non-local rejection test --- smoketest/scripts/cli/test_service_ntp.py | 27 ++++++++++++++++++++++++--- src/conf_mode/service_ntp.py | 16 +++++++++++----- 2 files changed, 35 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/smoketest/scripts/cli/test_service_ntp.py b/smoketest/scripts/cli/test_service_ntp.py index 6f1bb82e2..75fb3172c 100755 --- a/smoketest/scripts/cli/test_service_ntp.py +++ b/smoketest/scripts/cli/test_service_ntp.py @@ -27,6 +27,8 @@ from vyos.xml_ref import default_value PROCESS_NAME = 'chronyd' NTP_CONF = '/run/chrony/chrony.conf' base_path = ['service', 'ntp'] +dummy_interface = 'dum9159' +dummy_if_path = ['interfaces', 'dummy', dummy_interface] class TestSystemNTP(VyOSUnitTestSHIM.TestCase): @classmethod @@ -130,6 +132,10 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): self.assertIn(f'bindacqaddress {address}', config) def test_source_address_rejects_multiple_ipv4(self): + # both addresses must be locally assigned, so it's the duplicate-per- + # family check being tested here, not the local-assignment check + self.cli_set(dummy_if_path + ['address', '192.0.2.1/32']) + self.cli_set(dummy_if_path + ['address', '192.0.2.2/32']) self.cli_set(base_path + ['source-address', '192.0.2.1']) self.cli_set(base_path + ['source-address', '192.0.2.2']) with self.assertRaises(ConfigSessionError): @@ -138,9 +144,14 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): # remove the invalid subtree entirely (rather than leaving a # non-local address configured) so chronyd is left running for tearDown self.cli_delete(base_path + ['source-address']) + self.cli_delete(dummy_if_path) self.cli_commit() def test_source_address_rejects_multiple_ipv6(self): + # both addresses must be locally assigned, so it's the duplicate-per- + # family check being tested here, not the local-assignment check + self.cli_set(dummy_if_path + ['address', '2001:db8::1/128']) + self.cli_set(dummy_if_path + ['address', '2001:db8::2/128']) self.cli_set(base_path + ['source-address', '2001:db8::1']) self.cli_set(base_path + ['source-address', '2001:db8::2']) with self.assertRaises(ConfigSessionError): @@ -148,6 +159,16 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): # remove the invalid subtree entirely (rather than leaving a # non-local address configured) so chronyd is left running for tearDown + self.cli_delete(base_path + ['source-address']) + self.cli_delete(dummy_if_path) + self.cli_commit() + + def test_source_address_rejects_non_local(self): + # source-address must be assigned to a local interface + self.cli_set(base_path + ['source-address', '192.0.2.1']) + with self.assertRaises(ConfigSessionError): + self.cli_commit() + self.cli_delete(base_path + ['source-address']) self.cli_commit() @@ -259,9 +280,9 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): if mode != 'smear': self.assertIn(f'leapsecmode {mode}', config) else: - self.assertIn(f'leapsecmode slew', config) - self.assertIn(f'maxslewrate 1000', config) - self.assertIn(f'smoothtime 400 0.001024 leaponly', config) + self.assertIn('leapsecmode slew', config) + self.assertIn('maxslewrate 1000', config) + self.assertIn('smoothtime 400 0.001024 leaponly', config) def test_interleave_option(self): # "interleave" option differs from some others in that the diff --git a/src/conf_mode/service_ntp.py b/src/conf_mode/service_ntp.py index adc161a9d..7a49be6c1 100755 --- a/src/conf_mode/service_ntp.py +++ b/src/conf_mode/service_ntp.py @@ -25,6 +25,7 @@ from vyos.netlink import timestamp from vyos.utils.process import call from vyos.utils.permission import chmod_750 from vyos.utils.network import get_interface_config +from vyos.utils.network import is_addr_assigned from vyos.template import render from vyos.template import is_ipv4 from vyos import ConfigError @@ -49,7 +50,8 @@ def get_config(config=None): ntp['user'] = user_group tmp = is_node_changed(conf, base + ['vrf']) - if tmp: ntp.update({'restart_required': {}}) + if tmp: + ntp.update({'restart_required': {}}) # We have gathered the dict representation of the CLI, but there are default # options which we need to update into the dictionary retrieved. @@ -104,22 +106,26 @@ def verify(ntp): else: ipv6_addresses += 1 if ipv4_addresses > 1: - raise ConfigError(f'NTP Only admits one ipv4 value for listen-address parameter ') + raise ConfigError('NTP Only admits one ipv4 value for listen-address parameter') if ipv6_addresses > 1: - raise ConfigError(f'NTP Only admits one ipv6 value for listen-address parameter ') + raise ConfigError('NTP Only admits one ipv6 value for listen-address parameter') if 'source_address' in ntp: ipv4_addresses = 0 ipv6_addresses = 0 + vrf = ntp.get('vrf') for address in ntp['source_address']: if is_ipv4(address): ipv4_addresses += 1 else: ipv6_addresses += 1 + if not is_addr_assigned(address, vrf): + raise ConfigError(f'NTP source-address "{address}" not assigned ' + 'to any interface!') if ipv4_addresses > 1: - raise ConfigError(f'NTP Only admits one ipv4 value for source-address parameter ') + raise ConfigError('NTP Only admits one ipv4 value for source-address parameter') if ipv6_addresses > 1: - raise ConfigError(f'NTP Only admits one ipv6 value for source-address parameter ') + raise ConfigError('NTP Only admits one ipv6 value for source-address parameter') if 'server' in ntp: for host, server in ntp['server'].items(): -- cgit v1.2.3 From d3b48b76bd71fec1d09db6c72963de797f8ccb93 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Mon, 10 Aug 2026 15:40:48 +0200 Subject: ntp: T9159: apply darker/black formatting --- smoketest/scripts/cli/test_service_ntp.py | 13 +++++++------ src/conf_mode/service_ntp.py | 27 +++++++++++++++++++-------- 2 files changed, 26 insertions(+), 14 deletions(-) (limited to 'src') diff --git a/smoketest/scripts/cli/test_service_ntp.py b/smoketest/scripts/cli/test_service_ntp.py index 03caa73b3..0b8094963 100755 --- a/smoketest/scripts/cli/test_service_ntp.py +++ b/smoketest/scripts/cli/test_service_ntp.py @@ -145,8 +145,8 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): self.cli_set(base_path + ['source-address', '192.0.2.2']) try: with self.assertRaisesRegex( - ConfigSessionError, - 'Only admits one ipv4 value for source-address'): + ConfigSessionError, 'Only admits one ipv4 value for source-address' + ): self.cli_commit() finally: # remove the invalid subtree entirely (rather than leaving a @@ -172,8 +172,8 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): self.cli_set(base_path + ['source-address', '2001:db8::2']) try: with self.assertRaisesRegex( - ConfigSessionError, - 'Only admits one ipv6 value for source-address'): + ConfigSessionError, 'Only admits one ipv6 value for source-address' + ): self.cli_commit() finally: # see the ipv4 test above for why this is in a finally block @@ -185,8 +185,9 @@ class TestSystemNTP(VyOSUnitTestSHIM.TestCase): # source-address must be assigned to a local interface self.cli_set(base_path + ['source-address', '192.0.2.1']) try: - with self.assertRaisesRegex(ConfigSessionError, - 'not assigned to any interface'): + with self.assertRaisesRegex( + ConfigSessionError, 'not assigned to any interface' + ): self.cli_commit() finally: self.cli_delete(base_path + ['source-address']) diff --git a/src/conf_mode/service_ntp.py b/src/conf_mode/service_ntp.py index 7a49be6c1..66cf775a3 100755 --- a/src/conf_mode/service_ntp.py +++ b/src/conf_mode/service_ntp.py @@ -94,8 +94,10 @@ def verify(ntp): tmp = get_interface_config(source_interface) vrf_name = ntp['vrf'] if 'master' not in tmp or tmp['master'] != vrf_name: - raise ConfigError(f'NTP runs in VRF "{vrf_name}" - "{source_interface}" '\ - f'does not belong to this VRF!') + raise ConfigError( + f'NTP runs in VRF "{vrf_name}" - "{source_interface}" ' + f'does not belong to this VRF!' + ) if 'listen_address' in ntp: ipv4_addresses = 0 @@ -106,9 +108,13 @@ def verify(ntp): else: ipv6_addresses += 1 if ipv4_addresses > 1: - raise ConfigError('NTP Only admits one ipv4 value for listen-address parameter') + raise ConfigError( + 'NTP Only admits one ipv4 value for listen-address parameter' + ) if ipv6_addresses > 1: - raise ConfigError('NTP Only admits one ipv6 value for listen-address parameter') + raise ConfigError( + 'NTP Only admits one ipv6 value for listen-address parameter' + ) if 'source_address' in ntp: ipv4_addresses = 0 @@ -120,12 +126,17 @@ def verify(ntp): else: ipv6_addresses += 1 if not is_addr_assigned(address, vrf): - raise ConfigError(f'NTP source-address "{address}" not assigned ' - 'to any interface!') + raise ConfigError( + f'NTP source-address "{address}" not assigned ' 'to any interface!' + ) if ipv4_addresses > 1: - raise ConfigError('NTP Only admits one ipv4 value for source-address parameter') + raise ConfigError( + 'NTP Only admits one ipv4 value for source-address parameter' + ) if ipv6_addresses > 1: - raise ConfigError('NTP Only admits one ipv6 value for source-address parameter') + raise ConfigError( + 'NTP Only admits one ipv6 value for source-address parameter' + ) if 'server' in ntp: for host, server in ntp['server'].items(): -- cgit v1.2.3