Support uploading multiple certificates per authority service.
This request is a proposition of extending current mode=authority
with support for multiple certificates instead of single one.
This review addresses also this issue:
https://github.com/openbmc/phosphor-certificate-manager/issues/3
but with a restriction to mode=authority. Other modes still operates
on a single certification file.
New mode requires that user provides directory path instead of certificate path
as --path argument if using --type=authority.
Tested:
- Manually tested Install, Remove and Replace paths for existing modes
to confirm no change of behavior occurs (authority, client, server)
- Manually tested Install, Remove and Replace paths for authority mode
to confirm that it behaves as expected i.e. filename is changed on certificate
replacement that mirrors change in certificate hash
- Confirmed no regression in unit tests
Change-Id: Icd33723c1fc2580679aaaf54b3e99dfb09342402
Signed-off-by: Kowalski, Kamil <kamil.kowalski@intel.com>
Signed-off-by: Zbigniew Kurzynski <zbigniew.kurzynski@intel.com>
diff --git a/certificate.cpp b/certificate.cpp
index 0b4b0ed..a070e07 100644
--- a/certificate.cpp
+++ b/certificate.cpp
@@ -69,6 +69,18 @@
{NID_ad_timeStamping, "Timestamping"},
{NID_code_sign, "CodeSigning"}};
+std::string Certificate::getSubjectHash(const X509_STORE_CTX_Ptr& storeCtx)
+{
+ X509_Ptr cert(X509_STORE_CTX_get_current_cert(storeCtx.get()), ::X509_free);
+ unsigned long hash = X509_subject_name_hash(cert.get());
+
+ char hashBuf[9];
+
+ sprintf(hashBuf, "%08lx", hash);
+
+ return std::string(hashBuf);
+}
+
Certificate::Certificate(sdbusplus::bus::bus& bus, const std::string& objPath,
const CertificateType& type,
const UnitsToRestart& unit,
@@ -107,10 +119,17 @@
Certificate::~Certificate()
{
- if (!fs::remove(certInstallPath))
+ std::string installPath = certInstallPath;
+
+ if (certType == phosphor::certs::AUTHORITY)
+ {
+ installPath += "/" + certHash + ".0";
+ }
+
+ if (!fs::remove(installPath))
{
log<level::INFO>("Certificate file not found!",
- entry("PATH=%s", certInstallPath.c_str()));
+ entry("PATH=%s", installPath.c_str()));
}
else if (!unitToRestart.empty())
{
@@ -270,10 +289,30 @@
}
compIter->second(filePath);
+ std::string newHash = getSubjectHash(storeCtx);
+ std::string installPath = certInstallPath;
+
+ if (certType == phosphor::certs::AUTHORITY)
+ {
+ // Save under OpenSSL lib acceptable name
+ installPath += "/" + newHash + ".0";
+
+ // Check if we are not trying to overwrite already existing certificate
+ if (certHash != newHash && fs::exists(installPath) &&
+ filePath != installPath)
+ {
+ using NotAllowed =
+ sdbusplus::xyz::openbmc_project::Common::Error::NotAllowed;
+ using Reason = xyz::openbmc_project::Common::NotAllowed::REASON;
+
+ elog<NotAllowed>(Reason("Certificate already exist"));
+ }
+ }
+
// Copy the certificate to the installation path
// During bootup will be parsing existing file so no need to
// copy it.
- if (filePath != certInstallPath)
+ if (filePath != installPath)
{
std::ifstream inputCertFileStream;
std::ofstream outputCertFileStream;
@@ -283,10 +322,11 @@
outputCertFileStream.exceptions(std::ofstream::failbit |
std::ofstream::badbit |
std::ofstream::eofbit);
+
try
{
inputCertFileStream.open(filePath);
- outputCertFileStream.open(certInstallPath, std::ios::out);
+ outputCertFileStream.open(installPath, std::ios::out);
outputCertFileStream << inputCertFileStream.rdbuf() << std::flush;
inputCertFileStream.close();
outputCertFileStream.close();
@@ -296,9 +336,28 @@
log<level::ERR>("Failed to copy certificate",
entry("ERR=%s", e.what()),
entry("SRC=%s", filePath.c_str()),
- entry("DST=%s", certInstallPath.c_str()));
+ entry("DST=%s", installPath.c_str()));
elog<InternalFailure>();
}
+
+ if (certHash != newHash && !certHash.empty() &&
+ certType == phosphor::certs::AUTHORITY)
+ {
+ std::string oldPath = certInstallPath + "/" + certHash + ".0";
+
+ // Remove previous file
+ try
+ {
+ fs::remove(oldPath);
+ }
+ catch (const std::exception& e)
+ {
+ log<level::ERR>("Failed to remove old certificate",
+ entry("ERR=%s", e.what()),
+ entry("OLD=%s", oldPath.c_str()),
+ entry("NEW=%s", installPath.c_str()));
+ }
+ }
}
if (!isSkipUnitReload)
@@ -310,8 +369,11 @@
}
}
+ // Store current hash
+ certHash = newHash;
+
// Parse the certificate file and populate properties
- populateProperties();
+ populateProperties(installPath);
// restart watch
if (certWatchPtr)
@@ -322,7 +384,17 @@
void Certificate::populateProperties()
{
- X509_Ptr cert = std::move(loadCert(certInstallPath));
+ populateProperties(certInstallPath);
+}
+
+const std::string& Certificate::getHash() const
+{
+ return certHash;
+}
+
+void Certificate::populateProperties(const std::string& certPath)
+{
+ X509_Ptr cert = std::move(loadCert(certPath));
// Update properties if no error thrown
BIO_MEM_Ptr certBio(BIO_new(BIO_s_mem()), BIO_free);
PEM_write_bio_X509(certBio.get(), cert.get());
diff --git a/certificate.hpp b/certificate.hpp
index aea3ce6..56256ac 100644
--- a/certificate.hpp
+++ b/certificate.hpp
@@ -40,6 +40,8 @@
// RAII support for openSSL functions.
using X509_Ptr = std::unique_ptr<X509, decltype(&::X509_free)>;
+using X509_STORE_CTX_Ptr =
+ std::unique_ptr<X509_STORE_CTX, decltype(&::X509_STORE_CTX_free)>;
/** @class Certificate
* @brief OpenBMC Certificate entry implementation.
@@ -83,7 +85,23 @@
*/
void populateProperties();
+ /**
+ * @brief Obtain certificate's subject hash
+ *
+ * @return certificate's subject hash
+ */
+ const std::string& getHash() const;
+
private:
+ /**
+ * @brief Populate certificate properties by parsing given certificate file
+ *
+ * @param[in] certPath Path to certificate that should be parsed
+ *
+ * @return void
+ */
+ void populateProperties(const std::string& certPath);
+
/** @brief Validate and Replace/Install the certificate file
* Install/Replace the existing certificate file with another
* (possibly CA signed) Certificate file.
@@ -121,6 +139,16 @@
*/
void reloadOrReset(const UnitsToRestart& unit);
+ /**
+ * @brief Extracts subject hash
+ *
+ * @param[in] storeCtx Pointer to X509_STORE_CTX containing certificate
+ *
+ * @return Subject hash as formatted string
+ */
+ static inline std::string
+ getSubjectHash(const X509_STORE_CTX_Ptr& storeCtx);
+
/** @brief Type specific function pointer map **/
std::unordered_map<InputType, InstallFunc> typeFuncMap;
@@ -144,6 +172,9 @@
/** @brief Certificate file create/update watch */
const CertWatchPtr& certWatchPtr;
+
+ /** @brief Stores certificate subject hash */
+ std::string certHash;
};
} // namespace certs
diff --git a/certs_manager.cpp b/certs_manager.cpp
index d2d9342..76ce09f 100644
--- a/certs_manager.cpp
+++ b/certs_manager.cpp
@@ -54,10 +54,7 @@
}
// restore any existing certificates
- if (fs::exists(certInstallPath))
- {
- createCertificate();
- }
+ createCertificates();
// watch is not required for authority certificates
if (certType != AUTHORITY)
@@ -68,17 +65,17 @@
try
{
// if certificate file existing update it
- if (certificatePtr != nullptr)
+ if (!installedCerts.empty())
{
log<level::INFO>(
"Inotify callback to update certificate properties");
- certificatePtr->populateProperties();
+ installedCerts[0]->populateProperties();
}
else
{
log<level::INFO>(
"Inotify callback to create certificate object");
- createCertificate();
+ createCertificates();
}
}
catch (const InternalFailure& e)
@@ -91,6 +88,24 @@
}
});
}
+ else
+ {
+ const std::string signleCertPath = "/etc/ssl/certs/Root-CA.pem";
+ if (fs::exists(signleCertPath) && !fs::is_empty(signleCertPath))
+ {
+ log<level::NOTICE>(
+ "Legacy certificate detected, will be installed from: ",
+ entry("SINGLE_CERTPATH=%s", signleCertPath.c_str()));
+ install(signleCertPath);
+ if (!fs::remove(signleCertPath))
+ {
+ log<level::ERR>(
+ "Unable to remove old certificate from: ",
+ entry("SINGLE_CERTPATH=%s", signleCertPath.c_str()));
+ elog<InternalFailure>();
+ }
+ }
+ }
}
std::string Manager::install(const std::string filePath)
@@ -98,18 +113,17 @@
using NotAllowed =
sdbusplus::xyz::openbmc_project::Common::Error::NotAllowed;
using Reason = xyz::openbmc_project::Common::NotAllowed::REASON;
- // TODO: Issue#3 At present supporting only one certificate to be
- // uploaded this need to be revisited to support multiple
- // certificates
- if (certificatePtr != nullptr)
+
+ if (certType != phosphor::certs::AUTHORITY && !installedCerts.empty())
{
elog<NotAllowed>(Reason("Certificate already exist"));
}
- auto certObjectPath = objectPath + '/' + '1';
- certificatePtr = std::make_unique<Certificate>(
+ auto certObjectPath = objectPath + '/' + std::to_string(certIdCounter++);
+
+ installedCerts.emplace_back(std::make_unique<Certificate>(
bus, certObjectPath, certType, unitToRestart, certInstallPath, filePath,
- false, certWatchPtr);
+ false, certWatchPtr));
return certObjectPath;
}
@@ -120,10 +134,7 @@
// certificate object for the auto-generated certificate file as
// deletion if only applicable for REST server and Bmcweb does not allow
// deletion of certificates
- if (certificatePtr != nullptr)
- {
- certificatePtr.reset(nullptr);
- }
+ installedCerts.clear();
}
std::string Manager::generateCSR(
@@ -214,9 +225,9 @@
return csrObjectPath;
}
-CertificatePtr& Manager::getCertificate()
+std::vector<std::unique_ptr<Certificate>>& Manager::getCertificates()
{
- return certificatePtr;
+ return installedCerts;
}
void Manager::generateCSRHelper(
@@ -521,26 +532,61 @@
std::fclose(fp);
}
-void Manager::createCertificate()
+void Manager::createCertificates()
{
- try
+ auto certObjectPath = objectPath + '/';
+
+ if (certType == phosphor::certs::AUTHORITY)
{
- // TODO: Issue#3 At present supporting only one certificate to be
- // uploaded this need to be revisited to support multiple
- // certificates
- auto certObjectPath = objectPath + '/' + '1';
- certificatePtr = std::make_unique<Certificate>(
- bus, certObjectPath, certType, unitToRestart, certInstallPath,
- certInstallPath, true, certWatchPtr);
+ // Create directory
+ fs::create_directories(certInstallPath);
+
+ // Check if above created proper path
+ if (!fs::is_directory(certInstallPath))
+ {
+ log<level::ERR>("Certificate installation path exists and it is "
+ "not a directory");
+ elog<InternalFailure>();
+ return;
+ }
+
+ for (auto& path : fs::directory_iterator(certInstallPath))
+ {
+ try
+ {
+ installedCerts.emplace_back(std::make_unique<Certificate>(
+ bus, certObjectPath + std::to_string(certIdCounter++),
+ certType, unitToRestart, certInstallPath, path.path(), true,
+ certWatchPtr));
+ }
+ catch (const InternalFailure& e)
+ {
+ report<InternalFailure>();
+ }
+ catch (const InvalidCertificate& e)
+ {
+ report<InvalidCertificate>(
+ Reason("Existing certificate file is corrupted"));
+ }
+ }
}
- catch (const InternalFailure& e)
+ else if (fs::exists(certInstallPath))
{
- report<InternalFailure>();
- }
- catch (const InvalidCertificate& e)
- {
- report<InvalidCertificate>(
- Reason("Existing certificate file is corrupted"));
+ try
+ {
+ installedCerts.emplace_back(std::make_unique<Certificate>(
+ bus, certObjectPath + '1', certType, unitToRestart,
+ certInstallPath, certInstallPath, true, certWatchPtr));
+ }
+ catch (const InternalFailure& e)
+ {
+ report<InternalFailure>();
+ }
+ catch (const InvalidCertificate& e)
+ {
+ report<InvalidCertificate>(
+ Reason("Existing certificate file is corrupted"));
+ }
}
}
diff --git a/certs_manager.hpp b/certs_manager.hpp
index 91c5e43..9dd128d 100644
--- a/certs_manager.hpp
+++ b/certs_manager.hpp
@@ -151,11 +151,11 @@
std::string organizationalUnit, std::string state, std::string surname,
std::string unstructuredName) override;
- /** @brief Get reference to certificate
+ /** @brief Get reference to certificates' collection
*
- * @return Reference to certificate
+ * @return Reference to certificates' collection
*/
- CertificatePtr& getCertificate();
+ std::vector<std::unique_ptr<Certificate>>& getCertificates();
private:
void generateCSRHelper(std::vector<std::string> alternativeNames,
@@ -219,7 +219,7 @@
/** @brief Load certifiate
* Load certificate and create certificate object
*/
- void createCertificate();
+ void createCertificates();
/** @brief Create RSA private key file
* Create RSA private key file by generating rsa key if not created
@@ -251,8 +251,8 @@
/** @brief Certificate file installation path **/
CertInstallPath certInstallPath;
- /** @brief pointer to certificate */
- CertificatePtr certificatePtr = nullptr;
+ /** @brief Collection of pointers to certificate */
+ std::vector<std::unique_ptr<Certificate>> installedCerts;
/** @brief pointer to CSR */
std::unique_ptr<CSR> csrPtr = nullptr;
@@ -265,6 +265,9 @@
/** @brif Parent path i.e certificate directory path */
fs::path certParentInstallPath;
+
+ /** @brief Certificate ID pool */
+ uint64_t certIdCounter = 1;
};
} // namespace certs
} // namespace phosphor
diff --git a/test/certs_manager_test.cpp b/test/certs_manager_test.cpp
index d4fd213..0db4885 100644
--- a/test/certs_manager_test.cpp
+++ b/test/certs_manager_test.cpp
@@ -38,21 +38,10 @@
throw std::bad_alloc();
}
certDir = dirPtr;
- certificateFile = "cert.pem";
- CSRFile = "domain.csr";
- privateKeyFile = "privkey.pem";
- rsaPrivateKeyFilePath = certDir + "/.rsaprivkey.pem";
- std::string cmd = "openssl req -x509 -sha256 -newkey rsa:2048 ";
- cmd += "-keyout cert.pem -out cert.pem -days 3650 ";
- cmd += "-subj "
- "/O=openbmc-project.xyz/CN=localhost"
- " -nodes";
- auto val = std::system(cmd.c_str());
- if (val)
- {
- std::cout << "COMMAND Error: " << val << std::endl;
- }
+
+ createNewCertificate();
}
+
void TearDown() override
{
fs::remove_all(certDir);
@@ -61,6 +50,28 @@
fs::remove(privateKeyFile);
}
+ void createNewCertificate(bool setNewCertId = false)
+ {
+ certificateFile = "cert.pem";
+ CSRFile = "domain.csr";
+ privateKeyFile = "privkey.pem";
+ rsaPrivateKeyFilePath = certDir + "/.rsaprivkey.pem";
+ std::string cmd = "openssl req -x509 -sha256 -newkey rsa:2048 ";
+ cmd += "-keyout cert.pem -out cert.pem -days 3650 -nodes";
+ cmd += " -subj /O=openbmc-project.xyz/CN=localhost";
+
+ if (setNewCertId)
+ {
+ cmd += std::to_string(certId++);
+ }
+
+ auto val = std::system(cmd.c_str());
+ if (val)
+ {
+ std::cout << "COMMAND Error: " << val << std::endl;
+ }
+ }
+
bool compareFiles(const std::string& file1, const std::string& file2)
{
std::ifstream f1(file1, std::ifstream::binary | std::ifstream::ate);
@@ -89,6 +100,7 @@
std::string certificateFile, CSRFile, privateKeyFile, rsaPrivateKeyFilePath;
std::string certDir;
+ uint64_t certId;
};
class MainApp
@@ -177,24 +189,94 @@
EXPECT_TRUE(fs::exists(verifyPath));
}
-/** @brief Check if authority install routine is invoked for authority setup
+/** @brief Check if storage install routine is invoked for storage setup
*/
TEST_F(TestCertificates, InvokeAuthorityInstall)
{
std::string endpoint("ldap");
std::string unit("");
std::string type("authority");
- std::string installPath(certDir + "/" + certificateFile);
- std::string verifyPath(installPath);
+ std::string verifyDir(certDir);
UnitsToRestart verifyUnit(unit);
auto objPath = std::string(OBJPATH) + '/' + type + '/' + endpoint;
auto event = sdeventplus::Event::get_default();
// Attach the bus to sd_event to service user requests
bus.attach_event(event.get(), SD_EVENT_PRIORITY_NORMAL);
Manager manager(bus, event, objPath.c_str(), type, std::move(unit),
- std::move(installPath));
+ std::move(certDir));
MainApp mainApp(&manager);
mainApp.install(certificateFile);
+
+ std::vector<std::unique_ptr<Certificate>>& certs =
+ manager.getCertificates();
+
+ EXPECT_FALSE(certs.empty());
+
+ std::string verifyPath = verifyDir + "/" + certs[0]->getHash() + ".0";
+
+ // Check that certificate has been created at installation directory
+ EXPECT_FALSE(fs::is_empty(verifyDir));
+ EXPECT_TRUE(fs::exists(verifyPath));
+
+ // Check that installed cert is identical to input one
+ EXPECT_TRUE(compareFiles(certificateFile, verifyPath));
+}
+
+/** @brief Check if in athority mode user can install a certificate with certain
+ * subject hash once, but cannot install another one with the same hash
+ */
+TEST_F(TestCertificates, InvokeInstallSameSubjectTwice)
+{
+ using NotAllowed =
+ sdbusplus::xyz::openbmc_project::Common::Error::NotAllowed;
+
+ std::string endpoint("ldap");
+ std::string unit("");
+ std::string type("authority");
+ std::string verifyDir(certDir);
+ UnitsToRestart verifyUnit(unit);
+ auto objPath = std::string(OBJPATH) + '/' + type + '/' + endpoint;
+ auto event = sdeventplus::Event::get_default();
+ // Attach the bus to sd_event to service user requests
+ bus.attach_event(event.get(), SD_EVENT_PRIORITY_NORMAL);
+ Manager manager(bus, event, objPath.c_str(), type, std::move(unit),
+ std::move(certDir));
+ MainApp mainApp(&manager);
+ mainApp.install(certificateFile);
+
+ std::vector<std::unique_ptr<Certificate>>& certs =
+ manager.getCertificates();
+
+ EXPECT_FALSE(certs.empty());
+
+ std::string verifyPath = verifyDir + "/" + certs[0]->getHash() + ".0";
+
+ // Check that certificate has been created at installation directory
+ EXPECT_FALSE(fs::is_empty(verifyDir));
+ EXPECT_TRUE(fs::exists(verifyPath));
+
+ // Check that installed cert is identical to input one
+ EXPECT_TRUE(compareFiles(certificateFile, verifyPath));
+
+ // Try to install another one with the same subject
+ createNewCertificate(false);
+
+ EXPECT_THROW(
+ {
+ try
+ {
+ // install second certificate
+ mainApp.install(certificateFile);
+ }
+ catch (const NotAllowed& e)
+ {
+ throw;
+ }
+ },
+ NotAllowed);
+
+ // Check that the original certificate has been not removed
+ EXPECT_FALSE(fs::is_empty(verifyDir));
EXPECT_TRUE(fs::exists(verifyPath));
}
@@ -271,11 +353,58 @@
MainApp mainApp(&manager);
mainApp.install(certificateFile);
EXPECT_TRUE(fs::exists(verifyPath));
+ std::vector<std::unique_ptr<Certificate>>& certs =
+ manager.getCertificates();
+ EXPECT_FALSE(certs.empty());
+ EXPECT_NE(certs[0], nullptr);
+ certs[0]->replace(certificateFile);
EXPECT_TRUE(fs::exists(verifyPath));
- CertificatePtr& ptr = manager.getCertificate();
- EXPECT_NE(ptr, nullptr);
- ptr->replace(certificateFile);
- EXPECT_TRUE(fs::exists(verifyPath));
+}
+
+/** @brief Test replacing existing certificate
+ */
+TEST_F(TestCertificates, TestAuthorityReplaceCertificate)
+{
+ std::string endpoint("ldap");
+ std::string unit("");
+ std::string type("authority");
+ std::string verifyDir(certDir);
+ UnitsToRestart verifyUnit(unit);
+ auto objPath = std::string(OBJPATH) + '/' + type + '/' + endpoint;
+ auto event = sdeventplus::Event::get_default();
+ // Attach the bus to sd_event to service user requests
+ bus.attach_event(event.get(), SD_EVENT_PRIORITY_NORMAL);
+ Manager manager(bus, event, objPath.c_str(), type, std::move(unit),
+ std::move(certDir));
+ MainApp mainApp(&manager);
+ mainApp.install(certificateFile);
+
+ std::vector<std::unique_ptr<Certificate>>& certs =
+ manager.getCertificates();
+ constexpr const unsigned int REPLACE_ITERATIONS = 10;
+
+ for (unsigned int i = 0; i < REPLACE_ITERATIONS; i++)
+ {
+ // Certificate successfully installed
+ EXPECT_FALSE(certs.empty());
+
+ std::string verifyPath = verifyDir + "/" + certs[0]->getHash() + ".0";
+
+ // Check that certificate has been created at installation directory
+ EXPECT_FALSE(fs::is_empty(verifyDir));
+ EXPECT_TRUE(fs::exists(verifyPath));
+
+ // Check that installed cert is identical to input one
+ EXPECT_TRUE(compareFiles(certificateFile, verifyPath));
+
+ // Create new certificate
+ createNewCertificate(true);
+
+ certs[0]->replace(certificateFile);
+
+ // Verify that old certificate has been removed
+ EXPECT_FALSE(fs::exists(verifyPath));
+ }
}
/** @brief Check if install fails if certificate file is empty