From: Remi Gacogne Date: Wed, 12 Jul 2017 08:57:51 +0000 (+0200) Subject: rec: Fix cache handling of ECS queries with a source length of 0 X-Git-Tag: auth-4.1.0-rc1~7^2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F5515%2Fhead;p=thirdparty%2Fpdns.git rec: Fix cache handling of ECS queries with a source length of 0 The current behavior is to send a query without ECS information and to cache it as a non ECS-specific entry. Unfortunately doing so meant that we wouldn't send any further ECS-specific queries for this (qname,qtype) since we could fallback to the non ECS-specific cached answer. This commit adds a new parameter, `ecs-scope-zero-addr`, allowing to specify the address sent to ECS-whitelisted authoritative servers in that case. If unset, the default is to take the first usable (non-any) address in `query-local-address` then in `query-local-address6`, and finally to use `127.0.0.1` as a last resort. --- diff --git a/pdns/iputils.hh b/pdns/iputils.hh index bf6ba4c9ff..cb2553c500 100644 --- a/pdns/iputils.hh +++ b/pdns/iputils.hh @@ -435,7 +435,7 @@ public: } return result; } - int getBits() const + uint8_t getBits() const { return d_bits; } diff --git a/pdns/pdns_recursor.cc b/pdns/pdns_recursor.cc index 8e84f339d0..17de605879 100644 --- a/pdns/pdns_recursor.cc +++ b/pdns/pdns_recursor.cc @@ -2880,6 +2880,33 @@ static int serviceMain(int argc, char*argv[]) SyncRes::s_ecsipv4limit = ::arg().asNum("ecs-ipv4-bits"); SyncRes::s_ecsipv6limit = ::arg().asNum("ecs-ipv6-bits"); + if (!::arg().isEmpty("ecs-scope-zero-address")) { + ComboAddress scopeZero(::arg()["ecs-scope-zero-address"]); + SyncRes::setECSScopeZeroAddress(Netmask(scopeZero, scopeZero.isIPv4() ? 32 : 128)); + } + else { + bool found = false; + for (const auto& addr : g_localQueryAddresses4) { + if (!IsAnyAddress(addr)) { + SyncRes::setECSScopeZeroAddress(Netmask(addr, 32)); + found = true; + break; + } + } + if (!found) { + for (const auto& addr : g_localQueryAddresses6) { + if (!IsAnyAddress(addr)) { + SyncRes::setECSScopeZeroAddress(Netmask(addr, 128)); + found = true; + break; + } + } + if (!found) { + SyncRes::setECSScopeZeroAddress(Netmask("127.0.0.1/32")); + } + } + } + g_networkTimeoutMsec = ::arg().asNum("network-timeout"); g_initialDomainMap = parseAuthAndForwards(); @@ -3277,6 +3304,7 @@ int main(int argc, char **argv) ::arg().set("ecs-ipv4-bits", "Number of bits of IPv4 address to pass for EDNS Client Subnet")="24"; ::arg().set("ecs-ipv6-bits", "Number of bits of IPv6 address to pass for EDNS Client Subnet")="56"; ::arg().set("edns-subnet-whitelist", "List of netmasks and domains that we should enable EDNS subnet for")=""; + ::arg().set("ecs-scope-zero-address", "Address to send to whitelisted authoritative servers for incoming queries with ECS prefix-length source of 0")=""; ::arg().setSwitch( "use-incoming-edns-subnet", "Pass along received EDNS Client Subnet information")="no"; ::arg().setSwitch( "pdns-distributes-queries", "If PowerDNS itself should distribute queries over threads")="yes"; ::arg().setSwitch( "root-nx-trust", "If set, believe that an NXDOMAIN from the root means the TLD does not exist")="yes"; diff --git a/pdns/recursordist/docs/settings.rst b/pdns/recursordist/docs/settings.rst index 08263e544e..abce979422 100644 --- a/pdns/recursordist/docs/settings.rst +++ b/pdns/recursordist/docs/settings.rst @@ -344,6 +344,22 @@ Number of bits of client IPv4 address to pass when sending EDNS Client Subnet ad Number of bits of client IPv6 address to pass when sending EDNS Client Subnet address information. +.. _setting-ecs-scope-zero-address: + +``ecs-scope-zero-address`` +-------------------------- +.. versionadded:: 4.1.0 + +- IPv4 or IPv6 Address +- Default: empty + +The IP address sent via EDNS Client Subnet to authoritative servers listed in +`edns-subnet-whitelist`_ when `use-incoming-ecs`_ is set and the query has +an ECS source prefix-length set to 0. +The default is to look for the first usable (not an `any` one) address in +`query-local-address`_ then `query-local-address6`_. If no suitable address is +found, the recursor fallbacks to sending 127.0.0.1. + .. _setting-edns-outgoing-bufsize: ``edns-outgoing-bufsize`` diff --git a/pdns/syncres.cc b/pdns/syncres.cc index ade576f3f7..7a9c781c07 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -41,6 +41,7 @@ std::unordered_set SyncRes::s_delegationOnly; std::unique_ptr SyncRes::s_dontQuery{nullptr}; NetmaskGroup SyncRes::s_ednssubnets; SuffixMatchNode SyncRes::s_ednsdomains; +EDNSSubnetOpts SyncRes::s_ecsScopeZero; string SyncRes::s_serverID; SyncRes::LogMode SyncRes::s_lm; @@ -2415,22 +2416,52 @@ int SyncRes::doResolveAt(NsSet &nameservers, DNSName auth, bool flawedNSSet, con return -1; } +void SyncRes::setIncomingECS(boost::optional incomingECS) +{ + d_incomingECS = incomingECS; + if (incomingECS) { + if (d_incomingECS->source.getBits() == 0) { + /* RFC7871 says we MUST NOT send any ECS if the source scope is 0. + But using an empty ECS in that case would mean inserting + a non ECS-specific entry into the cache, preventing any further + ECS-specific query to be sent. + So instead we use the trick described in section 7.1.2: + "The subsequent Recursive Resolver query to the Authoritative Nameserver + will then either not include an ECS option or MAY optionally include + its own address information, which is what the Authoritative + Nameserver will almost certainly use to generate any Tailored + Response in lieu of an option. This allows the answer to be handled + by the same caching mechanism as other queries, with an explicit + indicator of the applicable scope. Subsequent Stub Resolver queries + for /0 can then be answered from this cached response. + */ + d_incomingECS = s_ecsScopeZero; + d_incomingECSNetwork = s_ecsScopeZero.source.getMaskedNetwork(); + } + else { + uint8_t bits = std::min(incomingECS->source.getBits(), (incomingECS->source.isIpv4() ? s_ecsipv4limit : s_ecsipv6limit)); + d_incomingECS->source = Netmask(incomingECS->source.getNetwork(), bits); + d_incomingECSNetwork = d_incomingECS->source.getMaskedNetwork(); + } + } + else { + d_incomingECSNetwork = ComboAddress(); + } +} + boost::optional SyncRes::getEDNSSubnetMask(const ComboAddress& local, const DNSName&dn, const ComboAddress& rem) { boost::optional result; ComboAddress trunc; uint8_t bits; if(d_incomingECSFound) { - if (d_incomingECS->source.getBits() == 0) { - /* RFC7871 says we MUST NOT send any ECS if the source scope is 0 */ - return result; - } trunc = d_incomingECSNetwork; bits = d_incomingECS->source.getBits(); } else if(!local.isIPv4() || local.sin4.sin_addr.s_addr) { // detect unset 'requestor' trunc = local; bits = local.isIPv4() ? 32 : 128; + bits = std::min(bits, (trunc.isIPv4() ? s_ecsipv4limit : s_ecsipv6limit)); } else { /* nothing usable */ @@ -2438,7 +2469,6 @@ boost::optional SyncRes::getEDNSSubnetMask(const ComboAddress& local, c } if(s_ednsdomains.check(dn) || s_ednssubnets.match(rem)) { - bits = std::min(bits, (trunc.isIPv4() ? s_ecsipv4limit : s_ecsipv6limit)); trunc.truncate(bits); return boost::optional(Netmask(trunc, bits)); } diff --git a/pdns/syncres.hh b/pdns/syncres.hh index cbbae96c6e..afc52bf8d1 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -536,6 +536,11 @@ public: return t_sstorage.domainmap; } + static void setECSScopeZeroAddress(const Netmask& scopeZeroMask) + { + s_ecsScopeZero.source = scopeZeroMask; + } + explicit SyncRes(const struct timeval& now); int beginResolve(const DNSName &qname, const QType &qtype, uint16_t qclass, vector&ret); @@ -625,16 +630,7 @@ public: d_skipCNAMECheck = skip; } - void setIncomingECS(boost::optional incomingECS) - { - d_incomingECS = incomingECS; - if (incomingECS) { - d_incomingECSNetwork = incomingECS->source.getMaskedNetwork(); - } - else { - d_incomingECSNetwork = ComboAddress(); - } - } + void setIncomingECS(boost::optional incomingECS); #ifdef HAVE_PROTOBUF void setInitialRequestId(boost::optional initialRequestId) @@ -702,6 +698,7 @@ private: static std::unordered_set s_delegationOnly; static NetmaskGroup s_ednssubnets; static SuffixMatchNode s_ednsdomains; + static EDNSSubnetOpts s_ecsScopeZero; static LogMode s_lm; static std::unique_ptr s_dontQuery; @@ -773,7 +770,7 @@ private: zonesStates_t d_cutStates; ostringstream d_trace; shared_ptr d_pdl; - boost::optional d_incomingECS; + boost::optional d_incomingECS; ComboAddress d_incomingECSNetwork; #ifdef HAVE_PROTOBUF boost::optional d_initialRequestId; diff --git a/regression-tests.recursor-dnssec/test_ECS.py b/regression-tests.recursor-dnssec/test_ECS.py index 2e51c7b888..f4d351db65 100644 --- a/regression-tests.recursor-dnssec/test_ECS.py +++ b/regression-tests.recursor-dnssec/test_ECS.py @@ -12,6 +12,7 @@ from twisted.internet import reactor emptyECSText = 'No ECS received' nameECS = 'ecs-echo.example.' ttlECS = 60 +ecsReactorRunning = False class ECSTest(RecursorTest): _config_template_default = """ @@ -58,22 +59,21 @@ disable-syslog=yes @classmethod def startResponders(cls): + global ecsReactorRunning print("Launching responders..") address = cls._PREFIX + '.21' port = 53 - if not reactor.running: + if not ecsReactorRunning: reactor.listenUDP(port, UDPECSResponder(), interface=address) + ecsReactorRunning = True + if not reactor.running: cls._UDPResponder = threading.Thread(name='UDP ECS Responder', target=reactor.run, args=(False,)) cls._UDPResponder.setDaemon(True) cls._UDPResponder.start() - @classmethod - def tearDownResponders(cls): - reactor.stop() - @classmethod def setUpClass(cls): cls.setUpSockets() @@ -110,6 +110,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testIncomingNoECS(ECSTest): _confdir = 'IncomingNoECS' @@ -131,6 +138,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testECSByName(ECSTest): _confdir = 'ECSByName' @@ -154,6 +168,14 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/24') + + # the request for no ECS is ignored because use-incoming-edns-subnet is not set + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testECSByNameLarger(ECSTest): _confdir = 'ECSByNameLarger' @@ -178,6 +200,14 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.1/32') + + # the request for no ECS is ignored because use-incoming-edns-subnet is not set + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testECSByNameSmaller(ECSTest): _confdir = 'ECSByNameLarger' @@ -197,12 +227,21 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/16') + + # the request for no ECS is ignored because use-incoming-edns-subnet is not set + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testIncomingECSByName(ECSTest): _confdir = 'ECSIncomingByName' _config_template = """edns-subnet-whitelist=ecs-echo.example. use-incoming-edns-subnet=yes forward-zones=ecs-echo.example=%s.21 +ecs-scope-zero-address=2001:db8::42 """ % (os.environ['PREFIX']) def testSendECS(self): @@ -227,6 +266,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected, ttlECS) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', "2001:db8::42/128") + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected, ttlECS) + class testIncomingECSByNameLarger(ECSTest): _confdir = 'ECSIncomingByNameLarger' @@ -234,6 +280,7 @@ class testIncomingECSByNameLarger(ECSTest): use-incoming-edns-subnet=yes ecs-ipv4-bits=32 forward-zones=ecs-echo.example=%s.21 +ecs-scope-zero-address=192.168.0.1 """ % (os.environ['PREFIX']) def testSendECS(self): @@ -249,6 +296,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected, ttlECS) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '192.168.0.1/32') + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected, ttlECS) + class testIncomingECSByNameSmaller(ECSTest): _confdir = 'ECSIncomingByNameSmaller' @@ -256,6 +310,7 @@ class testIncomingECSByNameSmaller(ECSTest): use-incoming-edns-subnet=yes ecs-ipv4-bits=16 forward-zones=ecs-echo.example=%s.21 +ecs-scope-zero-address=192.168.0.1 """ % (os.environ['PREFIX']) def testSendECS(self): @@ -269,6 +324,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '192.168.0.1/32') + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected, ttlECS) + class testIncomingECSByNameV6(ECSTest): _confdir = 'ECSIncomingByNameV6' @@ -276,6 +338,7 @@ class testIncomingECSByNameV6(ECSTest): use-incoming-edns-subnet=yes ecs-ipv6-bits=128 forward-zones=ecs-echo.example=%s.21 +query-local-address6=::1 """ % (os.environ['PREFIX']) def testSendECS(self): @@ -291,6 +354,15 @@ forward-zones=ecs-echo.example=%s.21 res = self.sendUDPQuery(query) self.sendECSQuery(query, expected, ttlECS) + def testRequireNoECS(self): + # we should get ::1/128 because neither ecs-scope-zero-addr nor query-local-address are set, + # but query-local-address6 is set to ::1 + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', "::1/128") + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected, ttlECS) + class testECSNameMismatch(ECSTest): _confdir = 'ECSNameMismatch' @@ -309,6 +381,13 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testECSByIP(ECSTest): _confdir = 'ECSByIP' @@ -327,12 +406,21 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '127.0.0.0/24') + + # the request for no ECS is ignored because use-incoming-edns-subnet is not set + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class testIncomingECSByIP(ECSTest): _confdir = 'ECSIncomingByIP' _config_template = """edns-subnet-whitelist=%s.21 use-incoming-edns-subnet=yes forward-zones=ecs-echo.example=%s.21 +ecs-scope-zero-address=::1 """ % (os.environ['PREFIX'], os.environ['PREFIX']) def testSendECS(self): @@ -347,6 +435,14 @@ forward-zones=ecs-echo.example=%s.21 query = dns.message.make_query(nameECS, 'TXT') self.sendECSQuery(query, expected) + def testRequireNoECS(self): + # we will get ::1 because ecs-scope-zero-addr is set to ::1 + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', '::1/128') + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected, ttlECS) + class testECSIPMismatch(ECSTest): _confdir = 'ECSIPMismatch' @@ -366,6 +462,13 @@ forward-zones=ecs-echo.example=%s.21 res = self.sendUDPQuery(query) self.sendECSQuery(query, expected) + def testRequireNoECS(self): + expected = dns.rrset.from_text(nameECS, ttlECS, dns.rdataclass.IN, 'TXT', emptyECSText) + + ecso = clientsubnetoption.ClientSubnetOption('0.0.0.0', 0) + query = dns.message.make_query(nameECS, 'TXT', 'IN', use_edns=True, options=[ecso], payload=512) + self.sendECSQuery(query, expected) + class UDPECSResponder(DatagramProtocol): @staticmethod def ipToStr(option): diff --git a/regression-tests.recursor-dnssec/test_Interop.py b/regression-tests.recursor-dnssec/test_Interop.py index 74a51e6fc0..4e4c3eb885 100644 --- a/regression-tests.recursor-dnssec/test_Interop.py +++ b/regression-tests.recursor-dnssec/test_Interop.py @@ -130,9 +130,10 @@ forward-zones+=undelegated.insecure.example=%s.12 reactor.listenUDP(port, UDPResponder(), interface=address) - cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.setDaemon(True) - cls._UDPResponder.start() + if not reactor.running: + cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,)) + cls._UDPResponder.setDaemon(True) + cls._UDPResponder.start() @classmethod def tearDownResponders(cls):