dnsdist: Better coverage of the backend's discovery code

diff --git a/pdns/dnsdistdist/dnsdist-discovery.cc b/pdns/dnsdistdist/dnsdist-discovery.cc
index 84ddb422e1895844cb3a4e5d568b4b28cc609fa8..df9b6922bccb7732f923d4e85821370e83b78e62 100644 (file)
--- a/pdns/dnsdistdist/dnsdist-discovery.cc
+++ b/pdns/dnsdistdist/dnsdist-discovery.cc
@@ -50,10 +50,6 @@ struct DesignatedResolvers
 
 static bool parseSVCParams(const PacketBuffer& answer, std::map<uint16_t, DesignatedResolvers>& resolvers)
 {
-  if (answer.size() <= sizeof(struct dnsheader)) {
-    throw std::runtime_error("Looking for SVC records in a packet smaller than a DNS header");
-  }
-
   std::map<DNSName, std::vector<ComboAddress>> hints;
   const struct dnsheader* dh = reinterpret_cast<const struct dnsheader*>(answer.data());
   PacketReader pr(pdns_string_view(reinterpret_cast<const char*>(answer.data()), answer.size()));
@@ -399,17 +395,7 @@ bool ServiceDiscovery::tryToUpgradeBackend(const UpgradeableBackend& backend)
 
   DownstreamState::Config config(backend.d_ds->d_config);
   config.remote = discoveredConfig.d_addr;
-  if (discoveredConfig.d_port != 0) {
-    config.remote.setPort(discoveredConfig.d_port);
-  }
-  else {
-    if (discoveredConfig.d_protocol == dnsdist::Protocol::DoT) {
-      config.remote.setPort(853);
-    }
-    else if (discoveredConfig.d_protocol == dnsdist::Protocol::DoH) {
-      config.remote.setPort(443);
-    }
-  }
+  config.remote.setPort(discoveredConfig.d_port);
 
   ComboAddress::addressOnlyEqual comparator;
   config.d_dohPath = discoveredConfig.d_dohPath;

diff --git a/regression-tests.dnsdist/test_BackendDiscovery.py b/regression-tests.dnsdist/test_BackendDiscovery.py
index fc06160a8d18f7738472002459f8a3f494e84788..b8ee41f4332985e7498547a0b61b313bcc49e551 100644 (file)
--- a/regression-tests.dnsdist/test_BackendDiscovery.py
+++ b/regression-tests.dnsdist/test_BackendDiscovery.py
@@ -17,11 +17,20 @@ class TestBackendDiscovery(DNSDistTest):
     _svcUpgradeDoTUnreachableBackendPort = 10606
     _svcBrokenDNSResponseBackendPort = 10607
     _svcUpgradeDoHBackendWithoutPathPort = 10608
+    _connectionRefusedBackendPort = 10609
+    _eofBackendPort = 10610
+    _servfailBackendPort = 10611
+    _wrongNameBackendPort = 10612
+    _wrongIDBackendPort = 10613
+    _tooManyQuestionsBackendPort = 10614
+    _badQNameBackendPort = 10615
+    _svcUpgradeDoTNoPortBackendPort = 10616
+    _svcUpgradeDoHNoPortBackendPort = 10617
     _upgradedBackendsPool = 'upgraded'
 
     _consoleKey = DNSDistTest.generateConsoleKey()
     _consoleKeyB64 = base64.b64encode(_consoleKey).decode('ascii')
-    _config_params = ['_consoleKeyB64', '_consolePort', '_noSVCBackendPort', '_svcNoUpgradeBackendPort', '_svcUpgradeDoTBackendPort', '_upgradedBackendsPool', '_svcUpgradeDoHBackendPort', '_svcUpgradeDoTBackendDifferentAddrPort1', '_svcUpgradeDoTBackendDifferentAddrPort2', '_svcUpgradeDoTUnreachableBackendPort', '_svcBrokenDNSResponseBackendPort', '_svcUpgradeDoHBackendWithoutPathPort']
+    _config_params = ['_consoleKeyB64', '_consolePort', '_noSVCBackendPort', '_svcNoUpgradeBackendPort', '_svcUpgradeDoTBackendPort', '_upgradedBackendsPool', '_svcUpgradeDoHBackendPort', '_svcUpgradeDoTBackendDifferentAddrPort1', '_svcUpgradeDoTBackendDifferentAddrPort2', '_svcUpgradeDoTUnreachableBackendPort', '_svcBrokenDNSResponseBackendPort', '_svcUpgradeDoHBackendWithoutPathPort', '_connectionRefusedBackendPort', '_eofBackendPort', '_servfailBackendPort', '_wrongNameBackendPort', '_wrongIDBackendPort', '_tooManyQuestionsBackendPort', '_badQNameBackendPort', '_svcUpgradeDoTNoPortBackendPort', '_svcUpgradeDoHNoPortBackendPort']
     _config_template = """
     setKey("%s")
     controlSocket("127.0.0.1:%d")
@@ -35,10 +44,10 @@ class TestBackendDiscovery(DNSDistTest):
     newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
 
     -- SVCB upgrade to DoT, same address, keep the backend, different pool
-    newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradePool='%s', autoUpgradeKeep=true}:setUp()
+    newServer{address="127.0.0.1:%s", caStore='ca.pem', pool={'', 'another-pool'}, autoUpgrade=true, autoUpgradePool='%s', autoUpgradeKeep=true, source='127.0.0.1@lo'}:setUp()
 
     -- SVCB upgrade to DoH, same address, do not keep the backend, same pool
-    newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
+    newServer{address="127.0.0.1:%s", caStore='ca.pem', pool={'another-pool'}, autoUpgrade=true, autoUpgradeKeep=false}:setUp()
 
     -- SVCB upgrade to DoT, different address, certificate is valid for the initial address
     newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
@@ -54,6 +63,33 @@ class TestBackendDiscovery(DNSDistTest):
 
     -- SVCB upgrade to DoH except the path is not specified
     newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
+
+    -- Connection refused
+    newServer({address="127.0.0.1:%s", caStore='ca.pem', pool={"", "other-pool"}, autoUpgrade=true, source='127.0.0.1@lo'}):setUp()
+
+    -- EOF
+    newServer({address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true}):setUp()
+
+    -- ServFail
+    newServer({address="127.0.0.1:%s", autoUpgrade=true}):setUp()
+
+    -- Wrong name
+    newServer({address="127.0.0.1:%s", autoUpgrade=true}):setUp()
+
+    -- Wrong ID
+    newServer({address="127.0.0.1:%s", autoUpgrade=true}):setUp()
+
+    -- Too many questions
+    newServer({address="127.0.0.1:%s", autoUpgrade=true}):setUp()
+
+    -- Bad QName
+    newServer({address="127.0.0.1:%s", autoUpgrade=true}):setUp()
+
+    -- SVCB upgrade to DoT, same address, no port specified via SVCB
+    newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
+
+    -- SVCB upgrade to DoH, same address, no port specified via SVCB
+    newServer{address="127.0.0.1:%s", caStore='ca.pem', autoUpgrade=true, autoUpgradeKeep=false}:setUp()
     """
     _verboseMode = True
 
@@ -98,6 +134,7 @@ class TestBackendDiscovery(DNSDistTest):
                                     dns.rdataclass.IN,
                                     dns.rdatatype.A,
                                     '127.0.0.1')
