From: Otto Moerbeek Date: Mon, 15 Sep 2025 09:48:23 +0000 (+0200) Subject: Process review comments from @rgacogne X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=refs%2Fpull%2F15412%2Fhead;p=thirdparty%2Fpdns.git Process review comments from @rgacogne Signed-off-by: Otto Moerbeek --- diff --git a/pdns/recursordist/docs/manpages/rec_control.1.rst b/pdns/recursordist/docs/manpages/rec_control.1.rst index 47011b6a1..fb0469d9e 100644 --- a/pdns/recursordist/docs/manpages/rec_control.1.rst +++ b/pdns/recursordist/docs/manpages/rec_control.1.rst @@ -55,6 +55,8 @@ Commands -------- add-cookies-unsupported *IP* [*IP*...] Add non-expiring IPs of servers that do not support cookies to the cookie table. + Optionally *IP:port* can be specified, the default is to use port 53. + The listed addresses will be placed as ``Unsupported`` in the cookie support table and will not be pruned. add-dont-throttle-names *NAME* [*NAME*...] Add names for nameserver domains that may not be throttled. @@ -75,6 +77,7 @@ current-queries clear-cookies [*IP*...] Remove entries from cookie table. If *IP* is ``*``, remove all. + Optionally *IP:port* can be specified, the default is to use port 53. clear-dont-throttle-names *NAME* [*NAME*...] Remove names that are not allowed to be throttled. If *NAME* is ``*``, remove all diff --git a/pdns/recursordist/docs/upgrade.rst b/pdns/recursordist/docs/upgrade.rst index a5eb34520..2dbca467f 100644 --- a/pdns/recursordist/docs/upgrade.rst +++ b/pdns/recursordist/docs/upgrade.rst @@ -11,7 +11,7 @@ New Settings ^^^^^^^^^^^^ - 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`. -- The :ref:`setting-yaml-outgoing.cookies_unsupported` setting has been introduced to to permanently mark authoritative servers as not supporting cookies. +- The :ref:`setting-yaml-outgoing.cookies_unsupported` setting has been introduced to permanently mark authoritative servers as not supporting cookies. :program:`rec_control` ^^^^^^^^^^^^^^^^^^^^^^ diff --git a/pdns/recursordist/lwres.cc b/pdns/recursordist/lwres.cc index 68d5032ff..2147f17ce 100644 --- a/pdns/recursordist/lwres.cc +++ b/pdns/recursordist/lwres.cc @@ -58,14 +58,17 @@ static bool g_cookies = false; -void enableOutgoingCookies(bool flag, const string& unsupported) +std::string enableOutgoingCookies(bool flag, const string& unsupported) { g_cookies = flag; if (g_cookies) { std::vector parts; stringtok(parts, unsupported, ", "); - addCookiesUnsupported(parts.begin(), parts.end()); + std::string errors; + addCookiesUnsupported(parts.begin(), parts.end(), errors); + return errors; } + return {}; } thread_local TCPOutConnectionManager t_tcp_manager; @@ -75,7 +78,7 @@ bool g_ECSHardening; static LockGuarded s_cookiestore; -uint64_t addCookiesUnsupported(vector::iterator begin, vector::iterator end) +uint64_t addCookiesUnsupported(vector::iterator begin, vector::iterator end, string& errors) { auto lock = s_cookiestore.lock(); uint64_t count = 0; @@ -90,15 +93,18 @@ uint64_t addCookiesUnsupported(vector::iterator begin, vector::i } ++count; } - catch (const PDNSException&) { - ; + catch (const PDNSException& error) { + if (!errors.empty()) { + errors += ", "; + } + errors += error.reason; } ++begin; } return count; } -uint64_t clearCookies(vector::iterator begin, vector::iterator end) +uint64_t clearCookies(vector::iterator begin, vector::iterator end, string& errors) { auto lock = s_cookiestore.lock(); uint64_t count = 0; @@ -114,8 +120,11 @@ uint64_t clearCookies(vector::iterator begin, vector::iterator e try { count += lock->erase(ComboAddress(*begin, 53)); } - catch (const PDNSException&) { - ; + catch (const PDNSException& error) { + if (!errors.empty()) { + errors += ", "; + } + errors += error.reason; } ++begin; } @@ -493,7 +502,7 @@ static void outgoingCookie(const OptLog& log, const ComboAddress& address, const cookieSentOut = found->d_cookie; addressToBindTo = found->d_localaddress; opts.emplace_back(EDNSOptionCode::COOKIE, cookieSentOut->makeOptString()); - found->d_lastupdate = now.tv_sec; + found->d_lastused = now.tv_sec; VLOG(log, "Sending stored cookie info to " << address.toString() << ": " << found->d_cookie.toDisplayString() << endl); break; case CookieEntry::Support::Unsupported: diff --git a/pdns/recursordist/lwres.hh b/pdns/recursordist/lwres.hh index 89cc45f19..ad5dff6f8 100644 --- a/pdns/recursordist/lwres.hh +++ b/pdns/recursordist/lwres.hh @@ -100,7 +100,7 @@ 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); -uint64_t clearCookies(vector::iterator begin, vector::iterator end); -uint64_t addCookiesUnsupported(vector::iterator begin, vector::iterator end); +uint64_t clearCookies(vector::iterator begin, vector::iterator end, string& errors); +uint64_t addCookiesUnsupported(vector::iterator begin, vector::iterator end, string& errors); void pruneCookies(time_t cutoff); -void enableOutgoingCookies(bool flag, const std::string& unsupported); +std::string enableOutgoingCookies(bool flag, const std::string& unsupported); diff --git a/pdns/recursordist/rec-cookiestore.cc b/pdns/recursordist/rec-cookiestore.cc index fd91565ab..8b660496b 100644 --- a/pdns/recursordist/rec-cookiestore.cc +++ b/pdns/recursordist/rec-cookiestore.cc @@ -50,7 +50,7 @@ uint64_t CookieStore::dump(int fileDesc) const entry.d_address.toStringWithPortExcept(53).c_str(), entry.d_localaddress.toString().c_str(), entry.d_cookie.toDisplayString().c_str(), CookieEntry::toString(entry.d_support).c_str(), - entry.d_lastupdate == std::numeric_limits::max() ? "Forever" : timestamp(entry.d_lastupdate, tmp)); + entry.d_lastused == std::numeric_limits::max() ? "Forever" : timestamp(entry.d_lastused, tmp)); } return count; } diff --git a/pdns/recursordist/rec-cookiestore.hh b/pdns/recursordist/rec-cookiestore.hh index b51f8cc63..00d802596 100644 --- a/pdns/recursordist/rec-cookiestore.hh +++ b/pdns/recursordist/rec-cookiestore.hh @@ -25,7 +25,7 @@ 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. + - Cookies are stored with an auth IP:port address as primary index and are generated randomly. - 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. @@ -78,7 +78,7 @@ struct CookieEntry void setSupport(Support support, time_t now) const // modifying mutable field { - d_lastupdate = now; + d_lastused = now; d_support = support; } @@ -90,13 +90,13 @@ 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 time_t d_lastused{}; mutable Support d_support{Support::Unsupported}; }; class CookieStore : public multi_index_container, member>, - ordered_non_unique, member>>> + ordered_non_unique, member>>> { public: void prune(time_t cutoff); diff --git a/pdns/recursordist/rec-main.cc b/pdns/recursordist/rec-main.cc index 611ca8250..11773172c 100644 --- a/pdns/recursordist/rec-main.cc +++ b/pdns/recursordist/rec-main.cc @@ -2203,6 +2203,7 @@ static int serviceMain(Logr::log_t log) g_paddingOutgoing = ::arg().mustDo("edns-padding-out"); g_ECSHardening = ::arg().mustDo("edns-subnet-harden"); + // Ignong errors return value, as YAML parsing already checked the format of the entries. enableOutgoingCookies(::arg().mustDo("outgoing-cookies"), ::arg()["outgoing-cookies-unsupported"]); RecThreadInfo::setNumDistributorThreads(::arg().asNum("distributor-threads")); diff --git a/pdns/recursordist/rec-rust-lib/table.py b/pdns/recursordist/rec-rust-lib/table.py index 559c1d4a6..02a54af3d 100644 --- a/pdns/recursordist/rec-rust-lib/table.py +++ b/pdns/recursordist/rec-rust-lib/table.py @@ -3634,11 +3634,11 @@ Enable DNS cookies (:rfc:`7873`, :rfc:`9018`) when contacting authoritative serv 'name' : 'cookies_unsupported', 'section' : 'outgoing', 'oldname': 'outgoing-cookies-unsupported', - 'type': LType.ListStrings, + 'type': LType.ListSocketAddresses, 'default': '', - 'help': 'Addresses of authoritative servers that do not support cookies', + 'help': 'Addresses (with optional port) of authoritative servers that do not support cookies', 'doc': ''' -Addresses of servers that do not properly support DNS cookies (:rfc:`7873`, :rfc:`9018`). Recursor wil not even try to probe these servers for cookie support. +Addresses of servers that do not properly support DNS cookies (:rfc:`7873`, :rfc:`9018`). Recursor wil not even try to probe these servers for cookie support. If no port is specified port 53 is used. ''', 'versionadded': '5.3.0', }, diff --git a/pdns/recursordist/rec_channel_rec.cc b/pdns/recursordist/rec_channel_rec.cc index b4ef29de1..e251d13b8 100644 --- a/pdns/recursordist/rec_channel_rec.cc +++ b/pdns/recursordist/rec_channel_rec.cc @@ -2109,12 +2109,20 @@ RecursorControlChannel::Answer RecursorControlParser::getAnswer(int socket, cons return doDumpCache(socket, begin, end); } if (cmd == "clear-cookies") { - auto count = clearCookies(begin, end); - return {0, "Cleared " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " from cookies table\n"}; + string errors; + auto count = clearCookies(begin, end, errors); + if (errors.empty()) { + return {0, "Cleared " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " from cookies table\n"}; + } + return {1, "Cleared " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " from cookies table, errors: " + errors + "\n"}; } if (cmd == "add-cookies-unsupported") { - auto count = addCookiesUnsupported(begin, end); - return {0, "Added " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " to cookies table\n"}; + string errors; + auto count = addCookiesUnsupported(begin, end, errors); + if (errors.empty()) { + return {0, "Added " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " to cookies table\n"}; + } + return {1, "Added " + std::to_string(count) + " entr" + addS(count, "y", "ies") + " to cookies table, errors: " + errors + "\n"}; } if (cmd == "dump-cookies") { return doDumpToFile(socket, pleaseDumpCookiesMap, cmd, false); diff --git a/regression-tests.recursor-dnssec/test_Cookies.py b/regression-tests.recursor-dnssec/test_Cookies.py index 3a2514363..731d128fe 100644 --- a/regression-tests.recursor-dnssec/test_Cookies.py +++ b/regression-tests.recursor-dnssec/test_Cookies.py @@ -106,7 +106,7 @@ packetcache: tcp2 = self.recControl(confdir, 'get tcp-outqueries') self.assertEqual(tcp1, tcp2) - # Case: we get a an correct client and server cookie back + # Case: we get a 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('supported2.cookies.example.', 'A') expected = dns.rrset.from_text('supported2.cookies.example.', 15, dns.rdataclass.IN, 'A', '127.0.0.1') @@ -133,12 +133,15 @@ packetcache: 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', '*') + tcp1 = self.recControl(confdir, 'get tcp-outqueries') 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) self.checkCookies('Supported') + tcp2 = int(self.recControl(confdir, 'get tcp-outqueries')) + self.assertEqual(int(tcp1) + 1, int(tcp2)) def testAuthSendsMalformedCookie(self): confdir = os.path.join('configs', self._confdir) @@ -163,8 +166,8 @@ packetcache: 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 + # Case: we get a correct client and server cookie back + # We HAVE cleared the cookie tables, so the old server cookie is forgotten 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')