security: Correct KeyChain::addCertificate semantics
The documentation of KeyChain::addCertificate was ambiguous. On the one
hand, it stated that the certificate with the same name will be
overwritten. On the other hand, it stated that it will thrown an
exception if certificate with the same name exists. This commit ensures
the former behavior, which is consistent with the old KeyChain.
Change-Id: I911f7c86c634caf260ecb9a5dbdf13b066f8711f
diff --git a/src/security/pib/detail/identity-impl.cpp b/src/security/pib/detail/identity-impl.cpp
index af01dc5..284f14c 100644
--- a/src/security/pib/detail/identity-impl.cpp
+++ b/src/security/pib/detail/identity-impl.cpp
@@ -49,10 +49,6 @@
{
BOOST_ASSERT(m_keys.isConsistent());
- if (m_keys.find(keyName) != m_keys.end()) {
- BOOST_THROW_EXCEPTION(Pib::Error("Cannot overwrite existing key " + keyName.toUri()));
- }
-
return m_keys.add(key, keyLen, keyName);
}
diff --git a/src/security/pib/detail/identity-impl.hpp b/src/security/pib/detail/identity-impl.hpp
index b654348..ceff37d 100644
--- a/src/security/pib/detail/identity-impl.hpp
+++ b/src/security/pib/detail/identity-impl.hpp
@@ -64,10 +64,10 @@
* @brief Add a @p key of @p keyLen bytes with @p keyName (in PKCS#8 format).
*
* If no default key is set before, the new key will be set as the default key of the identity.
+ * If a key with the same name already exists, overwrite the key.
*
* @return the added key.
* @throw std::invalid_argument key name does not match identity
- * @throw Pib::Error a key with the same name already exists
*/
Key
addKey(const uint8_t* key, size_t keyLen, const Name& keyName);
diff --git a/src/security/pib/detail/key-impl.cpp b/src/security/pib/detail/key-impl.cpp
index 35a0de6..c0cd839 100644
--- a/src/security/pib/detail/key-impl.cpp
+++ b/src/security/pib/detail/key-impl.cpp
@@ -39,15 +39,11 @@
{
BOOST_ASSERT(impl != nullptr);
- if (m_impl->hasKey(m_keyName)) {
- BOOST_THROW_EXCEPTION(Pib::Error("Cannot overwrite existing key " + m_keyName.toUri()));
- }
-
transform::PublicKey publicKey;
try {
publicKey.loadPkcs8(key, keyLen);
}
- catch (transform::PublicKey::Error&) {
+ catch (const transform::PublicKey::Error&) {
BOOST_THROW_EXCEPTION(std::invalid_argument("Invalid key bits"));
}
m_keyType = publicKey.getKeyType();
@@ -75,11 +71,6 @@
KeyImpl::addCertificate(const v2::Certificate& certificate)
{
BOOST_ASSERT(m_certificates.isConsistent());
-
- if (m_certificates.find(certificate.getName()) != m_certificates.end()) {
- BOOST_THROW_EXCEPTION(Pib::Error("Cannot overwrite existing certificate " + certificate.getName().toUri()));
- }
-
m_certificates.add(certificate);
}
diff --git a/src/security/pib/detail/key-impl.hpp b/src/security/pib/detail/key-impl.hpp
index c05e77e..a7e683f 100644
--- a/src/security/pib/detail/key-impl.hpp
+++ b/src/security/pib/detail/key-impl.hpp
@@ -107,8 +107,10 @@
* If no default certificate is set before, the new certificate will be set as the default
* certificate of the key.
*
+ * If a certificate with the same name (without implicit digest) already exists, overwrite
+ * the certificate.
+ *
* @throw std::invalid_argument certificate name does not match key name
- * @throw Pib::Error a certificate with the same name already exists
*/
void
addCertificate(const v2::Certificate& certificate);
diff --git a/src/security/pib/identity.hpp b/src/security/pib/identity.hpp
index 2bb1706..cc46bfd 100644
--- a/src/security/pib/identity.hpp
+++ b/src/security/pib/identity.hpp
@@ -113,7 +113,8 @@
* @brief Add a @p key of @p keyLen bytes (in PKCS#8 format) with @p keyName.
* @return the handle of added key
* @throw std::invalid_argument key name does not match identity
- * @throw Pib::Error a key with the same name already exists
+ *
+ * If a key with the same name already exists, overwrite the key.
*/
Key
addKey(const uint8_t* key, size_t keyLen, const Name& keyName) const;
diff --git a/src/security/pib/key-container.cpp b/src/security/pib/key-container.cpp
index 34f96d0..4084cd3 100644
--- a/src/security/pib/key-container.cpp
+++ b/src/security/pib/key-container.cpp
@@ -120,10 +120,8 @@
"`" + m_identity.toUri() + "`"));
}
- if (m_keyNames.count(keyName) == 0) {
- m_keyNames.insert(keyName);
- m_keys[keyName] = shared_ptr<detail::KeyImpl>(new detail::KeyImpl(keyName, key, keyLen, m_impl));
- }
+ m_keyNames.insert(keyName);
+ m_keys[keyName] = shared_ptr<detail::KeyImpl>(new detail::KeyImpl(keyName, key, keyLen, m_impl));
return get(keyName);
}
diff --git a/src/security/pib/key-container.hpp b/src/security/pib/key-container.hpp
index 45d9127..0c88249 100644
--- a/src/security/pib/key-container.hpp
+++ b/src/security/pib/key-container.hpp
@@ -94,6 +94,8 @@
/**
* @brief Add @p key of @p keyLen bytes with @p keyName into the container
* @throw std::invalid_argument @p keyName does not match the identity
+ *
+ * If a key with the same name already exists, overwrite the key.
*/
Key
add(const uint8_t* key, size_t keyLen, const Name& keyName);
diff --git a/src/security/pib/key.hpp b/src/security/pib/key.hpp
index 4acc54a..90f4f5c 100644
--- a/src/security/pib/key.hpp
+++ b/src/security/pib/key.hpp
@@ -137,7 +137,9 @@
/**
* @brief Add @p certificate.
* @throw std::invalid_argument certificate name does not match key name
- * @throw Pib::Error a certificate with the same name already exists
+ *
+ * If a certificate with the same name (without implicit digest) already exists, overwrite
+ * the certificate.
*/
void
addCertificate(const v2::Certificate& certificate) const;
@@ -161,7 +163,6 @@
/**
* @brief Add @p certificate and set it as the default certificate of the key
* @throw std::invalid_argument @p certificate does not match key name
- * @throw Pib::Error the certificate with the same name already exists.
* @return the default certificate
*/
const v2::Certificate&
diff --git a/src/security/v2/key-chain.hpp b/src/security/v2/key-chain.hpp
index 7cb71f2..f0299c3 100644
--- a/src/security/v2/key-chain.hpp
+++ b/src/security/v2/key-chain.hpp
@@ -207,7 +207,6 @@
*
* @pre @p key must be valid.
* @throw std::invalid_argument @p key does not match @p certificate
- * @throw Pib::Error a certificate with the same name already exists
*/
void
addCertificate(const Key& key, const Certificate& certificate);
diff --git a/tests/unit-tests/security/pib/detail/identity-impl.t.cpp b/tests/unit-tests/security/pib/detail/identity-impl.t.cpp
index 10c7bb8..2f35d2f 100644
--- a/tests/unit-tests/security/pib/detail/identity-impl.t.cpp
+++ b/tests/unit-tests/security/pib/detail/identity-impl.t.cpp
@@ -121,6 +121,18 @@
BOOST_CHECK_THROW(identity1.getDefaultKey(), Pib::Error);
}
+BOOST_AUTO_TEST_CASE(Overwrite)
+{
+ auto pibImpl = make_shared<pib::PibMemory>();
+ IdentityImpl identity1(id1, pibImpl, true);
+
+ identity1.addKey(id1Key1.buf(), id1Key1.size(), id1Key1Name);
+ BOOST_CHECK(identity1.getKey(id1Key1Name).getPublicKey() == id1Key1);
+
+ identity1.addKey(id1Key2.buf(), id1Key2.size(), id1Key1Name); // overwriting key should work
+ BOOST_CHECK(identity1.getKey(id1Key1Name).getPublicKey() == id1Key2);
+}
+
BOOST_AUTO_TEST_CASE(Errors)
{
auto pibImpl = make_shared<pib::PibMemory>();
@@ -129,7 +141,6 @@
IdentityImpl identity1(id1, pibImpl, true);
identity1.addKey(id1Key1.buf(), id1Key1.size(), id1Key1Name);
- BOOST_CHECK_THROW(identity1.addKey(id1Key1.buf(), id1Key1.size(), id1Key1Name), Pib::Error);
BOOST_CHECK_THROW(identity1.addKey(id2Key1.buf(), id2Key1.size(), id2Key1Name), std::invalid_argument);
BOOST_CHECK_THROW(identity1.removeKey(id2Key1Name), std::invalid_argument);
BOOST_CHECK_THROW(identity1.getKey(id2Key1Name), std::invalid_argument);
diff --git a/tests/unit-tests/security/pib/detail/key-impl.t.cpp b/tests/unit-tests/security/pib/detail/key-impl.t.cpp
index b25a870..5257113 100644
--- a/tests/unit-tests/security/pib/detail/key-impl.t.cpp
+++ b/tests/unit-tests/security/pib/detail/key-impl.t.cpp
@@ -25,6 +25,7 @@
#include "../pib-data-fixture.hpp"
#include "boost-test.hpp"
+#include "identity-management-fixture.hpp"
namespace ndn {
namespace security {
@@ -130,13 +131,49 @@
BOOST_CHECK_EQUAL(key11.getCertificates().size(), 0);
}
+class OverwriteFixture : public ndn::security::tests::PibDataFixture,
+ public ndn::tests::IdentityManagementFixture
+{
+};
+
+BOOST_FIXTURE_TEST_CASE(Overwrite, OverwriteFixture)
+{
+ auto pibImpl = make_shared<pib::PibMemory>();
+
+ BOOST_CHECK_THROW(KeyImpl(id1Key1Name, pibImpl), Pib::Error);
+ KeyImpl(id1Key1Name, id1Key1.buf(), id1Key1.size(), pibImpl);
+ KeyImpl key1(id1Key1Name, pibImpl);
+
+ KeyImpl(id1Key1Name, id1Key2.buf(), id1Key2.size(), pibImpl); // overwriting of the key should work
+ KeyImpl key2(id1Key1Name, pibImpl);
+
+ BOOST_CHECK(key1.getPublicKey() != key2.getPublicKey()); // key1 cached the original public key
+ BOOST_CHECK(key2.getPublicKey() == id1Key2);
+
+ key1.addCertificate(id1Key1Cert1);
+ BOOST_CHECK_EQUAL(key1.getCertificate(id1Key1Cert1.getName()), id1Key1Cert1);
+
+ auto otherCert = id1Key1Cert1;
+ SignatureInfo info;
+ info.setValidityPeriod(ValidityPeriod(time::system_clock::now(),
+ time::system_clock::now() + time::seconds(1)));
+ m_keyChain.sign(otherCert, SigningInfo().setSignatureInfo(info));
+
+ BOOST_CHECK_EQUAL(otherCert.getName(), id1Key1Cert1.getName());
+ BOOST_CHECK(otherCert.getContent() == id1Key1Cert1.getContent());
+ BOOST_CHECK_NE(otherCert, id1Key1Cert1);
+
+ key1.addCertificate(otherCert);
+
+ BOOST_CHECK_EQUAL(key1.getCertificate(id1Key1Cert1.getName()), otherCert);
+}
+
BOOST_AUTO_TEST_CASE(Errors)
{
auto pibImpl = make_shared<pib::PibMemory>();
BOOST_CHECK_THROW(KeyImpl(id1Key1Name, pibImpl), Pib::Error);
KeyImpl key11(id1Key1Name, id1Key1.buf(), id1Key1.size(), pibImpl);
- BOOST_CHECK_THROW(KeyImpl(id1Key1Name, id1Key1.buf(), id1Key1.size(), pibImpl), Pib::Error);
BOOST_CHECK_THROW(KeyImpl(Name("/wrong"), pibImpl), std::invalid_argument);
BOOST_CHECK_THROW(KeyImpl(Name("/wrong"), id1Key1.buf(), id1Key1.size(), pibImpl), std::invalid_argument);
@@ -144,7 +181,6 @@
BOOST_CHECK_THROW(KeyImpl(id1Key2Name, wrongKey.buf(), wrongKey.size(), pibImpl), std::invalid_argument);
key11.addCertificate(id1Key1Cert1);
- BOOST_CHECK_THROW(key11.addCertificate(id1Key1Cert1), Pib::Error);
BOOST_CHECK_THROW(key11.addCertificate(id1Key2Cert1), std::invalid_argument);
BOOST_CHECK_THROW(key11.removeCertificate(id1Key2Cert1.getName()), std::invalid_argument);
BOOST_CHECK_THROW(key11.getCertificate(id1Key2Cert1.getName()), std::invalid_argument);
diff --git a/tests/unit-tests/security/v2/key-chain.t.cpp b/tests/unit-tests/security/v2/key-chain.t.cpp
index 950b97e..4eaf75e 100644
--- a/tests/unit-tests/security/v2/key-chain.t.cpp
+++ b/tests/unit-tests/security/v2/key-chain.t.cpp
@@ -228,8 +228,7 @@
m_keyChain.addCertificate(key3, key3Cert1);
BOOST_CHECK_EQUAL(key3.getCertificates().size(), 1);
BOOST_REQUIRE_NO_THROW(key3.getDefaultCertificate());
- // Overwrite the same cert again, should throw Pib::Error.
- BOOST_REQUIRE_THROW(m_keyChain.addCertificate(key3, key3Cert1), Pib::Error);
+ m_keyChain.addCertificate(key3, key3Cert1); // overwriting the cert should work
BOOST_CHECK_EQUAL(key3.getCertificates().size(), 1);
// Add another cert
Certificate key3Cert2 = key3Cert1;