]> git.ipfire.org Git - thirdparty/pdns.git/commitdiff
Move ZoneToCache from a separate thread to the handler, so that we can resolve e...
authorOtto Moerbeek <otto.moerbeek@open-xchange.com>
Wed, 19 Jan 2022 12:34:09 +0000 (13:34 +0100)
committerOtto Moerbeek <otto.moerbeek@open-xchange.com>
Fri, 21 Jan 2022 11:06:01 +0000 (12:06 +0100)
Config and logic of ZoneMD wrt DNSSEC is too complex, needs more thought.

pdns/rec-lua-conf.cc
pdns/rec-lua-conf.hh
pdns/recursordist/rec-main.cc
pdns/recursordist/rec-zonetocache.cc
pdns/recursordist/rec-zonetocache.hh
pdns/recursordist/test-rec-zonetocache.cc
pdns/zonemd.cc

index 59af03d20c8933699b476d4aa776539c6d0d3d3f..d0b73955d9c84b8982c1c5ee3bb290263d0be9e7 100644 (file)
@@ -354,7 +354,7 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de
 
   /* we can get: "1.2.3.4"
                  {"1.2.3.4", "4.5.6.7"}
-                {"1.2.3.4", {"4.5.6.7", "8.9.10.11"}}
+                 {"1.2.3.4", {"4.5.6.7", "8.9.10.11"}}
   */
 
   map<string, DNSFilterEngine::PolicyKind> pmap{
@@ -398,7 +398,7 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de
 
   typedef std::unordered_map<std::string, boost::variant<uint32_t, std::string>> zoneToCacheOptions_t;
 
-  Lua.writeFunction("zoneToCache", [&delayedThreads](const string& zoneName, const string& method, const boost::variant<string, std::vector<std::pair<int, string>>>& srcs, boost::optional<zoneToCacheOptions_t> options) {
+  Lua.writeFunction("zoneToCache", [&lci](const string& zoneName, const string& method, const boost::variant<string, std::vector<std::pair<int, string>>>& srcs, boost::optional<zoneToCacheOptions_t> options) {
     try {
       RecZoneToCache::Config conf;
       DNSName validZoneName(zoneName);
@@ -447,23 +447,24 @@ void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& de
         if (have.count("zonemdValidation")) {
           string zonemdValidation = boost::get<string>(have.at("zonemdValidation"));
           const map<string, pdns::ZoneMD::Config> nameToVal = {
-            { "ignore",  pdns::ZoneMD::Config::Ignore},
-            { "process",  pdns::ZoneMD::Config::Process},
-            { "logonly",  pdns::ZoneMD::Config::LogOnly},
-            { "required",  pdns::ZoneMD::Config::Required},
-            { "requiredWithDNSSEC",  pdns::ZoneMD::Config::RequiredWithDNSSEC},
-            { "requiredButIgnoreDNSSEC",  pdns::ZoneMD::Config::RequiredButIgnoreDNSSEC},
-            };
+            {"ignore", pdns::ZoneMD::Config::Ignore},
+            {"process", pdns::ZoneMD::Config::Process},
+            {"logonly", pdns::ZoneMD::Config::LogOnly},
+            {"required", pdns::ZoneMD::Config::Required},
+            {"requiredWithDNSSEC", pdns::ZoneMD::Config::RequiredWithDNSSEC},
+            {"requiredButIgnoreDNSSEC", pdns::ZoneMD::Config::RequiredButIgnoreDNSSEC},
+          };
           auto it = nameToVal.find(zonemdValidation);
           if (it == nameToVal.end()) {
             throw std::runtime_error(zonemdValidation + " is not a valid value for `zonemdValidation`");
-          } else {
+          }
+          else {
             conf.d_zonemd = it->second;
           }
         }
       }
 
-      delayedThreads.ztcConfigs.push_back(conf);
+      lci.ztcConfigs[validZoneName] = conf;
     }
     catch (const std::exception& e) {
       g_log << Logger::Error << "Problem configuring zoneToCache for zone '" << zoneName << "': " << e.what() << endl;
@@ -710,19 +711,4 @@ void startLuaConfigDelayedThreads(const luaConfigDelayedThreads& delayedThreads,
       exit(1);
     }
   }
-
-  for (const auto& ztcConfig : delayedThreads.ztcConfigs) {
-    try {
-      std::thread t(RecZoneToCache::ZoneToCache, ztcConfig, generation);
-      t.detach();
-    }
-    catch (const std::exception& e) {
-      g_log << Logger::Error << "Problem starting zoneToCache thread: " << e.what() << endl;
-      exit(1);
-    }
-    catch (const PDNSException& e) {
-      g_log << Logger::Error << "Problem starting zoneToCache thread: " << e.reason << endl;
-      exit(1);
-    }
-  }
 }
index 985d2e6bd0b70d3282573a4c97a7feb143079def..830c9987aac22b5aba7f80ea2ab104ae16262cae 100644 (file)
@@ -71,6 +71,7 @@ public:
   TrustAnchorFileInfo trustAnchorFileInfo; // Used to update the Trust Anchors from file periodically
   map<DNSName, dsmap_t> dsAnchors;
   map<DNSName, std::string> negAnchors;
+  map<DNSName, RecZoneToCache::Config> ztcConfigs;
   ProtobufExportConfig protobufExportConfig;
   ProtobufExportConfig outgoingProtobufExportConfig;
   FrameStreamExportConfig frameStreamExportConfig;
@@ -88,7 +89,6 @@ struct luaConfigDelayedThreads
 {
   // Please make sure that the tuple below only contains value types since they are used as parameters in a thread ct
   std::vector<std::tuple<std::vector<ComboAddress>, boost::optional<DNSFilterEngine::Policy>, bool, uint32_t, size_t, TSIGTriplet, size_t, ComboAddress, uint16_t, uint32_t, std::shared_ptr<SOARecordContent>, std::string>> rpzPrimaryThreads;
-  std::vector<RecZoneToCache::Config> ztcConfigs;
 };
 
 void loadRecursorLuaConfig(const std::string& fname, luaConfigDelayedThreads& delayedThreads);
index cab9f4a301c807f75dcd543befe517b4ae2cf4ef..65ea478fb37b012e977b48c7b71bed843755cac9 100644 (file)
@@ -1576,7 +1576,6 @@ static int serviceMain(int argc, char* argv[])
 
   startLuaConfigDelayedThreads(delayedLuaThreads, g_luaconfs.getCopy().generation);
   delayedLuaThreads.rpzPrimaryThreads.clear(); // no longer needed
-  delayedLuaThreads.ztcConfigs.clear(); // no longer needed
 
   makeThreadPipes();
 
@@ -1808,6 +1807,11 @@ static void houseKeeping(void*)
     }
 
     if (isHandlerThread()) {
+      static map<DNSName, RecZoneToCache::State> ztcState;
+      for (auto& ztc : luaconfsLocal->ztcConfigs) {
+        RecZoneToCache::ZoneToCache(ztc.second, ztcState[ztc.first]);
+      }
+
       if (now.tv_sec - s_last_RC_prune > 5) {
         g_recCache->doPrune(g_maxCacheEntries);
         g_negCache->prune(g_maxCacheEntries / 10);
index 0f4c4e911cf384b0497d752b866dc7ee5431b885..a059500bdd29b1bf247bc56a8578c859cd837919 100644 (file)
@@ -63,8 +63,8 @@ struct ZoneData
   void parseDRForCache(DNSRecord& dr);
   pdns::ZoneMD::Result getByAXFR(const RecZoneToCache::Config&, pdns::ZoneMD&);
   pdns::ZoneMD::Result processLines(const std::vector<std::string>& lines, const RecZoneToCache::Config& config, pdns::ZoneMD&);
-  void ZoneToCache(const RecZoneToCache::Config& config, uint64_t gen);
-  vState dnssecValidate(pdns::ZoneMD&) const;
+  void ZoneToCache(const RecZoneToCache::Config& config);
+  vState dnssecValidate(pdns::ZoneMD&, size_t& zonemdCount) const;
 };
 
 bool ZoneData::isRRSetAuth(const DNSName& qname, QType qtype) const
@@ -236,40 +236,33 @@ pdns::ZoneMD::Result ZoneData::processLines(const vector<string>& lines, const R
   return pdns::ZoneMD::Result::OK;
 }
 
-vState ZoneData::dnssecValidate(pdns::ZoneMD &zonemd) const
+vState ZoneData::dnssecValidate(pdns::ZoneMD& zonemd, size_t& zonemdCount) const
 {
-  vector<DNSRecord> dsRecords;
-  vState dsState = vState::Indeterminate;
+  zonemdCount = 0;
 
-  // XXX TA/NTA processsing, plus we have nog guarantee the DS is
-  // is in the cache. We assume for now it's cached magically
+  SyncRes sr({d_now, 0});
+  sr.setDoDNSSEC(true);
+  sr.setDNSSECValidationRequested(true);
 
-  // Get the DS records
-  if (g_recCache->get(d_now, d_zone, QType::DS,  false, &dsRecords, ComboAddress(), false, boost::none, nullptr, nullptr, nullptr, &dsState) <= 0) {
-    return vState::BogusUnableToGetDSs;
-  }
+  dsmap_t dsmap; // Actually a set
+  vState dsState = sr.getDSRecords(d_zone, dsmap, false, 0);
   if (dsState != vState::Secure) {
-    return vState::BogusUnableToGetDSs; // XXX is this the right status?
-  }
-
-  // Collect DNSKEYs and validate them using the DS records
-  dsmap_t dsRecordContents;
-  for (const auto& ds : dsRecords) {
-    dsRecordContents.insert(*getRR<DSRecordContent>(ds).get());
+    cerr << "getDSRecords says" << dsState << endl;
+    return vState::Insecure;
   }
 
-  skeyset_t  dnsKeys;
+  skeyset_t dnsKeys;
   sortedRecords_t records;
   if (zonemd.getDNSKEYs().size() == 0) {
     return vState::BogusUnableToGetDNSKEYs;
   }
-  for (const auto& key: zonemd.getDNSKEYs()) {
+  for (const auto& key : zonemd.getDNSKEYs()) {
     dnsKeys.emplace(key);
     records.emplace(key);
   }
 
   skeyset_t validKeys;
-  vState dnsKeyState = validateDNSKeysAgainstDS(d_now, d_zone, dsRecordContents, dnsKeys, records, zonemd.getRRSIGs(), validKeys);
+  vState dnsKeyState = validateDNSKeysAgainstDS(d_now, d_zone, dsmap, dnsKeys, records, zonemd.getRRSIGs(), validKeys);
   if (dnsKeyState != vState::Secure) {
     return dnsKeyState;
   }
@@ -277,21 +270,24 @@ vState ZoneData::dnssecValidate(pdns::ZoneMD &zonemd) const
   if (validKeys.size() == 0) {
     return vState::BogusNoValidDNSKEY;
   }
-  if (zonemd.getZONEMDs().size() == 0) {
-    // XXX is this the right status? Also per RFC we should actuelly prove non-existence of ZONEMD
+
+  auto zonemdRecords = zonemd.getZONEMDs();
+  zonemdCount = zonemdRecords.size();
+  if (zonemdCount == 0) {
+    // Per RFC we should actually prove non-existence of ZONEMD
     // as downgrade attacks could be possible by an in the middle party zapping ZONEMDs
     return dnsKeyState;
   }
   records.clear();
 
   // Collect the ZONEMD records and validate them using the validted DNSSKEYs
-  for (const auto& rec : zonemd.getZONEMDs()) {
+  for (const auto& rec : zonemdRecords) {
     records.emplace(rec);
   }
-  return validateWithKeySet(d_now, d_zone,  records, zonemd.getRRSIGs(), validKeys);
+  return validateWithKeySet(d_now, d_zone, records, zonemd.getRRSIGs(), validKeys);
 }
 
-void ZoneData::ZoneToCache(const RecZoneToCache::Config& config, uint64_t configGeneration)
+void ZoneData::ZoneToCache(const RecZoneToCache::Config& config)
 {
   if (config.d_sources.size() > 1) {
     d_log->info("Multiple sources not yet supported, using first");
@@ -321,25 +317,28 @@ void ZoneData::ZoneToCache(const RecZoneToCache::Config& config, uint64_t config
   }
 
   if (config.d_zonemd == pdns::ZoneMD::Config::RequiredWithDNSSEC && g_dnssecmode == DNSSECMode::Off) {
-    throw PDNSException("ZONEMD DNSSEC validation failure: dnssec is switched of but required by ztc");
+    throw PDNSException("ZONEMD DNSSEC validation failure: dnssec is switched of but required by ZoneToCache");
   }
 
   // Validate DNSKEYs and ZONEMD, rest of records are validated on-demand by SyncRes
-  if (config.d_zonemd == pdns::ZoneMD::Config::RequiredWithDNSSEC  ||
-      (g_dnssecmode == DNSSECMode::ValidateAll && config.d_zonemd != pdns::ZoneMD::Config::RequiredButIgnoreDNSSEC)) {
-    auto validationStatus = dnssecValidate(zonemd);
-    d_log->info("ZONEMD DNSSEC validation done", "validationStatus", Logging::Loggable(validationStatus));
-    if (config.d_zonemd == pdns::ZoneMD::Config::RequiredWithDNSSEC) {
+  if (config.d_zonemd == pdns::ZoneMD::Config::RequiredWithDNSSEC || (g_dnssecmode != DNSSECMode::Off && config.d_zonemd != pdns::ZoneMD::Config::RequiredButIgnoreDNSSEC)) {
+    size_t zonemdCount;
+    auto validationStatus = dnssecValidate(zonemd, zonemdCount);
+    d_log->info("ZONEMD record related DNSSEC validation done", "validationStatus", Logging::Loggable(validationStatus),
+                "zonemdCount", Logging::Loggable(zonemdCount));
+    if (config.d_zonemd == pdns::ZoneMD::Config::RequiredWithDNSSEC || g_dnssecmode == DNSSECMode::ValidateAll) {
       if (validationStatus != vState::Secure) {
         throw PDNSException("ZONEMD required DNSSEC validation failed");
       }
     }
-    // XXX handle other cases
+    if (validationStatus != vState::Secure && validationStatus != vState::Insecure) {
+      throw PDNSException("ZONEMD record DNSSEC Validation failed");
+    }
   }
 
   if (pdns::ZoneMD::validationRequired(config.d_zonemd) && result != pdns::ZoneMD::Result::OK) {
     // We do not accept NoValidationDone in this case
-    throw PDNSException("ZONEMD validation failure");
+    throw PDNSException("ZONEMD digest validation failure");
     return;
   }
   if (config.d_zonemd == pdns::ZoneMD::Config::Process && result == pdns::ZoneMD::Result::ValidationFailure) {
@@ -361,12 +360,6 @@ void ZoneData::ZoneToCache(const RecZoneToCache::Config& config, uint64_t config
     }
   }
 
-  // Extra check before we are touching the cache
-  auto luaconfsLocal = g_luaconfs.getLocal();
-  if (luaconfsLocal->generation != configGeneration) {
-    return;
-  }
-
   // Rerun, now inserting the rrsets into the cache with associated sigs
   d_now = time(nullptr);
   for (const auto& [key, v] : d_all) {
@@ -395,45 +388,32 @@ void ZoneData::ZoneToCache(const RecZoneToCache::Config& config, uint64_t config
   }
 }
 
-// Config must be a copy, so call by value!
-void RecZoneToCache::ZoneToCache(RecZoneToCache::Config config, uint64_t configGeneration)
+void RecZoneToCache::ZoneToCache(const RecZoneToCache::Config& config, RecZoneToCache::State& state)
 {
-  setThreadName("pdns-r/ztc/" + config.d_zone);
-  auto luaconfsLocal = g_luaconfs.getLocal();
+  if (state.d_waittime == 0 && state.d_lastrun > 0) {
+    // single shot
+    return;
+  }
+  if (state.d_lastrun > 0 && state.d_lastrun + state.d_waittime > time(nullptr)) {
+    return;
+  }
   auto log = g_slog->withName("ztc")->withValues("zone", Logging::Loggable(config.d_zone));
 
-  while (true) {
-    if (luaconfsLocal->generation != configGeneration) {
-      /* the configuration has been reloaded, meaning that a new thread
-         has been started to handle that zone and we are now obsolete.
-      */
-      log->info("A more recent configuration has been found, stopping the old update thread");
-      return;
-    }
-
-    time_t refresh = config.d_retryOnError;
-    try {
-      ZoneData data(log, config.d_zone);
-      data.ZoneToCache(config, configGeneration);
-      if (luaconfsLocal->generation != configGeneration) {
-        log->info("A more recent configuration has been found, stopping the old update thread");
-        return;
-      }
-      refresh = config.d_refreshPeriod;
-      log->info("Loaded zone into cache", "refresh", Logging::Loggable(refresh));
-    }
-    catch (const PDNSException& e) {
-      log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable(e.reason), "refresh", Logging::Loggable(refresh));
-    }
-    catch (const std::runtime_error& e) {
-      log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable(e.what()), "refresh", Logging::Loggable(refresh));
-    }
-    catch (...) {
-      log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable("unknown"), "refresh", Logging::Loggable(refresh));
-    }
-    if (refresh == 0) {
-      return; // single shot
-    }
-    sleep(refresh);
+  state.d_waittime = config.d_retryOnError;
+  try {
+    ZoneData data(log, config.d_zone);
+    data.ZoneToCache(config);
+    state.d_waittime = config.d_refreshPeriod;
+    log->info("Loaded zone into cache", "refresh", Logging::Loggable(state.d_waittime));
+  }
+  catch (const PDNSException& e) {
+    log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable(e.reason), "refresh", Logging::Loggable(state.d_waittime));
+  }
+  catch (const std::runtime_error& e) {
+    log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable(e.what()), "refresh", Logging::Loggable(state.d_waittime));
+  }
+  catch (...) {
+    log->info("Unable to load zone into cache, will retry", "exception", Logging::Loggable("unknown"), "refresh", Logging::Loggable(state.d_waittime));
   }
+  state.d_lastrun = time(nullptr);
 }
index 4b48809f2e4a127fb9b3f7db20b7a74f586a153f..7284895eb34eb92432fe5312543146ddeb72b223 100644 (file)
@@ -43,5 +43,12 @@ public:
     uint32_t d_timeout{20}; // timeout in seconds
     pdns::ZoneMD::Config d_zonemd{pdns::ZoneMD::Config::Process};
   };
-  static void ZoneToCache(Config config, uint64_t gen);
+
+  struct State
+  {
+    time_t d_lastrun{0};
+    time_t d_waittime{0};
+  };
+
+  static void ZoneToCache(const Config& config, State& state);
 };
index 2081124ab536532729a369a98fa9a01352de5db5..52b37a8756aa2c465c1adf9952955b4932e40cf5 100644 (file)
@@ -70,13 +70,14 @@ BOOST_AUTO_TEST_CASE(test_zonetocache)
   BOOST_REQUIRE(fclose(fp) == 0);
 
   RecZoneToCache::Config config{".", "file", {temp}, ComboAddress(), TSIGTriplet()};
+  RecZoneToCache::State state;
   config.d_refreshPeriod = 0;
   config.d_retryOnError = 0;
 
   // Start with a new, empty cache
   g_recCache = std::make_unique<MemRecursorCache>();
   BOOST_CHECK_EQUAL(g_recCache->size(), 0U);
-  RecZoneToCache::ZoneToCache(config, 0);
+  RecZoneToCache::ZoneToCache(config, state);
   unlink(temp);
   BOOST_CHECK_EQUAL(g_recCache->size(), 17U);
 
index 3173996bf048464b42928ced338f235f275f3196..0fe904a45a9e60b050b68e5f2e3cd5cece703913 100644 (file)
@@ -37,7 +37,7 @@ void pdns::ZoneMD::readRecords(ZoneParserTNG& zpt)
       case QType::DNSKEY:
         d_dnskeys.emplace(std::dynamic_pointer_cast<DNSKEYRecordContent>(drc));
         break;
-      case QType::ZONEMD :{
+      case QType::ZONEMD{
         auto zonemd = std::dynamic_pointer_cast<ZONEMDRecordContent>(drc);
         auto inserted = d_zonemdRecords.insert({pair(zonemd->d_scheme, zonemd->d_hashalgo), {zonemd, false}});
         if (!inserted.second) {
@@ -171,7 +171,7 @@ void pdns::ZoneMD::verify(bool& validationDone, bool& validationOK)
     if (sorted.empty()) {
       // continue;
     }
-    
+
     if (qtype != QType::RRSIG) {
       RRSIGRecordContent rrc;
       rrc.d_originalttl = d_resourceRecordSetTTLs[rrset.first];