From: Remi Gacogne Date: Tue, 15 Mar 2016 16:57:19 +0000 (+0100) Subject: dnsdist: Refactor duplicated query handling code (UDP/TCP) X-Git-Tag: dnsdist-1.0.0-beta1~79^2~1 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=e91084ce6d51a71d9d2a2c163c8f27e8c23ba3de;p=thirdparty%2Fpdns.git dnsdist: Refactor duplicated query handling code (UDP/TCP) Merge the UDP/TCP code handling: * dynamic blocks * blockfilter * rules This fixes an issue with DNSCrypt, where cached responses were not being encrypted. --- diff --git a/pdns/dnsdist-tcp.cc b/pdns/dnsdist-tcp.cc index 1df6b06d24..6c742b4e95 100644 --- a/pdns/dnsdist-tcp.cc +++ b/pdns/dnsdist-tcp.cc @@ -167,7 +167,6 @@ void* tcpClientThread(int pipefd) /* we get launched with a pipe on which we receive file descriptors from clients that we own from that point on */ - typedef std::function blockfilter_t; bool outstanding = false; blockfilter_t blockFilter = 0; @@ -215,6 +214,9 @@ void* tcpClientThread(int pipefd) if(!getNonBlockingMsgLen(ci.fd, &qlen, g_tcpRecvTimeout)) break; + ci.cs->queries++; + g_stats.queries++; + if (qlen < sizeof(dnsheader)) { g_stats.nonCompliantQueries++; break; @@ -227,6 +229,7 @@ void* tcpClientThread(int pipefd) char queryBuffer[querySize]; const char* query = queryBuffer; readn2WithTimeout(ci.fd, queryBuffer, qlen, g_tcpRecvTimeout); + #ifdef HAVE_DNSCRYPT std::shared_ptr dnsCryptQuery = 0; @@ -257,92 +260,24 @@ void* tcpClientThread(int pipefd) goto drop; } + if (dh->rd) { + g_stats.rdQueries++; + } + + const uint16_t* flags = getFlagsFromDNSHeader(dh); + uint16_t origFlags = *flags; uint16_t qtype, qclass; unsigned int consumed = 0; DNSName qname(query, qlen, sizeof(dnsheader), false, &qtype, &qclass, &consumed); DNSQuestion dq(&qname, qtype, qclass, &ci.cs->local, &ci.remote, (dnsheader*)query, querySize, qlen, true); - string ruleresult; - const uint16_t * flags = getFlagsFromDNSHeader(dq.dh); - uint16_t origFlags = *flags; + + string poolname; + int delayMsec=0; struct timespec now; clock_gettime(CLOCK_MONOTONIC, &now); - { - WriteLock wl(&g_rings.queryLock); - g_rings.queryRing.push_back({now,ci.remote,qname,dq.len,dq.qtype,*dq.dh}); - } - - g_stats.queries++; - if (ci.cs) { - ci.cs->queries++; - } - - if(auto got=localDynBlockNMG->lookup(ci.remote)) { - if(now < got->second.until) { - vinfolog("Query from %s dropped because of dynamic block", ci.remote.toStringWithPort()); - g_stats.dynBlocked++; - got->second.blocks++; - goto drop; - } - } - - if (dq.dh->rd) { - g_stats.rdQueries++; - } - - if(blockFilter) { - std::lock_guard lock(g_luamutex); - - if(blockFilter(&dq)) { - g_stats.blockFilter++; - goto drop; - } - if(dq.dh->tc && dq.dh->qr) { // don't truncate on TCP/IP! - dq.dh->tc=false; // maybe we should just pass blockFilter the TCP status - dq.dh->qr=false; - } - } - - bool done=false; - DNSAction::Action action=DNSAction::Action::None; - for(const auto& lr : *localRulactions) { - if(lr.first->matches(&dq)) { - lr.first->d_matches++; - action=(*lr.second)(&dq, &ruleresult); - switch(action) { - case DNSAction::Action::Allow: - done = true; - break; - case DNSAction::Action::Drop: - g_stats.ruleDrop++; - goto drop; - break; - case DNSAction::Action::Nxdomain: - dq.dh->rcode = RCode::NXDomain; - dq.dh->qr=true; - g_stats.ruleNXDomain++; - done = true; - break; - case DNSAction::Action::Spoof: - spoofResponseFromString(dq, ruleresult); - done = true; - break; - case DNSAction::Action::HeaderModify: - done = true; - break; - case DNSAction::Action::Pool: - poolname=ruleresult; - done = true; - break; - /* non-terminal actions follow */ - case DNSAction::Action::Delay: - case DNSAction::Action::None: - break; - } - if(done) { - break; - } - } + if (!processQuery(localDynBlockNMG, localRulactions, blockFilter, dq, ci.remote, poolname, &delayMsec, now)) { + goto drop; } if(dq.dh->qr) { // something turned it into a response diff --git a/pdns/dnsdist.cc b/pdns/dnsdist.cc index cb42bd76b9..c510feadf2 100644 --- a/pdns/dnsdist.cc +++ b/pdns/dnsdist.cc @@ -334,7 +334,7 @@ void* responderThread(std::shared_ptr state) --state->outstanding; // you'd think an attacker could game this, but we're using connected socket if(dh->tc && g_truncateTC) { - truncateTC(response, (uint16_t*) &responseLen); + truncateTC(response, &responseLen); } dh->id = ids->origID; @@ -648,6 +648,76 @@ void spoofResponseFromString(DNSQuestion& dq, const string& spoofContent) } } +bool processQuery(LocalStateHolder >& localDynBlock, LocalStateHolder, std::shared_ptr > > >& localRulactions, blockfilter_t blockFilter, DNSQuestion& dq, const ComboAddress& remote, string& poolname, int* delayMsec, const struct timespec& now) +{ + { + WriteLock wl(&g_rings.queryLock); + g_rings.queryRing.push_back({now,remote,*dq.qname,dq.len,dq.qtype,*dq.dh}); + } + + if(auto got=localDynBlock->lookup(remote)) { + if(now < got->second.until) { + vinfolog("Query from %s dropped because of dynamic block", remote.toStringWithPort()); + g_stats.dynBlocked++; + got->second.blocks++; + return false; + } + } + + if(blockFilter) { + std::lock_guard lock(g_luamutex); + + if(blockFilter(&dq)) { + g_stats.blockFilter++; + return false; + } + } + + DNSAction::Action action=DNSAction::Action::None; + string ruleresult; + for(const auto& lr : *localRulactions) { + if(lr.first->matches(&dq)) { + lr.first->d_matches++; + action=(*lr.second)(&dq, &ruleresult); + + switch(action) { + case DNSAction::Action::Allow: + return true; + break; + case DNSAction::Action::Drop: + g_stats.ruleDrop++; + return false; + break; + case DNSAction::Action::Nxdomain: + dq.dh->rcode = RCode::NXDomain; + dq.dh->qr=true; + g_stats.ruleNXDomain++; + return true; + break; + case DNSAction::Action::Spoof: + spoofResponseFromString(dq, ruleresult); + return true; + break; + case DNSAction::Action::HeaderModify: + return true; + break; + case DNSAction::Action::Pool: + poolname=ruleresult; + return true; + break; + /* non-terminal actions follow */ + case DNSAction::Action::Delay: + *delayMsec = static_cast(pdns_stou(ruleresult)); // sorry + break; + case DNSAction::Action::None: + break; + } + } + } + + return true; +} + static ssize_t udpClientSendRequestToBackend(DownstreamState* ss, const int sd, const char* request, const size_t requestLen) { if (ss->sourceItf == 0) { @@ -672,7 +742,6 @@ try string largerQuery; uint16_t qtype, qclass; - typedef std::function blockfilter_t; blockfilter_t blockFilter = 0; { std::lock_guard lock(g_luamutex); @@ -703,6 +772,12 @@ try ssize_t ret = recvmsg(cs->udpFD, &msgh, 0); queryId = 0; + if(!acl->match(remote)) { + vinfolog("Query from %s dropped because of ACL", remote.toStringWithPort()); + g_stats.aclDrops++; + continue; + } + cs->queries++; g_stats.queries++; @@ -718,12 +793,6 @@ try continue; } - if(!acl->match(remote)) { - vinfolog("Query from %s dropped because of ACL", remote.toStringWithPort()); - g_stats.aclDrops++; - continue; - } - uint16_t len = (uint16_t) ret; #ifdef HAVE_DNSCRYPT if (cs->dnscryptCtx) { @@ -739,11 +808,7 @@ try if(!HarvestDestinationAddress(&msgh, &dest)) { dest.sin4.sin_family = 0; } - sendUDPResponse(cs->udpFD, reinterpret_cast(response.data()), response.size(), response.size(), -#ifdef HAVE_DNSCRYPT - nullptr, -#endif - 0, dest, remote); + sendUDPResponse(cs->udpFD, reinterpret_cast(response.data()), response.size(), response.size(), nullptr, 0, dest, remote); } continue; } @@ -772,83 +837,15 @@ try const uint16_t origFlags = *flags; unsigned int consumed = 0; DNSName qname(query, len, sizeof(dnsheader), false, &qtype, &qclass, &consumed); - DNSQuestion dq(&qname, qtype, qclass, &cs->local, &remote, dh, sizeof(packet), len, false); - struct timespec now; - clock_gettime(CLOCK_MONOTONIC, &now); - { - WriteLock wl(&g_rings.queryLock); - g_rings.queryRing.push_back({now,remote,qname,dq.len,dq.qtype,*dq.dh}); - } - - if(auto got=localDynBlock->lookup(remote)) { - if(now < got->second.until) { - vinfolog("Query from %s dropped because of dynamic block", remote.toStringWithPort()); - g_stats.dynBlocked++; - got->second.blocks++; - continue; - } - } - - if(blockFilter) { - std::lock_guard lock(g_luamutex); - - if(blockFilter(&dq)) { - g_stats.blockFilter++; - continue; - } - } - - DNSAction::Action action=DNSAction::Action::None; - string ruleresult; string poolname; int delayMsec=0; - bool done=false; - for(const auto& lr : *localRulactions) { - if(lr.first->matches(&dq)) { - lr.first->d_matches++; - action=(*lr.second)(&dq, &ruleresult); - - switch(action) { - case DNSAction::Action::Allow: - done = true; - break; - case DNSAction::Action::Drop: - g_stats.ruleDrop++; - done = true; - break; - case DNSAction::Action::Nxdomain: - dq.dh->rcode = RCode::NXDomain; - dq.dh->qr=true; - g_stats.ruleNXDomain++; - done = true; - break; - case DNSAction::Action::Spoof: - spoofResponseFromString(dq, ruleresult); - done = true; - break; - case DNSAction::Action::HeaderModify: - done = true; - break; - case DNSAction::Action::Pool: - poolname=ruleresult; - done = true; - break; - /* non-terminal actions follow */ - case DNSAction::Action::Delay: - delayMsec = static_cast(pdns_stou(ruleresult)); // sorry - break; - case DNSAction::Action::None: - break; - } - if (done) { - break; - } - } - } + struct timespec now; + clock_gettime(CLOCK_MONOTONIC, &now); - if (action == DNSAction::Action::Drop) { + if (!processQuery(localDynBlock, localRulactions, blockFilter, dq, remote, poolname, &delayMsec, now)) + { continue; } diff --git a/pdns/dnsdist.hh b/pdns/dnsdist.hh index 946ceed3bd..fe43e83cf1 100644 --- a/pdns/dnsdist.hh +++ b/pdns/dnsdist.hh @@ -381,6 +381,7 @@ struct DNSQuestion bool skipCache{false}; }; +typedef std::function blockfilter_t; template using NumberedVector = std::vector >; void* responderThread(std::shared_ptr state); @@ -507,6 +508,7 @@ bool getLuaNoSideEffect(); // set if there were only explicit declarations of _n void resetLuaSideEffect(); // reset to indeterminate state bool responseContentMatches(const char* response, const uint16_t responseLen, const DNSName& qname, const uint16_t qtype, const uint16_t qclass, const ComboAddress& remote); +bool processQuery(LocalStateHolder >& localDynBlock, LocalStateHolder, std::shared_ptr > > >& localRulactions, blockfilter_t blockFilter, DNSQuestion& dq, const ComboAddress& remote, string& poolname, int* delayMsec, const struct timespec& now); bool fixUpResponse(char** response, uint16_t* responseLen, size_t* responseSize, const DNSName& qname, uint16_t origFlags, bool ednsAdded, #ifdef HAVE_DNSCRYPT std::shared_ptr dnsCryptQuery, diff --git a/regression-tests.dnsdist/test_DNSCrypt.py b/regression-tests.dnsdist/test_DNSCrypt.py index abf1da5ca4..c20bdcdc32 100644 --- a/regression-tests.dnsdist/test_DNSCrypt.py +++ b/regression-tests.dnsdist/test_DNSCrypt.py @@ -128,6 +128,7 @@ class TestDNSCryptWithCache(DNSDistTest): """ DNSCrypt: encrypted A query served from cache """ + misses = 0 client = dnscrypt.DNSCryptClient(self._providerName, self._providerFingerprint, "127.0.0.1", 8443) name = 'cacheda.dnscrypt.tests.powerdns.com.' query = dns.message.make_query(name, 'A', 'IN') @@ -152,6 +153,7 @@ class TestDNSCryptWithCache(DNSDistTest): receivedQuery.id = query.id self.assertEquals(query, receivedQuery) self.assertEquals(response, receivedResponse) + misses += 1 # second query should get a cached response data = client.query(query.to_wire()) @@ -163,3 +165,7 @@ class TestDNSCryptWithCache(DNSDistTest): self.assertEquals(receivedQuery, None) self.assertTrue(receivedResponse) self.assertEquals(response, receivedResponse) + total = 0 + for key in self._responsesCounter: + total += self._responsesCounter[key] + self.assertEquals(total, misses)