security: don't crash if received segment lacks KeyLocator

And while at it:
 * move afterSegmentValidated() to a lambda
 * remove unused loopback parameter from setInterestFilter()
 * delete unused clear() method
 * improve logging
 * prevent building without PSync if tests are enabled, since that
   configuration is currently unsupported

Change-Id: I930744296d3fa295787c16e6829d1dc27b06a195

src/security/certificate-store.cpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2022, The University of Memphis,
+ * Copyright (c) 2014-2024, The University of Memphis,
  * Regents of the University of California,
  * Arizona Board of Regents.
  *
@@ -22,6 +22,7 @@
 #include "certificate-store.hpp"
 #include "conf-parameter.hpp"
 #include "logger.hpp"
+#include "lsdb.hpp"
 
 #include <ndn-cxx/util/io.hpp>
 #include <fstream>
@@ -43,15 +44,26 @@ CertificateStore::CertificateStore(ndn::Face& face, ConfParameter& confParam, Ls
 
  registerKeyPrefixes();
 
- m_afterSegmentValidatedConnection = lsdb.afterSegmentValidatedSignal.connect(
- [this] (const ndn::Data& data) { afterFetcherSignalEmitted(data); });
+ m_afterSegmentValidatedConn = lsdb.afterSegmentValidatedSignal.connect([this] (const auto& data) {
+ const auto kl = data.getKeyLocator();
+ if (!kl || kl->getType() != ndn::tlv::Name) {
+ NLSR_LOG_TRACE("Cannot determine KeyLocator Name for: " << data.getName());
+ }
+ else if (const auto klName = kl->getName(); !find(klName)) {
+ NLSR_LOG_TRACE("Publishing certificate for: " << klName);
+ publishCertFromCache(klName);
+ }
+ else {
+ NLSR_LOG_TRACE("Certificate is already in the store: " << klName);
+ }
+ });
 }
 
 void
 CertificateStore::insert(const ndn::security::Certificate& certificate)
 {
  m_certificates[certificate.getKeyName()] = certificate;
- NLSR_LOG_TRACE("Certificate inserted successfully");
+ NLSR_LOG_TRACE("Certificate inserted successfully\n" << certificate);
 }
 
 const ndn::security::Certificate*
@@ -81,15 +93,9 @@ CertificateStore::findByCertName(const ndn::Name& certName) const
 }
 
 void