+        response.additional.append(rrset)
         rrset = dns.rrset.from_text('tls.tests.dnsdist.org.',
                                     60,
                                     dns.rdataclass.IN,
@@ -162,6 +199,62 @@ class TestBackendDiscovery(DNSDistTest):
         response.answer.append(rrset)
         return response.to_wire()
 
+    def EOFCallback(request):
+        return None
+
+    def ServFailCallback(request):
+        response = dns.message.make_response(request)
+        response.set_rcode(dns.rcode.SERVFAIL)
+        return response.to_wire()
+
+    def WrongNameCallback(request):
+        query = dns.message.make_query('not-the-right-one.', dns.rdatatype.SVCB)
+        response = dns.message.make_response(query)
+        response.id = request.id
+        return response.to_wire()
+
+    def WrongIDCallback(request):
+        response = dns.message.make_response(request)
+        response.id = request.id ^ 42
+        return response.to_wire()
+
+    def WrongIDCallback(request):
+        response = dns.message.make_response(request)
+        response.id = request.id ^ 42
+        return response.to_wire()
+
+    def TooManyQuestionsCallback(request):
+        response = dns.message.make_response(request)
+        response.question.append(response.question[0])
+        return response.to_wire()
+
+    def BadQNameCallback(request):
+        response = dns.message.make_response(request)
+        wire = bytearray(response.to_wire())
+        # mess up the first label length
+        wire[12] = 0xFF
+        return wire
+
+    def UpgradeDoTNoPortCallback(request):
+        response = dns.message.make_response(request)
+        rrset = dns.rrset.from_text(request.question[0].name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.SVCB,
+                                    '1 tls.tests.dnsdist.org. alpn="dot" ipv4hint=127.0.0.1')
+        response.answer.append(rrset)
+        return response.to_wire()
+
+    def UpgradeDoHNoPortCallback(request):
+        response = dns.message.make_response(request)
+        rrset = dns.rrset.from_text(request.question[0].name,
+                                    60,
+                                    dns.rdataclass.IN,
+                                    dns.rdatatype.SVCB,
+                                    '1 tls.tests.dnsdist.org. alpn="h2" ipv4hint=127.0.0.1 key7="/dns-query{?dns}"')
+        response.answer.append(rrset)
+        return response.to_wire()
+
     @classmethod
     def startResponders(cls):
         tlsContext = ssl.SSLContext(ssl.PROTOCOL_TLS_SERVER)
@@ -221,6 +314,39 @@ class TestBackendDiscovery(DNSDistTest):
         DOHMissingPathResponder.setDaemon(True)
         DOHMissingPathResponder.start()
 
+        EOFResponder = threading.Thread(name='EOF Responder', target=cls.TCPResponder, args=[cls._eofBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.EOFCallback])
+        EOFResponder.setDaemon(True)
+        EOFResponder.start()
+
+        ServFailResponder = threading.Thread(name='ServFail Responder', target=cls.TCPResponder, args=[cls._servfailBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.ServFailCallback])
+        ServFailResponder.setDaemon(True)
+        ServFailResponder.start()
+
+        WrongNameResponder = threading.Thread(name='Wrong Name Responder', target=cls.TCPResponder, args=[cls._wrongNameBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.WrongNameCallback])
+        WrongNameResponder.setDaemon(True)
+        WrongNameResponder.start()
+
+        WrongIDResponder = threading.Thread(name='Wrong ID Responder', target=cls.TCPResponder, args=[cls._wrongIDBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.WrongIDCallback])
+        WrongIDResponder.setDaemon(True)
+        WrongIDResponder.start()
+
+        TooManyQuestionsResponder = threading.Thread(name='Too many questions Responder', target=cls.TCPResponder, args=[cls._tooManyQuestionsBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.TooManyQuestionsCallback])
+        TooManyQuestionsResponder.setDaemon(True)
+        TooManyQuestionsResponder.start()
+
+        badQNameResponder = threading.Thread(name='Bad QName Responder', target=cls.TCPResponder, args=[cls._badQNameBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.BadQNameCallback])
+        badQNameResponder.setDaemon(True)
+        badQNameResponder.start()
+
+        TCPUpgradeToDoTNoPortResponder = threading.Thread(name='TCP upgrade to DoT (no port) Responder', target=cls.TCPResponder, args=[cls._svcUpgradeDoTNoPortBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.UpgradeDoTNoPortCallback])
+        TCPUpgradeToDoTNoPortResponder.setDaemon(True)
+        TCPUpgradeToDoTNoPortResponder.start()
+
+        TCPUpgradeToDoHNoPortResponder = threading.Thread(name='TCP upgrade to DoH (no port) Responder', target=cls.TCPResponder, args=[cls._svcUpgradeDoHNoPortBackendPort, cls._toResponderQueue, cls._fromResponderQueue, False, False, cls.UpgradeDoHNoPortCallback])
+        TCPUpgradeToDoHNoPortResponder.setDaemon(True)
+        TCPUpgradeToDoHNoPortResponder.start()
+
+
     def checkBackendsUpgraded(self):
         output = self.sendConsoleCommand('showServers()')
         print(output)
@@ -240,15 +366,25 @@ class TestBackendDiscovery(DNSDistTest):
         expected = {
             '127.0.0.1:10600': '',
             '127.0.0.1:10601': '',
-            '127.0.0.1:10602': '',
+            '127.0.0.1:10602': 'another-pool',
             # 10603 has been upgraded to 10653 and removed
             # 10604 has been upgraded to 10654 and removed
             '127.0.0.2:10605': '',
             '127.0.0.1:10606': '',
             '127.0.0.1:10607': '',
             '127.0.0.1:10608': '',
+            '127.0.0.1:10609': 'other-pool',
+            '127.0.0.1:10610': '',
+            '127.0.0.1:10611': '',
+            '127.0.0.1:10612': '',
+            '127.0.0.1:10613': '',
+            '127.0.0.1:10614': '',
+            '127.0.0.1:10615': '',
+            # these two are not upgraded because there is no backend listening on the default ports (443 and 853)
+            '127.0.0.1:10616': '',
+            '127.0.0.1:10617': '',
             '127.0.0.1:10652': 'upgraded',
-            '127.0.0.1:10653': '',
+            '127.0.0.1:10653': 'another-pool',
             '127.0.0.2:10654': ''
         }
         print(backends)
@@ -258,7 +394,6 @@ class TestBackendDiscovery(DNSDistTest):
         """
         Backend Discovery: Upgrade
         """
-
         # enough time for discovery to happen
         # 5s is not enough with TSAN
         time.sleep(10)