security: return a span from Certificate::getPublicKey()
Also, tighten the FreshnessPeriod check in the constructor
and expand test coverage of various functions
Change-Id: I70837e20bbea8f1174baf2b252f39894e5346c34
diff --git a/ndn-cxx/security/certificate.cpp b/ndn-cxx/security/certificate.cpp
index 3060fd3..fdce8ad 100644
--- a/ndn-cxx/security/certificate.cpp
+++ b/ndn-cxx/security/certificate.cpp
@@ -33,6 +33,8 @@
BOOST_CONCEPT_ASSERT((WireEncodable<Certificate>));
BOOST_CONCEPT_ASSERT((WireDecodable<Certificate>));
+static_assert(std::is_base_of<Data::Error, Certificate::Error>::value,
+ "Certificate::Error must inherit from Data::Error");
// /<IdentityName>/KEY/<KeyId>/<IssuerId>/<Version>
const ssize_t Certificate::VERSION_OFFSET = -1;
@@ -47,22 +49,20 @@
Certificate::Certificate()
{
setContentType(tlv::ContentType_Key);
+ setFreshnessPeriod(1_h);
}
Certificate::Certificate(Data&& data)
: Data(std::move(data))
{
if (!isValidName(getName())) {
- NDN_THROW(Data::Error("Name does not follow the naming convention for certificate"));
+ NDN_THROW(Error("Certificate name does not follow the naming conventions"));
}
if (getContentType() != tlv::ContentType_Key) {
- NDN_THROW(Data::Error("Expecting ContentType Key, got " + to_string(getContentType())));
+ NDN_THROW(Error("Expecting ContentType=Key, got " + to_string(getContentType())));
}
- if (getFreshnessPeriod() < time::seconds::zero()) {
- NDN_THROW(Data::Error("FreshnessPeriod is not set"));
- }
- if (getContent().value_size() == 0) {
- NDN_THROW(Data::Error("Content is empty"));
+ if (getFreshnessPeriod() <= 0_ms) {
+ NDN_THROW(Error("Certificate FreshnessPeriod cannot be zero"));
}
}
@@ -77,17 +77,17 @@
}
Name
-Certificate::getKeyName() const
-{
- return getName().getPrefix(KEY_ID_OFFSET + 1);
-}
-
-Name
Certificate::getIdentity() const
{
return getName().getPrefix(KEY_COMPONENT_OFFSET);
}
+Name
+Certificate::getKeyName() const
+{
+ return getName().getPrefix(KEY_ID_OFFSET + 1);
+}
+
name::Component
Certificate::getKeyId() const
{
@@ -100,15 +100,6 @@
return getName().at(ISSUER_ID_OFFSET);
}
-Buffer
-Certificate::getPublicKey() const
-{
- if (getContent().value_size() == 0)
- NDN_THROW(Data::Error("Certificate Content is empty"));
-
- return {getContent().value_begin(), getContent().value_end()};
-}
-
ValidityPeriod
Certificate::getValidityPeriod() const
{
@@ -172,11 +163,11 @@
os << key.getKeySize() << "-bit " << key.getKeyType();
}
catch (const std::runtime_error&) {
- os << "Unknown (" << cert.getContent().value_size() << " bytes)";
+ os << "Unknown (" << cert.getPublicKey().size() << " bytes)";
}
os << "\n";
- if (cert.getContent().value_size() > 0) {
+ if (!cert.getPublicKey().empty()) {
util::IndentedStream os2(os, " ");
bufferSource(cert.getPublicKey()) >> base64Encode() >> streamSink(os2);
}
diff --git a/ndn-cxx/security/certificate.hpp b/ndn-cxx/security/certificate.hpp
index dbd3f75..16b2c17 100644
--- a/ndn-cxx/security/certificate.hpp
+++ b/ndn-cxx/security/certificate.hpp
@@ -43,7 +43,6 @@
* Key Name
* @endcode
*
- * Notes:
* - `KeyId` is an opaque name component to identify an instance of the public key for the
* certificate namespace. The value of KeyId is controlled by the namespace owner. The
* library includes helpers for generation of key IDs using 8-byte random number, SHA-256
@@ -52,14 +51,20 @@
* value is controlled by the issuer. The library includes helpers to set issuer ID to a
* 8-byte random number, SHA-256 digest of the issuer's public key, or a specified numerical
* identifier.
- * - `Key %Name` is a logical name of the key used for management purposes. The key name
+ * - `Key %Name` is the logical name of the key used for management purposes. The key name
* includes the identity name, the keyword `KEY`, and the `KeyId` component.
*
- * @see doc/specs/certificate.rst
+ * @sa <a href="../specs/certificate.html">Certificate Format</a>
*/
class Certificate : public Data
{
public:
+ class Error : public Data::Error
+ {
+ public:
+ using Data::Error::Error;
+ };
+
Certificate();
/**
@@ -84,18 +89,18 @@
Certificate(const Block& block);
/**
- * @brief Get key name
- */
- Name
- getKeyName() const;
-
- /**
* @brief Get identity name
*/
Name
getIdentity() const;
/**
+ * @brief Get key name
+ */
+ Name
+ getKeyName() const;
+
+ /**
* @brief Get key ID
*/
name::Component
@@ -108,11 +113,14 @@
getIssuerId() const;
/**
- * @brief Get public key bits (in PKCS#8 format)
- * @throw Error If content is empty
+ * @brief Return the public key as a DER-encoded SubjectPublicKeyInfo structure,
+ * i.e., exactly as it appears in the serialized certificate.
*/
- Buffer
- getPublicKey() const;
+ span<const uint8_t>
+ getPublicKey() const noexcept
+ {
+ return getContent().value_bytes();
+ }
/**
* @brief Get validity period of the certificate
diff --git a/ndn-cxx/security/key-chain.cpp b/ndn-cxx/security/key-chain.cpp
index b412153..f65b286 100644
--- a/ndn-cxx/security/key-chain.cpp
+++ b/ndn-cxx/security/key-chain.cpp
@@ -313,13 +313,10 @@
{
BOOST_ASSERT(static_cast<bool>(key));
- const auto& certContent = certificate.getContent();
- if (certContent.value_size() == 0) {
- NDN_THROW(std::invalid_argument("Certificate `" + certificate.getName().toUri() + "` is empty"));
- }
-
+ auto pk = key.getPublicKey();
+ auto pkCert = certificate.getPublicKey();
if (key.getName() != certificate.getKeyName() ||
- !std::equal(certContent.value_begin(), certContent.value_end(), key.getPublicKey().begin())) {
+ !std::equal(pk.begin(), pk.end(), pkCert.begin(), pkCert.end())) {
NDN_THROW(std::invalid_argument("Key `" + key.getName().toUri() + "` "
"does not match certificate `" + certificate.getName().toUri() + "`"));
}
@@ -372,7 +369,6 @@
Certificate cert(std::move(certData));
Name identity = cert.getIdentity();
Name keyName = cert.getKeyName();
- const auto publicKeyBits = cert.getPublicKey();
if (m_tpm->hasKey(keyName)) {
NDN_THROW(Error("Private key `" + keyName.toUri() + "` already exists"));
@@ -408,7 +404,7 @@
{
using namespace transform;
PublicKey publicKey;
- publicKey.loadPkcs8(publicKeyBits);
+ publicKey.loadPkcs8(cert.getPublicKey());
bufferSource(content) >> verifierFilter(DigestAlgorithm::SHA256, publicKey, *sigBits)
>> boolSink(isVerified);
}
@@ -497,7 +493,7 @@
KeyChain::makeCertificate(const Certificate& certRequest, const SigningInfo& params,
const MakeCertificateOptions& opts)
{
- auto pkcs8 = certRequest.getContent().value_bytes();
+ auto pkcs8 = certRequest.getPublicKey();
try {
transform::PublicKey pub;
pub.loadPkcs8(pkcs8);
@@ -586,11 +582,8 @@
SigningInfo params, const MakeCertificateOptions& opts)
{
if (opts.freshnessPeriod <= 0_ms) {
- // Certificate format requires FreshnessPeriod field to appear in the packet.
- // We cannot rely on Certificate constructor to check this, because:
- // (1) Metadata::wireEncode omits zero FreshnessPeriod in the wire encoding, but
- // Certificate constructor does not throw in this condition
- // (2) Certificate constructor throws Data::Error, not a std::invalid_argument
+ // We cannot rely on Certificate constructor to check this, because
+ // it throws Certificate::Error, not std::invalid_argument
NDN_THROW(std::invalid_argument("FreshnessPeriod is not positive"));
}
diff --git a/ndn-cxx/security/pib/impl/pib-memory.cpp b/ndn-cxx/security/pib/impl/pib-memory.cpp
index 4ccdb1f..973d9d6 100644
--- a/ndn-cxx/security/pib/impl/pib-memory.cpp
+++ b/ndn-cxx/security/pib/impl/pib-memory.cpp
@@ -201,7 +201,7 @@
const Name& keyName = certificate.getKeyName();
// ensure key exists
- addKey(certificate.getIdentity(), keyName, certificate.getContent().value_bytes());
+ addKey(certificate.getIdentity(), keyName, certificate.getPublicKey());
m_certs[certName] = certificate;
if (m_defaultCerts.count(keyName) == 0) {
diff --git a/ndn-cxx/security/pib/impl/pib-sqlite3.cpp b/ndn-cxx/security/pib/impl/pib-sqlite3.cpp
index 0d08fd8..514f6de 100644
--- a/ndn-cxx/security/pib/impl/pib-sqlite3.cpp
+++ b/ndn-cxx/security/pib/impl/pib-sqlite3.cpp
@@ -474,7 +474,7 @@
PibSqlite3::addCertificate(const Certificate& certificate)
{
// ensure key exists
- addKey(certificate.getIdentity(), certificate.getKeyName(), certificate.getContent().value_bytes());
+ addKey(certificate.getIdentity(), certificate.getKeyName(), certificate.getPublicKey());
if (!hasCertificate(certificate.getName())) {
Sqlite3Statement statement(m_database,
diff --git a/ndn-cxx/security/verification-helpers.cpp b/ndn-cxx/security/verification-helpers.cpp
index 8e342ac..89b4427 100644
--- a/ndn-cxx/security/verification-helpers.cpp
+++ b/ndn-cxx/security/verification-helpers.cpp
@@ -219,7 +219,7 @@
{
auto parsed = parse(data);
if (cert) {
- return verifySignature(parsed, cert->getContent().value_bytes());
+ return verifySignature(parsed, cert->getPublicKey());
}
else if (parsed.info.getSignatureType() == tlv::SignatureTypeValue::DigestSha256) {
return verifyDigest(parsed, DigestAlgorithm::SHA256);
@@ -235,7 +235,7 @@
{
auto parsed = parse(interest);
if (cert) {
- return verifySignature(parsed, cert->getContent().value_bytes());
+ return verifySignature(parsed, cert->getPublicKey());
}
else if (parsed.info.getSignatureType() == tlv::SignatureTypeValue::DigestSha256) {
return verifyDigest(parsed, DigestAlgorithm::SHA256);
diff --git a/tests/unit/security/certificate.t.cpp b/tests/unit/security/certificate.t.cpp
index a9df438..f113ca9 100644
--- a/tests/unit/security/certificate.t.cpp
+++ b/tests/unit/security/certificate.t.cpp
@@ -141,18 +141,20 @@
{
Block block(CERT);
Certificate certificate(block);
+ const ValidityPeriod vp(time::fromIsoString("20150814T223739"),
+ time::fromIsoString("20150818T223738"));
BOOST_CHECK_EQUAL(certificate.getName(), "/ndn/site1/KEY/ksk-1416425377094/0123/%FD%00%00%01I%C9%8B");
BOOST_CHECK_EQUAL(certificate.getKeyName(), "/ndn/site1/KEY/ksk-1416425377094");
BOOST_CHECK_EQUAL(certificate.getIdentity(), "/ndn/site1");
- BOOST_CHECK_EQUAL(certificate.getIssuerId(), name::Component("0123"));
BOOST_CHECK_EQUAL(certificate.getKeyId(), name::Component("ksk-1416425377094"));
+ BOOST_CHECK_EQUAL(certificate.getIssuerId(), name::Component("0123"));
+ BOOST_TEST(certificate.getPublicKey() == PUBLIC_KEY, boost::test_tools::per_element());
BOOST_CHECK_EQUAL(certificate.getKeyLocator().value().getName(), "/ndn/site1/KEY/ksk-2516425377094");
- BOOST_CHECK_EQUAL(boost::lexical_cast<std::string>(certificate.getValidityPeriod()),
- "(20150814T223739, 20150818T223738)");
+ BOOST_CHECK_EQUAL(certificate.getValidityPeriod(), vp);
- BOOST_CHECK_THROW(certificate.getExtension(12345), Data::Error);
- BOOST_CHECK_NO_THROW(certificate.getPublicKey());
+ BOOST_CHECK_EQUAL(certificate.getExtension(tlv::ValidityPeriod), vp.wireEncode());
+ BOOST_CHECK_THROW(certificate.getExtension(12345), tlv::Error);
Data data(block);
Certificate certificate2(std::move(data));
@@ -167,17 +169,20 @@
certificate.setContent(PUBLIC_KEY);
generateFakeSignature(certificate);
+ const ValidityPeriod vp(time::fromIsoString("20141111T050000"),
+ time::fromIsoString("20141111T060000"));
+
BOOST_CHECK_EQUAL(certificate.getName(), "/ndn/site1/KEY/ksk-1416425377094/0123/%FD%00%00%01I%C9%8B");
BOOST_CHECK_EQUAL(certificate.getKeyName(), "/ndn/site1/KEY/ksk-1416425377094");
BOOST_CHECK_EQUAL(certificate.getIdentity(), "/ndn/site1");
- BOOST_CHECK_EQUAL(certificate.getIssuerId(), name::Component("0123"));
BOOST_CHECK_EQUAL(certificate.getKeyId(), name::Component("ksk-1416425377094"));
+ BOOST_CHECK_EQUAL(certificate.getIssuerId(), name::Component("0123"));
+ BOOST_TEST(certificate.getPublicKey() == PUBLIC_KEY, boost::test_tools::per_element());
BOOST_CHECK_EQUAL(certificate.getKeyLocator().value().getName(), "/ndn/site1/KEY/ksk-2516425377094");
- BOOST_CHECK_EQUAL(boost::lexical_cast<std::string>(certificate.getValidityPeriod()),
- "(20141111T050000, 20141111T060000)");
+ BOOST_CHECK_EQUAL(certificate.getValidityPeriod(), vp);
- BOOST_CHECK_THROW(certificate.getExtension(12345), Data::Error);
- BOOST_CHECK_NO_THROW(certificate.getPublicKey());
+ BOOST_CHECK_EQUAL(certificate.getExtension(tlv::ValidityPeriod), vp.wireEncode());
+ BOOST_CHECK_THROW(certificate.getExtension(12345), tlv::Error);
}
BOOST_AUTO_TEST_CASE(ValidityPeriodChecking)
@@ -216,35 +221,30 @@
{
Data data(m_certBase);
data.setName("/ndn/site1/ksk-1416425377094/0123/%FD%00%00%01I%C9%8B");
- generateFakeSignature(data);
- BOOST_CHECK_THROW((Certificate(data)), Certificate::Error);
- BOOST_CHECK_THROW((Certificate(std::move(data))), Certificate::Error);
+ BOOST_CHECK_EXCEPTION(Certificate{std::move(data)}, Certificate::Error, [] (const auto& e) {
+ return e.what() == "Certificate name does not follow the naming conventions"s;
+ });
}
-BOOST_FIXTURE_TEST_CASE(InvalidType, InvalidCertFixture)
+BOOST_FIXTURE_TEST_CASE(InvalidContentType, InvalidCertFixture)
{
Data data(m_certBase);
data.setContentType(tlv::ContentType_Blob);
- generateFakeSignature(data);
- BOOST_CHECK_THROW((Certificate(data)), Certificate::Error);
- BOOST_CHECK_THROW((Certificate(std::move(data))), Certificate::Error);
+ BOOST_CHECK_EXCEPTION(Certificate{std::move(data)}, Certificate::Error, [] (const auto& e) {
+ return e.what() == "Expecting ContentType=Key, got 0"s;
+ });
}
-BOOST_FIXTURE_TEST_CASE(EmptyContent, InvalidCertFixture)
+BOOST_FIXTURE_TEST_CASE(InvalidFreshnessPeriod, InvalidCertFixture)
{
Data data(m_certBase);
- data.setContent(span<uint8_t>{});
- generateFakeSignature(data);
+ data.setFreshnessPeriod(0_ms);
- BOOST_CHECK_THROW(Certificate{data}, Certificate::Error);
- BOOST_CHECK_THROW(Certificate{std::move(data)}, Certificate::Error);
-
- Certificate cert(m_certBase);
- cert.setContent(span<uint8_t>{});
- generateFakeSignature(cert);
- BOOST_CHECK_THROW(cert.getPublicKey(), Certificate::Error);
+ BOOST_CHECK_EXCEPTION(Certificate{std::move(data)}, Certificate::Error, [] (const auto& e) {
+ return e.what() == "Certificate FreshnessPeriod cannot be zero"s;
+ });
}
BOOST_AUTO_TEST_CASE(Print)
@@ -372,6 +372,25 @@
BOOST_CHECK_EQUAL(boost::lexical_cast<std::string>(cert6), expected6);
}
+BOOST_AUTO_TEST_CASE(Helpers)
+{
+ BOOST_CHECK_EQUAL(extractIdentityFromCertName("/KEY/hello/world/v=1"), "/");
+ BOOST_CHECK_EQUAL(extractIdentityFromCertName("/hello/world/KEY/!/self/v=42"), "/hello/world");
+
+ BOOST_CHECK_THROW(extractIdentityFromCertName("/hello"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractIdentityFromCertName("/hello/KEY/keyid"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractIdentityFromCertName("/hello/KEY/keyid/issuer"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractIdentityFromCertName("/a/long/enough/but/invalid/name"), std::invalid_argument);
+
+ BOOST_CHECK_EQUAL(extractKeyNameFromCertName("/KEY/hello/world/v=1"), "/KEY/hello");
+ BOOST_CHECK_EQUAL(extractKeyNameFromCertName("/hello/world/KEY/!/self/v=42"), "/hello/world/KEY/!");
+
+ BOOST_CHECK_THROW(extractKeyNameFromCertName("/hello"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractKeyNameFromCertName("/hello/KEY/keyid"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractKeyNameFromCertName("/hello/KEY/keyid/issuer"), std::invalid_argument);
+ BOOST_CHECK_THROW(extractKeyNameFromCertName("/a/long/enough/but/invalid/name"), std::invalid_argument);
+}
+
BOOST_AUTO_TEST_SUITE_END() // TestCertificate
BOOST_AUTO_TEST_SUITE_END() // Security
diff --git a/tests/unit/security/detail/certificate-bundle-decoder.t.cpp b/tests/unit/security/detail/certificate-bundle-decoder.t.cpp
index 1199994..a9fe998 100644
--- a/tests/unit/security/detail/certificate-bundle-decoder.t.cpp
+++ b/tests/unit/security/detail/certificate-bundle-decoder.t.cpp
@@ -156,7 +156,7 @@
BOOST_CHECK_EQUAL(nCertsCompleted, 1);
BOOST_CHECK_EXCEPTION(cbd.append(d2.getContent()), tlv::Error, [] (const auto& e) {
- return e.what() == "Name does not follow the naming convention for certificate"s;
+ return e.what() == "Certificate name does not follow the naming conventions"s;
});
BOOST_CHECK_EQUAL(cbd.hasError(), true);
BOOST_CHECK_EQUAL(nCertsCompleted, 1);
diff --git a/tests/unit/security/key-chain.t.cpp b/tests/unit/security/key-chain.t.cpp
index b3ea456..ba84081 100644
--- a/tests/unit/security/key-chain.t.cpp
+++ b/tests/unit/security/key-chain.t.cpp
@@ -611,8 +611,7 @@
BOOST_CHECK_EQUAL(cert.getContentType(), tlv::ContentType_Key);
BOOST_CHECK_EQUAL(cert.getFreshnessPeriod(), 1_h);
- BOOST_TEST(cert.getContent().value_bytes() == requester.getPublicKey(),
- boost::test_tools::per_element());
+ BOOST_TEST(cert.getPublicKey() == requester.getPublicKey(), boost::test_tools::per_element());
checkKeyLocatorName(cert);
diff --git a/tests/unit/security/pib/pib-data-fixture.cpp b/tests/unit/security/pib/pib-data-fixture.cpp
index c160042..488d11e 100644
--- a/tests/unit/security/pib/pib-data-fixture.cpp
+++ b/tests/unit/security/pib/pib-data-fixture.cpp
@@ -374,10 +374,10 @@
, id2Key1Name(id2Key1Cert1.getKeyName())
, id2Key2Name(id2Key2Cert1.getKeyName())
- , id1Key1(id1Key1Cert1.getPublicKey())
- , id1Key2(id1Key2Cert1.getPublicKey())
- , id2Key1(id2Key1Cert1.getPublicKey())
- , id2Key2(id2Key2Cert1.getPublicKey())
+ , id1Key1(id1Key1Cert1.getPublicKey().begin(), id1Key1Cert1.getPublicKey().end())
+ , id1Key2(id1Key2Cert1.getPublicKey().begin(), id1Key2Cert1.getPublicKey().end())
+ , id2Key1(id2Key1Cert1.getPublicKey().begin(), id2Key1Cert1.getPublicKey().end())
+ , id2Key2(id2Key2Cert1.getPublicKey().begin(), id2Key2Cert1.getPublicKey().end())
{
BOOST_ASSERT(id1Key1Cert2.getIdentity() == id1);
BOOST_ASSERT(id1Key2Cert1.getIdentity() == id1);
@@ -391,10 +391,10 @@
BOOST_ASSERT(id2Key1Cert2.getKeyName() == id2Key1Name);
BOOST_ASSERT(id2Key2Cert2.getKeyName() == id2Key2Name);
- BOOST_ASSERT(id1Key1Cert2.getPublicKey() == id1Key1);
- BOOST_ASSERT(id1Key2Cert2.getPublicKey() == id1Key2);
- BOOST_ASSERT(id2Key1Cert2.getPublicKey() == id2Key1);
- BOOST_ASSERT(id2Key2Cert2.getPublicKey() == id2Key2);
+ BOOST_ASSERT(id1Key1Cert2.getContent() == id1Key1Cert1.getContent());
+ BOOST_ASSERT(id1Key2Cert2.getContent() == id1Key2Cert1.getContent());
+ BOOST_ASSERT(id2Key1Cert2.getContent() == id2Key1Cert1.getContent());
+ BOOST_ASSERT(id2Key2Cert2.getContent() == id2Key2Cert1.getContent());
}
shared_ptr<PibImpl>