From b6adfe10c34c96eeb1439de0a2788d7992aebae0 Mon Sep 17 00:00:00 2001 From: Davide Pesavento Date: Fri, 5 Jul 2024 13:03:15 -0400 Subject: [PATCH] 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 | 58 ++++++++----------- src/security/certificate-store.hpp | 29 ++++------ .../communication/test-sync-logic-handler.cpp | 4 +- tests/security/test-certificate-store.cpp | 25 +++----- wscript | 5 +- 5 files changed, 52 insertions(+), 69 deletions(-) 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 #include @@ -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"); } } 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 #include -#include #include #include +#include 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 @@ 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; @@ -81,10 +78,7 @@ class CertificateStore 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 CertMap; - CertMap m_certificates; + std::map 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 @@ class SyncLogicFixture : public IoKeyChainFixture getSync() { if (m_sync == nullptr) { - m_sync.reset(new SyncLogicHandler(face, m_keyChain, testIsLsaNew, opts)); + m_sync = std::make_unique(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 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 @@ 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() diff --git a/wscript b/wscript index 74dcee0..f070b74 100644 --- a/wscript +++ b/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