[chrony-dev] [PATCH] ntp: allow NTS pools to use multiple negotiated servers

[Sofia Scalzo <sofiascalzo@xxxxxxxx>]

From: Sofia Scalzo <scalzosofiapriv@xxxxxxxxx>

Allow an unresolved member of an NTS pool to reuse the NTS-KE address
after another member moves to a negotiated NTP server address.

This enables a pool resolving to one NTS-KE endpoint to discover
multiple distinct NTP responders up to maxsources. Each tentative member
retains independent NTS keys, cookies, and protocol state.

Duplicate negotiated addresses continue to be rejected through the
existing source address collision handling. Preserve the behavior of
sources that are not part of a pool and normal DNS pool resolution.

Add deterministic unit coverage for immediate and deferred address
updates, duplicate responders, port only updates, pool pruning, and
sources that are not part of a pool.
---

Testing:

The new unit tests cover immediate and deferred address updates,
duplicate negotiated addresses, port only changes, sources that are not
part of a pool, confirmation and pruning at maxsources, and pool
reconstruction.

I also tested the patched build against the Cloudflare NTS pool:

$ cat /etc/chrony.conf
pool time.cloudflare.com nts iburst maxsources 4

$ sudo ~/chrony/chronyc -n authdata
Name/IP address             Mode KeyID Type KLen Last Atmp NAK Cook CLen
========================================================================
162.159.200.123              NTS     1   30  128  140    0   0    8   64
162.159.200.1                NTS     1   30  128  140    0   0    8   64

$ sudo ~/chrony/chronyc sources
MS Name/IP address         Stratum Poll Reach LastRx Last sample
===============================================================================
^* time.cloudflare.com           3   6    77    24   -29us[ +94us] +/- 13ms
^+ time.cloudflare.com           3   6    77    24   -71us[ -71us] +/- 13ms

The live test discovered two distinct pool addresses. Both sources
authenticated independently with NTS and received eight cookies. The
handoff from a single resolved endpoint is exercised directly by the
deterministic unit test.

The focused ntp_sources unit test, ten seeded runs, and the full unit
test suite passed. make quickcheck completed without failures.
Simulation and system tests were skipped because their required
environments were not available.

I'm happy to make any changes you'd find appropriate.

Best,
Sofia

 ntp_sources.c           |  48 ++++++++--
 test/unit/ntp_sources.c | 195 +++++++++++++++++++++++++++++++++++++++-
 2 files changed, 233 insertions(+), 10 deletions(-)

diff --git a/ntp_sources.c b/ntp_sources.c
index c120118..e59d90a 100644
--- a/ntp_sources.c
+++ b/ntp_sources.c
@@ -156,6 +156,8 @@ static struct AddressUpdate saved_address_update;
 static void resolve_sources(void);
 static void rehash_records(void);
 static void handle_saved_address_update(void);
+static NSR_Status update_source_ntp_address(NTP_Remote_Address *old_addr,
+                                            NTP_Remote_Address *new_addr);
 static void clean_source_record(SourceRecord *record);
 static void remove_pool_sources(int pool_id, int tentative, int unresolved);
 static void remove_unresolved_source(struct UnresolvedSource *us);
@@ -499,17 +501,19 @@ change_source_address(NTP_Remote_Address *old_addr, NTP_Remote_Address *new_addr
 static void
 handle_saved_address_update(void)
 {
+  struct AddressUpdate update;
+
   if (!UTI_IsIPReal(&saved_address_update.old_address.ip_addr))
     return;

-  if (change_source_address(&saved_address_update.old_address,
-                            &saved_address_update.new_address, 0) != NSR_Success)
+  update = saved_address_update;
+  saved_address_update.old_address.ip_addr.family = IPADDR_UNSPEC;
+
+  if (update_source_ntp_address(&update.old_address, &update.new_address) != NSR_Success)
     /* This is expected to happen only if the old address is wrong */
     LOG(LOGS_ERR, "Could not change %s to %s",
-        UTI_IPSockAddrToString(&saved_address_update.old_address),
-        UTI_IPSockAddrToString(&saved_address_update.new_address));
-
-  saved_address_update.old_address.ip_addr.family = IPADDR_UNSPEC;
+        UTI_IPSockAddrToString(&update.old_address),
+        UTI_IPSockAddrToString(&update.new_address));
 }

 /* ================================================== */
