From 70d292e935bf754cee37f9855a356cf036b0c3cc Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 5 Feb 2025 14:23:57 +0100 Subject: [PATCH 01/22] Initial very raw version --- pdns/ednscookies.cc | 23 +++++ pdns/ednscookies.hh | 2 + pdns/recursordist/Makefile.am | 1 + pdns/recursordist/lwres.cc | 121 +++++++++++++++++++++++++-- pdns/recursordist/lwres.hh | 7 +- pdns/recursordist/pdns_recursor.cc | 20 +++-- pdns/recursordist/rec-cookiestore.cc | 60 +++++++++++++ pdns/recursordist/rec-cookiestore.hh | 69 +++++++++++++++ pdns/recursordist/rec-main.cc | 5 ++ pdns/recursordist/rec-main.hh | 4 +- pdns/recursordist/rec_channel_rec.cc | 9 ++ pdns/recursordist/rec_control.cc | 1 + pdns/recursordist/syncres.cc | 24 +++++- 13 files changed, 325 insertions(+), 21 deletions(-) create mode 100644 pdns/recursordist/rec-cookiestore.cc create mode 100644 pdns/recursordist/rec-cookiestore.hh diff --git a/pdns/ednscookies.cc b/pdns/ednscookies.cc index c390865ef7a0..4adc0ad1f676 100644 --- a/pdns/ednscookies.cc +++ b/pdns/ednscookies.cc @@ -65,6 +65,29 @@ string EDNSCookiesOpt::makeOptString() const return ret; } +string EDNSCookiesOpt::toDisplayString() const +{ + std::string ret = makeHexDump(client, "");; + if (!server.empty()) { + ret += '|'; + if (server.length() != 16) { + // It isn't a rfc9018 one + ret += makeHexDump(server, ""); + } + else { + // It very likely is a rfc9018 one + ret += makeHexDump(server.substr(0, 1), ""); // Version + ret += '|'; + ret += makeHexDump(server.substr(1, 3), ""); // Reserved + ret += '|'; + ret += makeHexDump(server.substr(4, 4), ""); // Timestamp + ret += '|'; + ret += makeHexDump(server.substr(8, 8), ""); // Hash + } + } + return ret; +} + void EDNSCookiesOpt::getEDNSCookiesOptFromString(const char* option, unsigned int len) { client.clear(); diff --git a/pdns/ednscookies.hh b/pdns/ednscookies.hh index 204ef235622d..829a234faa9a 100644 --- a/pdns/ednscookies.hh +++ b/pdns/ednscookies.hh @@ -54,7 +54,9 @@ struct EDNSCookiesOpt [[nodiscard]] bool isValid(const std::string& secret, const ComboAddress& source) const; void makeClientCookie(); bool makeServerCookie(const std::string& secret, const ComboAddress& source); + [[nodiscard]] std::string makeOptString() const; + [[nodiscard]] std::string toDisplayString() const; [[nodiscard]] std::string getServer() const { return server; diff --git a/pdns/recursordist/Makefile.am b/pdns/recursordist/Makefile.am index e2c2ed1f2d52..f56e19a50fd8 100644 --- a/pdns/recursordist/Makefile.am +++ b/pdns/recursordist/Makefile.am @@ -184,6 +184,7 @@ pdns_recursor_SOURCES = \ ratelimitedlog.hh \ rcpgenerator.cc rcpgenerator.hh \ rec-carbon.cc \ + rec-cookiestore.cc rec-cookiestore.hh \ rec-eventtrace.cc rec-eventtrace.hh \ rec-lua-conf.hh rec-lua-conf.cc \ rec-main.hh rec-main.cc \ diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 00af5b02471e..e4668c1bcd6d 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -54,11 +54,32 @@ #include "rec-protozero.hh" #include "uuid-utils.hh" #include "rec-tcpout.hh" +#include "rec-cookiestore.hh" + +static bool g_cookies = true; thread_local TCPOutConnectionManager t_tcp_manager; std::shared_ptr g_slogout; bool g_paddingOutgoing; +static LockGuarded s_cookiestore; + +void pruneCookies(time_t cutoff) +{ + auto lock = s_cookiestore.lock(); + lock->prune(cutoff); +} + +uint64_t dumpCookies(int fileDesc) +{ + CookieStore copy; + { + auto lock = s_cookiestore.lock(); + copy = *lock; + } + return CookieStore::dump(copy, fileDesc); +} + void remoteLoggerQueueData(RemoteLoggerInterface& rli, const std::string& data) { auto ret = rli.queueData(data); @@ -423,6 +444,9 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& bool weWantEDNSSubnet = false; uint8_t outgoingECSBits = 0; ComboAddress outgoingECSAddr; + std::optional addressToBindTo; + std::optional cookieSentOut; + if (EDNS0Level > 0) { DNSPacketWriter::optvect_t opts; if (srcmask) { @@ -434,6 +458,34 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& weWantEDNSSubnet = true; } + if (g_cookies) { + auto lock = s_cookiestore.lock(); + auto found = lock->find(address); + if (found != lock->end()) { + if (found->d_support) { + cookieSentOut = found->d_cookie; + addressToBindTo = found->d_localaddress; + opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); + found->d_lastupdate = now->tv_sec; + cerr << "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl; + } + else { + cerr << "This server does not support cookies" << endl; + } + } + else { + CookieEntry entry; + entry.d_address = address; + entry.d_cookie.makeClientCookie(); + cookieSentOut = entry.d_cookie; + entry.d_lastupdate = now->tv_sec; + entry.d_support = false; + lock->emplace(entry); + opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); + cerr << "We're sending new client cookie info from to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl; + } + } + if (dnsOverTLS && g_paddingOutgoing) { addPadding(pw, bufsize, opts); } @@ -459,12 +511,12 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& srcmask = boost::none; // this is also our return value, even if EDNS0Level == 0 - // We only store the localip if needed for fstrm logging + // We only store the localip if needed for fstrm logging or cookie support ComboAddress localip; -#ifdef HAVE_FSTRM bool fstrmQEnabled = false; bool fstrmREnabled = false; +#ifdef HAVE_FSTRM if (isEnabledForQueries(fstrmLoggers)) { fstrmQEnabled = true; } @@ -475,9 +527,18 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& if (!doTCP) { int queryfd; - - ret = asendto(vpacket.data(), vpacket.size(), 0, address, qid, domain, type, weWantEDNSSubnet, &queryfd, *now); - + try { + ret = asendto(vpacket.data(), vpacket.size(), 0, address, addressToBindTo, qid, domain, type, weWantEDNSSubnet, &queryfd, *now); + } + catch (const PDNSException& e) { + if (addressToBindTo) { + // Cookie info already has been added to packet, so we must retry from a higher level + auto lock = s_cookiestore.lock(); + lock->erase(address); + return LWResult::Result::BindError; + } + throw; + } if (ret != LWResult::Result::Success) { return ret; } @@ -486,9 +547,8 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& *chained = true; } -#ifdef HAVE_FSTRM if (!*chained) { - if (fstrmQEnabled || fstrmREnabled) { + if (cookieSentOut || fstrmQEnabled || fstrmREnabled) { localip.sin4.sin_family = address.sin4.sin_family; socklen_t slen = address.getSocklen(); (void)getsockname(queryfd, reinterpret_cast(&localip), &slen); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)) @@ -497,7 +557,6 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& logFstreamQuery(fstrmLoggers, queryTime, localip, address, DnstapMessage::ProtocolType::DoUDP, context.d_auth ? context.d_auth : boost::none, vpacket); } } -#endif /* HAVE_FSTRM */ // sleep until we see an answer to this, interface to mtasker ret = arecvfrom(buf, 0, address, len, qid, domain, type, queryfd, *now); @@ -511,6 +570,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // peer has closed it on error, so we retry. At some point we // *will* get a new connection, so this loop is not endless. isNew = true; // tcpconnect() might throw for new connections. In that case, we want to break the loop, scanbuild complains here, which is a false positive afaik + // XXX cookie case: bind to local address isNew = tcpconnect(address, connection, dnsOverTLS, nsName); ret = tcpsendrecv(address, connection, localip, vpacket, len, buf); #ifdef HAVE_FSTRM @@ -598,9 +658,46 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } EDNSOpts edo; + bool cookieFoundInReply = false; if (EDNS0Level > 0 && getEDNSOpts(mdp, &edo)) { lwr->d_haveEDNS = true; - + if (g_cookies && !*chained) { + for (const auto& opt : edo.d_options) { + if (opt.first == EDNSOptionCode::COOKIE) { + EDNSCookiesOpt received; + if (received.makeFromString(opt.second)) { + cookieFoundInReply = true; + cerr << "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl; + auto lock = s_cookiestore.lock(); + auto found = lock->find(address); + if (found != lock->end()) { + if (received.getClient() == cookieSentOut->getClient()) { + cerr << "Client cookie matched! Storing with localAddress " << localip.toString() << endl; + found->d_localaddress = localip; + found->d_cookie = received; + found->d_lastupdate = now->tv_sec; + found->d_support = true; + uint16_t ercode = (edo.d_extRCode << 4) | lwr->d_rcode; + if (ercode == ERCode::BADCOOKIE) { + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; + } + } + else { + // Server responded with a wrong client cookie, fall back to TCP + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; + } + } + else { + // We sent a cookie out but forgot it? + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; + } + } + } + } + } if (weWantEDNSSubnet) { for (const auto& opt : edo.d_options) { if (opt.first == EDNSOptionCode::ECS) { @@ -621,6 +718,12 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } } + // Case: we sent out a cookie but did not get one back + if (cookieSentOut && !cookieFoundInReply && !*chained) { + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; + } + if (outgoingLoggers) { logIncomingResponse(outgoingLoggers, context.d_initialRequestId, uuid, address, domain, type, qid, doTCP, dnsOverTLS, srcmask, len, lwr->d_rcode, lwr->d_records, queryTime, exportTypes, nsName); } diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index 3b2bc43a1d02..b74f50bcd375 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -71,6 +71,8 @@ public: OSLimitError = 3, Spoofed = 4, /* Spoofing attempt (too many near-misses) */ ChainLimitError = 5, + BadCookie = 6, + BindError = 7, }; [[nodiscard]] static bool isLimitError(Result res) @@ -86,9 +88,12 @@ public: bool d_haveEDNS{false}; }; -LWResult::Result asendto(const void* data, size_t len, int flags, const ComboAddress& toAddress, uint16_t qid, +LWResult::Result asendto(const void* data, size_t len, int flags, const ComboAddress& toAddress, + std::optional& localAddress, uint16_t qid, const DNSName& domain, uint16_t qtype, bool ecs, int* fileDesc, timeval& now); LWResult::Result arecvfrom(PacketBuffer& packet, int flags, const ComboAddress& fromAddr, size_t& len, uint16_t qid, const DNSName& domain, uint16_t qtype, int fileDesc, const struct timeval& now); LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); +uint64_t dumpCookies(int fileDesc); +void pruneCookies(time_t cutoff); diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 98ad79f2f957..2e0b5c3e963a 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -99,9 +99,9 @@ GlobalStateHolder g_dontThrottleNetmasks; GlobalStateHolder g_DoTToAuthNames; uint64_t g_latencyStatSize; -LWResult::Result UDPClientSocks::getSocket(const ComboAddress& toaddr, int* fileDesc) +LWResult::Result UDPClientSocks::getSocket(const ComboAddress& toaddr, const std::optional& localAddress, int* fileDesc) { - *fileDesc = makeClientSocket(toaddr.sin4.sin_family); + *fileDesc = makeClientSocket(toaddr.sin4.sin_family, localAddress); if (*fileDesc < 0) { // temporary error - receive exception otherwise return LWResult::Result::OSLimitError; } @@ -149,7 +149,7 @@ void UDPClientSocks::returnSocket(int fileDesc) } // returns -1 for errors which might go away, throws for ones that won't -int UDPClientSocks::makeClientSocket(int family) +int UDPClientSocks::makeClientSocket(int family, const std::optional& localAddress) { int ret = socket(family, SOCK_DGRAM, 0); // turns out that setting CLO_EXEC and NONBLOCK from here is not a performance win on Linux (oddly enough) @@ -181,7 +181,15 @@ int UDPClientSocks::makeClientSocket(int family) } while (g_avoidUdpSourcePorts.count(port) != 0); } - sin = pdns::getQueryLocalAddress(family, port); // does htons for us + if (localAddress) { + cerr << "Binding to local address associated with cookie: " << localAddress->toString() << endl; + sin = *localAddress; + sin.setPort(port); + } + else { + sin = pdns::getQueryLocalAddress(family, port); // does htons for us + cerr << "Bound to random local address " << sin.toString() << endl; + } if (::bind(ret, reinterpret_cast(&sin), sin.getSocklen()) >= 0) { // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast) break; } @@ -280,7 +288,7 @@ unsigned int authWaitTimeMSec(const std::unique_ptr& mtasker) /* these two functions are used by LWRes */ LWResult::Result asendto(const void* data, size_t len, int /* flags */, - const ComboAddress& toAddress, uint16_t qid, const DNSName& domain, uint16_t qtype, bool ecs, int* fileDesc, timeval& now) + const ComboAddress& toAddress, std::optional& localAddress, uint16_t qid, const DNSName& domain, uint16_t qtype, bool ecs, int* fileDesc, timeval& now) { auto pident = std::make_shared(); @@ -322,7 +330,7 @@ LWResult::Result asendto(const void* data, size_t len, int /* flags */, } } - auto ret = t_udpclientsocks->getSocket(toAddress, fileDesc); + auto ret = t_udpclientsocks->getSocket(toAddress, localAddress, fileDesc); if (ret != LWResult::Result::Success) { return ret; } diff --git a/pdns/recursordist/rec-cookiestore.cc b/pdns/recursordist/rec-cookiestore.cc new file mode 100644 index 000000000000..321b9c090918 --- /dev/null +++ b/pdns/recursordist/rec-cookiestore.cc @@ -0,0 +1,60 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ + +#include "misc.hh" +#include "rec-cookiestore.hh" + +using timebuf_t = std::array; + +extern const char* timestamp(time_t arg, timebuf_t& buf); // XXX + +void CookieStore::prune(time_t cutoff) +{ + auto& ind = get(); + ind.erase(ind.begin(), ind.upper_bound(cutoff)); +} + +uint64_t CookieStore::dump(const CookieStore& copy, int fileDesc) +{ + int newfd = dup(fileDesc); + if (newfd == -1) { + return 0; + } + auto filePtr = pdns::UniqueFilePtr(fdopen(newfd, "w")); + if (!filePtr) { + close(newfd); + return 0; + } + uint64_t count = 0; + + fprintf(filePtr.get(), "; cookie dump follows\n; server\tlocal\tcookie\tsupport\tts\n"); + for (const auto& entry : copy) { + count++; + timebuf_t tmp; + fprintf(filePtr.get(), "%s\t%s\t%s\t%s\t%s\n", + entry.d_address.toString().c_str(), entry.d_localaddress.toString().c_str(), + entry.d_cookie.toDisplayString().c_str(), + entry.d_support ? "yes" : "no", + timestamp(entry.d_lastupdate, tmp)); + } + return count; +} diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh new file mode 100644 index 000000000000..a78810ad5359 --- /dev/null +++ b/pdns/recursordist/rec-cookiestore.hh @@ -0,0 +1,69 @@ +/* + * This file is part of PowerDNS or dnsdist. + * Copyright -- PowerDNS.COM B.V. and its contributors + * + * This program is free software; you can redistribute it and/or modify + * it under the terms of version 2 of the GNU General Public License as + * published by the Free Software Foundation. + * + * In addition, for the avoidance of any doubt, permission is granted to + * link this program with OpenSSL and to (re)distribute the binaries + * produced as the result of such linking. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program; if not, write to the Free Software + * Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. + */ +#pragma once + +/* + CookieStore is used to keep track of client cookies used for contacting authoritative servers. + According to RFC 7873 and RFC 9018, it has the following design. + + - Cookies are stored with an auth IP address as primary index and are generated randomly. + + - If the the does not support cookies, this is marked as such and no cookies will be sent to it + for a period of time. When a cookie is sent again, it must be a newly generated one. + + - A cookie is stored together with the client IP (as rec can have many). If a server is to be + contacted again, it should use the same bound IP. + + - Although it is perfectly fine for a client cookie to live for a long time, this design will + flush entries older that a certain period of time, to avoid an ever growing CookieStore. + +*/ + +#include +#include +#include +#include +#include +#include + +#include "iputils.hh" +#include "ednscookies.hh" + +using namespace ::boost::multi_index; + +struct CookieEntry +{ + ComboAddress d_address; + mutable ComboAddress d_localaddress; // The address we were bound to, see RFC 9018 + mutable EDNSCookiesOpt d_cookie; // Contains both client and server cookie + mutable time_t d_lastupdate{}; + mutable bool d_support; +}; + +class CookieStore : public multi_index_container < CookieEntry, + indexed_by < ordered_unique, member>, + ordered_non_unique, member>>> +{ +public: + void prune(time_t cutoff); + static uint64_t dump(const CookieStore&, int fileDesc); +}; diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 64c8e9fe6eda..9882252d4032 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -2656,6 +2656,11 @@ static void houseKeepingWork(Logr::log_t log) SyncRes::pruneSaveParentsNSSets(now.tv_sec); }); + static PeriodicTask pruneCookiesTask{"pruneCookiesTask", 30}; + pruneCookiesTask.runIfDue(now, [now]() { + pruneCookies(now.tv_sec - 1800); + }); + // By default, refresh at 80% of max-cache-ttl with a minimum period of 10s const unsigned int minRootRefreshInterval = 10; static PeriodicTask rootUpdateTask{"rootUpdateTask", std::max(SyncRes::s_maxcachettl * 8 / 10, minRootRefreshInterval)}; diff --git a/pdns/recursordist/rec-main.hh b/pdns/recursordist/rec-main.hh index 21e3927a83d1..7cd942b519d9 100644 --- a/pdns/recursordist/rec-main.hh +++ b/pdns/recursordist/rec-main.hh @@ -174,14 +174,14 @@ public: { } - LWResult::Result getSocket(const ComboAddress& toaddr, int* fileDesc); + LWResult::Result getSocket(const ComboAddress& toaddr, const std::optional& localAddress, int* fileDesc); // return a socket to the pool, or simply erase it void returnSocket(int fileDesc); private: // returns -1 for errors which might go away, throws for ones that won't - static int makeClientSocket(int family); + static int makeClientSocket(int family, const std::optional& localAddress); }; enum class PaddingMode diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index 293ae1dbca22..f519cfeaba91 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -352,6 +352,11 @@ static uint64_t dumpAggressiveNSECCache(int fd) return g_aggressiveNSECCache->dumpToFile(filePtr, now); } +static uint64_t* pleaseDumpCookiesMap(int fd) +{ + return new uint64_t(dumpCookies(fd)); +} + static uint64_t* pleaseDumpEDNSMap(int fd) { return new uint64_t(SyncRes::doEDNSDump(fd)); @@ -1856,6 +1861,7 @@ static RecursorControlChannel::Answer help() "clear-nta [DOMAIN]... Clear the Negative Trust Anchor for DOMAINs, if no DOMAIN is specified, remove all\n" "clear-ta [DOMAIN]... Clear the Trust Anchor for DOMAINs\n" "dump-cache [type...] dump cache contents to the named file, type is r, n, p or a\n" + "dump-cookies dump the contents of the cookie data to the namewd file\n" "dump-dot-probe-map dump the contents of the DoT probe map to the named file\n" "dump-edns [status] dump EDNS status to the named file\n" "dump-failedservers dump the failed servers to the named file\n" @@ -2069,6 +2075,9 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int socket, cons if (cmd == "dump-cache") { return doDumpCache(socket, begin, end); } + if (cmd == "dump-cookies") { + return doDumpToFile(socket, pleaseDumpCookiesMap, cmd, false); + } if (cmd == "dump-dot-probe-map") { return doDumpToFile(socket, pleaseDumpDoTProbeMap, cmd, false); } diff --git a/pdns/recursordist/rec_control.cc b/pdns/recursordist/rec_control.cc index 9b65630cf894..924893dc4d49 100644 --- a/pdns/recursordist/rec_control.cc +++ b/pdns/recursordist/rec_control.cc @@ -329,6 +329,7 @@ int main(int argc, char** argv) const set fileCommands = { "dump-cache", + "dump-cookies", "dump-edns", "dump-ednsstatus", "dump-nsspeeds", diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 6d29075199c5..9c75340314ea 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1117,7 +1117,7 @@ const char* isoDateTimeMillis(const struct timeval& tval, timebuf_t& buf) return buf.data(); } -static const char* timestamp(time_t arg, timebuf_t& buf) +const char* timestamp(time_t arg, timebuf_t& buf) { const std::string s_timestampFormat = "%Y-%m-%dT%T"; struct tm tmval{}; @@ -1620,12 +1620,26 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& address, bool auto lock = s_ednsstatus.lock(); // all three branches below need a lock // Determine new mode + if (ret == LWResult::Result::BindError) { + cerr << "BindError, retrying with new client cookie and no specific address to bind to" << endl; + // BindError is only generated when cookies are active and we failed to bind to a local + // address associated with a cookie, see RFC9018 section 3 last paragraph. We assume the + // called code alread erased the cookie info. + // This is the first path that re-iterates the loop + continue; + } + else if (res->d_validpacket && res->d_haveEDNS && ret == LWResult::Result::BadCookie) { + cerr << "Retrying with received server cookie" << endl; + // We assume the received cookie was stored and will be used in the second iteration + // This is the second path that re-iterates the loop + continue; + } if (res->d_validpacket && !res->d_haveEDNS && res->d_rcode == RCode::FormErr) { mode = EDNSStatus::NOEDNS; auto ednsstatus = lock->insert(address).first; auto& ind = lock->get(); lock->setMode(ind, ednsstatus, mode, d_now.tv_sec); - // This is the only path that re-iterates the loop + // This is the third path that re-iterates the loop continue; } if (!res->d_haveEDNS) { @@ -5555,6 +5569,8 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, ednsStats(ednsmask, qname, prefix); } + cerr << "asyncrW: returns " << int(resolveret) << " rcode is " << int(lwr.d_rcode) << endl; + /* preoutquery killed the query by setting dq.rcode to -3 */ if (preOutQueryRet == -3) { throw ImmediateServFailException("Query killed by policy"); @@ -5562,7 +5578,8 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, d_totUsec += lwr.d_usec; - if (resolveret == LWResult::Result::Spoofed) { + if (resolveret == LWResult::Result::Spoofed || resolveret == LWResult::Result::BadCookie) { + cerr << "Acting as we got a spoof" << endl; spoofed = true; return false; } @@ -6067,6 +6084,7 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con } if (forceTCP || (spoofed || (gotAnswer && truncated))) { /* retry, over TCP this time */ + cerr << "Retry over TCP" << endl; gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, tns->first, *remoteIP, true, doDoT, truncated, spoofed, context.extendedError); } From fd9a851fb8cedcb91d27b73994d3e09cdffe373c Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 18 Feb 2025 10:53:44 +0100 Subject: [PATCH 02/22] Tidy rec-tcpout.?? --- pdns/recursordist/rec-tcpout.cc | 16 ++++++++-------- pdns/recursordist/rec-tcpout.hh | 12 ++++++------ 2 files changed, 14 insertions(+), 14 deletions(-) diff --git a/pdns/recursordist/rec-tcpout.cc b/pdns/recursordist/rec-tcpout.cc index 8b65110e0fb0..469370183539 100644 --- a/pdns/recursordist/rec-tcpout.cc +++ b/pdns/recursordist/rec-tcpout.cc @@ -51,33 +51,33 @@ void TCPOutConnectionManager::cleanup(const struct timeval& now) } } -void TCPOutConnectionManager::store(const struct timeval& now, const ComboAddress& ip, Connection&& connection) +void TCPOutConnectionManager::store(const struct timeval& now, const ComboAddress& remoteAddress, Connection&& connection) { ++connection.d_numqueries; if (s_maxQueries > 0 && connection.d_numqueries >= s_maxQueries) { return; } - if (d_idle_connections.size() >= s_maxIdlePerThread || d_idle_connections.count(ip) >= s_maxIdlePerAuth) { + if (d_idle_connections.size() >= s_maxIdlePerThread || d_idle_connections.count(remoteAddress) >= s_maxIdlePerAuth) { cleanup(now); } if (d_idle_connections.size() >= s_maxIdlePerThread) { return; } - if (d_idle_connections.count(ip) >= s_maxIdlePerAuth) { + if (d_idle_connections.count(remoteAddress) >= s_maxIdlePerAuth) { return; } gettimeofday(&connection.d_last_used, nullptr); - d_idle_connections.emplace(ip, std::move(connection)); + d_idle_connections.emplace(remoteAddress, std::move(connection)); } -TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const ComboAddress& ip) +TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const ComboAddress& remoteAddress) { - if (d_idle_connections.count(ip) > 0) { - auto h = d_idle_connections.extract(ip); - return h.mapped(); + if (d_idle_connections.count(remoteAddress) > 0) { + auto connection = d_idle_connections.extract(remoteAddress); + return connection.mapped(); } return Connection{}; } diff --git a/pdns/recursordist/rec-tcpout.hh b/pdns/recursordist/rec-tcpout.hh index 9c2cfa4d5167..9c433be2d58c 100644 --- a/pdns/recursordist/rec-tcpout.hh +++ b/pdns/recursordist/rec-tcpout.hh @@ -39,7 +39,7 @@ public: struct Connection { - std::string toString() const + [[nodiscard]] std::string toString() const { if (d_handler) { return std::to_string(d_handler->getDescriptor()) + ' ' + std::to_string(d_handler.use_count()); @@ -52,17 +52,17 @@ public: size_t d_numqueries{0}; }; - void store(const struct timeval& now, const ComboAddress& ip, Connection&& connection); - Connection get(const ComboAddress& ip); + void store(const struct timeval& now, const ComboAddress& remoteAddress, Connection&& connection); + Connection get(const ComboAddress& remoteAddress); void cleanup(const struct timeval& now); - size_t size() const + [[nodiscard]] size_t size() const { return d_idle_connections.size(); } - uint64_t* getSize() const + [[nodiscard]] uint64_t* getSize() const { - return new uint64_t(size()); + return new uint64_t(size()); // NOLINT(cppcoreguidelines-owning-memory): it's the API } private: From 93d896a807ed79f42d62a66c763694b945fb53ab Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 18 Feb 2025 12:21:02 +0100 Subject: [PATCH 03/22] TCP support for cookies, taking into account idle outgoing connections --- pdns/recursordist/lwres.cc | 49 +++++++++++++++++++++++++-------- pdns/recursordist/rec-tcpout.cc | 14 +++++----- pdns/recursordist/rec-tcpout.hh | 9 ++++-- pdns/recursordist/syncres.cc | 2 +- 4 files changed, 51 insertions(+), 23 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index e4668c1bcd6d..7f9db84f326f 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -308,22 +308,41 @@ static void logIncomingResponse(const std::shared_ptr localBind, TCPOutConnectionManager::Connection& connection, bool& dnsOverTLS, const std::string& nsName) +{ + dnsOverTLS = SyncRes::s_dot_to_port_853 && remote.getPort() == 853; + + connection = t_tcp_manager.get(std::make_pair(remote, localBind)); if (connection.d_handler) { return false; } const struct timeval timeout{ g_networkTimeoutMsec / 1000, static_cast(g_networkTimeoutMsec) % 1000 * 1000}; - Socket s(ip.sin4.sin_family, SOCK_STREAM); + Socket s(remote.sin4.sin_family, SOCK_STREAM); s.setNonBlocking(); setTCPNoDelay(s.getHandle()); - ComboAddress localip = pdns::getQueryLocalAddress(ip.sin4.sin_family, 0); - s.bind(localip); + ComboAddress localip = localBind ? *localBind : pdns::getQueryLocalAddress(remote.sin4.sin_family, 0); + if (localBind) { + cerr << "Connecting TCP to " << remote.toString() << " with specific local address " << localip.toString() << endl; + } + else { + cerr << "Connecting TCP to " << remote.toString() << " with no specific local address" << endl; + } + + try { + s.bind(localip); + } + catch (const NetworkError& e) { + if (localBind) { + throw BindError(); + } + throw; + } std::shared_ptr tlsCtx{nullptr}; if (dnsOverTLS) { @@ -334,14 +353,15 @@ static bool tcpconnect(const ComboAddress& ip, TCPOutConnectionManager::Connecti tlsCtx = getTLSContext(tlsParams); if (tlsCtx == nullptr) { SLOG(g_log << Logger::Error << "DoT to " << ip << " requested but not available" << endl, - g_slogout->info(Logr::Error, "DoT requested but not available", "server", Logging::Loggable(ip))); + g_slogout->info(Logr::Error, "DoT requested but not available", "server", Logging::Loggable(remote))); dnsOverTLS = false; } } connection.d_handler = std::make_shared(nsName, false, s.releaseHandle(), timeout, tlsCtx); + connection.d_local = localBind; // Returned state ignored // This can throw an exception, retry will need to happen at higher level - connection.d_handler->tryConnect(SyncRes::s_tcp_fast_open_connect, ip); + connection.d_handler->tryConnect(SyncRes::s_tcp_fast_open_connect, remote); return true; } @@ -562,7 +582,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& ret = arecvfrom(buf, 0, address, len, qid, domain, type, queryfd, *now); } else { - bool isNew; + bool isNew{}; do { try { // If we get a new (not re-used) TCP connection that does not @@ -570,8 +590,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // peer has closed it on error, so we retry. At some point we // *will* get a new connection, so this loop is not endless. isNew = true; // tcpconnect() might throw for new connections. In that case, we want to break the loop, scanbuild complains here, which is a false positive afaik - // XXX cookie case: bind to local address - isNew = tcpconnect(address, connection, dnsOverTLS, nsName); + isNew = tcpconnect(address, addressToBindTo, connection, dnsOverTLS, nsName); ret = tcpsendrecv(address, connection, localip, vpacket, len, buf); #ifdef HAVE_FSTRM if (fstrmQEnabled) { @@ -583,6 +602,12 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } connection.d_handler->close(); } + catch (const BindError&) { + // Cookie info already has been added to packet, so we must retry from a higher level + auto lock = s_cookiestore.lock(); + lock->erase(address); + return LWResult::Result::BindError; + } catch (const NetworkError&) { ret = LWResult::Result::OSLimitError; // OS limits error } @@ -770,7 +795,7 @@ LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain if (doTCP) { if (connection.d_handler && lwr->d_validpacket) { - t_tcp_manager.store(*now, address, std::move(connection)); + t_tcp_manager.store(*now, std::make_pair(address, connection.d_local), std::move(connection)); } } return ret; diff --git a/pdns/recursordist/rec-tcpout.cc b/pdns/recursordist/rec-tcpout.cc index 469370183539..a4427771f2ca 100644 --- a/pdns/recursordist/rec-tcpout.cc +++ b/pdns/recursordist/rec-tcpout.cc @@ -51,32 +51,32 @@ void TCPOutConnectionManager::cleanup(const struct timeval& now) } } -void TCPOutConnectionManager::store(const struct timeval& now, const ComboAddress& remoteAddress, Connection&& connection) +void TCPOutConnectionManager::store(const struct timeval& now, const pair_t& pair, Connection&& connection) { ++connection.d_numqueries; if (s_maxQueries > 0 && connection.d_numqueries >= s_maxQueries) { return; } - if (d_idle_connections.size() >= s_maxIdlePerThread || d_idle_connections.count(remoteAddress) >= s_maxIdlePerAuth) { + if (d_idle_connections.size() >= s_maxIdlePerThread || d_idle_connections.count(pair) >= s_maxIdlePerAuth) { cleanup(now); } if (d_idle_connections.size() >= s_maxIdlePerThread) { return; } - if (d_idle_connections.count(remoteAddress) >= s_maxIdlePerAuth) { + if (d_idle_connections.count(pair) >= s_maxIdlePerAuth) { return; } gettimeofday(&connection.d_last_used, nullptr); - d_idle_connections.emplace(remoteAddress, std::move(connection)); + d_idle_connections.emplace(pair, std::move(connection)); } -TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const ComboAddress& remoteAddress) +TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const pair_t& pair) { - if (d_idle_connections.count(remoteAddress) > 0) { - auto connection = d_idle_connections.extract(remoteAddress); + if (d_idle_connections.count(pair) > 0) { + auto connection = d_idle_connections.extract(pair); return connection.mapped(); } return Connection{}; diff --git a/pdns/recursordist/rec-tcpout.hh b/pdns/recursordist/rec-tcpout.hh index 9c433be2d58c..e9bf48f8e3ad 100644 --- a/pdns/recursordist/rec-tcpout.hh +++ b/pdns/recursordist/rec-tcpout.hh @@ -48,12 +48,15 @@ public: } std::shared_ptr d_handler; + std::optional d_local; timeval d_last_used{0, 0}; size_t d_numqueries{0}; }; - void store(const struct timeval& now, const ComboAddress& remoteAddress, Connection&& connection); - Connection get(const ComboAddress& remoteAddress); + using pair_t = std::pair>; + + void store(const struct timeval& now, const pair_t& pair, Connection&& connection); + Connection get(const pair_t& remoteAddress); void cleanup(const struct timeval& now); [[nodiscard]] size_t size() const @@ -68,7 +71,7 @@ public: private: // This does not take into account that we can have multiple connections with different hosts (via SNI) to the same IP. // That is OK, since we are connecting by IP only at the moment. - std::multimap d_idle_connections; + std::multimap d_idle_connections; }; extern thread_local TCPOutConnectionManager t_tcp_manager; diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9c75340314ea..b5785d6f2a09 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -6073,7 +6073,7 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con if (SyncRes::s_dot_to_port_853 && remoteIP->getPort() == 853) { doDoT = true; } - bool forceTCP = doDoT; + bool forceTCP = doDoT | true; if (!doDoT && s_max_busy_dot_probes > 0) { submitTryDotTask(*remoteIP, auth, tns->first, d_now.tv_sec); From 33c30b0ffc97fc0ace27201df66d189567e8466b Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 19 Feb 2025 09:55:48 +0100 Subject: [PATCH 04/22] Add setting, remove debug lines --- pdns/recursordist/lwres.cc | 7 ++++++- pdns/recursordist/lwres.hh | 1 + pdns/recursordist/rec-main.cc | 2 ++ pdns/recursordist/rec-rust-lib/table.py | 12 ++++++++++++ pdns/recursordist/syncres.cc | 8 +------- 5 files changed, 22 insertions(+), 8 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 7f9db84f326f..d47f9ce01e56 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -56,7 +56,12 @@ #include "rec-tcpout.hh" #include "rec-cookiestore.hh" -static bool g_cookies = true; +static bool g_cookies = false; + +void setAuthCookies(bool flag) +{ + g_cookies = flag; +} thread_local TCPOutConnectionManager t_tcp_manager; std::shared_ptr g_slogout; diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index b74f50bcd375..bac767c7195b 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -97,3 +97,4 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int flags, const ComboAddress& LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); uint64_t dumpCookies(int fileDesc); void pruneCookies(time_t cutoff); +void setAuthCookies(bool flag); diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 9882252d4032..45f0a1b3f305 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -2296,6 +2296,8 @@ static int serviceMain(Logr::log_t log) g_paddingTag = ::arg().asNum("edns-padding-tag"); g_paddingOutgoing = ::arg().mustDo("edns-padding-out"); + setAuthCookies(::arg().mustDo("outgoing-cookies")); + RecThreadInfo::setNumDistributorThreads(::arg().asNum("distributor-threads")); RecThreadInfo::setNumUDPWorkerThreads(::arg().asNum("threads")); if (RecThreadInfo::numUDPWorkers() < 1) { diff --git a/pdns/recursordist/rec-rust-lib/table.py b/pdns/recursordist/rec-rust-lib/table.py index bafbeba43a8f..173e5c87606d 100644 --- a/pdns/recursordist/rec-rust-lib/table.py +++ b/pdns/recursordist/rec-rust-lib/table.py @@ -3595,4 +3595,16 @@ 'versionadded': '5.2.0', 'runtime': ['reload-lua-config', 'reload-yaml'], }, + { + 'name' : 'cookies', + 'section' : 'outgoing', + 'oldname': 'outgoing-cookies', + 'type': LType.Bool, + 'default': 'false', + 'help': 'Enable DNS cookies when contacting authoritative servers or forwarders', + 'doc': ''' +Enable DNS cookies (:rfc:`7873`, :rfc:`9018`) when contacting authoritative servers or forwarders. +''', + 'versionadded': '5.3.0', + }, ] diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index b5785d6f2a09..9dfc42db292b 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1621,7 +1621,6 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& address, bool // Determine new mode if (ret == LWResult::Result::BindError) { - cerr << "BindError, retrying with new client cookie and no specific address to bind to" << endl; // BindError is only generated when cookies are active and we failed to bind to a local // address associated with a cookie, see RFC9018 section 3 last paragraph. We assume the // called code alread erased the cookie info. @@ -1629,7 +1628,6 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& address, bool continue; } else if (res->d_validpacket && res->d_haveEDNS && ret == LWResult::Result::BadCookie) { - cerr << "Retrying with received server cookie" << endl; // We assume the received cookie was stored and will be used in the second iteration // This is the second path that re-iterates the loop continue; @@ -5569,8 +5567,6 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, ednsStats(ednsmask, qname, prefix); } - cerr << "asyncrW: returns " << int(resolveret) << " rcode is " << int(lwr.d_rcode) << endl; - /* preoutquery killed the query by setting dq.rcode to -3 */ if (preOutQueryRet == -3) { throw ImmediateServFailException("Query killed by policy"); @@ -5579,7 +5575,6 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, d_totUsec += lwr.d_usec; if (resolveret == LWResult::Result::Spoofed || resolveret == LWResult::Result::BadCookie) { - cerr << "Acting as we got a spoof" << endl; spoofed = true; return false; } @@ -6073,7 +6068,7 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con if (SyncRes::s_dot_to_port_853 && remoteIP->getPort() == 853) { doDoT = true; } - bool forceTCP = doDoT | true; + bool forceTCP = doDoT; if (!doDoT && s_max_busy_dot_probes > 0) { submitTryDotTask(*remoteIP, auth, tns->first, d_now.tv_sec); @@ -6084,7 +6079,6 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con } if (forceTCP || (spoofed || (gotAnswer && truncated))) { /* retry, over TCP this time */ - cerr << "Retry over TCP" << endl; gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, tns->first, *remoteIP, true, doDoT, truncated, spoofed, context.extendedError); } From e9fe4ab44d64cea8e540b1a4b25a9adb328cb1fb Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 19 Feb 2025 11:09:05 +0100 Subject: [PATCH 05/22] Basic cookies test: enable cookies in rec and talk to auths with cookies disabled and enabled --- pdns/recursordist/lwres.cc | 21 +- pdns/recursordist/lwres.hh | 1 + pdns/recursordist/rec_channel_rec.cc | 4 + pdns/recursordist/syncres.cc | 2 + .../recursortests.py | 10 + .../requirements.in | 1 + .../test_Cookies.py | 212 ++++++++++++++++++ .../test_SimpleCookies.py | 142 ++++++++++++ 8 files changed, 390 insertions(+), 3 deletions(-) create mode 100644 regression-tests.recursor-dnssec/test_Cookies.py create mode 100644 regression-tests.recursor-dnssec/test_SimpleCookies.py diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index d47f9ce01e56..a524e72b8a71 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -69,6 +69,13 @@ bool g_paddingOutgoing; static LockGuarded s_cookiestore; +std::string clearCookies() +{ + auto lock = s_cookiestore.lock(); + lock->clear(); + return ""; +} + void pruneCookies(time_t cutoff) { auto lock = s_cookiestore.lock(); @@ -715,16 +722,23 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } else { // Server responded with a wrong client cookie, fall back to TCP + cerr << "Wrong cookie" << endl; lwr->d_validpacket = true; - return LWResult::Result::BadCookie; + return LWResult::Result::Spoofed; } } else { // We sent a cookie out but forgot it? + cerr << "Cookie not found back"<< endl; lwr->d_validpacket = true; - return LWResult::Result::BadCookie; + return LWResult::Result::BadCookie; // XXX } } + else { + cerr << "Malformed cookie in reply"<< endl; + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; // XXX + } } } } @@ -750,8 +764,9 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // Case: we sent out a cookie but did not get one back if (cookieSentOut && !cookieFoundInReply && !*chained) { + cerr << "No cookie in reply"<< endl; lwr->d_validpacket = true; - return LWResult::Result::BadCookie; + return LWResult::Result::BadCookie; // XXX } if (outgoingLoggers) { diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index bac767c7195b..977f20da1667 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -96,5 +96,6 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int flags, const ComboAddress& LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); uint64_t dumpCookies(int fileDesc); +std::string clearCookies(); void pruneCookies(time_t cutoff); void setAuthCookies(bool flag); diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index f519cfeaba91..66c9bb936262 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -1855,6 +1855,7 @@ static RecursorControlChannel::Answer help() "add-nta DOMAIN [REASON] add a Negative Trust Anchor for DOMAIN with the comment REASON\n" "add-ta DOMAIN DSRECORD add a Trust Anchor for DOMAIN with data DSRECORD\n" "current-queries show currently active queries\n" + // "clear-cookies clear cookie table\n" XXX undocumented for now "clear-dont-throttle-names [N...] remove names that are not allowed to be throttled. If N is '*', remove all\n" "clear-dont-throttle-netmasks [N...]\n" " remove netmasks that are not allowed to be throttled. If N is '*', remove all\n" @@ -2075,6 +2076,9 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int socket, cons if (cmd == "dump-cache") { return doDumpCache(socket, begin, end); } + if (cmd == "clear-cookies") { + return {0, clearCookies()}; + } if (cmd == "dump-cookies") { return doDumpToFile(socket, pleaseDumpCookiesMap, cmd, false); } diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 9dfc42db292b..23b298aeb542 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -5562,6 +5562,7 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, s_ecsqueries++; } updateQueryCounts(prefix, qname, remoteIP, doTCP, doDoT); + cerr << "doTCP " << doTCP << endl; resolveret = asyncresolveWrapper(remoteIP, d_doDNSSEC, qname, auth, qtype.getCode(), doTCP, sendRDQuery, &d_now, ednsmask, &lwr, &chained, nsName); // <- we go out on the wire! ednsStats(ednsmask, qname, prefix); @@ -6077,6 +6078,7 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, tns->first, *remoteIP, false, false, truncated, spoofed, context.extendedError); } + cerr << "Got spoofed?!" << spoofed << endl; if (forceTCP || (spoofed || (gotAnswer && truncated))) { /* retry, over TCP this time */ gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, diff --git a/regression-tests.recursor-dnssec/recursortests.py b/regression-tests.recursor-dnssec/recursortests.py index 5e718dcf208a..0b7c9e1ccabe 100644 --- a/regression-tests.recursor-dnssec/recursortests.py +++ b/regression-tests.recursor-dnssec/recursortests.py @@ -732,6 +732,16 @@ def wipeRecursorCache(cls, confdir, name='.$'): except subprocess.CalledProcessError as e: raise AssertionError('%s failed (%d): %s' % (rec_controlCmd, e.returncode, e.output)) + @classmethod + def recControl(cls, confdir, *command): + rec_controlCmd = [os.environ['RECCONTROL'], + '--config-dir=%s' % confdir + ] + list(command) + try: + return subprocess.check_output(rec_controlCmd, text=True, stderr=subprocess.STDOUT) + except subprocess.CalledProcessError as e: + raise AssertionError('%s failed (%d): %s' % (rec_controlCmd, e.returncode, e.output)) + @classmethod def setUpSockets(cls): print("Setting up UDP socket..") diff --git a/regression-tests.recursor-dnssec/requirements.in b/regression-tests.recursor-dnssec/requirements.in index 9f4dab033bd3..fcff612dea2b 100644 --- a/regression-tests.recursor-dnssec/requirements.in +++ b/regression-tests.recursor-dnssec/requirements.in @@ -9,3 +9,4 @@ pysnmp>=5,<6 requests>=2.1.0 Twisted>0.15.0 pyyaml==6.0.1 +siphash diff --git a/regression-tests.recursor-dnssec/test_Cookies.py b/regression-tests.recursor-dnssec/test_Cookies.py new file mode 100644 index 000000000000..857557ab72be --- /dev/null +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -0,0 +1,212 @@ +import dns +import socket +import os +import time +import threading +from twisted.internet.protocol import Factory +from twisted.internet.protocol import Protocol +from twisted.internet.protocol import DatagramProtocol +from twisted.internet import reactor + +from recursortests import RecursorTest + +class CookiesTest(RecursorTest): + _confdir = 'Cookies' + + _config_template = """ +recursor: + forward_zones: + - zone: cookies.example + forwarders: [%s.25] +outgoing: + cookies: true""" % (os.environ['PREFIX']) + + _expectedCookies = 'no' + @classmethod + def generateRecursorConfig(cls, confdir): + super(CookiesTest, cls).generateRecursorYamlConfig(confdir) + + @classmethod + def setUpClass(cls): + cls.setUpSockets() + + cls.startResponders() + + confdir = os.path.join('configs', cls._confdir) + cls.createConfigDir(confdir) + + cls.generateRecursorConfig(confdir) + cls.startRecursor(confdir, cls._recursorPort) + + print("Launching tests..") + + @classmethod + def tearDownClass(cls): + cls.tearDownRecursor() + + @classmethod + def startResponders(cls): + print("Launching responders..") + + address = cls._PREFIX + '.25' + port = 53 + + reactor.listenUDP(port, UDPResponder(), interface=address) + reactor.listenTCP(port, TCPFactory(), interface=address) + + if not reactor.running: + cls.Responder = threading.Thread(name='Responder', target=reactor.run, args=(False,)) + cls.Responder.daemon = True + cls.Responder.start() + #cls._TCPResponder = threading.Thread(name='TCP Responder', target=reactor.run, args=(False,)) + #cls._TCPResponder.daemon = True + #cls._TCPResponder.start() + + def checkCookies(self, support): + confdir = os.path.join('configs', self._confdir) + output = self.recControl(confdir, 'dump-cookies', '-') + for line in output.splitlines(): + tokens = line.split() + if tokens[0] != '127.0.0.25': + continue + print(tokens) + self.assertEqual(len(tokens), 5) + self.assertEqual(tokens[3], support) + + def testAuthDoesnotSendCookies(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec does not get a cookie back + expected = dns.rrset.from_text('a.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('a.cookies.example.', 'A') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('no') + + def testAuthRepliesWithCookies(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec gets a proper client and server cookie back + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('b.cookies.example.', 'A') + expected = dns.rrset.from_text('b.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('yes') + + # Case: we get a an correct client and server cookie back + # We do not clear the cookie tables, so the old server cookie gets re-used + query = dns.message.make_query('c.cookies.example.', 'A') + expected = dns.rrset.from_text('c.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('yes') + + def testAuthSendsIncorrectClientCookie(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec gets a an incorrect client cookie back + # Fails at the moment, as we do not do the right thing yet server side XXXX + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('d.cookies.example.', 'A') + expected = dns.rrset.from_text('d.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('yes') + + def testAuthSendsBADCOOKIEOverUDP(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec gets a BADCOOKIE, even on retry and should fall back to TCP + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('e.cookies.example.', 'A') + expected = dns.rrset.from_text('e.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('yes') + + +class UDPResponder(DatagramProtocol): + def getCookie(self, message): + for option in message.options: + if option.otype == dns.edns.COOKIE: #and isinstance(option, dns.edns.CookieOption): + return option.data + return None + + def createCookie(self, clientcookie): + clientcookie = clientcookie[0:8] + timestamp = int(time.time()) + server = clientcookie + b'\x01\x00\x00\x00' + timestamp.to_bytes(4, 'big') + h = hash(server + b'\x01\x00\x00\x7f' + b'secret') % pow(2, 64) + full = dns.edns.GenericOption(dns.edns.COOKIE, server + h.to_bytes(8, 'big')) + return full + + def question(self, datagram, tcp=False): + request = dns.message.from_wire(datagram) + + response = dns.message.make_response(request) + response.flags = dns.flags.AA + dns.flags.QR + + question = request.question[0] + + # Case: do not send cookie back + if question.name == dns.name.from_text('a.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('a.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + response.answer.append(answer) + + # Case: do send cookie back + elif question.name == dns.name.from_text('b.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('b.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + response.answer.append(answer) + + # We get a good client and server cookie + elif question.name == dns.name.from_text('c.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('c.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if len(clientcookie) != 24: + raise AssertionError("expected full cookie, got len " + str(len(clientcookie))) + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + response.answer.append(answer) + + # Case: do send incorrect client cookie back + elif question.name == dns.name.from_text('d.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('d.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if clientcookie is not None: + mod = bytearray(clientcookie) + mod[0] = 1 + response.use_edns(options = [self.createCookie(bytes(mod))]) + response.answer.append(answer) + + # Case: do send BADCOOKIE cookie back if UDP + elif question.name == dns.name.from_text('e.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('e.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + if not tcp: + response.set_rcode(23) # BADCOOKIE + response.answer.append(answer) + + return response.to_wire() + + def datagramReceived(self, datagram, address): + response = self.question(datagram) + self.transport.write(response, address) + +class TCPResponder(Protocol): + def dataReceived(self, data): + handler = UDPResponder() + response = handler.question(data[2:], True) + length = len(response) + header = length.to_bytes(2, 'big') + self.transport.write(header + response) + +class TCPFactory(Factory): + def buildProtocol(self, addr): + return TCPResponder() diff --git a/regression-tests.recursor-dnssec/test_SimpleCookies.py b/regression-tests.recursor-dnssec/test_SimpleCookies.py new file mode 100644 index 000000000000..d6a7ccb8eeab --- /dev/null +++ b/regression-tests.recursor-dnssec/test_SimpleCookies.py @@ -0,0 +1,142 @@ +import dns +import os +from recursortests import RecursorTest + +class SimpleCookiesTest(RecursorTest): + _confdir = 'SimpleCookies' + + _config_template = """ +recursor: + auth_zones: + - zone: authzone.example + file: configs/%s/authzone.zone +dnssec: + validation: validate +outgoing: + cookies: true""" % _confdir + + _expectedCookies = 'no' + @classmethod + def generateRecursorConfig(cls, confdir): + authzonepath = os.path.join(confdir, 'authzone.zone') + with open(authzonepath, 'w') as authzone: + authzone.write("""$ORIGIN authzone.example. +@ 3600 IN SOA {soa} +@ 3600 IN A 192.0.2.88 +""".format(soa=cls._SOA)) + super(SimpleCookiesTest, cls).generateRecursorYamlConfig(confdir) + + def checkCookies(self): + confdir = os.path.join('configs', self._confdir) + output = self.recControl(confdir, 'dump-cookies', '-') + for line in output.splitlines(): + tokens = line.split() + if tokens[0] == ';' or tokens[0] == 'dump-cookies:': + continue + print(tokens) + self.assertEqual(len(tokens), 5) + self.assertEqual(tokens[3], self._expectedCookies) + + def testSOAs(self): + for zone in ['.', 'example.', 'secure.example.']: + expected = dns.rrset.from_text(zone, 0, dns.rdataclass.IN, 'SOA', self._SOA) + query = dns.message.make_query(zone, 'SOA', want_dnssec=True) + query.flags |= dns.flags.AD + + res = self.sendUDPQuery(query) + + self.assertMessageIsAuthenticated(res) + self.assertRRsetInAnswer(res, expected) + self.assertMatchingRRSIGInAnswer(res, expected) + self.checkCookies() + + def testA(self): + expected = dns.rrset.from_text('ns.secure.example.', 0, dns.rdataclass.IN, 'A', '{prefix}.9'.format(prefix=self._PREFIX)) + query = dns.message.make_query('ns.secure.example', 'A', want_dnssec=True) + query.flags |= dns.flags.AD + + res = self.sendUDPQuery(query) + + self.assertMessageIsAuthenticated(res) + self.assertRRsetInAnswer(res, expected) + self.assertMatchingRRSIGInAnswer(res, expected) + self.checkCookies() + + def testDelegation(self): + query = dns.message.make_query('example', 'NS', want_dnssec=True) + query.flags |= dns.flags.AD + + expectedNS = dns.rrset.from_text('example.', 0, 'IN', 'NS', 'ns1.example.', 'ns2.example.') + + res = self.sendUDPQuery(query) + + self.assertMessageIsAuthenticated(res) + self.assertRRsetInAnswer(res, expectedNS) + self.checkCookies() + + def testBogus(self): + query = dns.message.make_query('ted.bogus.example', 'A', want_dnssec=True) + + res = self.sendUDPQuery(query) + + self.assertRcodeEqual(res, dns.rcode.SERVFAIL) + self.checkCookies() + + def testAuthZone(self): + query = dns.message.make_query('authzone.example', 'A', want_dnssec=True) + + expectedA = dns.rrset.from_text('authzone.example.', 0, 'IN', 'A', '192.0.2.88') + + res = self.sendUDPQuery(query) + + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expectedA) + self.checkCookies() + + def testLocalhost(self): + queryA = dns.message.make_query('localhost', 'A', want_dnssec=True) + expectedA = dns.rrset.from_text('localhost.', 0, 'IN', 'A', '127.0.0.1') + + queryPTR = dns.message.make_query('1.0.0.127.in-addr.arpa', 'PTR', want_dnssec=True) + expectedPTR = dns.rrset.from_text('1.0.0.127.in-addr.arpa.', 0, 'IN', 'PTR', 'localhost.') + + resA = self.sendUDPQuery(queryA) + resPTR = self.sendUDPQuery(queryPTR) + + self.assertRcodeEqual(resA, dns.rcode.NOERROR) + self.assertRRsetInAnswer(resA, expectedA) + + self.assertRcodeEqual(resPTR, dns.rcode.NOERROR) + self.assertRRsetInAnswer(resPTR, expectedPTR) + self.checkCookies() + + def testLocalhostSubdomain(self): + queryA = dns.message.make_query('foo.localhost', 'A', want_dnssec=True) + expectedA = dns.rrset.from_text('foo.localhost.', 0, 'IN', 'A', '127.0.0.1') + + resA = self.sendUDPQuery(queryA) + + self.assertRcodeEqual(resA, dns.rcode.NOERROR) + self.assertRRsetInAnswer(resA, expectedA) + self.checkCookies() + + def testIslandOfSecurity(self): + query = dns.message.make_query('cname-to-islandofsecurity.secure.example.', 'A', want_dnssec=True) + + expectedCNAME = dns.rrset.from_text('cname-to-islandofsecurity.secure.example.', 0, 'IN', 'CNAME', 'node1.islandofsecurity.example.') + expectedA = dns.rrset.from_text('node1.islandofsecurity.example.', 0, 'IN', 'A', '192.0.2.20') + + res = self.sendUDPQuery(query) + + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expectedA) + self.checkCookies() + +class SimpleCookiesAuthEnabledTest(SimpleCookiesTest): + _confdir = 'SimpleCookiesAuthEnabled' + _expectedCookies = 'yes' + + @classmethod + def generateAuthConfig(cls, confdir, threads): + super(SimpleCookiesAuthEnabledTest, cls).generateAuthConfig(confdir, threads, "edns-cookie-secret=01234567890123456789012345678901") + From 90ab1be843babea51729007762157e3d72c30d86 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 21 Mar 2025 09:51:11 +0100 Subject: [PATCH 06/22] four states: unknown, supported, unsupported and probing --- pdns/recursordist/lwres.cc | 20 +++++++++----- pdns/recursordist/meson.build | 1 + pdns/recursordist/rec-cookiestore.cc | 2 +- pdns/recursordist/rec-cookiestore.hh | 39 +++++++++++++++++++++++++++- 4 files changed, 53 insertions(+), 9 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index a524e72b8a71..e849a38d76ec 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -494,24 +494,30 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& auto lock = s_cookiestore.lock(); auto found = lock->find(address); if (found != lock->end()) { - if (found->d_support) { + switch (found->getSupport()) { + case CookieEntry::Support::Supported: + case CookieEntry::Support::Probing: cookieSentOut = found->d_cookie; addressToBindTo = found->d_localaddress; opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); found->d_lastupdate = now->tv_sec; cerr << "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl; - } - else { - cerr << "This server does not support cookies" << endl; + break; + case CookieEntry::Support::Unknown: + assert(0); + case CookieEntry::Support::Unsupported: + default: + cerr << "This server does not support cookies or we don't know yet:" << endl; } } else { + // Server not in table CookieEntry entry; entry.d_address = address; entry.d_cookie.makeClientCookie(); cookieSentOut = entry.d_cookie; entry.d_lastupdate = now->tv_sec; - entry.d_support = false; + entry.setSupport(CookieEntry::Support::Probing); lock->emplace(entry); opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); cerr << "We're sending new client cookie info from to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl; @@ -713,7 +719,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& found->d_localaddress = localip; found->d_cookie = received; found->d_lastupdate = now->tv_sec; - found->d_support = true; + found->setSupport(CookieEntry::Support::Supported); uint16_t ercode = (edo.d_extRCode << 4) | lwr->d_rcode; if (ercode == ERCode::BADCOOKIE) { lwr->d_validpacket = true; @@ -728,7 +734,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } } else { - // We sent a cookie out but forgot it? + // We sent a cookie out but it's not in the table? cerr << "Cookie not found back"<< endl; lwr->d_validpacket = true; return LWResult::Result::BadCookie; // XXX diff --git a/pdns/recursordist/meson.build b/pdns/recursordist/meson.build index 40da702aa80e..56fd6b8980ff 100644 --- a/pdns/recursordist/meson.build +++ b/pdns/recursordist/meson.build @@ -150,6 +150,7 @@ common_sources += files( src_dir / 'rec-zonetocache.cc', src_dir / 'rec_channel.cc', src_dir / 'rec_channel_rec.cc', + src_dir / 'rec-cookiestore.cc', src_dir / 'rec-xfr.cc', src_dir / 'rec-xfrtracker.cc', src_dir / 'recpacketcache.cc', diff --git a/pdns/recursordist/rec-cookiestore.cc b/pdns/recursordist/rec-cookiestore.cc index 321b9c090918..adda2ecbb37b 100644 --- a/pdns/recursordist/rec-cookiestore.cc +++ b/pdns/recursordist/rec-cookiestore.cc @@ -53,7 +53,7 @@ uint64_t CookieStore::dump(const CookieStore& copy, int fileDesc) fprintf(filePtr.get(), "%s\t%s\t%s\t%s\t%s\n", entry.d_address.toString().c_str(), entry.d_localaddress.toString().c_str(), entry.d_cookie.toDisplayString().c_str(), - entry.d_support ? "yes" : "no", + CookieEntry::toString(entry.d_support).c_str(), timestamp(entry.d_lastupdate, tmp)); } return count; diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index a78810ad5359..d79221c4c03b 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -52,11 +52,48 @@ using namespace ::boost::multi_index; struct CookieEntry { + enum class Support : uint8_t + { + Unknown, + Unsupported, + Supported, + Probing + }; + + static std::string toString(Support support) + { + static const std::array names = { + "Unknown", + "Unsupported", + "Supported", + "Probing"}; + const auto index = static_cast(support); + if (index >= names.size()) { + return "?"; + } + return names.at(index); + } + + Support getSupport() const + { + return d_support; + } + + void setSupport(Support support) const // modifying mutable field + { + d_support = support; + } + + bool supported() const + { + return d_support == Support::Supported; + } + ComboAddress d_address; mutable ComboAddress d_localaddress; // The address we were bound to, see RFC 9018 mutable EDNSCookiesOpt d_cookie; // Contains both client and server cookie mutable time_t d_lastupdate{}; - mutable bool d_support; + mutable Support d_support{Support::Unknown}; }; class CookieStore : public multi_index_container < CookieEntry, From 395ad57970adff8724662a79eb8eb9f9133da93e Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 21 Mar 2025 12:05:20 +0100 Subject: [PATCH 07/22] Invalid cookie case, plus test of moving to next NS in that case --- pdns/recursordist/lwres.cc | 55 ++++++++++++----- pdns/recursordist/syncres.cc | 2 +- .../test_Cookies.py | 61 +++++++++++++------ 3 files changed, 83 insertions(+), 35 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index e849a38d76ec..41964b0fcb6a 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -506,8 +506,8 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& case CookieEntry::Support::Unknown: assert(0); case CookieEntry::Support::Unsupported: - default: - cerr << "This server does not support cookies or we don't know yet:" << endl; + cerr << "This server does not support cookies" << endl; + break; } } else { @@ -723,27 +723,33 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& uint16_t ercode = (edo.d_extRCode << 4) | lwr->d_rcode; if (ercode == ERCode::BADCOOKIE) { lwr->d_validpacket = true; - return LWResult::Result::BadCookie; + return LWResult::Result::BadCookie; // proper use of BacCookie, we did update he entry } } else { - // Server responded with a wrong client cookie, fall back to TCP - cerr << "Wrong cookie" << endl; - lwr->d_validpacket = true; - return LWResult::Result::Spoofed; + if (!doTCP) { + // Server responded with a wrong client cookie, fall back to TCP + cerr << "Wrong cookie" << endl; + lwr->d_validpacket = true; + return LWResult::Result::Spoofed; + } + // ignore bad cookie when already doing TCP } } else { - // We sent a cookie out but it's not in the table? + // We receivd cookie (we might have sent one out) but it's not in the table? cerr << "Cookie not found back"<< endl; - lwr->d_validpacket = true; - return LWResult::Result::BadCookie; // XXX + lwr->d_rcode = RCode::FormErr; + lwr->d_validpacket = false; + return LWResult::Result::Success; // success - oddly enough } } else { - cerr << "Malformed cookie in reply"<< endl; - lwr->d_validpacket = true; - return LWResult::Result::BadCookie; // XXX + cerr << "Malformed cookie in reply" << endl; + // Do something special if we get malformed repeatedly? And or consider current status: Supported + lwr->d_rcode = RCode::FormErr; + lwr->d_validpacket = false; + return LWResult::Result::Success; // succes - odly enough } } } @@ -771,8 +777,27 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // Case: we sent out a cookie but did not get one back if (cookieSentOut && !cookieFoundInReply && !*chained) { cerr << "No cookie in reply"<< endl; - lwr->d_validpacket = true; - return LWResult::Result::BadCookie; // XXX + auto lock = s_cookiestore.lock(); + auto found = lock->find(address); + if (found != lock->end()) { + switch (found->getSupport()) { + case CookieEntry::Support::Unknown: + assert(0); + case CookieEntry::Support::Probing: + cerr << "Was probing, setting support to Unsupported" << endl; + found->setSupport(CookieEntry::Support::Unsupported); + break; + case CookieEntry::Support::Unsupported: + break; + case CookieEntry::Support::Supported: + lwr->d_validpacket = true; + return LWResult::Result::BadCookie; // XXX, we did not update cookie info... + break; + } + } + else { + // Table entry lost? XXX + } } if (outgoingLoggers) { diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 23b298aeb542..625210dfb2e7 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -6078,7 +6078,7 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, tns->first, *remoteIP, false, false, truncated, spoofed, context.extendedError); } - cerr << "Got spoofed?!" << spoofed << endl; + if (spoofed) cerr << "Got spoofed! " << spoofed << endl; if (forceTCP || (spoofed || (gotAnswer && truncated))) { /* retry, over TCP this time */ gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, diff --git a/regression-tests.recursor-dnssec/test_Cookies.py b/regression-tests.recursor-dnssec/test_Cookies.py index 857557ab72be..6ec7467912b7 100644 --- a/regression-tests.recursor-dnssec/test_Cookies.py +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -12,14 +12,13 @@ class CookiesTest(RecursorTest): _confdir = 'Cookies' - _config_template = """ recursor: forward_zones: - zone: cookies.example - forwarders: [%s.25] + forwarders: [%s.25, %s.26] outgoing: - cookies: true""" % (os.environ['PREFIX']) + cookies: true""" % (os.environ['PREFIX'], os.environ['PREFIX']) _expectedCookies = 'no' @classmethod @@ -48,28 +47,28 @@ def tearDownClass(cls): def startResponders(cls): print("Launching responders..") - address = cls._PREFIX + '.25' + address1 = cls._PREFIX + '.25' + address2 = cls._PREFIX + '.26' port = 53 - reactor.listenUDP(port, UDPResponder(), interface=address) - reactor.listenTCP(port, TCPFactory(), interface=address) + reactor.listenUDP(port, UDPResponder(), interface=address1) + reactor.listenTCP(port, TCPFactory(), interface=address1) + reactor.listenUDP(port, UDPResponder(), interface=address2) + reactor.listenTCP(port, TCPFactory(), interface=address2) if not reactor.running: cls.Responder = threading.Thread(name='Responder', target=reactor.run, args=(False,)) cls.Responder.daemon = True cls.Responder.start() - #cls._TCPResponder = threading.Thread(name='TCP Responder', target=reactor.run, args=(False,)) - #cls._TCPResponder.daemon = True - #cls._TCPResponder.start() - def checkCookies(self, support): + def checkCookies(self, support, server='127.0.0.25'): confdir = os.path.join('configs', self._confdir) output = self.recControl(confdir, 'dump-cookies', '-') for line in output.splitlines(): tokens = line.split() - if tokens[0] != '127.0.0.25': + if tokens[0] != server: continue - print(tokens) + #print(tokens) self.assertEqual(len(tokens), 5) self.assertEqual(tokens[3], support) @@ -81,7 +80,7 @@ def testAuthDoesnotSendCookies(self): res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) - self.checkCookies('no') + self.checkCookies('Unsupported') def testAuthRepliesWithCookies(self): confdir = os.path.join('configs', self._confdir) @@ -92,7 +91,7 @@ def testAuthRepliesWithCookies(self): res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) - self.checkCookies('yes') + self.checkCookies('Supported') # Case: we get a an correct client and server cookie back # We do not clear the cookie tables, so the old server cookie gets re-used @@ -101,19 +100,18 @@ def testAuthRepliesWithCookies(self): res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) - self.checkCookies('yes') + self.checkCookies('Supported') def testAuthSendsIncorrectClientCookie(self): confdir = os.path.join('configs', self._confdir) - # Case: rec gets a an incorrect client cookie back - # Fails at the moment, as we do not do the right thing yet server side XXXX + # Case: rec gets a an incorrect client cookie back, we ignore that over TCP self.recControl(confdir, 'clear-cookies') query = dns.message.make_query('d.cookies.example.', 'A') expected = dns.rrset.from_text('d.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) - self.checkCookies('yes') + self.checkCookies('Probing') def testAuthSendsBADCOOKIEOverUDP(self): confdir = os.path.join('configs', self._confdir) @@ -124,7 +122,19 @@ def testAuthSendsBADCOOKIEOverUDP(self): res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) - self.checkCookies('yes') + self.checkCookies('Supported') + + def testAuthSendsMalformedCookie(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec gets a malformed cookie, should ignore packet + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('f.cookies.example.', 'A') + expected = dns.rrset.from_text('f.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('Probing', '127.0.0.25') + self.checkCookies('Supported', '127.0.0.26') class UDPResponder(DatagramProtocol): @@ -193,6 +203,19 @@ def question(self, datagram, tcp=False): response.set_rcode(23) # BADCOOKIE response.answer.append(answer) + # Case send malformed cookie for server .25 + elif question.name == dns.name.from_text('f.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('f.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + print(self.transport.getHost().host) + if self.transport.getHost().host == os.environ['PREFIX'] + '.26': + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + else: + full = dns.edns.GenericOption(dns.edns.COOKIE, '') + response.use_edns(options = [full]) + response.answer.append(answer) + return response.to_wire() def datagramReceived(self, datagram, address): From 8ac61af66975ea96c4f52dc8a6b8771cd6c7971e Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 21 Mar 2025 12:45:11 +0100 Subject: [PATCH 08/22] Better tracing --- pdns/recursordist/lwres.cc | 35 ++++++++++++++-------------- pdns/recursordist/lwres.hh | 6 ++--- pdns/recursordist/meson.build | 2 +- pdns/recursordist/pdns_recursor.cc | 2 -- pdns/recursordist/syncres.cc | 11 +++++---- pdns/recursordist/syncres.hh | 2 +- pdns/recursordist/test-syncres_cc.cc | 2 +- 7 files changed, 30 insertions(+), 30 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 41964b0fcb6a..48e4263f2703 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -324,7 +324,7 @@ class BindError { }; -static bool tcpconnect(const ComboAddress& remote, const std::optional localBind, TCPOutConnectionManager::Connection& connection, bool& dnsOverTLS, const std::string& nsName) +static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std::optional localBind, TCPOutConnectionManager::Connection& connection, bool& dnsOverTLS, const std::string& nsName) { dnsOverTLS = SyncRes::s_dot_to_port_853 && remote.getPort() == 853; @@ -340,10 +340,10 @@ static bool tcpconnect(const ComboAddress& remote, const std::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, [[maybe_unused]] const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained, TCPOutConnectionManager::Connection& connection) +static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, [[maybe_unused]] const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained, TCPOutConnectionManager::Connection& connection) { size_t len; size_t bufsize = g_outgoingEDNSBufsize; @@ -501,12 +501,12 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& addressToBindTo = found->d_localaddress; opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); found->d_lastupdate = now->tv_sec; - cerr << "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl; + VLOG(log, "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl); break; case CookieEntry::Support::Unknown: assert(0); case CookieEntry::Support::Unsupported: - cerr << "This server does not support cookies" << endl; + VLOG(log, "Server << " << address.toString() << " does not support cookies" << endl); break; } } @@ -520,7 +520,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& entry.setSupport(CookieEntry::Support::Probing); lock->emplace(entry); opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); - cerr << "We're sending new client cookie info from to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl; + VLOG(log, "Sending new client cookie info to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl); } } @@ -608,7 +608,7 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // peer has closed it on error, so we retry. At some point we // *will* get a new connection, so this loop is not endless. isNew = true; // tcpconnect() might throw for new connections. In that case, we want to break the loop, scanbuild complains here, which is a false positive afaik - isNew = tcpconnect(address, addressToBindTo, connection, dnsOverTLS, nsName); + isNew = tcpconnect(log, address, addressToBindTo, connection, dnsOverTLS, nsName); ret = tcpsendrecv(address, connection, localip, vpacket, len, buf); #ifdef HAVE_FSTRM if (fstrmQEnabled) { @@ -710,12 +710,12 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& EDNSCookiesOpt received; if (received.makeFromString(opt.second)) { cookieFoundInReply = true; - cerr << "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl; + VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); auto lock = s_cookiestore.lock(); auto found = lock->find(address); if (found != lock->end()) { if (received.getClient() == cookieSentOut->getClient()) { - cerr << "Client cookie matched! Storing with localAddress " << localip.toString() << endl; + VLOG(log, "Client cookie matched! Storing with localAddress " << localip.toString() << endl); found->d_localaddress = localip; found->d_cookie = received; found->d_lastupdate = now->tv_sec; @@ -723,13 +723,14 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& uint16_t ercode = (edo.d_extRCode << 4) | lwr->d_rcode; if (ercode == ERCode::BADCOOKIE) { lwr->d_validpacket = true; + VLOG(log, "Server: " << localip.toString() << " returned BADCOOKIE " << endl); return LWResult::Result::BadCookie; // proper use of BacCookie, we did update he entry } } else { if (!doTCP) { // Server responded with a wrong client cookie, fall back to TCP - cerr << "Wrong cookie" << endl; + VLOG(log, "Server responded with wrong client cookie, fall back to TCP" << endl); lwr->d_validpacket = true; return LWResult::Result::Spoofed; } @@ -738,14 +739,14 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& } else { // We receivd cookie (we might have sent one out) but it's not in the table? - cerr << "Cookie not found back"<< endl; + VLOG(log, "Cookie not found back in table" << endl); lwr->d_rcode = RCode::FormErr; lwr->d_validpacket = false; return LWResult::Result::Success; // success - oddly enough } } else { - cerr << "Malformed cookie in reply" << endl; + VLOG(log, "Malformed cookie in reply" << endl); // Do something special if we get malformed repeatedly? And or consider current status: Supported lwr->d_rcode = RCode::FormErr; lwr->d_validpacket = false; @@ -776,7 +777,6 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& // Case: we sent out a cookie but did not get one back if (cookieSentOut && !cookieFoundInReply && !*chained) { - cerr << "No cookie in reply"<< endl; auto lock = s_cookiestore.lock(); auto found = lock->find(address); if (found != lock->end()) { @@ -784,10 +784,11 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& case CookieEntry::Support::Unknown: assert(0); case CookieEntry::Support::Probing: - cerr << "Was probing, setting support to Unsupported" << endl; + VLOG(log, "No cookie in repy, was probing, setting support to Unsupported" << endl); found->setSupport(CookieEntry::Support::Unsupported); break; case CookieEntry::Support::Unsupported: + // We could have detected the server does not support cookies in the meantime break; case CookieEntry::Support::Supported: lwr->d_validpacket = true; @@ -839,10 +840,10 @@ static LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& return LWResult::Result::PermanentError; } -LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained) +LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained) { TCPOutConnectionManager::Connection connection; - auto ret = asyncresolve(address, domain, type, doTCP, sendRDQuery, EDNS0Level, now, srcmask, context, outgoingLoggers, fstrmLoggers, exportTypes, lwr, chained, connection); + auto ret = asyncresolve(log, address, domain, type, doTCP, sendRDQuery, EDNS0Level, now, srcmask, context, outgoingLoggers, fstrmLoggers, exportTypes, lwr, chained, connection); if (doTCP) { if (connection.d_handler && lwr->d_validpacket) { diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index 977f20da1667..fbd657e340f4 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -43,8 +43,8 @@ #include "fstrm_logger.hh" #include "resolve-context.hh" #include "noinitvector.hh" - -#include "logging.hh" +#include "logger.hh" +#include "logr.hh" // Helper to be defined by main program: queue data and log based on return value of queueData() void remoteLoggerQueueData(RemoteLoggerInterface&, const std::string&); @@ -94,7 +94,7 @@ LWResult::Result asendto(const void* data, size_t len, int flags, const ComboAdd LWResult::Result arecvfrom(PacketBuffer& packet, int flags, const ComboAddress& fromAddr, size_t& len, uint16_t qid, const DNSName& domain, uint16_t qtype, int fileDesc, const struct timeval& now); -LWResult::Result asyncresolve(const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); +LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); uint64_t dumpCookies(int fileDesc); std::string clearCookies(); void pruneCookies(time_t cutoff); diff --git a/pdns/recursordist/meson.build b/pdns/recursordist/meson.build index 56fd6b8980ff..53199d3c4d09 100644 --- a/pdns/recursordist/meson.build +++ b/pdns/recursordist/meson.build @@ -128,7 +128,6 @@ common_sources += files( src_dir / 'logging.cc', src_dir / 'lua-base4.cc', src_dir / 'lua-recursor4.cc', - src_dir / 'lwres.cc', src_dir / 'misc.cc', src_dir / 'mtasker_context.cc', src_dir / 'negcache.cc', @@ -433,6 +432,7 @@ tools = { src_dir / 'rec-tcpout.cc', src_dir / 'rec-snmp.cc', src_dir / 'rec-tcp.cc', + src_dir / 'lwres.cc', mplexer_sources, ], 'manpages': ['pdns_recursor.1'], diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index 2e0b5c3e963a..a655754e72e8 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -182,13 +182,11 @@ int UDPClientSocks::makeClientSocket(int family, const std::optionaltoString() << endl; sin = *localAddress; sin.setPort(port); } else { sin = pdns::getQueryLocalAddress(family, port); // does htons for us - cerr << "Bound to random local address " << sin.toString() << endl; } if (::bind(ret, reinterpret_cast(&sin), sin.getSocklen()) >= 0) { // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast) break; diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 625210dfb2e7..d8d0bb76d5bc 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1540,7 +1540,7 @@ uint64_t SyncRes::doDumpDoTProbeMap(int fileDesc) For now this means we can't be clever, but will turn off DNSSEC if you reply with FormError or gibberish. */ -LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, [[maybe_unused]] const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const +LWResult::Result SyncRes::asyncresolveWrapper(const OptLog& log, const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, [[maybe_unused]] const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const { /* what is your QUEST? the goal is to get as many remotes as possible on the best level of EDNS support @@ -1602,7 +1602,7 @@ LWResult::Result SyncRes::asyncresolveWrapper(const ComboAddress& address, bool ret = d_asyncResolve(address, sendQname, type, doTCP, sendRDQuery, EDNSLevel, now, srcmask, ctx, res, chained); } else { - ret = asyncresolve(address, sendQname, type, doTCP, sendRDQuery, EDNSLevel, now, srcmask, ctx, d_outgoingProtobufServers, d_frameStreamServers, luaconfsLocal->outgoingProtobufExportConfig.exportTypes, res, chained); + ret = asyncresolve(log, address, sendQname, type, doTCP, sendRDQuery, EDNSLevel, now, srcmask, ctx, d_outgoingProtobufServers, d_frameStreamServers, luaconfsLocal->outgoingProtobufExportConfig.exportTypes, res, chained); } if (ret == LWResult::Result::PermanentError || LWResult::isLimitError(ret) || ret == LWResult::Result::Spoofed) { @@ -5562,8 +5562,7 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, s_ecsqueries++; } updateQueryCounts(prefix, qname, remoteIP, doTCP, doDoT); - cerr << "doTCP " << doTCP << endl; - resolveret = asyncresolveWrapper(remoteIP, d_doDNSSEC, qname, auth, qtype.getCode(), + resolveret = asyncresolveWrapper(LogObject(prefix), remoteIP, d_doDNSSEC, qname, auth, qtype.getCode(), doTCP, sendRDQuery, &d_now, ednsmask, &lwr, &chained, nsName); // <- we go out on the wire! ednsStats(ednsmask, qname, prefix); } @@ -6078,7 +6077,9 @@ int SyncRes::doResolveAt(NsSet& nameservers, DNSName auth, bool flawedNSSet, con gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, tns->first, *remoteIP, false, false, truncated, spoofed, context.extendedError); } - if (spoofed) cerr << "Got spoofed! " << spoofed << endl; + if (spoofed) { + LOG(prefix << qname << ": potentially spoofed, retrying over TCP" << endl); + } if (forceTCP || (spoofed || (gotAnswer && truncated))) { /* retry, over TCP this time */ gotAnswer = doResolveAtThisIP(prefix, qname, qtype, lwr, ednsmask, auth, sendRDQuery, wasForwarded, diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index 61ee00bde1a5..ccfa345bbdb6 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -673,7 +673,7 @@ private: bool doSpecialNamesResolve(const DNSName& qname, QType qtype, QClass qclass, vector& ret); - LWResult::Result asyncresolveWrapper(const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const; + LWResult::Result asyncresolveWrapper(const OptLog&log, const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const; boost::optional getEDNSSubnetMask(const DNSName& name, const ComboAddress& rem); diff --git a/pdns/recursordist/test-syncres_cc.cc b/pdns/recursordist/test-syncres_cc.cc index a38c1569439b..af86179077ec 100644 --- a/pdns/recursordist/test-syncres_cc.cc +++ b/pdns/recursordist/test-syncres_cc.cc @@ -64,7 +64,7 @@ void RecursorLua4::getFeatures(Features& /* features */) { } -LWResult::Result asyncresolve(const ComboAddress& /* ip */, const DNSName& /* domain */, int /* type */, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, const std::shared_ptr>>& /* outgoingLoggers */, const std::shared_ptr>>& /* fstrmLoggers */, const std::set& /* exportTypes */, LWResult* /* res */, bool* /* chained */) +LWResult::Result asyncresolve(const OptLog& /* log */, const ComboAddress& /* ip */, const DNSName& /* domain */, int /* type */, bool /* doTCP */, bool /* sendRDQuery */, int /* EDNS0Level */, struct timeval* /* now */, boost::optional& /* srcmask */, const ResolveContext& /* context */, const std::shared_ptr>>& /* outgoingLoggers */, const std::shared_ptr>>& /* fstrmLoggers */, const std::set& /* exportTypes */, LWResult* /* res */, bool* /* chained */) { return LWResult::Result::Timeout; } From 2014d496e8b200d8984f047c0b1b04cc18a674d9 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Mon, 24 Mar 2025 12:58:14 +0100 Subject: [PATCH 09/22] Fix regression tests --- .../recursortests.py | 11 +++++++ .../test_Cookies.py | 30 ++++++++++++------- regression-tests.recursor-dnssec/test_ECS.py | 5 +--- .../test_EDNSBufferSize.py | 6 +--- .../test_Interop.py | 5 +--- regression-tests.recursor-dnssec/test_Lua.py | 5 +--- .../test_RoutingTag.py | 5 +--- .../test_SimpleCookies.py | 6 ++-- 8 files changed, 39 insertions(+), 34 deletions(-) diff --git a/regression-tests.recursor-dnssec/recursortests.py b/regression-tests.recursor-dnssec/recursortests.py index 0b7c9e1ccabe..6fa153e0eeff 100644 --- a/regression-tests.recursor-dnssec/recursortests.py +++ b/regression-tests.recursor-dnssec/recursortests.py @@ -13,6 +13,8 @@ import dns import dns.message import requests +import threading +from twisted.internet import reactor from proxyprotocol import ProxyProtocol @@ -444,6 +446,8 @@ class RecursorTest(AssertEqualDNSMessageMixin, unittest.TestCase): # 22: test_EDNSBuffer.py # 23: test_Lua.py # 24: test_RoutingTag.py + # 25: test_Cookies.py + # 26: test_Cookies.py _auth_cmd = ['authbind', os.environ['PDNS']] @@ -1253,3 +1257,10 @@ def checkMetrics(self, map): self.assertEqual(value, expected, key + ": value " + str(value) + " is not expected") count += 1 self.assertEqual(count, len(map)) + + @classmethod + def startReactor(cls): + if not reactor.running: + cls.Responder = threading.Thread(name='Responder', target=reactor.run, args=(False,)) + cls.Responder.daemon = True + cls.Responder.start() diff --git a/regression-tests.recursor-dnssec/test_Cookies.py b/regression-tests.recursor-dnssec/test_Cookies.py index 6ec7467912b7..490f5a31d1b8 100644 --- a/regression-tests.recursor-dnssec/test_Cookies.py +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -2,14 +2,19 @@ import socket import os import time -import threading + from twisted.internet.protocol import Factory from twisted.internet.protocol import Protocol from twisted.internet.protocol import DatagramProtocol from twisted.internet import reactor +import clientsubnetoption +import cookiesoption + from recursortests import RecursorTest +cookieReactorRunning = False + class CookiesTest(RecursorTest): _confdir = 'Cookies' _config_template = """ @@ -45,21 +50,21 @@ def tearDownClass(cls): @classmethod def startResponders(cls): + global cookieReactorRunning print("Launching responders..") address1 = cls._PREFIX + '.25' address2 = cls._PREFIX + '.26' port = 53 - reactor.listenUDP(port, UDPResponder(), interface=address1) - reactor.listenTCP(port, TCPFactory(), interface=address1) - reactor.listenUDP(port, UDPResponder(), interface=address2) - reactor.listenTCP(port, TCPFactory(), interface=address2) + if not cookieReactorRunning: + reactor.listenUDP(port, UDPResponder(), interface=address1) + reactor.listenTCP(port, TCPFactory(), interface=address1) + reactor.listenUDP(port, UDPResponder(), interface=address2) + reactor.listenTCP(port, TCPFactory(), interface=address2) + cookieReactorRunning = True - if not reactor.running: - cls.Responder = threading.Thread(name='Responder', target=reactor.run, args=(False,)) - cls.Responder.daemon = True - cls.Responder.start() + cls.startReactor() def checkCookies(self, support, server='127.0.0.25'): confdir = os.path.join('configs', self._confdir) @@ -140,8 +145,11 @@ def testAuthSendsMalformedCookie(self): class UDPResponder(DatagramProtocol): def getCookie(self, message): for option in message.options: - if option.otype == dns.edns.COOKIE: #and isinstance(option, dns.edns.CookieOption): - return option.data + if option.otype == dns.edns.COOKIE and isinstance(option, cookiesoption.CookiesOption): + data = option.client + if option.server is not None: + data += option.server + return data return None def createCookie(self, clientcookie): diff --git a/regression-tests.recursor-dnssec/test_ECS.py b/regression-tests.recursor-dnssec/test_ECS.py index be91694f7443..e62d396b7904 100644 --- a/regression-tests.recursor-dnssec/test_ECS.py +++ b/regression-tests.recursor-dnssec/test_ECS.py @@ -93,10 +93,7 @@ def startResponders(cls): reactor.listenUDP(53000, UDPECSResponder(), interface='::1') ecsReactorv6Running = True - if not reactor.running: - cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.daemon = True - cls._UDPResponder.start() + cls.startReactor() class NoECSTest(ECSTest): _confdir = 'NoECS' diff --git a/regression-tests.recursor-dnssec/test_EDNSBufferSize.py b/regression-tests.recursor-dnssec/test_EDNSBufferSize.py index 34c31d4b45ba..319c3866aab8 100644 --- a/regression-tests.recursor-dnssec/test_EDNSBufferSize.py +++ b/regression-tests.recursor-dnssec/test_EDNSBufferSize.py @@ -65,11 +65,7 @@ def startResponders(cls): reactor.listenUDP(port, UDPLargeResponder(), interface=address) ednsBufferReactorRunning = True - if not reactor.running: - cls._UDPResponder = threading.Thread( - name='UDP Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.setDaemon(True) - cls._UDPResponder.start() + cls.startReactor() def getMessage(self, testnum, payload=0): do_edns = payload > 0 diff --git a/regression-tests.recursor-dnssec/test_Interop.py b/regression-tests.recursor-dnssec/test_Interop.py index 33800beee96c..aa3d5125e78c 100644 --- a/regression-tests.recursor-dnssec/test_Interop.py +++ b/regression-tests.recursor-dnssec/test_Interop.py @@ -147,10 +147,7 @@ def startResponders(cls): reactor.listenUDP(port, UDPResponder(), interface=address) - if not reactor.running: - cls._UDPResponder = threading.Thread(name='UDP Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.setDaemon(True) - cls._UDPResponder.start() + cls.startReactor() class InteropProcessTest(RecursorTest): _confdir = 'InteropProcess' diff --git a/regression-tests.recursor-dnssec/test_Lua.py b/regression-tests.recursor-dnssec/test_Lua.py index 436a25f5199a..46cd2310acf4 100644 --- a/regression-tests.recursor-dnssec/test_Lua.py +++ b/regression-tests.recursor-dnssec/test_Lua.py @@ -335,10 +335,7 @@ def startResponders(cls): reactor.listenUDP(port, UDPHooksResponder(), interface=address) hooksReactorRunning = True - if not reactor.running: - cls._UDPResponder = threading.Thread(name='UDP Hooks Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.setDaemon(True) - cls._UDPResponder.start() + cls.startReactor() def testNoData(self): expected = dns.rrset.from_text('nodata.luahooks.example.', 3600, dns.rdataclass.IN, 'AAAA', '2001:DB8::1') diff --git a/regression-tests.recursor-dnssec/test_RoutingTag.py b/regression-tests.recursor-dnssec/test_RoutingTag.py index 9f0631b582df..24ddd5fa6d06 100644 --- a/regression-tests.recursor-dnssec/test_RoutingTag.py +++ b/regression-tests.recursor-dnssec/test_RoutingTag.py @@ -81,10 +81,7 @@ def startResponders(cls): reactor.listenUDP(port, UDPRoutingResponder(), interface=address) routingReactorRunning = True - if not reactor.running: - cls._UDPResponder = threading.Thread(name='UDP Routing Responder', target=reactor.run, args=(False,)) - cls._UDPResponder.setDaemon(True) - cls._UDPResponder.start() + cls.startReactor() @classmethod def tearDownClass(cls): diff --git a/regression-tests.recursor-dnssec/test_SimpleCookies.py b/regression-tests.recursor-dnssec/test_SimpleCookies.py index d6a7ccb8eeab..a6f0004e7349 100644 --- a/regression-tests.recursor-dnssec/test_SimpleCookies.py +++ b/regression-tests.recursor-dnssec/test_SimpleCookies.py @@ -4,6 +4,7 @@ class SimpleCookiesTest(RecursorTest): _confdir = 'SimpleCookies' + _auth_zones = RecursorTest._default_auth_zones _config_template = """ recursor: @@ -15,7 +16,7 @@ class SimpleCookiesTest(RecursorTest): outgoing: cookies: true""" % _confdir - _expectedCookies = 'no' + _expectedCookies = 'Unsupported' @classmethod def generateRecursorConfig(cls, confdir): authzonepath = os.path.join(confdir, 'authzone.zone') @@ -134,7 +135,8 @@ def testIslandOfSecurity(self): class SimpleCookiesAuthEnabledTest(SimpleCookiesTest): _confdir = 'SimpleCookiesAuthEnabled' - _expectedCookies = 'yes' + _auth_zones = SimpleCookiesTest._auth_zones + _expectedCookies = 'Supported' @classmethod def generateAuthConfig(cls, confdir, threads): From a4ecbdc7917d139b3f3cb15684c3b4d9316bdcbb Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Tue, 25 Mar 2025 12:01:11 +0100 Subject: [PATCH 10/22] Refactor cookie code out of asyncresolve --- pdns/recursordist/lwres.cc | 188 ++++++++++-------- pdns/recursordist/rec-cookiestore.hh | 7 +- pdns/recursordist/syncres.cc | 2 +- .../test_Cookies.py | 92 ++++++--- 4 files changed, 172 insertions(+), 117 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 48e4263f2703..cef7799ea25f 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -438,6 +438,96 @@ static void addPadding(const DNSPacketWriter& pw, size_t bufsize, DNSPacketWrite } } +static void outgoingCookie(const OptLog& log, const ComboAddress& address, const timeval& now, DNSPacketWriter::optvect_t& opts, std::optional& cookieSentOut, std::optional& addressToBindTo) +{ + auto lock = s_cookiestore.lock(); + auto found = lock->find(address); + if (found != lock->end()) { + switch (found->getSupport()) { + case CookieEntry::Support::Supported: + case CookieEntry::Support::Probing: + cookieSentOut = found->d_cookie; + addressToBindTo = found->d_localaddress; + opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); + found->d_lastupdate = now.tv_sec; + VLOG(log, "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl); + break; + case CookieEntry::Support::Unsupported: + VLOG(log, "Server " << address.toString() << " does not support cookies" << endl); + break; + } + } + else { + // Server not in table, it's either new or was purged + CookieEntry entry; + entry.d_address = address; + entry.d_cookie.makeClientCookie(); + cookieSentOut = entry.d_cookie; + entry.setSupport(CookieEntry::Support::Probing, now.tv_sec); + lock->emplace(entry); + opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); + VLOG(log, "Sending new client cookie info to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl); + } +} + +static std::pair incomingCookie(const OptLog& log, const ComboAddress& address, const ComboAddress& localip, const timeval& now, const std::optional& cookieSentOut, const EDNSOpts& edo, bool doTCP, LWResult& lwr, bool& cookieFoundInReply) +{ + auto lock = s_cookiestore.lock(); + auto found = lock->find(address); + + if (found == lock->end()) { + // We receivd cookie (we might have sent one out) but the server is not in the table? + // This is a case of cannot happen, unless rec_control clear-cookies was called + VLOG(log, "Cookie from " << address.toString() << " not found back in table" << endl); + lwr.d_rcode = RCode::FormErr; + lwr.d_validpacket = false; + return {true, LWResult::Result::Success}; // success - oddly enough + } + + // We have stored cookie info, scan for COOKIE option in EDNS + for (const auto& opt : edo.d_options) { + if (opt.first == EDNSOptionCode::COOKIE) { + if (EDNSCookiesOpt received; received.makeFromString(opt.second)) { + cookieFoundInReply = true; + VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); + if (received.getClient() == cookieSentOut->getClient()) { + VLOG(log, "Client cookie from " << address.toString() << " matched! Storing with localAddress " << localip.toString() << endl); + found->d_localaddress = localip; + found->d_cookie = received; + found->setSupport(CookieEntry::Support::Supported, now.tv_sec); + // check extended error code + uint16_t ercode = (edo.d_extRCode << 4) | lwr.d_rcode; + if (ercode == ERCode::BADCOOKIE) { + lwr.d_validpacket = true; + VLOG(log, "Server " << localip.toString() << " returned BADCOOKIE " << endl); + return {true, LWResult::Result::BadCookie}; // We did update the entry, retry should succeed + } + } + else { + if (!doTCP) { + // Server responded with a wrong client cookie, fall back to TCP, RFC 7873 5.3 + VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie, fall back to TCP" << endl); + lwr.d_validpacket = true; + return {true, LWResult::Result::Spoofed}; + } + // mismatched cookie when already doing TCP, ignore that + VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie over TCP, ignoring that" << endl); + } + } + else { + VLOG(log, "Malformed cookie in reply from " << address.toString() << ", dropping as if was a timeout" << endl); + // Do something special if we get malformed repeatedly? And or consider current status? + lwr.d_validpacket = false; + return {true, LWResult::Result::Timeout}; + } + break; // only consider first cookie option found, RFC 7873 5.3 + } // COOKIE option found + } // for + + // The cases where something special needs to be done have been handled above + return {false, LWResult::Result::Success}; +} + /** lwr is only filled out in case 1 was returned, and even when returning 1 for 'success', lwr might contain DNS errors Never throws! */ @@ -491,37 +581,7 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr } if (g_cookies) { - auto lock = s_cookiestore.lock(); - auto found = lock->find(address); - if (found != lock->end()) { - switch (found->getSupport()) { - case CookieEntry::Support::Supported: - case CookieEntry::Support::Probing: - cookieSentOut = found->d_cookie; - addressToBindTo = found->d_localaddress; - opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); - found->d_lastupdate = now->tv_sec; - VLOG(log, "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl); - break; - case CookieEntry::Support::Unknown: - assert(0); - case CookieEntry::Support::Unsupported: - VLOG(log, "Server << " << address.toString() << " does not support cookies" << endl); - break; - } - } - else { - // Server not in table - CookieEntry entry; - entry.d_address = address; - entry.d_cookie.makeClientCookie(); - cookieSentOut = entry.d_cookie; - entry.d_lastupdate = now->tv_sec; - entry.setSupport(CookieEntry::Support::Probing); - lock->emplace(entry); - opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); - VLOG(log, "Sending new client cookie info to " << address.toString() << ": " << entry.d_cookie.toDisplayString() << endl); - } + outgoingCookie(log, address, *now, opts, cookieSentOut, addressToBindTo); } if (dnsOverTLS && g_paddingOutgoing) { @@ -705,54 +765,9 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr if (EDNS0Level > 0 && getEDNSOpts(mdp, &edo)) { lwr->d_haveEDNS = true; if (g_cookies && !*chained) { - for (const auto& opt : edo.d_options) { - if (opt.first == EDNSOptionCode::COOKIE) { - EDNSCookiesOpt received; - if (received.makeFromString(opt.second)) { - cookieFoundInReply = true; - VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); - auto lock = s_cookiestore.lock(); - auto found = lock->find(address); - if (found != lock->end()) { - if (received.getClient() == cookieSentOut->getClient()) { - VLOG(log, "Client cookie matched! Storing with localAddress " << localip.toString() << endl); - found->d_localaddress = localip; - found->d_cookie = received; - found->d_lastupdate = now->tv_sec; - found->setSupport(CookieEntry::Support::Supported); - uint16_t ercode = (edo.d_extRCode << 4) | lwr->d_rcode; - if (ercode == ERCode::BADCOOKIE) { - lwr->d_validpacket = true; - VLOG(log, "Server: " << localip.toString() << " returned BADCOOKIE " << endl); - return LWResult::Result::BadCookie; // proper use of BacCookie, we did update he entry - } - } - else { - if (!doTCP) { - // Server responded with a wrong client cookie, fall back to TCP - VLOG(log, "Server responded with wrong client cookie, fall back to TCP" << endl); - lwr->d_validpacket = true; - return LWResult::Result::Spoofed; - } - // ignore bad cookie when already doing TCP - } - } - else { - // We receivd cookie (we might have sent one out) but it's not in the table? - VLOG(log, "Cookie not found back in table" << endl); - lwr->d_rcode = RCode::FormErr; - lwr->d_validpacket = false; - return LWResult::Result::Success; // success - oddly enough - } - } - else { - VLOG(log, "Malformed cookie in reply" << endl); - // Do something special if we get malformed repeatedly? And or consider current status: Supported - lwr->d_rcode = RCode::FormErr; - lwr->d_validpacket = false; - return LWResult::Result::Success; // succes - odly enough - } - } + auto [done, result] = incomingCookie(log, address, localip, *now, cookieSentOut, edo, doTCP, *lwr, cookieFoundInReply); + if (done) { + return result; } } if (weWantEDNSSubnet) { @@ -781,23 +796,24 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr auto found = lock->find(address); if (found != lock->end()) { switch (found->getSupport()) { - case CookieEntry::Support::Unknown: - assert(0); case CookieEntry::Support::Probing: - VLOG(log, "No cookie in repy, was probing, setting support to Unsupported" << endl); - found->setSupport(CookieEntry::Support::Unsupported); + VLOG(log, "No cookie in repy from " << address.toString() << ", was probing, setting support to Unsupported" << endl); + found->setSupport(CookieEntry::Support::Unsupported, now->tv_sec); break; case CookieEntry::Support::Unsupported: // We could have detected the server does not support cookies in the meantime + VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Unsupported, fine" << endl); break; case CookieEntry::Support::Supported: - lwr->d_validpacket = true; - return LWResult::Result::BadCookie; // XXX, we did not update cookie info... + // RFC says: ignore rpolies not containing any cookie info, equivalent to timeout + VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Supported, dropping packet as if it timed out)" << endl); + return LWResult::Result::Timeout; break; } } else { - // Table entry lost? XXX + VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Unknown, dropping packet as if it timed out" << endl); + return LWResult::Result::Timeout; } } diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index d79221c4c03b..72765e4f3be7 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -54,7 +54,6 @@ struct CookieEntry { enum class Support : uint8_t { - Unknown, Unsupported, Supported, Probing @@ -63,7 +62,6 @@ struct CookieEntry static std::string toString(Support support) { static const std::array names = { - "Unknown", "Unsupported", "Supported", "Probing"}; @@ -79,8 +77,9 @@ struct CookieEntry return d_support; } - void setSupport(Support support) const // modifying mutable field + void setSupport(Support support, time_t now) const // modifying mutable field { + d_lastupdate = now; d_support = support; } @@ -93,7 +92,7 @@ struct CookieEntry mutable ComboAddress d_localaddress; // The address we were bound to, see RFC 9018 mutable EDNSCookiesOpt d_cookie; // Contains both client and server cookie mutable time_t d_lastupdate{}; - mutable Support d_support{Support::Unknown}; + mutable Support d_support{Support::Unsupported}; }; class CookieStore : public multi_index_container < CookieEntry, diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index d8d0bb76d5bc..0918e436085c 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1627,7 +1627,7 @@ LWResult::Result SyncRes::asyncresolveWrapper(const OptLog& log, const ComboAddr // This is the first path that re-iterates the loop continue; } - else if (res->d_validpacket && res->d_haveEDNS && ret == LWResult::Result::BadCookie) { + if (res->d_validpacket && res->d_haveEDNS && ret == LWResult::Result::BadCookie) { // We assume the received cookie was stored and will be used in the second iteration // This is the second path that re-iterates the loop continue; diff --git a/regression-tests.recursor-dnssec/test_Cookies.py b/regression-tests.recursor-dnssec/test_Cookies.py index 490f5a31d1b8..23982c5d48f6 100644 --- a/regression-tests.recursor-dnssec/test_Cookies.py +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -23,9 +23,13 @@ class CookiesTest(RecursorTest): - zone: cookies.example forwarders: [%s.25, %s.26] outgoing: - cookies: true""" % (os.environ['PREFIX'], os.environ['PREFIX']) + cookies: true +packetcache: + disable: true +""" % (os.environ['PREFIX'], os.environ['PREFIX']) _expectedCookies = 'no' + @classmethod def generateRecursorConfig(cls, confdir): super(CookiesTest, cls).generateRecursorYamlConfig(confdir) @@ -80,19 +84,19 @@ def checkCookies(self, support, server='127.0.0.25'): def testAuthDoesnotSendCookies(self): confdir = os.path.join('configs', self._confdir) # Case: rec does not get a cookie back - expected = dns.rrset.from_text('a.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') - query = dns.message.make_query('a.cookies.example.', 'A') + expected = dns.rrset.from_text('unsupported.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('unsupported.cookies.example.', 'A') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) self.checkCookies('Unsupported') - def testAuthRepliesWithCookies(self): + def testAuthRepliesWithCookie(self): confdir = os.path.join('configs', self._confdir) # Case: rec gets a proper client and server cookie back self.recControl(confdir, 'clear-cookies') - query = dns.message.make_query('b.cookies.example.', 'A') - expected = dns.rrset.from_text('b.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('supported.cookies.example.', 'A') + expected = dns.rrset.from_text('supported.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) @@ -100,8 +104,8 @@ def testAuthRepliesWithCookies(self): # Case: we get a an correct client and server cookie back # We do not clear the cookie tables, so the old server cookie gets re-used - query = dns.message.make_query('c.cookies.example.', 'A') - expected = dns.rrset.from_text('c.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('supported2.cookies.example.', 'A') + expected = dns.rrset.from_text('supported2.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) @@ -111,8 +115,8 @@ def testAuthSendsIncorrectClientCookie(self): confdir = os.path.join('configs', self._confdir) # Case: rec gets a an incorrect client cookie back, we ignore that over TCP self.recControl(confdir, 'clear-cookies') - query = dns.message.make_query('d.cookies.example.', 'A') - expected = dns.rrset.from_text('d.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('wrongcc.cookies.example.', 'A') + expected = dns.rrset.from_text('wrongcc.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) @@ -122,8 +126,8 @@ def testAuthSendsBADCOOKIEOverUDP(self): confdir = os.path.join('configs', self._confdir) # Case: rec gets a BADCOOKIE, even on retry and should fall back to TCP self.recControl(confdir, 'clear-cookies') - query = dns.message.make_query('e.cookies.example.', 'A') - expected = dns.rrset.from_text('e.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('badcookie.cookies.example.', 'A') + expected = dns.rrset.from_text('badcookie.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) @@ -133,14 +137,34 @@ def testAuthSendsMalformedCookie(self): confdir = os.path.join('configs', self._confdir) # Case: rec gets a malformed cookie, should ignore packet self.recControl(confdir, 'clear-cookies') - query = dns.message.make_query('f.cookies.example.', 'A') - expected = dns.rrset.from_text('f.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + query = dns.message.make_query('malformed.cookies.example.', 'A') + expected = dns.rrset.from_text('malformed.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') res = self.sendUDPQuery(query) self.assertRcodeEqual(res, dns.rcode.NOERROR) self.assertRRsetInAnswer(res, expected) self.checkCookies('Probing', '127.0.0.25') self.checkCookies('Supported', '127.0.0.26') + def testForgottenCookie(self): + confdir = os.path.join('configs', self._confdir) + # Case: rec gets a proper client and server cookie back + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('supported3.cookies.example.', 'A') + expected = dns.rrset.from_text('supported3.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('Supported') + + # Case: we get a an correct client and server cookie back + # We HAVE cleared the cookie tables, so the old server cookie is fogotten + self.recControl(confdir, 'clear-cookies') + query = dns.message.make_query('supported4.cookies.example.', 'A') + expected = dns.rrset.from_text('supported4.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + res = self.sendUDPQuery(query) + self.assertRcodeEqual(res, dns.rcode.NOERROR) + self.assertRRsetInAnswer(res, expected) + self.checkCookies('Supported') class UDPResponder(DatagramProtocol): def getCookie(self, message): @@ -169,21 +193,21 @@ def question(self, datagram, tcp=False): question = request.question[0] # Case: do not send cookie back - if question.name == dns.name.from_text('a.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('a.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + if question.name == dns.name.from_text('unsupported.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('unsupported.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') response.answer.append(answer) # Case: do send cookie back - elif question.name == dns.name.from_text('b.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('b.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + elif question.name == dns.name.from_text('supported.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('supported.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') clientcookie = self.getCookie(request) if clientcookie is not None: response.use_edns(options = [self.createCookie(clientcookie)]) response.answer.append(answer) # We get a good client and server cookie - elif question.name == dns.name.from_text('c.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('c.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + elif question.name == dns.name.from_text('supported2.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('supported2.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') clientcookie = self.getCookie(request) if len(clientcookie) != 24: raise AssertionError("expected full cookie, got len " + str(len(clientcookie))) @@ -191,9 +215,25 @@ def question(self, datagram, tcp=False): response.use_edns(options = [self.createCookie(clientcookie)]) response.answer.append(answer) + # Case: do send cookie back + elif question.name == dns.name.from_text('supported3.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('supported3.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + response.answer.append(answer) + + # We get a new client cookie as the cookie store was cleared + elif question.name == dns.name.from_text('supported4.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('supported4.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + clientcookie = self.getCookie(request) + if clientcookie is not None: + response.use_edns(options = [self.createCookie(clientcookie)]) + response.answer.append(answer) + # Case: do send incorrect client cookie back - elif question.name == dns.name.from_text('d.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('d.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + elif question.name == dns.name.from_text('wrongcc.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('wrongcc.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') clientcookie = self.getCookie(request) if clientcookie is not None: mod = bytearray(clientcookie) @@ -202,8 +242,8 @@ def question(self, datagram, tcp=False): response.answer.append(answer) # Case: do send BADCOOKIE cookie back if UDP - elif question.name == dns.name.from_text('e.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('e.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + elif question.name == dns.name.from_text('badcookie.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('badcookie.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') clientcookie = self.getCookie(request) if clientcookie is not None: response.use_edns(options = [self.createCookie(clientcookie)]) @@ -212,8 +252,8 @@ def question(self, datagram, tcp=False): response.answer.append(answer) # Case send malformed cookie for server .25 - elif question.name == dns.name.from_text('f.cookies.example.') and question.rdtype == dns.rdatatype.A: - answer = dns.rrset.from_text('f.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') + elif question.name == dns.name.from_text('malformed.cookies.example.') and question.rdtype == dns.rdatatype.A: + answer = dns.rrset.from_text('malformed.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') clientcookie = self.getCookie(request) print(self.transport.getHost().host) if self.transport.getHost().host == os.environ['PREFIX'] + '.26': From fc18cb24bfdd925b21ab64ecf3a8d88982ba958c Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 26 Mar 2025 10:57:07 +0100 Subject: [PATCH 11/22] Add metrics for cookies --- pdns/recursordist/RECURSOR-MIB.txt | 74 ++++++++++++++++++- pdns/recursordist/lwres.cc | 12 ++- pdns/recursordist/metrics_table.py | 49 ++++++++++++ pdns/recursordist/rec-tcounters.hh | 8 ++ regression-tests.recursor-dnssec/test_SNMP.py | 2 +- 5 files changed, 142 insertions(+), 3 deletions(-) diff --git a/pdns/recursordist/RECURSOR-MIB.txt b/pdns/recursordist/RECURSOR-MIB.txt index 2f95b69bf5a8..b20d3e10fe6f 100644 --- a/pdns/recursordist/RECURSOR-MIB.txt +++ b/pdns/recursordist/RECURSOR-MIB.txt @@ -1291,6 +1291,70 @@ tcpOverflow OBJECT-TYPE "Incoming TCP limits reached" ::= { stats 152 } +cookieMalformed OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Malformed cookies received" + ::= { stats 153 } + +cookieMatched OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Matching cookies recieved" + ::= { stats 154 } + +cookieMismatchTcp OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Mismatched cookies received over TCP" + ::= { stats 155 } + +cookieMismatchUdp OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Mismatched cookies received over UDP" + ::= { stats 156 } + +cookieNotInReply OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Authoritative serve sent a reply bnack without cookie" + ::= { stats 157 } + +cookieRetry OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Retries because authoritative server sent a BADCOOKIE reply" + ::= { stats 158 } + +cookiesSupported OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Number of authoritative server IPs marked as supporting cookies" + ::= { stats 159 } + +cookiesUnsupported OBJECT-TYPE + SYNTAX Counter64 + MAX-ACCESS read-only + STATUS current + DESCRIPTION + "Number of authoritative server IPs marked as not supporting cookies" + ::= { stats 160 } + --- --- Traps / Notifications --- @@ -1489,7 +1553,15 @@ recGroup OBJECT-GROUP maxChainLength, maxChainWeight, chainLimits, - tcpOverflow + tcpOverflow, + cookieMalformed, + cookieMatched, + cookieMismatchTcp, + cookieMismatchUdp, + cookieNotInReply, + cookieRetry, + cookiesSupported, + cookiesUnsupported } STATUS current DESCRIPTION "Objects conformance group for PowerDNS Recursor" diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index cef7799ea25f..e2d740a9222f 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -492,13 +492,18 @@ static std::pair incomingCookie(const OptLog& log, const VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); if (received.getClient() == cookieSentOut->getClient()) { VLOG(log, "Client cookie from " << address.toString() << " matched! Storing with localAddress " << localip.toString() << endl); + ++t_Counters.at(rec::Counter::cookieMatched); found->d_localaddress = localip; found->d_cookie = received; + if (found->getSupport() == CookieEntry::Support::Probing) { + ++t_Counters.at(rec::Counter::cookiesSupported); + } found->setSupport(CookieEntry::Support::Supported, now.tv_sec); // check extended error code uint16_t ercode = (edo.d_extRCode << 4) | lwr.d_rcode; if (ercode == ERCode::BADCOOKIE) { lwr.d_validpacket = true; + ++t_Counters.at(rec::Counter::cookieRetry); VLOG(log, "Server " << localip.toString() << " returned BADCOOKIE " << endl); return {true, LWResult::Result::BadCookie}; // We did update the entry, retry should succeed } @@ -508,16 +513,19 @@ static std::pair incomingCookie(const OptLog& log, const // Server responded with a wrong client cookie, fall back to TCP, RFC 7873 5.3 VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie, fall back to TCP" << endl); lwr.d_validpacket = true; + ++t_Counters.at(rec::Counter::cookieMismatchedOverUDP); return {true, LWResult::Result::Spoofed}; } // mismatched cookie when already doing TCP, ignore that VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie over TCP, ignoring that" << endl); + ++t_Counters.at(rec::Counter::cookieMismatchedOverTCP); } } else { VLOG(log, "Malformed cookie in reply from " << address.toString() << ", dropping as if was a timeout" << endl); // Do something special if we get malformed repeatedly? And or consider current status? lwr.d_validpacket = false; + ++t_Counters.at(rec::Counter::cookieMalformed); return {true, LWResult::Result::Timeout}; } break; // only consider first cookie option found, RFC 7873 5.3 @@ -792,6 +800,7 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr // Case: we sent out a cookie but did not get one back if (cookieSentOut && !cookieFoundInReply && !*chained) { + ++t_Counters.at(rec::Counter::cookieNotInReply); auto lock = s_cookiestore.lock(); auto found = lock->find(address); if (found != lock->end()) { @@ -799,13 +808,14 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr case CookieEntry::Support::Probing: VLOG(log, "No cookie in repy from " << address.toString() << ", was probing, setting support to Unsupported" << endl); found->setSupport(CookieEntry::Support::Unsupported, now->tv_sec); + ++t_Counters.at(rec::Counter::cookiesUnsupported); break; case CookieEntry::Support::Unsupported: // We could have detected the server does not support cookies in the meantime VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Unsupported, fine" << endl); break; case CookieEntry::Support::Supported: - // RFC says: ignore rpolies not containing any cookie info, equivalent to timeout + // RFC says: ignore replies not containing any cookie info, equivalent to timeout VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Supported, dropping packet as if it timed out)" << endl); return LWResult::Result::Timeout; break; diff --git a/pdns/recursordist/metrics_table.py b/pdns/recursordist/metrics_table.py index f1ef6eb60858..0922018dc3c6 100644 --- a/pdns/recursordist/metrics_table.py +++ b/pdns/recursordist/metrics_table.py @@ -1356,6 +1356,54 @@ 'desc': 'Incoming TCP limits reached', 'snmp': 152, }, + { + 'name': 'cookie-malformed', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMalformed); }', + 'desc': 'Malformed cookies received', + 'snmp': 153, + }, + { + 'name': 'cookie-matched', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMatched); }', + 'desc': 'Matching cookies recieved', + 'snmp': 154, + }, + { + 'name': 'cookie-mismatch-tcp', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMismatchedOverTCP); }', + 'desc': 'Mismatched cookies received over TCP', + 'snmp': 155, + }, + { + 'name': 'cookie-mismatch-udp', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMismatchedOverUDP); }', + 'desc': 'Mismatched cookies received over UDP', + 'snmp': 156, + }, + { + 'name': 'cookie-not-in-reply', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieNotInReply); }', + 'desc': 'Authoritative serve sent a reply bnack without cookie', + 'snmp': 157, + }, + { + 'name': 'cookie-retry', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieRetry); }', + 'desc': 'Retries because authoritative server sent a BADCOOKIE reply', + 'snmp': 158, + }, + { + 'name': 'cookies-supported', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookiesSupported); }', + 'desc': 'Number of authoritative server IPs marked as supporting cookies', + 'snmp': 159, + }, + { + 'name': 'cookies-unsupported', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookiesUnsupported); }', + 'desc': 'Number of authoritative server IPs marked as not supporting cookies', + 'snmp': 160, + }, { 'name': 'remote-logger-count', 'lambda': '''[]() { @@ -1401,4 +1449,5 @@ 'pname': 'proxy-mapping-total-n-0', # For multicounters, state the first # No SNMP }, + ] diff --git a/pdns/recursordist/rec-tcounters.hh b/pdns/recursordist/rec-tcounters.hh index feeba0d6ab4e..448988ade8be 100644 --- a/pdns/recursordist/rec-tcounters.hh +++ b/pdns/recursordist/rec-tcounters.hh @@ -98,6 +98,14 @@ enum class Counter : uint8_t maxChainLength, maxChainWeight, chainLimits, + cookieMalformed, + cookieMatched, + cookieMismatchedOverTCP, + cookieMismatchedOverUDP, + cookieNotInReply, + cookieRetry, + cookiesSupported, + cookiesUnsupported, numberOfCounters }; diff --git a/regression-tests.recursor-dnssec/test_SNMP.py b/regression-tests.recursor-dnssec/test_SNMP.py index 40907ad3aba1..3fb840a85887 100644 --- a/regression-tests.recursor-dnssec/test_SNMP.py +++ b/regression-tests.recursor-dnssec/test_SNMP.py @@ -21,7 +21,7 @@ class SNMPTest(RecursorTest): """ def _checkStatsValues(self, results): - count = 152 + count = 160 for i in list(range(1, count)): oid = self._snmpOID + '.1.' + str(i) + '.0' self.assertTrue(oid in results) From d9a26a87d3796afcb758763502451dc8f3a9064f Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 26 Mar 2025 11:20:46 +0100 Subject: [PATCH 12/22] Comments and docs tweaks --- .../docs/manpages/rec_control.1.rst | 3 +++ pdns/recursordist/docs/upgrade.rst | 19 +++++++++++++++++-- pdns/recursordist/metrics_table.py | 1 - pdns/recursordist/rec-cookiestore.hh | 8 ++++---- 4 files changed, 24 insertions(+), 7 deletions(-) diff --git a/pdns/recursordist/docs/manpages/rec_control.1.rst b/pdns/recursordist/docs/manpages/rec_control.1.rst index 3f63486704e6..921fe4a9f19d 100644 --- a/pdns/recursordist/docs/manpages/rec_control.1.rst +++ b/pdns/recursordist/docs/manpages/rec_control.1.rst @@ -94,6 +94,9 @@ dump-cache *FILENAME* [*TYPE*...] select specific caches specify one or more *TYPE*s, separated by spaces. The value of *TYPE* can be r, n, p or a. +dump-cookies *FILENAME* + Dump the cookie store. + dump-dot-probe-map *FILENAME* Dump the contents of the DoT probe map to the *FILENAME* mentioned. diff --git a/pdns/recursordist/docs/upgrade.rst b/pdns/recursordist/docs/upgrade.rst index 37eddb0ab34a..35dc2d9a6494 100644 --- a/pdns/recursordist/docs/upgrade.rst +++ b/pdns/recursordist/docs/upgrade.rst @@ -4,8 +4,23 @@ Upgrade Guide Before upgrading, it is advised to read the :doc:`changelog/index`. When upgrading several versions, please read **all** notes applying to the upgrade. -5.1.0 to 5.2.0 and master -------------------------- +5.2.0 to master +--------------- + +New Settings +^^^^^^^^^^^^ + +- The :ref:`setting-yaml-outgoing.cookies3` settings has been introduced to implement cookie support for contacting authoritative servers and forwarders. See :rfc:`7873` and :rfc:`9018`. + +:program:`rec_control` +^^^^^^^^^^^^^^^^^^^^^^ + +The ``dump-cookies`` subcommand has been added to dump a table showing cookie support for each +authoritative server contacted recently. + + +5.1.0 to 5.2.0 +-------------- Changed behaviour ^^^^^^^^^^^^^^^^^ diff --git a/pdns/recursordist/metrics_table.py b/pdns/recursordist/metrics_table.py index 0922018dc3c6..6e2c0e59c701 100644 --- a/pdns/recursordist/metrics_table.py +++ b/pdns/recursordist/metrics_table.py @@ -1449,5 +1449,4 @@ 'pname': 'proxy-mapping-total-n-0', # For multicounters, state the first # No SNMP }, - ] diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index 72765e4f3be7..aa511ddd33f4 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -28,13 +28,13 @@ - Cookies are stored with an auth IP address as primary index and are generated randomly. - If the the does not support cookies, this is marked as such and no cookies will be sent to it - for a period of time. When a cookie is sent again, it must be a newly generated one. + for a period of time. When a cookie is sent again, it must be a newly generated one. - - A cookie is stored together with the client IP (as rec can have many). If a server is to be + - A cookie is stored together with the local IP (as rec can have many). If a server is to be contacted again, it should use the same bound IP. - - Although it is perfectly fine for a client cookie to live for a long time, this design will - flush entries older that a certain period of time, to avoid an ever growing CookieStore. + - Although it is perfectly fine for a client cookie to live for a long time, this design will + flush entries older that a certain period of time, to avoid an ever growing CookieStore. */ From 94cd6d503529644fe5bfd39e0b72ce5863515ac3 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 26 Mar 2025 15:26:35 +0100 Subject: [PATCH 13/22] Reformat --- pdns/ednscookies.cc | 4 ++-- pdns/recursordist/rec-cookiestore.hh | 6 +++--- pdns/recursordist/syncres.hh | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/pdns/ednscookies.cc b/pdns/ednscookies.cc index 4adc0ad1f676..4accc1997c7c 100644 --- a/pdns/ednscookies.cc +++ b/pdns/ednscookies.cc @@ -67,7 +67,7 @@ string EDNSCookiesOpt::makeOptString() const string EDNSCookiesOpt::toDisplayString() const { - std::string ret = makeHexDump(client, "");; + std::string ret = makeHexDump(client, ""); if (!server.empty()) { ret += '|'; if (server.length() != 16) { @@ -75,7 +75,7 @@ string EDNSCookiesOpt::toDisplayString() const ret += makeHexDump(server, ""); } else { - // It very likely is a rfc9018 one + // It very likely is a rfc9018 one ret += makeHexDump(server.substr(0, 1), ""); // Version ret += '|'; ret += makeHexDump(server.substr(1, 3), ""); // Reserved diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index aa511ddd33f4..df7af278a0d4 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -95,9 +95,9 @@ struct CookieEntry mutable Support d_support{Support::Unsupported}; }; -class CookieStore : public multi_index_container < CookieEntry, - indexed_by < ordered_unique, member>, - ordered_non_unique, member>>> +class CookieStore : public multi_index_container, member>, + ordered_non_unique, member>>> { public: void prune(time_t cutoff); diff --git a/pdns/recursordist/syncres.hh b/pdns/recursordist/syncres.hh index ccfa345bbdb6..5966ddaf6ed1 100644 --- a/pdns/recursordist/syncres.hh +++ b/pdns/recursordist/syncres.hh @@ -673,7 +673,7 @@ private: bool doSpecialNamesResolve(const DNSName& qname, QType qtype, QClass qclass, vector& ret); - LWResult::Result asyncresolveWrapper(const OptLog&log, const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const; + LWResult::Result asyncresolveWrapper(const OptLog& log, const ComboAddress& address, bool ednsMANDATORY, const DNSName& domain, const DNSName& auth, int type, bool doTCP, bool sendRDQuery, struct timeval* now, boost::optional& srcmask, LWResult* res, bool* chained, const DNSName& nsName) const; boost::optional getEDNSSubnetMask(const DNSName& name, const ComboAddress& rem); From e2c80febf594f89139e8a17ef14329c83121667b Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 26 Mar 2025 15:37:20 +0100 Subject: [PATCH 14/22] Fix ifdef botch --- pdns/recursordist/lwres.cc | 5 ++++- pdns/recursordist/resolve-context.hh | 2 -- pdns/recursordist/syncres.cc | 2 -- 3 files changed, 4 insertions(+), 5 deletions(-) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index e2d740a9222f..b03ebaf59563 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -120,8 +120,9 @@ void remoteLoggerQueueData(RemoteLoggerInterface& rli, const std::string& data) } } -#ifdef HAVE_FSTRM #include "dnstap.hh" + +#ifdef HAVE_FSTRM #include "fstrm_logger.hh" static bool isEnabledForQueries(const std::shared_ptr>>& fstreamLoggers) @@ -659,9 +660,11 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr socklen_t slen = address.getSocklen(); (void)getsockname(queryfd, reinterpret_cast(&localip), &slen); // NOLINT(cppcoreguidelines-pro-type-reinterpret-cast)) } +#ifdef HAVE_FSTRM if (fstrmQEnabled) { logFstreamQuery(fstrmLoggers, queryTime, localip, address, DnstapMessage::ProtocolType::DoUDP, context.d_auth ? context.d_auth : boost::none, vpacket); } +#endif } // sleep until we see an answer to this, interface to mtasker diff --git a/pdns/recursordist/resolve-context.hh b/pdns/recursordist/resolve-context.hh index c4b38c3d5666..ba16b8bad901 100644 --- a/pdns/recursordist/resolve-context.hh +++ b/pdns/recursordist/resolve-context.hh @@ -44,7 +44,5 @@ struct ResolveContext boost::optional d_initialRequestId; DNSName d_nsName; -#ifdef HAVE_FSTRM boost::optional d_auth; -#endif }; diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 0918e436085c..4ff193757ec1 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1577,9 +1577,7 @@ LWResult::Result SyncRes::asyncresolveWrapper(const OptLog& log, const ComboAddr int EDNSLevel = 0; auto luaconfsLocal = g_luaconfs.getLocal(); ResolveContext ctx(d_initialRequestId, nsName); -#ifdef HAVE_FSTRM ctx.d_auth = auth; -#endif LWResult::Result ret{}; From 86b0c8c410451f8130035c6a156f453f1b3917bc Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 30 Apr 2025 13:26:32 +0200 Subject: [PATCH 15/22] Typos in comments and docs from Miod Co-authored-by: Miod Vallat --- pdns/recursordist/RECURSOR-MIB.in | 5 ++++- pdns/recursordist/docs/upgrade.rst | 2 +- pdns/recursordist/metrics_table.py | 4 ++-- pdns/recursordist/rec-cookiestore.hh | 5 ++--- pdns/recursordist/syncres.cc | 2 +- 5 files changed, 10 insertions(+), 8 deletions(-) diff --git a/pdns/recursordist/RECURSOR-MIB.in b/pdns/recursordist/RECURSOR-MIB.in index 638b214df0cb..509befc2ceb1 100644 --- a/pdns/recursordist/RECURSOR-MIB.in +++ b/pdns/recursordist/RECURSOR-MIB.in @@ -15,12 +15,15 @@ IMPORTS FROM SNMPv2-CONF; rec MODULE-IDENTITY - LAST-UPDATED "202408280000Z" + LAST-UPDATED "202504290000Z" ORGANIZATION "PowerDNS BV" CONTACT-INFO "support@powerdns.com" DESCRIPTION "This MIB module describes information gathered through PowerDNS Recursor." + REVISION "202504290000Z" + DESCRIPTION "Added metrics related to cookies" + REVISION "202408280000Z" DESCRIPTION "Added metric for too many incoming TCP connections" diff --git a/pdns/recursordist/docs/upgrade.rst b/pdns/recursordist/docs/upgrade.rst index 35dc2d9a6494..2d82c4ae3411 100644 --- a/pdns/recursordist/docs/upgrade.rst +++ b/pdns/recursordist/docs/upgrade.rst @@ -10,7 +10,7 @@ When upgrading several versions, please read **all** notes applying to the upgra New Settings ^^^^^^^^^^^^ -- The :ref:`setting-yaml-outgoing.cookies3` settings has been introduced to implement cookie support for contacting authoritative servers and forwarders. See :rfc:`7873` and :rfc:`9018`. +- The :ref:`setting-yaml-outgoing.cookies3` setting has been introduced to implement cookie support for contacting authoritative servers and forwarders. See :rfc:`7873` and :rfc:`9018`. :program:`rec_control` ^^^^^^^^^^^^^^^^^^^^^^ diff --git a/pdns/recursordist/metrics_table.py b/pdns/recursordist/metrics_table.py index 6e2c0e59c701..6fa33b0fdee1 100644 --- a/pdns/recursordist/metrics_table.py +++ b/pdns/recursordist/metrics_table.py @@ -1365,7 +1365,7 @@ { 'name': 'cookie-matched', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMatched); }', - 'desc': 'Matching cookies recieved', + 'desc': 'Matching cookies received', 'snmp': 154, }, { @@ -1383,7 +1383,7 @@ { 'name': 'cookie-not-in-reply', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieNotInReply); }', - 'desc': 'Authoritative serve sent a reply bnack without cookie', + 'desc': 'Authoritative serve sent a reply back without cookie', 'snmp': 157, }, { diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index df7af278a0d4..76b56af9f387 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -27,15 +27,14 @@ - Cookies are stored with an auth IP address as primary index and are generated randomly. - - If the the does not support cookies, this is marked as such and no cookies will be sent to it + - If an auth does not support cookies, it is marked as such and no cookies will be sent to it for a period of time. When a cookie is sent again, it must be a newly generated one. - - A cookie is stored together with the local IP (as rec can have many). If a server is to be + - A cookie is stored together with the local IP (as rec can have many). If an auth is to be contacted again, it should use the same bound IP. - Although it is perfectly fine for a client cookie to live for a long time, this design will flush entries older that a certain period of time, to avoid an ever growing CookieStore. - */ #include diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index 4ff193757ec1..c72f2d743d09 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1621,7 +1621,7 @@ LWResult::Result SyncRes::asyncresolveWrapper(const OptLog& log, const ComboAddr if (ret == LWResult::Result::BindError) { // BindError is only generated when cookies are active and we failed to bind to a local // address associated with a cookie, see RFC9018 section 3 last paragraph. We assume the - // called code alread erased the cookie info. + // called code has already erased the cookie info. // This is the first path that re-iterates the loop continue; } From b5384b2918b62924809df7f94f3a4fb1db75d18d Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Wed, 30 Apr 2025 13:57:55 +0200 Subject: [PATCH 16/22] Tidy --- pdns/recursordist/RECURSOR-MIB.txt | 9 +++++--- pdns/recursordist/lwres.cc | 10 ++++---- pdns/recursordist/metrics_table.py | 2 +- pdns/recursordist/rec-tcpout.hh | 2 +- pdns/recursordist/rec_channel_rec.cc | 34 +++++++++++++++------------- pdns/recursordist/syncres.cc | 16 +++++++------ 6 files changed, 40 insertions(+), 33 deletions(-) diff --git a/pdns/recursordist/RECURSOR-MIB.txt b/pdns/recursordist/RECURSOR-MIB.txt index b20d3e10fe6f..05d27d2af0f5 100644 --- a/pdns/recursordist/RECURSOR-MIB.txt +++ b/pdns/recursordist/RECURSOR-MIB.txt @@ -15,12 +15,15 @@ IMPORTS FROM SNMPv2-CONF; rec MODULE-IDENTITY - LAST-UPDATED "202408280000Z" + LAST-UPDATED "202504290000Z" ORGANIZATION "PowerDNS BV" CONTACT-INFO "support@powerdns.com" DESCRIPTION "This MIB module describes information gathered through PowerDNS Recursor." + REVISION "202504290000Z" + DESCRIPTION "Added metrics related to cookies" + REVISION "202408280000Z" DESCRIPTION "Added metric for too many incoming TCP connections" @@ -1304,7 +1307,7 @@ cookieMatched OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Matching cookies recieved" + "Matching cookies received" ::= { stats 154 } cookieMismatchTcp OBJECT-TYPE @@ -1328,7 +1331,7 @@ cookieNotInReply OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Authoritative serve sent a reply bnack without cookie" + "Authoritative server sent a reply back without cookie" ::= { stats 157 } cookieRetry OBJECT-TYPE diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index b03ebaf59563..860f35f53c98 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -336,9 +336,9 @@ static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std: const struct timeval timeout{ g_networkTimeoutMsec / 1000, static_cast(g_networkTimeoutMsec) % 1000 * 1000}; - Socket s(remote.sin4.sin_family, SOCK_STREAM); - s.setNonBlocking(); - setTCPNoDelay(s.getHandle()); + Socket sock(remote.sin4.sin_family, SOCK_STREAM); + sock.setNonBlocking(); + setTCPNoDelay(sock.getHandle()); ComboAddress localip = localBind ? *localBind : pdns::getQueryLocalAddress(remote.sin4.sin_family, 0); if (localBind) { VLOG(log, "Connecting TCP to " << remote.toString() << " with specific local address " << localip.toString() << endl); @@ -348,7 +348,7 @@ static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std: } try { - s.bind(localip); + sock.bind(localip); } catch (const NetworkError& e) { if (localBind) { @@ -370,7 +370,7 @@ static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std: dnsOverTLS = false; } } - connection.d_handler = std::make_shared(nsName, false, s.releaseHandle(), timeout, tlsCtx); + connection.d_handler = std::make_shared(nsName, false, sock.releaseHandle(), timeout, tlsCtx); connection.d_local = localBind; // Returned state ignored // This can throw an exception, retry will need to happen at higher level diff --git a/pdns/recursordist/metrics_table.py b/pdns/recursordist/metrics_table.py index 6fa33b0fdee1..6d59a03c3481 100644 --- a/pdns/recursordist/metrics_table.py +++ b/pdns/recursordist/metrics_table.py @@ -1383,7 +1383,7 @@ { 'name': 'cookie-not-in-reply', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieNotInReply); }', - 'desc': 'Authoritative serve sent a reply back without cookie', + 'desc': 'Authoritative server sent a reply back without cookie', 'snmp': 157, }, { diff --git a/pdns/recursordist/rec-tcpout.hh b/pdns/recursordist/rec-tcpout.hh index e9bf48f8e3ad..5ff80ffcecf0 100644 --- a/pdns/recursordist/rec-tcpout.hh +++ b/pdns/recursordist/rec-tcpout.hh @@ -56,7 +56,7 @@ public: using pair_t = std::pair>; void store(const struct timeval& now, const pair_t& pair, Connection&& connection); - Connection get(const pair_t& remoteAddress); + Connection get(const pair_t& pair); void cleanup(const struct timeval& now); [[nodiscard]] size_t size() const diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index 66c9bb936262..3df442a0e515 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -352,45 +352,47 @@ static uint64_t dumpAggressiveNSECCache(int fd) return g_aggressiveNSECCache->dumpToFile(filePtr, now); } -static uint64_t* pleaseDumpCookiesMap(int fd) +// NOLINTBEGIN(cppcoreguidelines-owning-memory) +static uint64_t* pleaseDumpCookiesMap(int fileDesc) { - return new uint64_t(dumpCookies(fd)); + return new uint64_t(dumpCookies(fileDesc)); } -static uint64_t* pleaseDumpEDNSMap(int fd) +static uint64_t* pleaseDumpEDNSMap(int fileDesc) { - return new uint64_t(SyncRes::doEDNSDump(fd)); + return new uint64_t(SyncRes::doEDNSDump(fileDesc)); } -static uint64_t* pleaseDumpNSSpeeds(int fd) +static uint64_t* pleaseDumpNSSpeeds(int fileDesc) { - return new uint64_t(SyncRes::doDumpNSSpeeds(fd)); + return new uint64_t(SyncRes::doDumpNSSpeeds(fileDesc)); } -static uint64_t* pleaseDumpThrottleMap(int fd) +static uint64_t* pleaseDumpThrottleMap(int fileDesc) { - return new uint64_t(SyncRes::doDumpThrottleMap(fd)); + return new uint64_t(SyncRes::doDumpThrottleMap(fileDesc)); } -static uint64_t* pleaseDumpFailedServers(int fd) +static uint64_t* pleaseDumpFailedServers(int fileDesc) { - return new uint64_t(SyncRes::doDumpFailedServers(fd)); + return new uint64_t(SyncRes::doDumpFailedServers(fileDesc)); } -static uint64_t* pleaseDumpSavedParentNSSets(int fd) +static uint64_t* pleaseDumpSavedParentNSSets(int fileDesc) { - return new uint64_t(SyncRes::doDumpSavedParentNSSets(fd)); + return new uint64_t(SyncRes::doDumpSavedParentNSSets(fileDesc)); } -static uint64_t* pleaseDumpNonResolvingNS(int fd) +static uint64_t* pleaseDumpNonResolvingNS(int fileDesc) { - return new uint64_t(SyncRes::doDumpNonResolvingNS(fd)); + return new uint64_t(SyncRes::doDumpNonResolvingNS(fileDesc)); } -static uint64_t* pleaseDumpDoTProbeMap(int fd) +static uint64_t* pleaseDumpDoTProbeMap(int fileDesc) { - return new uint64_t(SyncRes::doDumpDoTProbeMap(fd)); + return new uint64_t(SyncRes::doDumpDoTProbeMap(fileDesc)); } +// NOLINTEND(cppcoreguidelines-owning-memory) // Generic dump to file command static RecursorControlChannel::Answer doDumpToFile(int s, uint64_t* (*function)(int s), const string& name, bool threads = true) diff --git a/pdns/recursordist/syncres.cc b/pdns/recursordist/syncres.cc index c72f2d743d09..51bab81fcf1f 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -5588,26 +5588,28 @@ bool SyncRes::doResolveAtThisIP(const std::string& prefix, const DNSName& qname, if (resolveret != LWResult::Result::Success) { /* Error while resolving */ - if (resolveret == LWResult::Result::Timeout) { + switch (resolveret) { + case LWResult::Result::Timeout: LOG(prefix << qname << ": Timeout resolving after " << lwr.d_usec / 1000.0 << " ms " << (doTCP ? "over TCP" : "") << endl); incTimeoutStats(remoteIP); - } - else if (resolveret == LWResult::Result::OSLimitError) { + break; + case LWResult::Result::OSLimitError: /* OS resource limit reached */ LOG(prefix << qname << ": Hit a local resource limit resolving" << (doTCP ? " over TCP" : "") << ", probable error: " << stringerror() << endl); t_Counters.at(rec::Counter::resourceLimits)++; - } - else if (resolveret == LWResult::Result::ChainLimitError) { + break; + case LWResult::Result::ChainLimitError: /* Chain resource limit reached */ LOG(prefix << qname << ": Hit a chain limit resolving" << (doTCP ? " over TCP" : "")); t_Counters.at(rec::Counter::chainLimits)++; - } - else { + break; + default: /* LWResult::Result::PermanentError */ t_Counters.at(rec::Counter::unreachables)++; d_unreachables++; // XXX questionable use of errno LOG(prefix << qname << ": Error resolving from " << remoteIP.toString() << (doTCP ? " over TCP" : "") << ", possible error: " << stringerror() << endl); + break; } // don't account for resource limits, they are our own fault From 8686ee8685e9a4078ce25e91d668af109d37ad8e Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 08:05:16 +0200 Subject: [PATCH 17/22] Typo Co-authored-by: Peter van Dijk --- pdns/recursordist/docs/upgrade.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pdns/recursordist/docs/upgrade.rst b/pdns/recursordist/docs/upgrade.rst index 2d82c4ae3411..2c5afa3f38d1 100644 --- a/pdns/recursordist/docs/upgrade.rst +++ b/pdns/recursordist/docs/upgrade.rst @@ -10,7 +10,7 @@ When upgrading several versions, please read **all** notes applying to the upgra New Settings ^^^^^^^^^^^^ -- The :ref:`setting-yaml-outgoing.cookies3` setting has been introduced to implement cookie support for contacting authoritative servers and forwarders. See :rfc:`7873` and :rfc:`9018`. +- The :ref:`setting-yaml-outgoing.cookies` setting has been introduced to implement cookie support for contacting authoritative servers and forwarders. See :rfc:`7873` and :rfc:`9018`. :program:`rec_control` ^^^^^^^^^^^^^^^^^^^^^^ From 8c1f41d20a18c7afb028d5819950d56092b86ef0 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 09:20:26 +0200 Subject: [PATCH 18/22] Use ostringstream for constructing cookie displaystring, add a few comments why binding to a given local address is needed --- pdns/ednscookies.cc | 23 ++++++++++++----------- pdns/recursordist/lwres.cc | 1 + pdns/recursordist/pdns_recursor.cc | 2 ++ pdns/recursordist/rec_channel_rec.cc | 2 +- 4 files changed, 16 insertions(+), 12 deletions(-) diff --git a/pdns/ednscookies.cc b/pdns/ednscookies.cc index 4accc1997c7c..96bea6a41b98 100644 --- a/pdns/ednscookies.cc +++ b/pdns/ednscookies.cc @@ -67,25 +67,26 @@ string EDNSCookiesOpt::makeOptString() const string EDNSCookiesOpt::toDisplayString() const { - std::string ret = makeHexDump(client, ""); + std::ostringstream str; + str << makeHexDump(client, ""); if (!server.empty()) { - ret += '|'; + str << '|'; if (server.length() != 16) { // It isn't a rfc9018 one - ret += makeHexDump(server, ""); + str << makeHexDump(server, ""); } else { // It very likely is a rfc9018 one - ret += makeHexDump(server.substr(0, 1), ""); // Version - ret += '|'; - ret += makeHexDump(server.substr(1, 3), ""); // Reserved - ret += '|'; - ret += makeHexDump(server.substr(4, 4), ""); // Timestamp - ret += '|'; - ret += makeHexDump(server.substr(8, 8), ""); // Hash + str << makeHexDump(server.substr(0, 1), ""); // Version + str << '|'; + str << makeHexDump(server.substr(1, 3), ""); // Reserved + str << '|'; + str << makeHexDump(server.substr(4, 4), ""); // Timestamp + str << '|'; + str << makeHexDump(server.substr(8, 8), ""); // Hash } } - return ret; + return str.str(); } void EDNSCookiesOpt::getEDNSCookiesOptFromString(const char* option, unsigned int len) diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 860f35f53c98..f2b93112a3f7 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -339,6 +339,7 @@ static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std: Socket sock(remote.sin4.sin_family, SOCK_STREAM); sock.setNonBlocking(); setTCPNoDelay(sock.getHandle()); + // Bind to the same address the cookie is associated with (RFC 9018 section 3 last paragraph) ComboAddress localip = localBind ? *localBind : pdns::getQueryLocalAddress(remote.sin4.sin_family, 0); if (localBind) { VLOG(log, "Connecting TCP to " << remote.toString() << " with specific local address " << localip.toString() << endl); diff --git a/pdns/recursordist/pdns_recursor.cc b/pdns/recursordist/pdns_recursor.cc index a655754e72e8..7813d8d36d09 100644 --- a/pdns/recursordist/pdns_recursor.cc +++ b/pdns/recursordist/pdns_recursor.cc @@ -181,6 +181,8 @@ int UDPClientSocks::makeClientSocket(int family, const std::optional [type...] dump cache contents to the named file, type is r, n, p or a\n" - "dump-cookies dump the contents of the cookie data to the namewd file\n" + "dump-cookies dump the contents of the cookie jar to the named file\n" "dump-dot-probe-map dump the contents of the DoT probe map to the named file\n" "dump-edns [status] dump EDNS status to the named file\n" "dump-failedservers dump the failed servers to the named file\n" From 007af599f16a1fed5caaac108af8ddd7ef83f2c4 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 11:56:53 +0200 Subject: [PATCH 19/22] Apply suggestions from Habbie (the trivial ones first) --- pdns/ednscookies.cc | 2 +- pdns/recursordist/lwres.cc | 21 ++++++++++----------- pdns/recursordist/lwres.hh | 4 ++-- pdns/recursordist/rec-cookiestore.cc | 4 ++-- pdns/recursordist/rec-cookiestore.hh | 4 ++-- pdns/recursordist/rec-main.cc | 2 +- pdns/recursordist/rec-tcpout.cc | 4 ++-- pdns/recursordist/rec-tcpout.hh | 8 ++++---- pdns/recursordist/rec_channel_rec.cc | 3 ++- 9 files changed, 26 insertions(+), 26 deletions(-) diff --git a/pdns/ednscookies.cc b/pdns/ednscookies.cc index 96bea6a41b98..a32b76a17472 100644 --- a/pdns/ednscookies.cc +++ b/pdns/ednscookies.cc @@ -68,7 +68,7 @@ string EDNSCookiesOpt::makeOptString() const string EDNSCookiesOpt::toDisplayString() const { std::ostringstream str; - str << makeHexDump(client, ""); + str << makeHexDump(client, ""); if (!server.empty()) { str << '|'; if (server.length() != 16) { diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index f2b93112a3f7..821efdf6539f 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -58,7 +58,7 @@ static bool g_cookies = false; -void setAuthCookies(bool flag) +void enableOutgoingCookies(bool flag) { g_cookies = flag; } @@ -69,11 +69,10 @@ bool g_paddingOutgoing; static LockGuarded s_cookiestore; -std::string clearCookies() +void clearCookies() { auto lock = s_cookiestore.lock(); lock->clear(); - return ""; } void pruneCookies(time_t cutoff) @@ -89,7 +88,7 @@ uint64_t dumpCookies(int fileDesc) auto lock = s_cookiestore.lock(); copy = *lock; } - return CookieStore::dump(copy, fileDesc); + return copy.dump(fileDesc); } void remoteLoggerQueueData(RemoteLoggerInterface& rli, const std::string& data) @@ -329,7 +328,7 @@ static bool tcpconnect(const OptLog& log, const ComboAddress& remote, const std: { dnsOverTLS = SyncRes::s_dot_to_port_853 && remote.getPort() == 853; - connection = t_tcp_manager.get(std::make_pair(remote, localBind)); + connection = t_tcp_manager.get({remote, localBind}); if (connection.d_handler) { return false; } @@ -478,7 +477,7 @@ static std::pair incomingCookie(const OptLog& log, const auto found = lock->find(address); if (found == lock->end()) { - // We receivd cookie (we might have sent one out) but the server is not in the table? + // We received cookie (we might have sent one out) but the server is not in the table? // This is a case of cannot happen, unless rec_control clear-cookies was called VLOG(log, "Cookie from " << address.toString() << " not found back in table" << endl); lwr.d_rcode = RCode::FormErr; @@ -525,7 +524,7 @@ static std::pair incomingCookie(const OptLog& log, const } else { VLOG(log, "Malformed cookie in reply from " << address.toString() << ", dropping as if was a timeout" << endl); - // Do something special if we get malformed repeatedly? And or consider current status? + // Do something special if we get malformed repeatedly? And/or consider current status? lwr.d_validpacket = false; ++t_Counters.at(rec::Counter::cookieMalformed); return {true, LWResult::Result::Timeout}; @@ -810,23 +809,23 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr if (found != lock->end()) { switch (found->getSupport()) { case CookieEntry::Support::Probing: - VLOG(log, "No cookie in repy from " << address.toString() << ", was probing, setting support to Unsupported" << endl); + VLOG(log, "No cookie in reply from " << address.toString() << ", was probing, setting support to Unsupported" << endl); found->setSupport(CookieEntry::Support::Unsupported, now->tv_sec); ++t_Counters.at(rec::Counter::cookiesUnsupported); break; case CookieEntry::Support::Unsupported: // We could have detected the server does not support cookies in the meantime - VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Unsupported, fine" << endl); + VLOG(log, "No cookie in reply from " << address.toString() << ", cookie state is Unsupported, fine" << endl); break; case CookieEntry::Support::Supported: // RFC says: ignore replies not containing any cookie info, equivalent to timeout - VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Supported, dropping packet as if it timed out)" << endl); + VLOG(log, "No cookie in reply from " << address.toString() << ", cookie state is Supported, dropping packet as if it timed out)" << endl); return LWResult::Result::Timeout; break; } } else { - VLOG(log, "No cookie in repy from " << address.toString() << ", cookie state is Unknown, dropping packet as if it timed out" << endl); + VLOG(log, "No cookie in reply from " << address.toString() << ", cookie state is Unknown, dropping packet as if it timed out" << endl); return LWResult::Result::Timeout; } } diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index fbd657e340f4..8e17bcb2b292 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -96,6 +96,6 @@ LWResult::Result arecvfrom(PacketBuffer& packet, int flags, const ComboAddress& LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& address, const DNSName& domain, int type, bool doTCP, bool sendRDQuery, int EDNS0Level, struct timeval* now, boost::optional& srcmask, const ResolveContext& context, const std::shared_ptr>>& outgoingLoggers, const std::shared_ptr>>& fstrmLoggers, const std::set& exportTypes, LWResult* lwr, bool* chained); uint64_t dumpCookies(int fileDesc); -std::string clearCookies(); +void clearCookies(); void pruneCookies(time_t cutoff); -void setAuthCookies(bool flag); +void enableOutgoingCookies(bool flag); diff --git a/pdns/recursordist/rec-cookiestore.cc b/pdns/recursordist/rec-cookiestore.cc index adda2ecbb37b..c19832d94f00 100644 --- a/pdns/recursordist/rec-cookiestore.cc +++ b/pdns/recursordist/rec-cookiestore.cc @@ -33,7 +33,7 @@ void CookieStore::prune(time_t cutoff) ind.erase(ind.begin(), ind.upper_bound(cutoff)); } -uint64_t CookieStore::dump(const CookieStore& copy, int fileDesc) +uint64_t CookieStore::dump(int fileDesc) const { int newfd = dup(fileDesc); if (newfd == -1) { @@ -47,7 +47,7 @@ uint64_t CookieStore::dump(const CookieStore& copy, int fileDesc) uint64_t count = 0; fprintf(filePtr.get(), "; cookie dump follows\n; server\tlocal\tcookie\tsupport\tts\n"); - for (const auto& entry : copy) { + for (const auto& entry : *this) { count++; timebuf_t tmp; fprintf(filePtr.get(), "%s\t%s\t%s\t%s\t%s\n", diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index 76b56af9f387..b51f8cc634e2 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -60,7 +60,7 @@ struct CookieEntry static std::string toString(Support support) { - static const std::array names = { + static const std::array names = { "Unsupported", "Supported", "Probing"}; @@ -100,5 +100,5 @@ class CookieStore : public multi_index_container 0 && connection.d_numqueries >= s_maxQueries) { @@ -73,7 +73,7 @@ void TCPOutConnectionManager::store(const struct timeval& now, const pair_t& pai d_idle_connections.emplace(pair, std::move(connection)); } -TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const pair_t& pair) +TCPOutConnectionManager::Connection TCPOutConnectionManager::get(const endpoints_t& pair) { if (d_idle_connections.count(pair) > 0) { auto connection = d_idle_connections.extract(pair); diff --git a/pdns/recursordist/rec-tcpout.hh b/pdns/recursordist/rec-tcpout.hh index 5ff80ffcecf0..26d973b3713a 100644 --- a/pdns/recursordist/rec-tcpout.hh +++ b/pdns/recursordist/rec-tcpout.hh @@ -53,10 +53,10 @@ public: size_t d_numqueries{0}; }; - using pair_t = std::pair>; + using endpoints_t = std::pair>; - void store(const struct timeval& now, const pair_t& pair, Connection&& connection); - Connection get(const pair_t& pair); + void store(const struct timeval& now, const endpoints_t& pair, Connection&& connection); + Connection get(const endpoints_t& pair); void cleanup(const struct timeval& now); [[nodiscard]] size_t size() const @@ -71,7 +71,7 @@ public: private: // This does not take into account that we can have multiple connections with different hosts (via SNI) to the same IP. // That is OK, since we are connecting by IP only at the moment. - std::multimap d_idle_connections; + std::multimap d_idle_connections; }; extern thread_local TCPOutConnectionManager t_tcp_manager; diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index fdf70afd614e..8e4c816dcda1 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -2079,7 +2079,8 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int socket, cons return doDumpCache(socket, begin, end); } if (cmd == "clear-cookies") { - return {0, clearCookies()}; + clearCookies(); + return {0, ""}; } if (cmd == "dump-cookies") { return doDumpToFile(socket, pleaseDumpCookiesMap, cmd, false); From 71eed996ef77c1cc04d801501f2bb47e285f83e7 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 12:20:49 +0200 Subject: [PATCH 20/22] Implement EDNSOpts::getFirstOption and use it in lwres --- pdns/dnsrecords.cc | 10 ++++ pdns/dnsrecords.hh | 2 + pdns/recursordist/lwres.cc | 103 ++++++++++++++++++------------------- 3 files changed, 61 insertions(+), 54 deletions(-) diff --git a/pdns/dnsrecords.cc b/pdns/dnsrecords.cc index 85ce9e471ef0..3cf5b6840708 100644 --- a/pdns/dnsrecords.cc +++ b/pdns/dnsrecords.cc @@ -1036,6 +1036,16 @@ void checkHostnameCorrectness(const DNSResourceRecord& rr) } } +vector>::const_iterator EDNSOpts::getFirstOption(uint16_t optionCode) const +{ + for (auto iter = d_options.cbegin(); iter != d_options.cend(); ++iter) { + if (iter->first == optionCode) { + return iter; + } + } + return d_options.cend(); +} + #if 0 static struct Reporter { diff --git a/pdns/dnsrecords.hh b/pdns/dnsrecords.hh index a79bd2fbb420..da1c8cb8d7d3 100644 --- a/pdns/dnsrecords.hh +++ b/pdns/dnsrecords.hh @@ -1299,6 +1299,8 @@ struct EDNSOpts uint16_t d_packetsize{0}; uint16_t d_extFlags{0}; uint8_t d_extRCode, d_version; + + [[nodiscard]] vector>::const_iterator getFirstOption(uint16_t optionCode) const; }; //! Convenience function that fills out EDNS0 options, and returns true if there are any diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 821efdf6539f..828fcf970404 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -486,52 +486,49 @@ static std::pair incomingCookie(const OptLog& log, const } // We have stored cookie info, scan for COOKIE option in EDNS - for (const auto& opt : edo.d_options) { - if (opt.first == EDNSOptionCode::COOKIE) { - if (EDNSCookiesOpt received; received.makeFromString(opt.second)) { - cookieFoundInReply = true; - VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); - if (received.getClient() == cookieSentOut->getClient()) { - VLOG(log, "Client cookie from " << address.toString() << " matched! Storing with localAddress " << localip.toString() << endl); - ++t_Counters.at(rec::Counter::cookieMatched); - found->d_localaddress = localip; - found->d_cookie = received; - if (found->getSupport() == CookieEntry::Support::Probing) { - ++t_Counters.at(rec::Counter::cookiesSupported); - } - found->setSupport(CookieEntry::Support::Supported, now.tv_sec); - // check extended error code - uint16_t ercode = (edo.d_extRCode << 4) | lwr.d_rcode; - if (ercode == ERCode::BADCOOKIE) { - lwr.d_validpacket = true; - ++t_Counters.at(rec::Counter::cookieRetry); - VLOG(log, "Server " << localip.toString() << " returned BADCOOKIE " << endl); - return {true, LWResult::Result::BadCookie}; // We did update the entry, retry should succeed - } + if (const auto opt = edo.getFirstOption(EDNSOptionCode::COOKIE); opt != edo.d_options.end()) { + if (EDNSCookiesOpt received; received.makeFromString(opt->second)) { + cookieFoundInReply = true; + VLOG(log, "Received cookie info back from " << address.toString() << ": " << received.toDisplayString() << endl); + if (received.getClient() == cookieSentOut->getClient()) { + VLOG(log, "Client cookie from " << address.toString() << " matched! Storing with localAddress " << localip.toString() << endl); + ++t_Counters.at(rec::Counter::cookieMatched); + found->d_localaddress = localip; + found->d_cookie = received; + if (found->getSupport() == CookieEntry::Support::Probing) { + ++t_Counters.at(rec::Counter::cookiesSupported); } - else { - if (!doTCP) { - // Server responded with a wrong client cookie, fall back to TCP, RFC 7873 5.3 - VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie, fall back to TCP" << endl); - lwr.d_validpacket = true; - ++t_Counters.at(rec::Counter::cookieMismatchedOverUDP); - return {true, LWResult::Result::Spoofed}; - } - // mismatched cookie when already doing TCP, ignore that - VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie over TCP, ignoring that" << endl); - ++t_Counters.at(rec::Counter::cookieMismatchedOverTCP); + found->setSupport(CookieEntry::Support::Supported, now.tv_sec); + // check extended error code + uint16_t ercode = (edo.d_extRCode << 4) | lwr.d_rcode; + if (ercode == ERCode::BADCOOKIE) { + lwr.d_validpacket = true; + ++t_Counters.at(rec::Counter::cookieRetry); + VLOG(log, "Server " << localip.toString() << " returned BADCOOKIE " << endl); + return {true, LWResult::Result::BadCookie}; // We did update the entry, retry should succeed } } else { - VLOG(log, "Malformed cookie in reply from " << address.toString() << ", dropping as if was a timeout" << endl); - // Do something special if we get malformed repeatedly? And/or consider current status? - lwr.d_validpacket = false; - ++t_Counters.at(rec::Counter::cookieMalformed); - return {true, LWResult::Result::Timeout}; + if (!doTCP) { + // Server responded with a wrong client cookie, fall back to TCP, RFC 7873 5.3 + VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie, fall back to TCP" << endl); + lwr.d_validpacket = true; + ++t_Counters.at(rec::Counter::cookieMismatchedOverUDP); + return {true, LWResult::Result::Spoofed}; + } + // mismatched cookie when already doing TCP, ignore that + VLOG(log, "Server " << localip.toString() << " responded with wrong client cookie over TCP, ignoring that" << endl); + ++t_Counters.at(rec::Counter::cookieMismatchedOverTCP); } - break; // only consider first cookie option found, RFC 7873 5.3 - } // COOKIE option found - } // for + } + else { + VLOG(log, "Malformed cookie in reply from " << address.toString() << ", dropping as if was a timeout" << endl); + // Do something special if we get malformed repeatedly? And/or consider current status? + lwr.d_validpacket = false; + ++t_Counters.at(rec::Counter::cookieMalformed); + return {true, LWResult::Result::Timeout}; + } + } // COOKIE option found // The cases where something special needs to be done have been handled above return {false, LWResult::Result::Success}; @@ -782,19 +779,17 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr } } if (weWantEDNSSubnet) { - for (const auto& opt : edo.d_options) { - if (opt.first == EDNSOptionCode::ECS) { - EDNSSubnetOpts reso; - if (EDNSSubnetOpts::getFromString(opt.second, &reso)) { - /* rfc7871 states that 0 "indicate[s] that the answer is suitable for all addresses in FAMILY", - so we might want to still pass the information along to be able to differentiate between - IPv4 and IPv6. Still I'm pretty sure it doesn't matter in real life, so let's not duplicate - entries in our cache. */ - if (reso.getScopePrefixLength() != 0) { - uint8_t bits = std::min(reso.getScopePrefixLength(), outgoingECSBits); - outgoingECSAddr.truncate(bits); - srcmask = Netmask(outgoingECSAddr, bits); - } + if (const auto opt = edo.getFirstOption(EDNSOptionCode::ECS); opt != edo.d_options.end()) { + EDNSSubnetOpts reso; + if (EDNSSubnetOpts::getFromString(opt->second, &reso)) { + /* rfc7871 states that 0 "indicate[s] that the answer is suitable for all addresses in FAMILY", + so we might want to still pass the information along to be able to differentiate between + IPv4 and IPv6. Still I'm pretty sure it doesn't matter in real life, so let's not duplicate + entries in our cache. */ + if (reso.getScopePrefixLength() != 0) { + uint8_t bits = std::min(reso.getScopePrefixLength(), outgoingECSBits); + outgoingECSAddr.truncate(bits); + srcmask = Netmask(outgoingECSAddr, bits); } } } From 950ee2e827d27ec3e09452255204fb705ce92095 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 12:29:40 +0200 Subject: [PATCH 21/22] Factor out the code to compute the EDNS Error code --- pdns/dnsrecords.hh | 4 ++++ pdns/recursordist/lwres.cc | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/pdns/dnsrecords.hh b/pdns/dnsrecords.hh index da1c8cb8d7d3..4dc32d6aad90 100644 --- a/pdns/dnsrecords.hh +++ b/pdns/dnsrecords.hh @@ -1301,6 +1301,10 @@ struct EDNSOpts uint8_t d_extRCode, d_version; [[nodiscard]] vector>::const_iterator getFirstOption(uint16_t optionCode) const; + [[nodiscard]] uint16_t getCombinedERCode(uint8_t rcode) const + { + return (static_cast(d_extRCode) << 4) | rcode; + } }; //! Convenience function that fills out EDNS0 options, and returns true if there are any diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 828fcf970404..dcd2a1722e28 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -500,7 +500,7 @@ static std::pair incomingCookie(const OptLog& log, const } found->setSupport(CookieEntry::Support::Supported, now.tv_sec); // check extended error code - uint16_t ercode = (edo.d_extRCode << 4) | lwr.d_rcode; + uint16_t ercode = edo.getCombinedERCode(lwr.d_rcode); if (ercode == ERCode::BADCOOKIE) { lwr.d_validpacket = true; ++t_Counters.at(rec::Counter::cookieRetry); From 1ec4c30b628b271c95fa3fe34deebbb3d1c76862 Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 9 May 2025 13:49:01 +0200 Subject: [PATCH 22/22] Better naming of cookie metrics --- pdns/recursordist/RECURSOR-MIB.txt | 16 ++++++++-------- pdns/recursordist/lwres.cc | 4 ++-- pdns/recursordist/metrics_table.py | 20 ++++++++++---------- pdns/recursordist/rec-tcounters.hh | 4 ++-- 4 files changed, 22 insertions(+), 22 deletions(-) diff --git a/pdns/recursordist/RECURSOR-MIB.txt b/pdns/recursordist/RECURSOR-MIB.txt index 05d27d2af0f5..6b03fb18dc9c 100644 --- a/pdns/recursordist/RECURSOR-MIB.txt +++ b/pdns/recursordist/RECURSOR-MIB.txt @@ -1299,7 +1299,7 @@ cookieMalformed OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Malformed cookies received" + "Number of malformed cookies received" ::= { stats 153 } cookieMatched OBJECT-TYPE @@ -1307,7 +1307,7 @@ cookieMatched OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Matching cookies received" + "Number of matching cookies received" ::= { stats 154 } cookieMismatchTcp OBJECT-TYPE @@ -1315,7 +1315,7 @@ cookieMismatchTcp OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Mismatched cookies received over TCP" + "Number of mismatched cookies received over TCP" ::= { stats 155 } cookieMismatchUdp OBJECT-TYPE @@ -1323,7 +1323,7 @@ cookieMismatchUdp OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Mismatched cookies received over UDP" + "Number of mismatched cookies received over UDP" ::= { stats 156 } cookieNotInReply OBJECT-TYPE @@ -1331,7 +1331,7 @@ cookieNotInReply OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Authoritative server sent a reply back without cookie" + "Number of times an authoritative server sent a reply without a cookie" ::= { stats 157 } cookieRetry OBJECT-TYPE @@ -1339,7 +1339,7 @@ cookieRetry OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Retries because authoritative server sent a BADCOOKIE reply" + "Number of retries because authoritative server sent a BADCOOKIE reply" ::= { stats 158 } cookiesSupported OBJECT-TYPE @@ -1347,7 +1347,7 @@ cookiesSupported OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Number of authoritative server IPs marked as supporting cookies" + "Number of authoritative server cookie probes resulting in success" ::= { stats 159 } cookiesUnsupported OBJECT-TYPE @@ -1355,7 +1355,7 @@ cookiesUnsupported OBJECT-TYPE MAX-ACCESS read-only STATUS current DESCRIPTION - "Number of authoritative server IPs marked as not supporting cookies" + "Number of authoritative server cookie probes not resulting in success" ::= { stats 160 } --- diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index dcd2a1722e28..348b3ff6f823 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -496,7 +496,7 @@ static std::pair incomingCookie(const OptLog& log, const found->d_localaddress = localip; found->d_cookie = received; if (found->getSupport() == CookieEntry::Support::Probing) { - ++t_Counters.at(rec::Counter::cookiesSupported); + ++t_Counters.at(rec::Counter::cookieProbeSupported); } found->setSupport(CookieEntry::Support::Supported, now.tv_sec); // check extended error code @@ -806,7 +806,7 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr case CookieEntry::Support::Probing: VLOG(log, "No cookie in reply from " << address.toString() << ", was probing, setting support to Unsupported" << endl); found->setSupport(CookieEntry::Support::Unsupported, now->tv_sec); - ++t_Counters.at(rec::Counter::cookiesUnsupported); + ++t_Counters.at(rec::Counter::cookieProbeUnsupported); break; case CookieEntry::Support::Unsupported: // We could have detected the server does not support cookies in the meantime diff --git a/pdns/recursordist/metrics_table.py b/pdns/recursordist/metrics_table.py index 6d59a03c3481..99fad74e1b5f 100644 --- a/pdns/recursordist/metrics_table.py +++ b/pdns/recursordist/metrics_table.py @@ -1359,49 +1359,49 @@ { 'name': 'cookie-malformed', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMalformed); }', - 'desc': 'Malformed cookies received', + 'desc': 'Number of malformed cookies received', 'snmp': 153, }, { 'name': 'cookie-matched', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMatched); }', - 'desc': 'Matching cookies received', + 'desc': 'Number of matching cookies received', 'snmp': 154, }, { 'name': 'cookie-mismatch-tcp', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMismatchedOverTCP); }', - 'desc': 'Mismatched cookies received over TCP', + 'desc': 'Number of mismatched cookies received over TCP', 'snmp': 155, }, { 'name': 'cookie-mismatch-udp', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieMismatchedOverUDP); }', - 'desc': 'Mismatched cookies received over UDP', + 'desc': 'Number of mismatched cookies received over UDP', 'snmp': 156, }, { 'name': 'cookie-not-in-reply', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieNotInReply); }', - 'desc': 'Authoritative server sent a reply back without cookie', + 'desc': 'Number of times an authoritative server sent a reply without a cookie', 'snmp': 157, }, { 'name': 'cookie-retry', 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieRetry); }', - 'desc': 'Retries because authoritative server sent a BADCOOKIE reply', + 'desc': 'Number of retries because authoritative server sent a BADCOOKIE reply', 'snmp': 158, }, { 'name': 'cookies-supported', - 'lambda': '[] { return g_Counters.sum(rec::Counter::cookiesSupported); }', - 'desc': 'Number of authoritative server IPs marked as supporting cookies', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieProbeSupported); }', + 'desc': 'Number of authoritative server cookie probes resulting in success', 'snmp': 159, }, { 'name': 'cookies-unsupported', - 'lambda': '[] { return g_Counters.sum(rec::Counter::cookiesUnsupported); }', - 'desc': 'Number of authoritative server IPs marked as not supporting cookies', + 'lambda': '[] { return g_Counters.sum(rec::Counter::cookieProbeUnsupported); }', + 'desc': 'Number of authoritative server cookie probes not resulting in success', 'snmp': 160, }, { diff --git a/pdns/recursordist/rec-tcounters.hh b/pdns/recursordist/rec-tcounters.hh index 448988ade8be..c52df897eab6 100644 --- a/pdns/recursordist/rec-tcounters.hh +++ b/pdns/recursordist/rec-tcounters.hh @@ -104,8 +104,8 @@ enum class Counter : uint8_t cookieMismatchedOverUDP, cookieNotInReply, cookieRetry, - cookiesSupported, - cookiesUnsupported, + cookieProbeSupported, + cookieProbeUnsupported, numberOfCounters };