-CertificateStore::clear()
-{
- m_certificates.clear();
-}
-
-void
-CertificateStore::setInterestFilter(const ndn::Name& prefix, bool loopback)
+CertificateStore::setInterestFilter(const ndn::Name& prefix)
 {
- m_face.setInterestFilter(ndn::InterestFilter(prefix).allowLoopback(loopback),
+ m_face.setInterestFilter(ndn::InterestFilter(prefix).allowLoopback(false),
  std::bind(&CertificateStore::onKeyInterest, this, _1, _2),
  std::bind(&CertificateStore::onKeyPrefixRegSuccess, this, _1),
  std::bind(&CertificateStore::registrationFailed, this, _1),
@@ -134,28 +140,28 @@ CertificateStore::registerKeyPrefixes()
 void
 CertificateStore::onKeyInterest(const ndn::Name&, const ndn::Interest& interest)
 {
- NLSR_LOG_DEBUG("Got interest for certificate. Interest: " << interest.getName());
+ NLSR_LOG_TRACE("Got certificate Interest: " << interest.getName());
 
  const auto* cert = find(interest.getName());
-
  if (!cert) {
- NLSR_LOG_TRACE("Certificate is not found for: " << interest);
+ NLSR_LOG_DEBUG("Certificate not found for: " << interest.getName());
  return;
  }
+
  m_face.put(*cert);
 }
 
 void
 CertificateStore::onKeyPrefixRegSuccess(const ndn::Name& name)
 {
- NLSR_LOG_DEBUG("KEY prefix: " << name << " registration is successful");
+ NLSR_LOG_DEBUG("Prefix registered successfully: " << name);
 }
 
 void
 CertificateStore::registrationFailed(const ndn::Name& name)
 {
- NLSR_LOG_ERROR("Failed to register prefix " << name);
- NDN_THROW(std::runtime_error("Prefix registration failed"));
+ NLSR_LOG_ERROR("Failed to register prefix: " << name);
+ NDN_THROW(std::runtime_error("Prefix registration failed: " + name.toUri()));
 }
 
 void
@@ -165,7 +171,6 @@ CertificateStore::publishCertFromCache(const ndn::Name& keyName)
 
  if (cert) {
  insert(*cert);
- NLSR_LOG_TRACE(*cert);
  ndn::Name certName = ndn::security::extractKeyNameFromCertName(cert->getName());
  NLSR_LOG_TRACE("Setting interest filter for: " << certName);
 
@@ -178,20 +183,7 @@ CertificateStore::publishCertFromCache(const ndn::Name& keyName)
  }
  else {
  // Happens for root cert
- NLSR_LOG_TRACE("Cert for " << keyName << " was not found in the Validator's cache. ");
- }
-}
-
-void
-CertificateStore::afterFetcherSignalEmitted(const ndn::Data& lsaSegment)
-{
- const auto keyName = lsaSegment.getSignatureInfo().getKeyLocator().getName();
- if (!find(keyName)) {
- NLSR_LOG_TRACE("Publishing certificate for: " << keyName);
- publishCertFromCache(keyName);
- }
- else {
- NLSR_LOG_TRACE("Certificate is already in the store: " << keyName);
+ NLSR_LOG_TRACE("Cert for " << keyName << " was not found in the Validator's cache");
  }
 }
 

src/security/certificate-store.hpp

@@ -1,6 +1,6 @@
 /* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
 /*
- * Copyright (c) 2014-2023, The University of Memphis,
+ * Copyright (c) 2014-2024, The University of Memphis,
  * Regents of the University of California,
  * Arizona Board of Regents.
  *
@@ -22,20 +22,20 @@
 #ifndef NLSR_CERTIFICATE_STORE_HPP
 #define NLSR_CERTIFICATE_STORE_HPP
 
-#include "common.hpp"
-#include "test-access-control.hpp"
-#include "lsdb.hpp"
-
+#include <ndn-cxx/face.hpp>
 #include <ndn-cxx/interest.hpp>
-#include <ndn-cxx/mgmt/nfd/controller.hpp>
 #include <ndn-cxx/security/certificate.hpp>
 #include <ndn-cxx/security/validator-config.hpp>
+#include <ndn-cxx/util/signal/scoped-connection.hpp>
 
 namespace nlsr {
+
 class ConfParameter;
+class Lsdb;
+
 namespace security {
 
-/*! \brief Store certificates for names
+/*! \brief Store certificates for names.
  *
  * Stores certificates that this router claims to be authoritative
  * for. That is, this stores only the certificates that we will reply
@@ -70,21 +70,15 @@ class CertificateStore
  void
  publishCertFromCache(const ndn::Name& keyName);
 
- void
- afterFetcherSignalEmitted(const ndn::Data& lsaSegment);
-
-PUBLIC_WITH_TESTS_ELSE_PRIVATE:
+private:
  const ndn::security::Certificate*
  findByKeyName(const ndn::Name& keyName) const;
 
  const ndn::security::Certificate*
  findByCertName(const ndn::Name& certName) const;
 
  void
- clear();
-
- void
- setInterestFilter(const ndn::Name& prefix, const bool loopback = false);
+ setInterestFilter(const ndn::Name& prefix);
 
  void
  registerKeyPrefixes();
@@ -99,12 +93,11 @@ class CertificateStore
  registrationFailed(const ndn::Name& name);
 
 private:
- typedef std::map<ndn::Name, ndn::security::Certificate> CertMap;
- CertMap m_certificates;
+ std::map<ndn::Name, ndn::security::Certificate> m_certificates;
  ndn::Face& m_face;
  ConfParameter& m_confParam;
  ndn::security::ValidatorConfig& m_validator;
- ndn::signal::ScopedConnection m_afterSegmentValidatedConnection;
+ ndn::signal::ScopedConnection m_afterSegmentValidatedConn;
 };
 
 } // namespace security

tests/communication/test-sync-logic-handler.cpp

@@ -39,7 +39,7 @@ class SyncLogicFixture : public IoKeyChainFixture
  getSync()
  {
  if (m_sync == nullptr) {
- m_sync.reset(new SyncLogicHandler(face, m_keyChain, testIsLsaNew, opts));
+ m_sync = std::make_unique<SyncLogicHandler>(face, m_keyChain, testIsLsaNew, opts);
  }
  return *m_sync;
  }
@@ -50,9 +50,11 @@ class SyncLogicFixture : public IoKeyChainFixture
  this->advanceClocks(1_ms, 10);
  face.sentInterests.clear();
 
+#ifdef HAVE_PSYNC
  std::vector<psync::MissingDataInfo> updates;
  updates.push_back({prefix, 0, seqNo, 0});
  getSync().m_syncLogic.onPSyncUpdate(updates);
+#endif
 
  this->advanceClocks(1_ms, 10);
  }

tests/security/test-certificate-store.cpp

@@ -85,7 +85,6 @@ class CertificateStoreFixture : public IoKeyChainFixture
  advanceClocks(20_ms);
  }
 
-public:
  void
  checkForInterest(ndn::Name& interstName)
  {
@@ -99,8 +98,8 @@ class CertificateStoreFixture : public IoKeyChainFixture
  BOOST_CHECK(didFindInterest);
  }
 
+protected:
  ndn::DummyClientFace face;
-
  ConfParameter conf;
  DummyConfFileProcessor confProcessor;
 
@@ -223,29 +222,23 @@ BOOST_AUTO_TEST_CASE(SegmentValidatedSignal)
  data.setContent(nameLsa.wireEncode());
  data.setFinalBlock(lsaDataName[-1]);
 
- // Sign data with this NLSR's key (in real it would be different NLSR)
- m_keyChain.sign(data, conf.m_signingInfo);
- face.put(data);
+ // Test with unsigned data first (lacks KeyLocator).
+ // This should not happen during normal operations, but CertificateStore
+ // should still be able to handle invalid packets without crashing
+ lsdb.emitSegmentValidatedSignal(data);
 
- this->advanceClocks(1_ms);
+ // Sign data with this NLSR's key (in reality it would be a different NLSR)
+ m_keyChain.sign(data, conf.m_signingInfo);
 
  // Make NLSR validate data signed by its own key
  conf.getValidator().validate(data,
- [] (const ndn::Data&) { BOOST_CHECK(true); },
- [] (const ndn::Data&, const ndn::security::ValidationError& e) {
- BOOST_ERROR(e);
- });
+ [] (const auto&) { BOOST_CHECK(true); },
+ [] (const auto&, const auto& err) { BOOST_ERROR(err); });
 
  lsdb.emitSegmentValidatedSignal(data);
  auto certName = data.getSignatureInfo().getKeyLocator().getName();
  auto keyName = ndn::security::extractKeyNameFromCertName(certName);
  BOOST_CHECK(certStore.find(keyName) != nullptr);
-
- // testing a callback after segment validation signal from lsdb
- ndn::signal::ScopedConnection connection = lsdb.afterSegmentValidatedSignal.connect(
- [&] (const ndn::Data& lsaSegment) {
- BOOST_CHECK_EQUAL(lsaSegment.getName(), data.getName());
- });
 }
 
 BOOST_AUTO_TEST_SUITE_END()

wscript

@@ -94,9 +94,12 @@ def configure(conf):
  uselib_store='SVS', pkg_config_path=pkg_config_path)
 
  if not any((conf.options.with_chronosync, conf.options.with_psync, conf.options.with_svs)):
- conf.fatal('Cannot compile without any Sync protocol. '
+ conf.fatal('Cannot compile without any Sync protocol.\n'
  'Specify at least one of --with-psync or --with-svs or --with-chronosync')
 
+ if conf.env.WITH_TESTS and not conf.options.with_psync:
+ conf.fatal('--with-tests requires --with-psync')
+
  conf.check_compiler_flags()
 
  # Loading "late" to prevent tests from being compiled with profiling flags