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.

squell · 2026-02-02T13:00:21Z

nameserv.c

+  if (service_nts)
+  {


Suggested change

if (service_nts)

{

if (service_nts) {

The code style of Chrony seems to be: opening braces for a function start on their own line, but if/for/while etc are in K&R (this happens a few times below--indicated)

squell · 2026-02-02T13:01:22Z

nameserv.c

+    if (dns_context == NULL)
+    {
+      reinit();
+      if (dns_context == NULL)
+      {


Suggested change

if (dns_context == NULL)

{

reinit();

if (dns_context == NULL)

{

if (dns_context == NULL) {

reinit();

if (dns_context == NULL) {

squell · 2026-02-02T13:01:38Z

nameserv.c

+    if (getdns_status)
+    {


Suggested change

if (getdns_status)

{

if (getdns_status) {

squell · 2026-02-02T13:01:52Z

nameserv.c

+    for (i = 0; i < returned_addresses; i++)
+    {


Suggested change

for (i = 0; i < returned_addresses; i++)

{

for (i = 0; i < returned_addresses; i++) {

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?

squell · 2026-02-02T15:38:54Z

ntp_sources.c

  /* Name to be resolved */
  char *name;
+  /* Whether it should be resolved for NTS */
+  int service_nts;


(Comment applies uniformly)

Since Chrony doesn't explicitly use the bool type, naming is extra important. It's not obvious that service_nts is an on/off switch for doing the SRV lookups; especially since "service" is a noun and a verb.

Replace this with perform_srv_lookup or something (at least something that mentions SRV explicitly and starts with an unambigous action verb)

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.

squell · 2026-02-02T16:11:02Z

nts_ntp_client.c


  old_address = inst->ntp_address;
-  new_address = *negotiated_address;
+  new_address.ip.service_name[0] = 0;


Shouldn't this also set the ip.service_port to 0?

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);

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?

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SRV records#2

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

squell commented Feb 2, 2026

Uh oh!

squell left a comment

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 2, 2026

Uh oh!

squell Feb 3, 2026

Uh oh!

squell Feb 3, 2026

Uh oh!

squell commented Feb 3, 2026 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

	for (i = 0; i < returned_addresses; i++)
	{
	for (i = 0; i < returned_addresses; i++) {

		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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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