From c21f8271503bc70b16af1db1a165b46a94ebeeeb Mon Sep 17 00:00:00 2001 From: Otto Moerbeek Date: Fri, 11 Feb 2022 08:46:03 +0100 Subject: [PATCH] Better DNSSEC handling: do not add Intermnediates if validation is required. Also remember the validation status of the main query. --- pdns/rec-lua-conf.cc | 39 +++++++++++++++++++++++++++++++++ pdns/rec-lua-conf.hh | 9 ++++++++ pdns/syncres.cc | 52 +++++++++++++++++++++++++++----------------- pdns/syncres.hh | 12 +++------- 4 files changed, 83 insertions(+), 29 deletions(-) diff --git a/pdns/rec-lua-conf.cc b/pdns/rec-lua-conf.cc index 1575dddef9..fd50f5aa05 100644 --- a/pdns/rec-lua-conf.cc +++ b/pdns/rec-lua-conf.cc @@ -682,6 +682,45 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de }); #endif /* HAVE_FSTRM */ + Lua->writeFunction("addAllowedAdditionalQType", [&lci](int qtype, std::unordered_map targetqtypes, boost::optional> options) { + switch (qtype) { + case QType::MX: + case QType::SRV: + case QType::SVCB: + case QType::HTTPS: + case QType::NAPTR: + break; + default: + g_log << Logger::Error << "addAllowedAdditionalQType does not support " << QType(qtype).toString() << endl; + return; + } + + std::set targets; + for (const auto& t : targetqtypes) { + targets.emplace(QType(t.second)); + } + + AdditionalMode mode = AdditionalMode::CacheOnlyRequireAuth; // Always cheap and should be safe + + if (options) { + if (const auto it = options->find("mode"); it != options->end()) { + const map modeMap = { + {"Ignore", AdditionalMode::Ignore}, + {"CacheOnly", AdditionalMode::CacheOnly}, + {"CacheOnlyRequireAuth", AdditionalMode::CacheOnlyRequireAuth}, + {"ResolveImmediately", AdditionalMode::ResolveImmediately}, + {"ResolveDeferred", AdditionalMode::ResolveDeferred} + }; + if (modeMap.find(it->second) == modeMap.end()) { + g_log << Logger::Error << "addAllowedAdditionalQType: unknown mode " << it->second << endl; + return; + } + mode = modeMap.at(it->second); + } + } + lci.allowAdditionalQTypes.insert_or_assign(qtype, pair(targets, mode)); + }); + try { Lua->executeCode(ifs); g_luaconfs.setState(std::move(lci)); diff --git a/pdns/rec-lua-conf.hh b/pdns/rec-lua-conf.hh index 830c9987aa..ef2370397c 100644 --- a/pdns/rec-lua-conf.hh +++ b/pdns/rec-lua-conf.hh @@ -62,6 +62,14 @@ struct TrustAnchorFileInfo std::string fname; }; +enum class AdditionalMode : uint8_t { + Ignore, + CacheOnly, + CacheOnlyRequireAuth, + ResolveImmediately, + ResolveDeferred +}; + class LuaConfigItems { public: @@ -72,6 +80,7 @@ public: map dsAnchors; map negAnchors; map ztcConfigs; + std::map, AdditionalMode>> allowAdditionalQTypes; ProtobufExportConfig protobufExportConfig; ProtobufExportConfig outgoingProtobufExportConfig; FrameStreamExportConfig frameStreamExportConfig; diff --git a/pdns/syncres.cc b/pdns/syncres.cc index ee9e91732f..ca8cddbb86 100644 --- a/pdns/syncres.cc +++ b/pdns/syncres.cc @@ -142,29 +142,34 @@ SyncRes::SyncRes(const struct timeval& now) : d_authzonequeries(0), d_outquerie static void allowAdditionalEntry(std::unordered_set& allowedAdditionals, const DNSRecord& rec); -static const std::map, SyncRes::AddtionalMode>> additionalTypes = { - {QType::MX, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnlyRequireAuth}}, - {QType::SRV, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::ResolveImmediately}}, - {QType::SVCB, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnly}}, - {QType::HTTPS, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnly}}, - {QType::NAPTR, {{QType::A, QType::AAAA, QType::SRV}, SyncRes::AddtionalMode::ResolveImmediately}} -}; - -void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AddtionalMode mode, std::vector& additionals, unsigned int depth) +// static const std::map, SyncRes::AddtionalMode>> additionalTypes = { +// {QType::MX, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnly}}, +// {QType::SRV, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::ResolveImmediately}}, +// {QType::SVCB, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnly}}, +// {QType::HTTPS, {{QType::A, QType::AAAA}, SyncRes::AddtionalMode::CacheOnly}}, +// {QType::NAPTR, {{QType::A, QType::AAAA, QType::SRV}, SyncRes::AddtionalMode::ResolveImmediately}} +// }; + +void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AdditionalMode mode, std::vector& additionals, unsigned int depth) { vector addRecords; vState state = vState::Indeterminate; switch (mode) { - case AddtionalMode::ResolveImmediately: { + case AdditionalMode::ResolveImmediately: { set beenthere; int res = doResolve(qname, qtype, addRecords, depth, beenthere, state); if (res != 0) { return; } + // We're conservative here. We do not add Bogus records in any circumstance, we add Indeterminates only if no + // validation is required. if (vStateIsBogus(state)) { return; } + if (shouldValidate() && state != vState::Secure && state != vState::Insecure) { + return; + } for (const auto& rec : addRecords) { if (rec.d_place == DNSResourceRecord::ANSWER) { additionals.push_back(rec); @@ -172,15 +177,19 @@ void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AddtionalMod } break; } - case AddtionalMode::CacheOnly: - case AddtionalMode::CacheOnlyRequireAuth: { + case AdditionalMode::CacheOnly: + case AdditionalMode::CacheOnlyRequireAuth: { // Peek into cache - if (g_recCache->get(d_now.tv_sec, qname, qtype, mode == AddtionalMode::CacheOnlyRequireAuth, &addRecords, d_cacheRemote, false, d_routingTag, nullptr, nullptr, nullptr, &state) <= 0) { + if (g_recCache->get(d_now.tv_sec, qname, qtype, mode == AdditionalMode::CacheOnlyRequireAuth, &addRecords, d_cacheRemote, false, d_routingTag, nullptr, nullptr, nullptr, &state) <= 0) { return; } + // See the comment for the ResolveImmediately case if (vStateIsBogus(state)) { return; } + if (shouldValidate() && state != vState::Secure && state != vState::Insecure) { + return; + } for (auto& rec : addRecords) { if (rec.d_place == DNSResourceRecord::ANSWER) { rec.d_ttl -= d_now.tv_sec ; @@ -189,13 +198,13 @@ void SyncRes::resolveAdditionals(const DNSName& qname, QType qtype, AddtionalMod } break; } - case AddtionalMode::ResolveDeferred: + case AdditionalMode::ResolveDeferred: // Not yet implemented // Look in cache for authoritative answer, if available return it // If not, look in nergache and submit if not there as well. The logic should be the same as // #11294, which is in review atm. break; - case AddtionalMode::Ignore: + case AdditionalMode::Ignore: break; } } @@ -211,15 +220,17 @@ void SyncRes::addAdditionals(QType qtype, const vector&start, vector< if (additionaldepth >= 5 || start.empty()) { return; } - const auto it = additionalTypes.find(qtype); - if (it == additionalTypes.end()) { + + auto luaLocal = g_luaconfs.getLocal(); + const auto it = luaLocal->allowAdditionalQTypes.find(qtype); + if (it == luaLocal->allowAdditionalQTypes.end()) { return; } std::unordered_set addnames; for (const auto& rec : start) { if (rec.d_place == DNSResourceRecord::ANSWER) { - // currently, this funtion only knows about names, we could also take the target types that are dependent on - // reord contents into account + // currently, this function only knows about names, we could also take the target types that are dependent on + // record contents into account // e.g. for NAPTR records, go only for SRV for flag value "s", or A/AAAA for flag value "a" allowAdditionalEntry(addnames, rec); } @@ -324,7 +335,8 @@ int SyncRes::beginResolve(const DNSName &qname, const QType qtype, QClass qclass } } - if (qclass == QClass::IN && additionalTypes.find(qtype) != additionalTypes.end()) { + auto luaLocal = g_luaconfs.getLocal(); + if (res == 0 && qclass == QClass::IN && luaLocal->allowAdditionalQTypes.find(qtype) != luaLocal->allowAdditionalQTypes.end()) { addAdditionals(qtype, ret, depth); } d_eventTrace.add(RecEventTrace::SyncRes, res, false); diff --git a/pdns/syncres.hh b/pdns/syncres.hh index 89cd86ccc9..30e1605ccd 100644 --- a/pdns/syncres.hh +++ b/pdns/syncres.hh @@ -68,6 +68,8 @@ extern GlobalStateHolder g_dontThrottleNames; extern GlobalStateHolder g_dontThrottleNetmasks; extern GlobalStateHolder g_DoTToAuthNames; +enum class AdditionalMode : uint8_t; // defined in rec-lua-conf.hh + class RecursorLua4; typedef std::unordered_map< @@ -269,14 +271,6 @@ public: enum class HardenNXD { No, DNSSEC, Yes }; - enum class AddtionalMode : uint8_t { - Ignore, - CacheOnly, - CacheOnlyRequireAuth, - ResolveImmediately, - ResolveDeferred - }; - //! This represents a number of decaying Ewmas, used to store performance per nameserver-name. /** Modelled to work mostly like the underlying DecayingEwma */ struct DecayingEwmaCollection @@ -846,7 +840,7 @@ private: typedef std::map zonesStates_t; enum StopAtDelegation { DontStop, Stop, Stopped }; - void resolveAdditionals(const DNSName& qname, QType qtype, AddtionalMode, std::vector& additionals, unsigned int depth); + void resolveAdditionals(const DNSName& qname, QType qtype, AdditionalMode, std::vector& additionals, unsigned int depth); void addAdditionals(QType qtype, const vector&start, vector&addditionals, std::set>& uniqueCalls, std::set>& uniqueResults, unsigned int depth, unsigned int adddepth); void addAdditionals(QType qtype, vector&ret, unsigned int depth); -- 2.47.2