SRV records by squell · Pull Request #2 · pendulum-project/chrony

squell · 2026-02-02T10:34:20Z

I just made this pull request to be able to use GitHub's commenting feature

This type allows carrying of an additional name that is needed when using the results from an SRV lookup.

When using a server through an SRV record, the name used for the TLS certificate should match the service name, not the dns name where the SRV record was found. Therefore the name used for certificate validation needs to be updated when using the result of a DNS resolve that went through an SRV record.

This will allow the DNS lookup code to do service lookups only when doing name resolving for NTSKE servers.

This allows the actual address of an NTSKE server to be behind an SRV record, allowing SRV record based pools. Since such records also modify the name expected on the certificate, we require succesfull DNSSEC validation for these records.

This solves issues with certificates not validating for the new domain name.

This allows the SRV record to specify a custom port for an NTSKE server.

squell

I'll craft a PR to address some of the more minor style comments.

nameserv.c

squell · 2026-02-02T13:03:24Z

configure

  --disable-readline     Disable line editing support
  --without-editline     Don't use editline even if it is available
  --disable-sechash      Disable support for hashes other than MD5
+  --without-getdns       Don't use getdns


This doesn't need changing per se, but wondering why we wouldn't simply offer just one --without/--disable flag. If you disable SRV, you don't need getdns right? And without getdns, you can't do SRV lookups. So isn't this a bit superfluous?

Chose this approach as it matches the current way configure works for chrony. Figured consistency in the ux, even though it results in superfluous options, is better than inconsistency.

ntp_sources.c

squell · 2026-02-02T15:43:43Z

nameserv.c

