clean up using directives and type alias
Most C++ style guides try to avoid using directives in headers and also
suggest using type alias carefully, according to which, this change does
the following clean up:
1. used Enum class to represent Certificate type
2. removed all using directives: e.g. the phosphor logging namespace;
instead, this change uses using declarations
3. removed unnecessary type alias; in existing codes, we only support
strings as types of UnitToRestart, InstallPath, UploadPath, etc; this
change uses std::string directly
4. moved all alias outside any class scope into source files or an
internal namespace
5. renamed types, constants, classes as per OpenBMC style guide
6. fixed all compilation errors and some warnings after the refactoring;
built with both Clang & GCC
Reference:
https://docs.microsoft.com/en-us/cpp/cpp/header-files-cpp?view=msvc-170#what-to-put-in-a-header-file
https://google.github.io/styleguide/cppguide.html#Namespaces
Tested:
Unit tests
Signed-off-by: Nan Zhou <nanzhoumails@gmail.com>
Change-Id: I58e026934a4e969f4d8877801c8f3c671990468a
diff --git a/certificate.cpp b/certificate.cpp
index f367267..6dfed96 100644
--- a/certificate.cpp
+++ b/certificate.cpp
@@ -19,20 +19,28 @@
namespace phosphor::certs
{
+
+namespace
+{
+namespace fs = std::filesystem;
+using ::phosphor::logging::elog;
+using ::phosphor::logging::entry;
+using ::phosphor::logging::level;
+using ::phosphor::logging::log;
+using ::phosphor::logging::report;
+using InvalidCertificateError =
+ ::sdbusplus::xyz::openbmc_project::Certs::Error::InvalidCertificate;
+using ::phosphor::logging::xyz::openbmc_project::Certs::InvalidCertificate;
+using ::sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
+
// RAII support for openSSL functions.
-using BIO_MEM_Ptr = std::unique_ptr<BIO, decltype(&::BIO_free)>;
-using X509_STORE_Ptr =
- std::unique_ptr<X509_STORE, decltype(&::X509_STORE_free)>;
-using X509_STORE_CTX_Ptr =
+using BIOMemPtr = std::unique_ptr<BIO, decltype(&::BIO_free)>;
+using X509StorePtr = std::unique_ptr<X509_STORE, decltype(&::X509_STORE_free)>;
+using X509StoreCtxPtr =
std::unique_ptr<X509_STORE_CTX, decltype(&::X509_STORE_CTX_free)>;
-using ASN1_TIME_ptr = std::unique_ptr<ASN1_TIME, decltype(&ASN1_STRING_free)>;
-using EVP_PKEY_Ptr = std::unique_ptr<EVP_PKEY, decltype(&::EVP_PKEY_free)>;
-using BUF_MEM_Ptr = std::unique_ptr<BUF_MEM, decltype(&::BUF_MEM_free)>;
-using InternalFailure =
- sdbusplus::xyz::openbmc_project::Common::Error::InternalFailure;
-using InvalidCertificate =
- sdbusplus::xyz::openbmc_project::Certs::Error::InvalidCertificate;
-using Reason = xyz::openbmc_project::Certs::InvalidCertificate::REASON;
+using ASN1TimePtr = std::unique_ptr<ASN1_TIME, decltype(&ASN1_STRING_free)>;
+using EVPPkeyPtr = std::unique_ptr<EVP_PKEY, decltype(&::EVP_PKEY_free)>;
+using BufMemPtr = std::unique_ptr<BUF_MEM, decltype(&::BUF_MEM_free)>;
// Trust chain related errors.`
#define TRUST_CHAIN_ERR(errnum) \
@@ -70,10 +78,11 @@
{NID_OCSP_sign, "OCSPSigning"},
{NID_ad_timeStamping, "Timestamping"},
{NID_code_sign, "CodeSigning"}};
+} // namespace
std::string Certificate::generateCertId(const std::string& certPath)
{
- const X509_Ptr cert = loadCert(certPath);
+ const internal::X509Ptr cert = loadCert(certPath);
unsigned long subjectNameHash = X509_subject_name_hash(cert.get());
unsigned long issuerSerialHash = X509_issuer_and_serial_hash(cert.get());
static constexpr auto CERT_ID_LENGTH = 17;
@@ -104,7 +113,7 @@
std::string Certificate::generateAuthCertFileX509Path(
const std::string& certSrcFilePath, const std::string& certDstDirPath)
{
- const X509_Ptr cert = loadCert(certSrcFilePath);
+ const internal::X509Ptr cert = loadCert(certSrcFilePath);
unsigned long hash = X509_subject_name_hash(cert.get());
static constexpr auto CERT_HASH_LENGTH = 9;
char hashBuf[CERT_HASH_LENGTH];
@@ -153,7 +162,7 @@
std::string
Certificate::generateCertFilePath(const std::string& certSrcFilePath)
{
- if (certType == phosphor::certs::AUTHORITY)
+ if (certType == CertificateType::Authority)
{
return generateAuthCertFilePath(certSrcFilePath);
}
@@ -164,32 +173,31 @@
}
Certificate::Certificate(sdbusplus::bus::bus& bus, const std::string& objPath,
- const CertificateType& type,
- const CertInstallPath& installPath,
- const CertUploadPath& uploadPath,
- const CertWatchPtr& certWatchPtr, Manager& parent) :
- CertIfaces(bus, objPath.c_str(), true),
- bus(bus), objectPath(objPath), certType(type), certInstallPath(installPath),
- certWatchPtr(certWatchPtr), manager(parent)
+ CertificateType type, const std::string& installPath,
+ const std::string& uploadPath, Watch* watch,
+ Manager& parent) :
+ internal::CertificateInterface(bus, objPath.c_str(), true),
+ objectPath(objPath), certType(type), certInstallPath(installPath),
+ certWatch(watch), manager(parent)
{
auto installHelper = [this](const auto& filePath) {
if (!compareKeys(filePath))
{
- elog<InvalidCertificate>(
- Reason("Private key does not match the Certificate"));
+ elog<InvalidCertificateError>(InvalidCertificate::REASON(
+ "Private key does not match the Certificate"));
};
};
- typeFuncMap[SERVER] = installHelper;
- typeFuncMap[CLIENT] = installHelper;
- typeFuncMap[AUTHORITY] = [](const std::string&) {};
+ typeFuncMap[CertificateType::Server] = installHelper;
+ typeFuncMap[CertificateType::Client] = installHelper;
+ typeFuncMap[CertificateType::Authority] = [](const std::string&) {};
auto appendPrivateKey = [this](const std::string& filePath) {
checkAndAppendPrivateKey(filePath);
};
- appendKeyMap[SERVER] = appendPrivateKey;
- appendKeyMap[CLIENT] = appendPrivateKey;
- appendKeyMap[AUTHORITY] = [](const std::string&) {};
+ appendKeyMap[CertificateType::Server] = appendPrivateKey;
+ appendKeyMap[CertificateType::Client] = appendPrivateKey;
+ appendKeyMap[CertificateType::Authority] = [](const std::string&) {};
// Generate certificate file path
certFilePath = generateCertFilePath(uploadPath);
@@ -221,9 +229,9 @@
auto errCode = X509_V_OK;
// stop watch for user initiated certificate install
- if (certWatchPtr)
+ if (certWatch != nullptr)
{
- certWatchPtr->stopWatch();
+ certWatch->stopWatch();
}
// Verify the certificate file
@@ -242,7 +250,8 @@
// file is empty
log<level::ERR>("File is empty",
entry("FILE=%s", certSrcFilePath.c_str()));
- elog<InvalidCertificate>(Reason("File is empty"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("File is empty"));
}
}
catch (const fs::filesystem_error& e)
@@ -253,7 +262,7 @@
}
// Create an empty X509_STORE structure for certificate validation.
- X509_STORE_Ptr x509Store(X509_STORE_new(), &X509_STORE_free);
+ X509StorePtr x509Store(X509_STORE_new(), &X509_STORE_free);
if (!x509Store)
{
log<level::ERR>("Error occurred during X509_STORE_new call");
@@ -278,12 +287,13 @@
{
log<level::ERR>("Error occurred during X509_LOOKUP_load_file call",
entry("FILE=%s", certSrcFilePath.c_str()));
- elog<InvalidCertificate>(Reason("Invalid certificate file format"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Invalid certificate file format"));
}
// Load Certificate file into the X509 structure.
- X509_Ptr cert = loadCert(certSrcFilePath);
- X509_STORE_CTX_Ptr storeCtx(X509_STORE_CTX_new(), ::X509_STORE_CTX_free);
+ internal::X509Ptr cert = loadCert(certSrcFilePath);
+ X509StoreCtxPtr storeCtx(X509_STORE_CTX_new(), ::X509_STORE_CTX_free);
if (!storeCtx)
{
log<level::ERR>("Error occurred during X509_STORE_CTX_new call",
@@ -337,16 +347,18 @@
if (errCode == X509_V_ERR_CERT_HAS_EXPIRED)
{
log<level::ERR>("Expired certificate ");
- elog<InvalidCertificate>(Reason("Expired Certificate"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Expired Certificate"));
}
// Logging general error here.
log<level::ERR>(
"Certificate validation failed", entry("ERRCODE=%d", errCode),
entry("ERROR_STR=%s", X509_verify_cert_error_string(errCode)));
- elog<InvalidCertificate>(Reason("Certificate validation failed"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Certificate validation failed"));
}
- validateCertificateStartDate(cert);
+ validateCertificateStartDate(*cert);
// Verify that the certificate can be used in a TLS context
const SSL_METHOD* method = TLS_method();
@@ -356,14 +368,16 @@
{
log<level::ERR>("Certificate is not usable",
entry("ERRCODE=%x", ERR_get_error()));
- elog<InvalidCertificate>(Reason("Certificate is not usable"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Certificate is not usable"));
}
// Invoke type specific append private key function.
auto appendIter = appendKeyMap.find(certType);
if (appendIter == appendKeyMap.end())
{
- log<level::ERR>("Unsupported Type", entry("TYPE=%s", certType.c_str()));
+ log<level::ERR>("Unsupported Type",
+ entry("TYPE=%s", certificateTypeToString(certType)));
elog<InternalFailure>();
}
appendIter->second(certSrcFilePath);
@@ -372,7 +386,8 @@
auto compIter = typeFuncMap.find(certType);
if (compIter == typeFuncMap.end())
{
- log<level::ERR>("Unsupported Type", entry("TYPE=%s", certType.c_str()));
+ log<level::ERR>("Unsupported Type",
+ entry("TYPE=%s", certificateTypeToString(certType)));
elog<InternalFailure>();
}
compIter->second(certSrcFilePath);
@@ -418,31 +433,31 @@
populateProperties(certFilePath);
// restart watch
- if (certWatchPtr)
+ if (certWatch != nullptr)
{
- certWatchPtr->startWatch();
+ certWatch->startWatch();
}
}
// Checks that notBefore is not earlier than the unix epoch given that
// the corresponding DBus interface is uint64_t.
-void Certificate::validateCertificateStartDate(const X509_Ptr& cert)
+void Certificate::validateCertificateStartDate(X509& cert)
{
int days = 0;
int secs = 0;
- ASN1_TIME_ptr epoch(ASN1_TIME_new(), ASN1_STRING_free);
+ ASN1TimePtr epoch(ASN1_TIME_new(), ASN1_STRING_free);
// Set time to 00:00am GMT, Jan 1 1970; format: YYYYMMDDHHMMSSZ
ASN1_TIME_set_string(epoch.get(), "19700101000000Z");
- ASN1_TIME* notBefore = X509_get_notBefore(cert.get());
+ ASN1_TIME* notBefore = X509_get_notBefore(&cert);
ASN1_TIME_diff(&days, &secs, epoch.get(), notBefore);
if (days < 0 || secs < 0)
{
log<level::ERR>("Certificate valid date starts before the Unix Epoch");
- elog<InvalidCertificate>(
- Reason("NotBefore should after 19700101000000Z"));
+ elog<InvalidCertificateError>(InvalidCertificate::REASON(
+ "NotBefore should after 19700101000000Z"));
}
}
@@ -463,7 +478,7 @@
void Certificate::storageUpdate()
{
- if (certType == phosphor::certs::AUTHORITY)
+ if (certType == CertificateType::Authority)
{
// Create symbolic link in the certificate directory
std::string certFileX509Path;
@@ -491,32 +506,32 @@
void Certificate::populateProperties(const std::string& certPath)
{
- X509_Ptr cert = loadCert(certPath);
+ internal::X509Ptr cert = loadCert(certPath);
// Update properties if no error thrown
- BIO_MEM_Ptr certBio(BIO_new(BIO_s_mem()), BIO_free);
+ BIOMemPtr certBio(BIO_new(BIO_s_mem()), BIO_free);
PEM_write_bio_X509(certBio.get(), cert.get());
- BUF_MEM_Ptr certBuf(BUF_MEM_new(), BUF_MEM_free);
+ BufMemPtr certBuf(BUF_MEM_new(), BUF_MEM_free);
BUF_MEM* buf = certBuf.get();
BIO_get_mem_ptr(certBio.get(), &buf);
std::string certStr(buf->data, buf->length);
- CertificateIface::certificateString(certStr);
+ certificateString(certStr);
static const int maxKeySize = 4096;
char subBuffer[maxKeySize] = {0};
- BIO_MEM_Ptr subBio(BIO_new(BIO_s_mem()), BIO_free);
+ BIOMemPtr subBio(BIO_new(BIO_s_mem()), BIO_free);
// This pointer cannot be freed independently.
X509_NAME* sub = X509_get_subject_name(cert.get());
X509_NAME_print_ex(subBio.get(), sub, 0, XN_FLAG_SEP_COMMA_PLUS);
BIO_read(subBio.get(), subBuffer, maxKeySize);
- CertificateIface::subject(subBuffer);
+ subject(subBuffer);
char issuerBuffer[maxKeySize] = {0};
- BIO_MEM_Ptr issuerBio(BIO_new(BIO_s_mem()), BIO_free);
+ BIOMemPtr issuerBio(BIO_new(BIO_s_mem()), BIO_free);
// This pointer cannot be freed independently.
X509_NAME* issuer_name = X509_get_issuer_name(cert.get());
X509_NAME_print_ex(issuerBio.get(), issuer_name, 0, XN_FLAG_SEP_COMMA_PLUS);
BIO_read(issuerBio.get(), issuerBuffer, maxKeySize);
- CertificateIface::issuer(issuerBuffer);
+ issuer(issuerBuffer);
std::vector<std::string> keyUsageList;
ASN1_BIT_STRING* usage;
@@ -549,29 +564,29 @@
sk_ASN1_OBJECT_value(extUsage, i))]);
}
}
- CertificateIface::keyUsage(keyUsageList);
+ keyUsage(keyUsageList);
int days = 0;
int secs = 0;
- ASN1_TIME_ptr epoch(ASN1_TIME_new(), ASN1_STRING_free);
+ ASN1TimePtr epoch(ASN1_TIME_new(), ASN1_STRING_free);
// Set time to 00:00am GMT, Jan 1 1970; format: YYYYMMDDHHMMSSZ
ASN1_TIME_set_string(epoch.get(), "19700101000000Z");
static const uint64_t dayToSeconds = 24 * 60 * 60;
ASN1_TIME* notAfter = X509_get_notAfter(cert.get());
ASN1_TIME_diff(&days, &secs, epoch.get(), notAfter);
- CertificateIface::validNotAfter((days * dayToSeconds) + secs);
+ validNotAfter((days * dayToSeconds) + secs);
ASN1_TIME* notBefore = X509_get_notBefore(cert.get());
ASN1_TIME_diff(&days, &secs, epoch.get(), notBefore);
- CertificateIface::validNotBefore((days * dayToSeconds) + secs);
+ validNotBefore((days * dayToSeconds) + secs);
}
-X509_Ptr Certificate::loadCert(const std::string& filePath)
+internal::X509Ptr Certificate::loadCert(const std::string& filePath)
{
// Read Certificate file
- X509_Ptr cert(X509_new(), ::X509_free);
+ internal::X509Ptr cert(X509_new(), ::X509_free);
if (!cert)
{
log<level::ERR>("Error occurred during X509_new call",
@@ -580,7 +595,7 @@
elog<InternalFailure>();
}
- BIO_MEM_Ptr bioCert(BIO_new_file(filePath.c_str(), "rb"), ::BIO_free);
+ BIOMemPtr bioCert(BIO_new_file(filePath.c_str(), "rb"), ::BIO_free);
if (!bioCert)
{
log<level::ERR>("Error occurred during BIO_new_file call",
@@ -600,7 +615,7 @@
void Certificate::checkAndAppendPrivateKey(const std::string& filePath)
{
- BIO_MEM_Ptr keyBio(BIO_new(BIO_s_file()), ::BIO_free);
+ BIOMemPtr keyBio(BIO_new(BIO_s_file()), ::BIO_free);
if (!keyBio)
{
log<level::ERR>("Error occurred during BIO_s_file call",
@@ -609,7 +624,7 @@
}
BIO_read_filename(keyBio.get(), filePath.c_str());
- EVP_PKEY_Ptr priKey(
+ EVPPkeyPtr priKey(
PEM_read_bio_PrivateKey(keyBio.get(), nullptr, nullptr, nullptr),
::EVP_PKEY_free);
if (!priKey)
@@ -657,7 +672,7 @@
{
log<level::INFO>("Certificate compareKeys",
entry("FILEPATH=%s", filePath.c_str()));
- X509_Ptr cert(X509_new(), ::X509_free);
+ internal::X509Ptr cert(X509_new(), ::X509_free);
if (!cert)
{
log<level::ERR>("Error occurred during X509_new call",
@@ -666,7 +681,7 @@
elog<InternalFailure>();
}
- BIO_MEM_Ptr bioCert(BIO_new_file(filePath.c_str(), "rb"), ::BIO_free);
+ BIOMemPtr bioCert(BIO_new_file(filePath.c_str(), "rb"), ::BIO_free);
if (!bioCert)
{
log<level::ERR>("Error occurred during BIO_new_file call",
@@ -677,16 +692,17 @@
X509* x509 = cert.get();
PEM_read_bio_X509(bioCert.get(), &x509, nullptr, nullptr);
- EVP_PKEY_Ptr pubKey(X509_get_pubkey(cert.get()), ::EVP_PKEY_free);
+ EVPPkeyPtr pubKey(X509_get_pubkey(cert.get()), ::EVP_PKEY_free);
if (!pubKey)
{
log<level::ERR>("Error occurred during X509_get_pubkey",
entry("FILE=%s", filePath.c_str()),
entry("ERRCODE=%lu", ERR_get_error()));
- elog<InvalidCertificate>(Reason("Failed to get public key info"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Failed to get public key info"));
}
- BIO_MEM_Ptr keyBio(BIO_new(BIO_s_file()), ::BIO_free);
+ BIOMemPtr keyBio(BIO_new(BIO_s_file()), ::BIO_free);
if (!keyBio)
{
log<level::ERR>("Error occurred during BIO_s_file call",
@@ -695,7 +711,7 @@
}
BIO_read_filename(keyBio.get(), filePath.c_str());
- EVP_PKEY_Ptr priKey(
+ EVPPkeyPtr priKey(
PEM_read_bio_PrivateKey(keyBio.get(), nullptr, nullptr, nullptr),
::EVP_PKEY_free);
if (!priKey)
@@ -703,7 +719,8 @@
log<level::ERR>("Error occurred during PEM_read_bio_PrivateKey",
entry("FILE=%s", filePath.c_str()),
entry("ERRCODE=%lu", ERR_get_error()));
- elog<InvalidCertificate>(Reason("Failed to get private key info"));
+ elog<InvalidCertificateError>(
+ InvalidCertificate::REASON("Failed to get private key info"));
}
#if (OPENSSL_VERSION_NUMBER < 0x30000000L)