@@ -1199,6 +1203,36 @@ NSR_RefreshAddresses(void)

 /* ================================================== */

+static NSR_Status
+update_source_ntp_address(NTP_Remote_Address *old_addr, NTP_Remote_Address *new_addr)
+{
+  struct UnresolvedSource *us;
+  SourceRecord *record;
+  IPAddr freed_addr;
+  NSR_Status status;
+  int slot;
+
+  us = NULL;
+  freed_addr = old_addr->ip_addr;
+  if (UTI_CompareIPs(&old_addr->ip_addr, &new_addr->ip_addr, NULL) != 0 &&
+      find_slot2(old_addr, &slot) == 2) {
+    record = get_record(slot);
+    if (record->pool_id != INVALID_POOL &&
+        get_pool(record->pool_id)->unresolved_sources > 0 &&
+        UTI_CompareIPs(&old_addr->ip_addr, &record->resolved_addr, NULL) == 0)
+      for (us = unresolved_sources; us && us->pool_id != record->pool_id; us = us->next)
+        ;
+  }
+
+  status = change_source_address(old_addr, new_addr, 0);
+  if (status == NSR_Success && us)
+    process_resolved_name(us, &freed_addr, 1);
+
+  return status;
+}
+
+/* ================================================== */
+
 NSR_Status
 NSR_UpdateSourceNtpAddress(NTP_Remote_Address *old_addr, NTP_Remote_Address *new_addr)
 {
@@ -1215,7 +1249,7 @@ NSR_UpdateSourceNtpAddress(NTP_Remote_Address *old_addr, NTP_Remote_Address *new
      source is just being created), postpone the change to avoid corruption */

   if (!record_lock)
-    return change_source_address(old_addr, new_addr, 0);
+    return update_source_ntp_address(old_addr, new_addr);

   if (UTI_IsIPReal(&saved_address_update.old_address.ip_addr))
     return NSR_TooManySources;
diff --git a/test/unit/ntp_sources.c b/test/unit/ntp_sources.c
index 5038675..7b06531 100644
--- a/test/unit/ntp_sources.c
+++ b/test/unit/ntp_sources.c
@@ -31,6 +31,12 @@
 static char *requested_name = NULL;
 static DNS_NameResolveHandler resolve_handler = NULL;
 static void *resolve_handler_arg = NULL;
+static int deterministic_mode = 0;
+static int forced_address_update = 0;
+static int process_rx_known = 0;
+static int server_connectable = 1;
+static NTP_Remote_Address forced_old_address;
+static NTP_Remote_Address forced_new_address;

 #define DNS_Name2IPAddressAsync(name, handler, arg) \
   requested_name = (name), \
@@ -38,8 +44,10 @@ static void *resolve_handler_arg = NULL;
   resolve_handler_arg = (arg)
 #define NCR_ChangeRemoteAddress(inst, remote_addr, ntp_only) \
   change_remote_address(inst, remote_addr, ntp_only)
-#define NCR_ProcessRxKnown(remote_addr, local_addr, ts, msg, len) (random() % 2)
-#define NIO_IsServerConnectable(addr) (random() % 2)
+#define NCR_ProcessRxKnown(remote_addr, local_addr, ts, msg, len) \
+  (deterministic_mode ? process_rx_known : random() % 2)
+#define NIO_IsServerConnectable(addr) \
+  (deterministic_mode ? server_connectable : random() % 2)
 #define SCH_GetLastEventMonoTime() get_mono_time()

 static void change_remote_address(NCR_Instance inst, NTP_Remote_Address *remote_addr,
@@ -94,10 +102,23 @@ update_random_address(NTP_Remote_Address *addr, int rand_bits)
 static void
 change_remote_address(NCR_Instance inst, NTP_Remote_Address *remote_addr, int ntp_only)
 {
-  int update = !ntp_only && random() % 4 == 0, update_pos = random() % 2, r = 0;
+  int update, update_pos, r = 0;

   TEST_CHECK(record_lock);

+  if (deterministic_mode) {
+    if (forced_address_update) {
+      forced_address_update = 0;
+      TEST_CHECK(NSR_UpdateSourceNtpAddress(&forced_old_address,
+                                            &forced_new_address) == NSR_Success);
+    }
+    NCR_ChangeRemoteAddress(inst, remote_addr, ntp_only);
+    return;
+  }
+
+  update = !ntp_only && random() % 4 == 0;
+  update_pos = random() % 2;
+
   if (update && update_pos == 0)
     r = update_random_address(random() % 2 ? remote_addr : NCR_GetRemoteAddress(inst), 4);

@@ -119,6 +140,172 @@ static double get_mono_time(void) {
   return t;
 }

+static NTP_Remote_Address
+make_remote_address(const char *address, int port)
+{
+  NTP_Remote_Address remote_addr;
+
+  TEST_CHECK(UTI_StringToIP(address, &remote_addr.ip_addr));
+  remote_addr.port = port;
+
+  return remote_addr;
+}
+
+static void
+remove_test_pool(struct UnresolvedSource *us, uint32_t conf_id)
+{
+  remove_unresolved_source(us);
+  NSR_RemoveSourcesById(conf_id);
+  TEST_CHECK(n_sources == 0);
+}
+
+static void
+test_address_update_controls(CPS_NTP_Source *source)
+{
+  NTP_Remote_Address server, new_server, pool_addrs[2], port_update;
+  IPAddr resolved_addrs[2];
+  struct UnresolvedSource *us;
+  uint32_t conf_id;
+  int pool_id, slot;
+
+  server = make_remote_address("192.0.2.10", 123);
+  new_server = make_remote_address("192.0.2.11", 123);
+  TEST_CHECK(NSR_AddSource(&server, NTP_SERVER, &source->params, &conf_id) == NSR_Success);
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&server, &new_server) == NSR_Success);
+  TEST_CHECK(n_sources == 1);
+  TEST_CHECK(find_slot2(&new_server, &slot) == 2);
+  NSR_RemoveSourcesById(conf_id);
+
+  source->params.max_sources = 2;
+  TEST_CHECK(NSR_AddSourceByName("pool.example.net", IPADDR_UNSPEC, 123, 1,
+                                 NTP_SERVER, &source->params, &conf_id) ==
+             NSR_UnresolvedName);
+  us = unresolved_sources;
+  pool_id = us->pool_id;
+  pool_addrs[0] = make_remote_address("192.0.2.20", 123);
+  pool_addrs[1] = make_remote_address("192.0.2.21", 123);
+  resolved_addrs[0] = pool_addrs[0].ip_addr;
+  resolved_addrs[1] = pool_addrs[1].ip_addr;
+  process_resolved_name(us, resolved_addrs, 2);
+  TEST_CHECK(find_slot2(&pool_addrs[0], &slot) == 2);
+  TEST_CHECK(find_slot2(&pool_addrs[1], &slot) == 2);
+  TEST_CHECK(get_pool(pool_id)->unresolved_sources == 2);
+
+  port_update = pool_addrs[0];
+  port_update.port++;
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&pool_addrs[0], &port_update) == NSR_Success);
+  TEST_CHECK(get_pool(pool_id)->unresolved_sources == 2);
+  TEST_CHECK(find_slot2(&port_update, &slot) == 2);
+
+  server_connectable = 0;
+  process_resolved_name(us, &server.ip_addr, 1);
+  TEST_CHECK(get_pool(pool_id)->unresolved_sources == 2);
+  server_connectable = 1;
+
+  remove_test_pool(us, conf_id);
+  source->params.max_sources = 4;
+}
+
+static void
+run_nts_pool_address_handoff(CPS_NTP_Source *source, NTP_Local_Address *local_addr,
+                              NTP_Local_Timestamp *local_ts, int test_deferred)
+{
+  NTP_Remote_Address ke, missing, port_update, responders[4];
+  struct UnresolvedSource *us;
+  struct SourcePool *pool;
+  NTP_Packet message;
+  uint32_t conf_id;
+  int i, pool_id, slot;
+
+  TEST_CHECK(NSR_AddSourceByName("nts-pool.example.net", IPADDR_UNSPEC, 123, 1,
+                                 NTP_SERVER, &source->params, &conf_id) ==
+             NSR_UnresolvedName);
+  us = unresolved_sources;
+  pool_id = us->pool_id;
+  pool = get_pool(pool_id);
+  TEST_CHECK(pool->sources == 8);
+  TEST_CHECK(pool->unresolved_sources == 8);
+
+  ke = make_remote_address("192.0.2.1", 123);
+  process_resolved_name(us, &ke.ip_addr, 1);
+  TEST_CHECK(find_slot2(&ke, &slot) == 2);
+  TEST_CHECK(pool->unresolved_sources == 7);
+
+  port_update = ke;
+  port_update.port++;
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &port_update) == NSR_Success);
+  ke = port_update;
+  TEST_CHECK(pool->unresolved_sources == 7);
+
+  missing = make_remote_address("192.0.2.2", 123);
+  responders[0] = make_remote_address("192.0.2.101", 123);
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&missing, &responders[0]) == NSR_NoSuchSource);
+  TEST_CHECK(pool->unresolved_sources == 7);
+
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &responders[0]) == NSR_Success);
+  TEST_CHECK(pool->unresolved_sources == 6);
+  ke.port = 123;
+  TEST_CHECK(find_slot2(&ke, &slot) == 2);
+
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &responders[0]) == NSR_AlreadyInUse);
+  TEST_CHECK(pool->unresolved_sources == 6);
+  TEST_CHECK(find_slot2(&ke, &slot) == 2);
+
+  responders[1] = make_remote_address("192.0.2.102", 123);
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &responders[1]) == NSR_Success);
+  TEST_CHECK(pool->unresolved_sources == 5);
+
+  responders[2] = make_remote_address("192.0.2.103", 123);
+  if (test_deferred) {
+    forced_old_address = ke;
+    forced_new_address = responders[2];
+    forced_address_update = 1;
+    port_update = responders[0];
+    port_update.port++;
+    TEST_CHECK(replace_source_connectable(&responders[0], &port_update));
+    responders[0] = port_update;
+    TEST_CHECK(!forced_address_update);
+    TEST_CHECK(!record_lock);
+    TEST_CHECK(!UTI_IsIPReal(&saved_address_update.old_address.ip_addr));
+  } else {
+    TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &responders[2]) == NSR_Success);
+  }
+  TEST_CHECK(pool->unresolved_sources == 4);
+
+  responders[3] = make_remote_address("192.0.2.104", 123);
+  TEST_CHECK(NSR_UpdateSourceNtpAddress(&ke, &responders[3]) == NSR_Success);
+  TEST_CHECK(pool->unresolved_sources == 3);
+
+  process_rx_known = 1;
+  message.lvm = NTP_LVM(0, NTP_VERSION, MODE_SERVER);
+  for (i = 0; i < 4; i++) {
+    NSR_ProcessRx(&responders[i], local_addr, local_ts, &message, 0);
+    TEST_CHECK(pool->confirmed_sources == i + 1);
+    TEST_CHECK(n_sources == (i < 3 ? 8 : 4));
+  }
+  TEST_CHECK(pool->unresolved_sources == 0);
+  TEST_CHECK(!find_slot(&ke.ip_addr, &slot));
+
+  remove_test_pool(us, conf_id);
+  process_rx_known = 0;
+}
+
+static void
+test_nts_pool_address_handoff(CPS_NTP_Source *source, NTP_Local_Address *local_addr,
+                              NTP_Local_Timestamp *local_ts)
+{
+  deterministic_mode = 1;
+  server_connectable = 1;
+  source->params.max_sources = 4;
+
+  test_address_update_controls(source);
+  run_nts_pool_address_handoff(source, local_addr, local_ts, 1);
+  run_nts_pool_address_handoff(source, local_addr, local_ts, 0);
+
+  deterministic_mode = 0;
+  server_connectable = 1;
+}
+
 void
 test_unit(void)
 {
@@ -223,6 +410,8 @@ test_unit(void)
   local_addr.sock_fd = 0;
   memset(&local_ts, 0, sizeof (local_ts));

+  test_nts_pool_address_handoff(&source, &local_addr, &local_ts);
+
   for (i = 0; i < 500; i++) {
     for (j = 0; j < 20; j++) {
       snprintf(name, sizeof (name), "ntp%d.example.net", (int)(random() % 10));

base-commit: d77973ee3ce2d2209ccd55cd0665103024a12a43
--
2.53.0-Meta

-- 
To unsubscribe email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "unsubscribe" in the subject.
For help email chrony-dev-request@xxxxxxxxxxxxxxxxxxxx with "help" in the subject.
Trouble?  Email listmaster@xxxxxxxxxxxxxxxxxxxx.