+        LOG_FATAL("Unrecoverable error calling getdns.");
+      if (getdns_dict_get_bindata(service_entry, "domain_name", &raw_data))
+        LOG_FATAL("Unrecoverable error calling getdns.");
+      if (getdns_convert_dns_name_to_fqdn(raw_data, &service_domain))
+        LOG_FATAL("Unrecoverable error calling getnds.");
+      /* Remove any potential trailing dot as it would interfere with certificate validation*/
+      domain_name_len = strlen(service_domain);
+      if (service_domain[domain_name_len-1] == '.')
+        service_domain[domain_name_len-1] = 0;
+      //*Ignore too-long domain names */
+      if (strlen(service_domain) >= DNS_SERVICE_NAME_LEN)
+        continue;
+      /* Ignore repeated names. This is needed to deal with multiple
+         addresses from the same service. */
+      if (last_name != NULL && strcmp(last_name, service_domain) == 0)
+        continue;
+      if (getdns_dict_get_bindata(service_entry, "address_data", &raw_data)) {
+        // No pre-populated address, recurse to resolve name
+        if (DNS_Name2IPAddress(service_domain, &addrs[write_idx], 1, 0) == DNS_Success) {
+          strncpy(addrs[write_idx].service_name, service_domain, DNS_SERVICE_NAME_LEN-1);
+          write_idx++;
+          free(last_name);
+          last_name = service_domain;
+          service_domain = NULL;
+        }
+      } else {
+        switch (raw_data->size) {
+          case sizeof (addrs[write_idx].ip.addr.in4):
+            if (address_family != IPADDR_UNSPEC && address_family != IPADDR_INET4)
+              continue;
+            /* copy first to deal with the fact that alignment of data might not be okay. */
+            memcpy(&addrs[write_idx].ip.addr.in4, raw_data->data,
+              sizeof (addrs[write_idx].ip.addr.in4));
+            addrs[write_idx].ip.addr.in4 = htonl(addrs[write_idx].ip.addr.in4);
+            addrs[write_idx].ip.family = IPADDR_INET4;
+            strncpy(addrs[write_idx].service_name, service_domain, DNS_SERVICE_NAME_LEN-1);
+            write_idx++;
+            free(last_name);
+            last_name = service_domain;
+            service_domain = NULL;
+            break;
+#ifdef FEAT_IPV6
+          case sizeof (addrs[write_idx].ip.addr.in6):
+            if (address_family != IPADDR_UNSPEC && address_family != IPADDR_INET6)
+              continue;
+            memcpy(addrs[write_idx].ip.addr.in6, raw_data->data,
+              sizeof(addrs[write_idx].ip.addr.in6));
+            addrs[write_idx].ip.family = IPADDR_INET6;
+            strncpy(addrs[write_idx].service_name, service_domain, DNS_SERVICE_NAME_LEN-1);
+            write_idx++;
+            free(last_name);
+            last_name = service_domain;
+            service_domain = NULL;
+            break;
+#endif


This is a too loooong section of C code without any line breaks and hard on the eyes (and the brain). Use copious amounts of whitespace to break up logical sections (as long as you don't use multiple white space lines or don't put every statement on its own insulated line, you almost cannot use too much whitespace in C).

Or even, consider spinning it off into (a) helper function(s) with names.

nts_ntp_client.c

squell · 2026-02-03T16:07:09Z

nameserv.c

+    strcpy(service_domain, NTS_SERVICE_NAME);
+    strcat(service_domain, name);


This is a bit of nit/micro-optimization, but strcat of course raises alarm bells (strcpy does as well, but maybe to a slightly lesser degree).

Its usage here is totally safe, though.

But I think I would be tempted to write something like (assuming NTS_SERVICE_NAME is a constant):

Suggested change

strcpy(service_domain, NTS_SERVICE_NAME);

strcat(service_domain, name);

strcpy(service_domain, NTS_SERVICE_NAME);

strcpy(service_domain + strlen(NTS_SERVICE_NAME), name);

that is basically writing out strcat. I dont think I agree with that being an improvement.

squell · 2026-02-03T16:09:34Z

nameserv.c

+            memcpy(&addrs[write_idx].ip.addr.in4, raw_data->data,
+              sizeof (addrs[write_idx].ip.addr.in4));


I would not put this third argument on a separate line (similar below). Maybe Chrony has a macro for array copies?

There doesn't seem to be one, and unfortunately the extra line is needed here not to go over the line length limit. Its perhaps not ideal, but I can live with this.

squell · 2026-02-03T16:16:54Z

In general, I would like to have an interface of the form where the function that currently take a int service_nts, instead take a DNS_SRVLookupResult *, where NULL is "don't use SRV" and non-null means it's a list of SRV names that matches those pointed to by DNS_SockIPAddr.

But I've tried to quickly add this in the code base and the changes bubble up all the way one call chain from two sides and hiding these changes (as this current PR does) in the DNS_LookupResult is less involved; although the whole "empty string has a magic meaning" feels a bit hacky.

Maybe I'll revisit this in the future to give it another go.

This makes code size a bit more manageable.

davidv1992 added 6 commits January 7, 2026 14:02

dns: Introduce separate type for a DNS name lookup result.

0950b93

This type allows carrying of an additional name that is needed when using the results from an SRV lookup.

dns: Add parameter to DNS lookups to indicate whether its for NTS.

b223688

This will allow the DNS lookup code to do service lookups only when doing name resolving for NTSKE servers.

dns: Trim trailing dot from resolved SRV domain name.

a0ea1fd

This solves issues with certificates not validating for the new domain name.

dns: Use SRV record port.

c24d0c1

This allows the SRV record to specify a custom port for an NTSKE server.

squell commented Feb 3, 2026

View reviewed changes

squell and others added 5 commits March 12, 2026 09:41

rename ip->ip_addr in DNS_SockAddrLookupResult

527cfc6

formatting style

d52c0d4

rename service_nts

39bf9db

Fixup unit tests for ip->ipaddr rename.

d4c791e

Split off srv lookups into their own function.

d22573f

This makes code size a bit more manageable.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SRV records#2

SRV records#2
squell wants to merge 11 commits intomasterfrom
srvrecord

squell commented Feb 2, 2026

Uh oh!

squell left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squell Feb 2, 2026

Uh oh!

davidv1992 Mar 12, 2026

Uh oh!

Uh oh!

squell Feb 2, 2026

Uh oh!

Uh oh!

squell Feb 3, 2026

Uh oh!

davidv1992 Mar 12, 2026

Uh oh!

squell Feb 3, 2026

Uh oh!

davidv1992 Mar 12, 2026

Uh oh!

squell commented Feb 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

		strcpy(service_domain, NTS_SERVICE_NAME);
		strcat(service_domain, name);

		memcpy(&addrs[write_idx].ip.addr.in4, raw_data->data,
		sizeof (addrs[write_idx].ip.addr.in4));

Conversation

squell commented Feb 2, 2026

Uh oh!

squell left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

squell Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

davidv1992 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

squell Feb 2, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

squell Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

davidv1992 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

squell Feb 3, 2026

Choose a reason for hiding this comment

Uh oh!

davidv1992 Mar 12, 2026

Choose a reason for hiding this comment

Uh oh!

squell commented Feb 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

squell commented Feb 3, 2026 •

edited

Loading