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
diff --git a/src/security/certificate-store.cpp b/src/security/certificate-store.cpp
index f679fb1..648be4b 100644
--- a/src/security/certificate-store.cpp
+++ b/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 @@
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 @@
}
void
-CertificateStore::clear()
+CertificateStore::setInterestFilter(const ndn::Name& prefix)
{
- m_certificates.clear();
-}
-
-void
-CertificateStore::setInterestFilter(const ndn::Name& prefix, bool loopback)
-{
- 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 @@
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 @@
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 @@
}
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");
}
}
diff --git a/src/security/certificate-store.hpp b/src/security/certificate-store.hpp
index 0c01bf0..a4708b4 100644
--- a/src/security/certificate-store.hpp
+++ b/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,10 +70,7 @@
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;
@@ -81,10 +78,7 @@
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 @@
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
diff --git a/tests/communication/test-sync-logic-handler.cpp b/tests/communication/test-sync-logic-handler.cpp
index c4a630b..9a0c7e3 100644
--- a/tests/communication/test-sync-logic-handler.cpp
+++ b/tests/communication/test-sync-logic-handler.cpp
@@ -39,7 +39,7 @@
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 @@
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);
}
diff --git a/tests/security/test-certificate-store.cpp b/tests/security/test-certificate-store.cpp
index 03e086a..484959e 100644
--- a/tests/security/test-certificate-store.cpp
+++ b/tests/security/test-certificate-store.cpp
@@ -85,7 +85,6 @@
advanceClocks(20_ms);
}
-public:
void
checkForInterest(ndn::Name& interstName)
{
@@ -99,8 +98,8 @@
BOOST_CHECK(didFindInterest);
}
+protected:
ndn::DummyClientFace face;
-
ConfParameter conf;
DummyConfFileProcessor confProcessor;
@@ -223,29 +222,23 @@
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()
diff --git a/wscript b/wscript
index 74dcee0..f070b74 100644
--- a/wscript
+++ b/wscript
@@ -94,9 +94,12 @@
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