From bec592084c505e87cc3b3c3c3c98e779410c1b43 Mon Sep 17 00:00:00 2001 From: Abtin Keshavarzian Date: Mon, 17 Jul 2023 13:09:06 -0700 Subject: [PATCH] [dnssd-server] skip additional records on a PTR query with multiple answers (#9281) This commit updates `Dns::ServiceDiscovery::Server` such that when answering a PTR query with more than one answer, it does not include additional records. This is to keep the size of the response small. This commit also updates the test scripts validating browse (PTR query) function to check the new behavior. In particular, a common python function `_parse_dns_service_info()` is added to parse service info in CLI output of "dns browse" or "dns service" commands and handle if output of "dns browse" does not include service info. --- src/cli/README.md | 18 +++-- src/core/net/dnssd_server.cpp | 9 +++ .../test_dnssd_instance_name_with_space.py | 2 + .../border_router/test_dnssd_server.py | 10 --- .../border_router/test_mdns_restart.py | 3 + tests/scripts/thread-cert/node.py | 77 +++++++++---------- tests/scripts/thread-cert/test_dnssd.py | 16 ++-- .../test_srp_server_anycast_mode.py | 3 +- 8 files changed, 72 insertions(+), 66 deletions(-) diff --git a/src/cli/README.md b/src/cli/README.md index 99876c8af..3ad8e6c20 100644 --- a/src/cli/README.md +++ b/src/cli/README.md @@ -1236,6 +1236,19 @@ Send a browse (service instance enumeration) DNS query to get the list of servic The parameters after `service-name` are optional. Any unspecified (or zero) value for these optional parameters is replaced by the value from the current default config (`dns config`). +```bash +> dns browse _service._udp.example.com +DNS browse response for _service._udp.example.com. +inst1 +inst2 +inst3 +Done +``` + +The detailed service info (port number, weight, host name, TXT data, host addresses) is outputted only when provided by server/resolver in the browse response (in additional Data Section). This is a SHOULD and not a MUST requirement, and servers/resolvers are not required to provide this. + +The recommended behavior, which is supported by the OpenThread DNS-SD resolver, is to only provide the additional data when there is a single instance in the response. However, users should assume that the browse response may only contain the list of matching service instances and not any detail service info. To resolve a service instance, users can use the `dns service` or `dns servicehost` commands. + ```bash > dns browse _service._udp.example.com DNS browse response for _service._udp.example.com. @@ -1244,11 +1257,6 @@ inst1 Host:host.example.com. HostAddress:fd00:0:0:0:0:0:0:abcd TTL:7200 TXT:[a=6531, b=6c12] TTL:7300 -instance2 - Port:1234, Priority:1, Weight:2, TTL:7200 - Host:host.example.com. - HostAddress:fd00:0:0:0:0:0:0:abcd TTL:7200 - TXT:[a=1234] TTL:7300 Done ``` diff --git a/src/core/net/dnssd_server.cpp b/src/core/net/dnssd_server.cpp index 19f5c8645..c9a7414ee 100644 --- a/src/core/net/dnssd_server.cpp +++ b/src/core/net/dnssd_server.cpp @@ -734,6 +734,15 @@ Header::Response Server::ResolveBySrp(Header &aResponseHeader, IgnoreError(aResponseMessage.Read(readOffset, question)); readOffset += sizeof(question); + if ((question.GetType() == ResourceRecord::kTypePtr) && (aResponseHeader.GetAnswerCount() > 1)) + { + // Skip adding additional records, when answering a + // PTR query with more than one answer. This is the + // recommended behavior to keep the size of the + // response small. + continue; + } + VerifyOrExit(Header::kResponseServerFailure != ResolveQuestionBySrp(name, question, aResponseHeader, aResponseMessage, aCompressInfo, /* aAdditional */ true), diff --git a/tests/scripts/thread-cert/border_router/test_dnssd_instance_name_with_space.py b/tests/scripts/thread-cert/border_router/test_dnssd_instance_name_with_space.py index 720b7bc36..b752ecbc2 100755 --- a/tests/scripts/thread-cert/border_router/test_dnssd_instance_name_with_space.py +++ b/tests/scripts/thread-cert/border_router/test_dnssd_instance_name_with_space.py @@ -114,6 +114,8 @@ class TestDnssdInstanceNameWithSpace(thread_cert.TestCase): full_instance_name = f'{INSTANCE_NAME}.{SERVICE_FULL_NAME}' EMPTY_TXT = {} + # In all cases, there is one match, so server should include + # service info in additional section of PTR query response. self._verify_service_browse_result(client.dns_browse(SERVICE_FULL_NAME, server=br1.get_rloc())) self._verify_service_browse_result(client.dns_browse(SERVICE_FULL_NAME, server=br2.get_rloc())) self._verify_service_browse_result(client.dns_browse(SERVICE_FULL_NAME.lower(), server=br2.get_rloc())) diff --git a/tests/scripts/thread-cert/border_router/test_dnssd_server.py b/tests/scripts/thread-cert/border_router/test_dnssd_server.py index d31bf0725..8bff2b2e6 100755 --- a/tests/scripts/thread-cert/border_router/test_dnssd_server.py +++ b/tests/scripts/thread-cert/border_router/test_dnssd_server.py @@ -136,16 +136,6 @@ class TestDnssdServerOnBr(thread_cert.TestCase): 'QUESTION': [(SERVICE_FULL_NAME, 'IN', 'PTR')], 'ANSWER': [(SERVICE_FULL_NAME, 'IN', 'PTR', f'ins1.{SERVICE_FULL_NAME}'), (SERVICE_FULL_NAME, 'IN', 'PTR', f'ins2.{SERVICE_FULL_NAME}')], - 'ADDITIONAL': [ - (ins1_full_name, 'IN', 'SRV', 1, 1, 11111, host1_full_name), - (ins1_full_name, 'IN', 'TXT', EMPTY_TXT), - (host1_full_name, 'IN', 'AAAA', client1_addrs[0]), - (host1_full_name, 'IN', 'AAAA', client1_addrs[1]), - (ins2_full_name, 'IN', 'SRV', 2, 2, 22222, host2_full_name), - (ins2_full_name, 'IN', 'TXT', EMPTY_TXT), - (host2_full_name, 'IN', 'AAAA', client2_addrs[0]), - (host2_full_name, 'IN', 'AAAA', client2_addrs[1]), - ], }) # check if SRV query works diff --git a/tests/scripts/thread-cert/border_router/test_mdns_restart.py b/tests/scripts/thread-cert/border_router/test_mdns_restart.py index 6bcd90813..ea2ef8155 100755 --- a/tests/scripts/thread-cert/border_router/test_mdns_restart.py +++ b/tests/scripts/thread-cert/border_router/test_mdns_restart.py @@ -152,6 +152,9 @@ class MdnsRestart(thread_cert.TestCase): self.assertEqual(len(host.browse_mdns_services('_ed2._tcp')), 1) ed1.dns_set_config(br1.get_ip6_address(config.ADDRESS_TYPE.ML_EID)) + + # Since there is only one match, server should include + # service info in additional section of its response. browsed_services = ed1.dns_browse('_ed2._tcp.default.service.arpa') self.assertEqual(len(browsed_services), 1) self.assertEqual(browsed_services['ed2']['port'], 12345) diff --git a/tests/scripts/thread-cert/node.py b/tests/scripts/thread-cert/node.py index a5e78e372..81e86576a 100755 --- a/tests/scripts/thread-cert/node.py +++ b/tests/scripts/thread-cert/node.py @@ -3310,6 +3310,31 @@ class NodeImpl: return list(zip(ip, ttl)) + def _parse_dns_service_info(self, output): + # Example of `output` + # Port:22222, Priority:2, Weight:2, TTL:7155 + # Host:host2.default.service.arpa. + # HostAddress:0:0:0:0:0:0:0:0 TTL:0 + # TXT:[a=00, b=02bb] TTL:7155 + + m = re.match( + r'.*Port:(\d+), Priority:(\d+), Weight:(\d+), TTL:(\d+)\s+Host:(.*?)\s+HostAddress:(\S+) TTL:(\d+)\s+TXT:\[(.*?)\] TTL:(\d+)', + '\r'.join(output)) + if not m: + return {} + port, priority, weight, srv_ttl, hostname, address, aaaa_ttl, txt_data, txt_ttl = m.groups() + return { + 'port': int(port), + 'priority': int(priority), + 'weight': int(weight), + 'host': hostname, + 'address': address, + 'txt_data': txt_data, + 'srv_ttl': int(srv_ttl), + 'txt_ttl': int(txt_ttl), + 'aaaa_ttl': int(aaaa_ttl), + } + def dns_resolve_service(self, instance, service, server=None, port=53): """ Resolves the service instance and returns the instance information as a dict. @@ -3335,33 +3360,10 @@ class NodeImpl: self.send_command(cmd) self.simulator.go(10) output = self._expect_command_output() - - # Example output: - # DNS service resolution response for ins2 for service _ipps._tcp.default.service.arpa. - # Port:22222, Priority:2, Weight:2, TTL:7155 - # Host:host2.default.service.arpa. - # HostAddress:0:0:0:0:0:0:0:0 TTL:0 - # TXT:[a=00, b=02bb] TTL:7155 - # Done - - m = re.match( - r'.*Port:(\d+), Priority:(\d+), Weight:(\d+), TTL:(\d+)\s+Host:(.*?)\s+HostAddress:(\S+) TTL:(\d+)\s+TXT:\[(.*?)\] TTL:(\d+)', - '\r'.join(output)) - if m: - port, priority, weight, srv_ttl, hostname, address, aaaa_ttl, txt_data, txt_ttl = m.groups() - return { - 'port': int(port), - 'priority': int(priority), - 'weight': int(weight), - 'host': hostname, - 'address': address, - 'txt_data': txt_data, - 'srv_ttl': int(srv_ttl), - 'txt_ttl': int(txt_ttl), - 'aaaa_ttl': int(aaaa_ttl), - } - else: + info = self._parse_dns_service_info(output) + if not info: raise Exception('dns resolve service failed: %s.%s' % (instance, service)) + return info @staticmethod def __parse_hex_string(hexstr: str) -> bytes: @@ -3404,9 +3406,10 @@ class NodeImpl: self.send_command(cmd) self.simulator.go(10) - output = '\n'.join(self._expect_command_output()) + output = self._expect_command_output() # Example output: + # DNS browse response for _ipps._tcp.default.service.arpa. # ins2 # Port:22222, Priority:2, Weight:2, TTL:7175 # Host:host2.default.service.arpa. @@ -3420,21 +3423,11 @@ class NodeImpl: # Done result = {} - for ins, port, priority, weight, srv_ttl, hostname, address, aaaa_ttl, txt_data, txt_ttl in re.findall( - r'(.*?)\s+Port:(\d+), Priority:(\d+), Weight:(\d+), TTL:(\d+)\s*Host:(\S+)\s+HostAddress:(\S+) TTL:(\d+)\s+TXT:\[(.*?)\] TTL:(\d+)', - output): - result[ins] = { - 'port': int(port), - 'priority': int(priority), - 'weight': int(weight), - 'host': hostname, - 'address': address, - 'txt_data': txt_data, - 'srv_ttl': int(srv_ttl), - 'txt_ttl': int(txt_ttl), - 'aaaa_ttl': int(aaaa_ttl), - } - + index = 1 # skip first line + while index < len(output): + ins = output[index].strip() + result[ins] = self._parse_dns_service_info(output[index + 1:index + 6]) + index = index + (5 if result[ins] else 1) return result def set_mliid(self, mliid: str): diff --git a/tests/scripts/thread-cert/test_dnssd.py b/tests/scripts/thread-cert/test_dnssd.py index e3c88babf..0110c1efb 100755 --- a/tests/scripts/thread-cert/test_dnssd.py +++ b/tests/scripts/thread-cert/test_dnssd.py @@ -172,16 +172,14 @@ class TestDnssd(thread_cert.TestCase): # Browse for main service service_instances = client1.dns_browse(f'{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) self.assertEqual({'ins1', 'ins2', 'ins3'}, set(service_instances.keys())) - self._assert_service_instance_equal(service_instances['ins1'], instance1_verify_info) - self._assert_service_instance_equal(service_instances['ins2'], instance2_verify_info) - self._assert_service_instance_equal(service_instances['ins3'], instance3_verify_info) # Browse for service sub-type _s1. service_instances = client1.dns_browse(f'_s1._sub.{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) self.assertEqual({'ins1', 'ins3'}, set(service_instances.keys())) - self._assert_service_instance_equal(service_instances['ins1'], instance1_verify_info) # Browse for service sub-type _s2. + # Since there is only one matching instance, validate that + # server included the service info in additional section. service_instances = client1.dns_browse(f'_s2._sub.{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) self.assertEqual({'ins1'}, set(service_instances.keys())) self._assert_service_instance_equal(service_instances['ins1'], instance1_verify_info) @@ -195,6 +193,9 @@ class TestDnssd(thread_cert.TestCase): service_instance = client1.dns_resolve_service('ins2', f'{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) self._assert_service_instance_equal(service_instance, instance2_verify_info) + service_instance = client1.dns_resolve_service('ins3', f'{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) + self._assert_service_instance_equal(service_instance, instance3_verify_info) + #--------------------------------------------------------------- # Add another service with TXT entries to the existing host and # verify that it is properly merged. @@ -204,10 +205,9 @@ class TestDnssd(thread_cert.TestCase): service_instances = client1.dns_browse(f'{SERVICE}.{DOMAIN}', server.get_mleid(), 53) self.assertEqual({'ins1', 'ins2', 'ins3', 'ins4'}, set(service_instances.keys())) - self._assert_service_instance_equal(service_instances['ins1'], instance1_verify_info) - self._assert_service_instance_equal(service_instances['ins2'], instance2_verify_info) - self._assert_service_instance_equal(service_instances['ins3'], instance3_verify_info) - self._assert_service_instance_equal(service_instances['ins4'], instance4_verify_info) + + service_instance = client1.dns_resolve_service('ins4', f'{SERVICE}.{DOMAIN}'.upper(), server.get_mleid(), 53) + self._assert_service_instance_equal(service_instance, instance4_verify_info) def _assert_service_instance_equal(self, instance, info): self.assertEqual(instance['host'].lower(), info['host'].lower(), instance) diff --git a/tests/scripts/thread-cert/test_srp_server_anycast_mode.py b/tests/scripts/thread-cert/test_srp_server_anycast_mode.py index 38a83218d..5086a7baa 100755 --- a/tests/scripts/thread-cert/test_srp_server_anycast_mode.py +++ b/tests/scripts/thread-cert/test_srp_server_anycast_mode.py @@ -177,7 +177,8 @@ class TestSrpServerAnycastMode(thread_cert.TestCase): #--------------------------------------------------------------- # Browse for a matching service name and verify that the registered - # service is successfully found. + # service is successfully found. Since there is only one match the + # server should include the service info in additional section. service_instances = browser.dns_browse(f'{SERVICE}.{DOMAIN}', server.get_mleid(), DNS_RESOLVER_PORT) self.assertEqual({INSTANCE}, set(service_instances.keys()))