security: modernize CertificateContainer; add logging
Change-Id: I127a7eefb794c83787748999f10ffaeb6614f8b7
diff --git a/ndn-cxx/security/pib/certificate-container.cpp b/ndn-cxx/security/pib/certificate-container.cpp
index a3ced13..d3d6cbb 100644
--- a/ndn-cxx/security/pib/certificate-container.cpp
+++ b/ndn-cxx/security/pib/certificate-container.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2013-2021 Regents of the University of California.
+ * Copyright (c) 2013-2022 Regents of the University of California.
*
* This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
*
@@ -22,20 +22,18 @@
#include "ndn-cxx/security/pib/certificate-container.hpp"
#include "ndn-cxx/security/pib/pib-impl.hpp"
#include "ndn-cxx/util/concepts.hpp"
+#include "ndn-cxx/util/logger.hpp"
namespace ndn {
namespace security {
namespace pib {
+NDN_LOG_INIT(ndn.security.CertificateContainer);
+
NDN_CXX_ASSERT_FORWARD_ITERATOR(CertificateContainer::const_iterator);
-CertificateContainer::const_iterator::const_iterator()
- : m_container(nullptr)
-{
-}
-
-CertificateContainer::const_iterator::const_iterator(std::set<Name>::const_iterator it,
- const CertificateContainer& container)
+CertificateContainer::const_iterator::const_iterator(NameSet::const_iterator it,
+ const CertificateContainer& container) noexcept
: m_it(it)
, m_container(&container)
{
@@ -48,36 +46,14 @@
return m_container->get(*m_it);
}
-CertificateContainer::const_iterator&
-CertificateContainer::const_iterator::operator++()
-{
- ++m_it;
- return *this;
-}
-
-CertificateContainer::const_iterator
-CertificateContainer::const_iterator::operator++(int)
-{
- BOOST_ASSERT(m_container != nullptr);
- const_iterator it(m_it, *m_container);
- ++m_it;
- return it;
-}
-
bool
CertificateContainer::const_iterator::operator==(const const_iterator& other) const
{
bool isThisEnd = m_container == nullptr || m_it == m_container->m_certNames.end();
bool isOtherEnd = other.m_container == nullptr || other.m_it == other.m_container->m_certNames.end();
- return ((isThisEnd || isOtherEnd) ?
- (isThisEnd == isOtherEnd) :
- m_container->m_pib == other.m_container->m_pib && m_it == other.m_it);
-}
-
-bool
-CertificateContainer::const_iterator::operator!=(const const_iterator& other) const
-{
- return !(*this == other);
+ if (isThisEnd)
+ return isOtherEnd;
+ return !isOtherEnd && m_container->m_pib == other.m_container->m_pib && m_it == other.m_it;
}
CertificateContainer::CertificateContainer(const Name& keyName, shared_ptr<PibImpl> pibImpl)
@@ -89,70 +65,61 @@
}
CertificateContainer::const_iterator
-CertificateContainer::begin() const
-{
- return {m_certNames.begin(), *this};
-}
-
-CertificateContainer::const_iterator
-CertificateContainer::end() const
-{
- return {};
-}
-
-CertificateContainer::const_iterator
CertificateContainer::find(const Name& certName) const
{
return {m_certNames.find(certName), *this};
}
-size_t
-CertificateContainer::size() const
-{
- return m_certNames.size();
-}
-
void
CertificateContainer::add(const Certificate& certificate)
{
- if (m_keyName != certificate.getKeyName())
- NDN_THROW(std::invalid_argument("Certificate name `" + certificate.getKeyName().toUri() + "` "
- "does not match key name"));
+ if (m_keyName != certificate.getKeyName()) {
+ NDN_THROW(std::invalid_argument("Certificate name `" + certificate.getName().toUri() + "` "
+ "does not match key `" + m_keyName.toUri() + "`"));
+ }
const Name& certName = certificate.getName();
- m_certNames.insert(certName);
- m_certs[certName] = certificate;
+ bool isNew = m_certNames.insert(certName).second;
+ NDN_LOG_DEBUG((isNew ? "Adding " : "Replacing ") << certName);
+
m_pib->addCertificate(certificate);
+ m_certs[certName] = certificate; // use insert_or_assign in C++17
}
void
CertificateContainer::remove(const Name& certName)
{
- if (!Certificate::isValidName(certName) || extractKeyNameFromCertName(certName) != m_keyName) {
+ if (m_keyName != extractKeyNameFromCertName(certName)) {
NDN_THROW(std::invalid_argument("Certificate name `" + certName.toUri() + "` "
- "is invalid or does not match key name"));
+ "does not match key `" + m_keyName.toUri() + "`"));
}
- m_certNames.erase(certName);
- m_certs.erase(certName);
+ if (m_certNames.erase(certName) > 0) {
+ NDN_LOG_DEBUG("Removing " << certName);
+ m_certs.erase(certName);
+ }
+ else {
+ // consistency check
+ BOOST_ASSERT(m_certs.find(certName) == m_certs.end());
+ }
m_pib->removeCertificate(certName);
}
Certificate
CertificateContainer::get(const Name& certName) const
{
- auto it = m_certs.find(certName);
-
- if (it != m_certs.end())
- return it->second;
-
- if (!Certificate::isValidName(certName) || extractKeyNameFromCertName(certName) != m_keyName) {
+ if (m_keyName != extractKeyNameFromCertName(certName)) {
NDN_THROW(std::invalid_argument("Certificate name `" + certName.toUri() + "` "
- "is invalid or does not match key name"));
+ "does not match key `" + m_keyName.toUri() + "`"));
}
- m_certs[certName] = m_pib->getCertificate(certName);
- return m_certs[certName];
+ auto it = m_certs.find(certName);
+ if (it != m_certs.end()) {
+ return it->second;
+ }
+
+ auto ret = m_certs.emplace(certName, m_pib->getCertificate(certName));
+ return ret.first->second;
}
bool
diff --git a/ndn-cxx/security/pib/certificate-container.hpp b/ndn-cxx/security/pib/certificate-container.hpp
index 3befdde..34f6488 100644
--- a/ndn-cxx/security/pib/certificate-container.hpp
+++ b/ndn-cxx/security/pib/certificate-container.hpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2013-2021 Regents of the University of California.
+ * Copyright (c) 2013-2022 Regents of the University of California.
*
* This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
*
@@ -39,13 +39,18 @@
} // namespace detail
/**
- * @brief Container of certificates of a key
+ * @brief Container of certificates of a key.
*
- * The container is used to search/enumerate certificates of a key.
- * The container can be created only by detail::KeyImpl.
+ * The container is used to search/enumerate the certificates of a key.
+ * It can be created only by the KeyImpl private class.
+ *
+ * @sa Key::getCertificates()
*/
class CertificateContainer : noncopyable
{
+private:
+ using NameSet = std::set<Name>;
+
public:
class const_iterator
{
@@ -56,113 +61,130 @@
using pointer = value_type*;
using reference = value_type&;
- const_iterator();
+ const_iterator() = default;
Certificate
operator*();
const_iterator&
- operator++();
+ operator++()
+ {
+ ++m_it;
+ return *this;
+ }
const_iterator
- operator++(int);
+ operator++(int)
+ {
+ const_iterator it(*this);
+ ++m_it;
+ return it;
+ }
bool
operator==(const const_iterator& other) const;
bool
- operator!=(const const_iterator& other) const;
+ operator!=(const const_iterator& other) const
+ {
+ return !this->operator==(other);
+ }
private:
- const_iterator(std::set<Name>::const_iterator it, const CertificateContainer& container);
+ const_iterator(NameSet::const_iterator it, const CertificateContainer& container) noexcept;
private:
- std::set<Name>::const_iterator m_it;
- const CertificateContainer* m_container;
+ NameSet::const_iterator m_it;
+ const CertificateContainer* m_container = nullptr;
- friend class CertificateContainer;
+ friend CertificateContainer;
};
using iterator = const_iterator;
public:
const_iterator
- begin() const;
+ begin() const noexcept
+ {
+ return {m_certNames.begin(), *this};
+ }
const_iterator
- end() const;
+ end() const noexcept
+ {
+ return {};
+ }
const_iterator
find(const Name& certName) const;
- size_t
- size() const;
+ /**
+ * @brief Check whether the container is empty.
+ */
+ NDN_CXX_NODISCARD bool
+ empty() const noexcept
+ {
+ return m_certNames.empty();
+ }
/**
- * @brief Add @p certificate into the container
- * @throw std::invalid_argument the name of @p certificate does not match the key name
+ * @brief Return the number of certificates in the container.
+ */
+ size_t
+ size() const noexcept
+ {
+ return m_certNames.size();
+ }
+
+ /**
+ * @brief Add @p certificate into the container.
+ * @throw std::invalid_argument The name of @p certificate does not match the key name.
*/
void
add(const Certificate& certificate);
/**
- * @brief Remove a certificate with @p certName from the container
- * @throw std::invalid_argument @p certName does not match the key name
+ * @brief Remove a certificate with @p certName from the container.
+ * @throw std::invalid_argument @p certName does not match the key name.
*/
void
remove(const Name& certName);
/**
- * @brief Get a certificate with @p certName from the container
- * @throw std::invalid_argument @p certName does not match the key name
- * @throw Pib::Error the certificate does not exist
+ * @brief Return a certificate by name.
+ * @throw Pib::Error The certificate does not exist.
+ * @throw std::invalid_argument @p certName does not match the key name.
*/
Certificate
get(const Name& certName) const;
/**
- * @brief Check if the container is consistent with the backend storage
- * @note this method is heavyweight and should be used in debugging mode only.
+ * @brief Check if the container is consistent with the backend storage.
+ * @note This method is heavyweight and should be used in debugging mode only.
*/
bool
isConsistent() const;
-NDN_CXX_PUBLIC_WITH_TESTS_ELSE_PRIVATE:
+NDN_CXX_PUBLIC_WITH_TESTS_ELSE_PRIVATE: // private interface for KeyImpl
/**
- * @brief Create certificate container for a key with @p keyName
+ * @brief Create certificate container for @p keyName.
* @param pibImpl The PIB backend implementation.
*/
CertificateContainer(const Name& keyName, shared_ptr<PibImpl> pibImpl);
- const std::set<Name>&
- getCertNames() const
- {
- return m_certNames;
- }
-
- const std::unordered_map<Name, Certificate>&
- getCache() const
- {
- return m_certs;
- }
-
-private:
- Name m_keyName;
- std::set<Name> m_certNames;
- /// @brief Cache of loaded certificates
+NDN_CXX_PUBLIC_WITH_TESTS_ELSE_PRIVATE:
+ // cache of loaded certificates
mutable std::unordered_map<Name, Certificate> m_certs;
- shared_ptr<PibImpl> m_pib;
+private:
+ NameSet m_certNames;
+ const Name m_keyName;
+ const shared_ptr<PibImpl> m_pib;
-#ifndef DOXYGEN
friend detail::KeyImpl;
-#endif
};
} // namespace pib
-
-using pib::CertificateContainer;
-
} // namespace security
} // namespace ndn
diff --git a/tests/unit/security/pib/certificate-container.t.cpp b/tests/unit/security/pib/certificate-container.t.cpp
index ebb8ca1..117e8f2 100644
--- a/tests/unit/security/pib/certificate-container.t.cpp
+++ b/tests/unit/security/pib/certificate-container.t.cpp
@@ -1,6 +1,6 @@
/* -*- Mode:C++; c-file-style:"gnu"; indent-tabs-mode:nil; -*- */
/*
- * Copyright (c) 2013-2021 Regents of the University of California.
+ * Copyright (c) 2013-2022 Regents of the University of California.
*
* This file is part of ndn-cxx library (NDN C++ library with eXperimental eXtensions).
*
@@ -21,7 +21,6 @@
#include "ndn-cxx/security/pib/certificate-container.hpp"
#include "ndn-cxx/security/pib/impl/pib-memory.hpp"
-#include "ndn-cxx/security/pib/pib.hpp"
#include "tests/boost-test.hpp"
#include "tests/unit/security/pib/pib-data-fixture.hpp"
@@ -37,82 +36,78 @@
BOOST_AUTO_TEST_SUITE(Pib)
BOOST_FIXTURE_TEST_SUITE(TestCertificateContainer, PibDataFixture)
-using pib::Pib;
-
-BOOST_AUTO_TEST_CASE(Basic)
+BOOST_AUTO_TEST_CASE(AddGetRemove)
{
auto pibImpl = make_shared<PibMemory>();
- // start with an empty container
- CertificateContainer container(id1Key1Name, pibImpl);
- BOOST_CHECK_EQUAL(container.size(), 0);
- BOOST_CHECK_EQUAL(container.getCache().size(), 0);
+ {
+ // start with an empty container
+ CertificateContainer container(id1Key1Name, pibImpl);
+ BOOST_CHECK_EQUAL(container.size(), 0);
+ BOOST_CHECK_EQUAL(container.m_certs.size(), 0);
- // add one cert
- container.add(id1Key1Cert1);
- BOOST_CHECK_EQUAL(container.size(), 1);
- BOOST_CHECK_EQUAL(container.getCache().size(), 1);
- BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
+ // add one cert
+ container.add(id1Key1Cert1);
+ BOOST_CHECK_EQUAL(container.size(), 1);
+ BOOST_CHECK_EQUAL(container.m_certs.size(), 1);
+ BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
- // add the same cert again
- container.add(id1Key1Cert1);
- BOOST_CHECK_EQUAL(container.size(), 1);
- BOOST_CHECK_EQUAL(container.getCache().size(), 1);
- BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
+ // add the same cert again
+ container.add(id1Key1Cert1);
+ BOOST_CHECK_EQUAL(container.size(), 1);
+ BOOST_CHECK_EQUAL(container.m_certs.size(), 1);
+ BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
- // add another cert
- container.add(id1Key1Cert2);
- BOOST_CHECK_EQUAL(container.size(), 2);
- BOOST_CHECK_EQUAL(container.getCache().size(), 2);
- BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
- BOOST_CHECK(container.find(id1Key1Cert2.getName()) != container.end());
+ // add another cert
+ container.add(id1Key1Cert2);
+ BOOST_CHECK_EQUAL(container.size(), 2);
+ BOOST_CHECK_EQUAL(container.m_certs.size(), 2);
+ BOOST_CHECK(container.find(id1Key1Cert1.getName()) != container.end());
+ BOOST_CHECK(container.find(id1Key1Cert2.getName()) != container.end());
- // get certs
- BOOST_REQUIRE_NO_THROW(container.get(id1Key1Cert1.getName()));
- BOOST_REQUIRE_NO_THROW(container.get(id1Key1Cert2.getName()));
- Name id1Key1Cert3Name = id1Key1Name;
- id1Key1Cert3Name.append("issuer").appendVersion(3);
- BOOST_CHECK_THROW(container.get(id1Key1Cert3Name), Pib::Error);
+ // check certs
+ Certificate cert1 = container.get(id1Key1Cert1.getName());
+ Certificate cert2 = container.get(id1Key1Cert2.getName());
+ BOOST_CHECK_EQUAL(cert1, id1Key1Cert1);
+ BOOST_CHECK_EQUAL(cert2, id1Key1Cert2);
+ Name id1Key1Cert3Name = Name(id1Key1Name).append("issuer").appendVersion(3);
+ BOOST_CHECK_THROW(container.get(id1Key1Cert3Name), pib::Pib::Error);
+ }
- // check cert
- Certificate cert1 = container.get(id1Key1Cert1.getName());
- Certificate cert2 = container.get(id1Key1Cert2.getName());
- BOOST_CHECK_EQUAL(cert1, id1Key1Cert1);
- BOOST_CHECK_EQUAL(cert2, id1Key1Cert2);
+ {
+ // create a container from an existing (non-empty) PibImpl
+ // names are loaded immediately but the certificate cache should initially be empty
+ CertificateContainer container2(id1Key1Name, pibImpl);
+ BOOST_CHECK_EQUAL(container2.size(), 2);
+ BOOST_CHECK_EQUAL(container2.m_certs.size(), 0);
- // create another container from the same PibImpl
- // cache should be empty
- CertificateContainer container2(id1Key1Name, pibImpl);
- BOOST_CHECK_EQUAL(container2.size(), 2);
- BOOST_CHECK_EQUAL(container2.getCache().size(), 0);
+ // fetching the certificates should populate the cache
+ BOOST_CHECK_EQUAL(container2.get(id1Key1Cert1.getName()), id1Key1Cert1);
+ BOOST_CHECK_EQUAL(container2.size(), 2);
+ BOOST_CHECK_EQUAL(container2.m_certs.size(), 1);
- // get certificate, cache should be filled
- BOOST_REQUIRE_NO_THROW(container2.get(id1Key1Cert1.getName()));
- BOOST_CHECK_EQUAL(container2.size(), 2);
- BOOST_CHECK_EQUAL(container2.getCache().size(), 1);
+ BOOST_CHECK_EQUAL(container2.get(id1Key1Cert2.getName()), id1Key1Cert2);
+ BOOST_CHECK_EQUAL(container2.size(), 2);
+ BOOST_CHECK_EQUAL(container2.m_certs.size(), 2);
- BOOST_REQUIRE_NO_THROW(container2.get(id1Key1Cert2.getName()));
- BOOST_CHECK_EQUAL(container2.size(), 2);
- BOOST_CHECK_EQUAL(container2.getCache().size(), 2);
+ // remove a certificate
+ container2.remove(id1Key1Cert1.getName());
+ BOOST_CHECK_EQUAL(container2.size(), 1);
+ BOOST_CHECK_EQUAL(container2.m_certs.size(), 1);
+ BOOST_CHECK(container2.find(id1Key1Cert1.getName()) == container2.end());
+ BOOST_CHECK(container2.find(id1Key1Cert2.getName()) != container2.end());
- // remove a certificate
- container2.remove(id1Key1Cert1.getName());
- BOOST_CHECK_EQUAL(container2.size(), 1);
- BOOST_CHECK_EQUAL(container2.getCache().size(), 1);
- BOOST_CHECK(container2.find(id1Key1Cert1.getName()) == container2.end());
- BOOST_CHECK(container2.find(id1Key1Cert2.getName()) != container2.end());
-
- // remove another certificate
- container2.remove(id1Key1Cert2.getName());
- BOOST_CHECK_EQUAL(container2.size(), 0);
- BOOST_CHECK_EQUAL(container2.getCache().size(), 0);
- BOOST_CHECK(container2.find(id1Key1Cert2.getName()) == container2.end());
+ // remove another certificate
+ container2.remove(id1Key1Cert2.getName());
+ BOOST_CHECK_EQUAL(container2.size(), 0);
+ BOOST_CHECK_EQUAL(container2.m_certs.size(), 0);
+ BOOST_CHECK(container2.find(id1Key1Cert2.getName()) == container2.end());
+ }
}
BOOST_AUTO_TEST_CASE(Errors)
{
auto pibImpl = make_shared<PibMemory>();
-
CertificateContainer container(id1Key1Name, pibImpl);
BOOST_CHECK_THROW(container.add(id1Key2Cert1), std::invalid_argument);
@@ -123,17 +118,13 @@
BOOST_AUTO_TEST_CASE(Iterator)
{
auto pibImpl = make_shared<PibMemory>();
-
- // start with an empty container
CertificateContainer container(id1Key1Name, pibImpl);
container.add(id1Key1Cert1);
container.add(id1Key1Cert2);
- std::set<Name> certNames;
- certNames.insert(id1Key1Cert1.getName());
- certNames.insert(id1Key1Cert2.getName());
+ const std::set<Name> certNames{id1Key1Cert1.getName(), id1Key1Cert2.getName()};
- auto it = container.begin();
+ CertificateContainer::const_iterator it = container.begin();
auto testIt = certNames.begin();
BOOST_CHECK_EQUAL((*it).getName(), *testIt);
it++;
@@ -143,7 +134,8 @@
testIt++;
BOOST_CHECK(it == container.end());
- size_t count = 0;
+ // test range-based for
+ int count = 0;
testIt = certNames.begin();
for (const auto& cert : container) {
BOOST_CHECK_EQUAL(cert.getName(), *testIt);