From: Otto Moerbeek Date: Tue, 25 Mar 2025 11:01:11 +0000 (+0100) Subject: Refactor cookie code out of asyncresolve X-Git-Tag: rec-5.4.0-alpha1~279^2~21 X-Git-Url: http://git.ipfire.org/cgi-bin/gitweb.cgi?a=commitdiff_plain;h=803e20037611dbd4b2870b9fe99d161301e4b266;p=thirdparty%2Fpdns.git Refactor cookie code out of asyncresolve --- diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index fb1b8b43f6..c724fa389a 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -435,6 +435,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! */ @@ -482,37 +572,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) { @@ -727,54 +787,9 @@ static LWResult::Result asyncresolve(const OptLog& log, const ComboAddress& addr } } 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; } } } @@ -785,23 +800,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 d79221c4c0..72765e4f3b 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 531a1ac3dd..9b386ab294 100644 --- a/pdns/recursordist/syncres.cc +++ b/pdns/recursordist/syncres.cc @@ -1524,7 +1524,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 490f5a31d1..23982c5d48 100644 --- a/regression-tests.recursor-dnssec/test_Cookies.py +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -23,9 +23,13 @@ recursor: - 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 @@ outgoing: 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 @@ outgoing: # 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 @@ outgoing: 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 @@ outgoing: 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 @@ outgoing: 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 @@ class UDPResponder(DatagramProtocol): 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 @@ class UDPResponder(DatagramProtocol): 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 @@ class UDPResponder(DatagramProtocol): 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 @@ class UDPResponder(DatagramProtocol): 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':