fgerlits commented on code in PR #1336:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1336#discussion_r875644534
##########
libminifi/src/utils/tls/CertificateUtils.cpp:
##########
@@ -33,6 +37,29 @@ namespace utils {
namespace tls {
#ifdef WIN32
+WindowsCertStore::WindowsCertStore(const WindowsCertStoreLocation& loc, const
std::string& cert_store) {
+ store_ptr_ = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0, NULL,
+ CERT_STORE_OPEN_EXISTING_FLAG |
CERT_STORE_READONLY_FLAG | loc.getBitfieldValue(),
+ cert_store.data());
Review Comment:
it would be useful to log `GetLastError()` if this fails
##########
libminifi/src/utils/tls/CertificateUtils.cpp:
##########
@@ -116,6 +143,110 @@ EVP_PKEY_unique_ptr extractPrivateKey(const
PCCERT_CONTEXT certificate) {
}
#endif // WIN32
+std::string getLatestOpenSSLErrorString() {
+ unsigned long err = ERR_peek_last_error(); // NOLINT
+ if (err == 0U) {
+ return "";
+ }
+ char buf[4096];
+ ERR_error_string_n(err, buf, sizeof(buf));
+ return buf;
+}
+
+std::optional<std::chrono::system_clock::time_point>
getCertificateExpiration(const X509_unique_ptr& cert) {
+ const ASN1_TIME* asn1_end = X509_get0_notAfter(cert.get());
+ if (!asn1_end) {
+ return {};
+ }
+ std::tm end{};
+ int ret = ASN1_time_parse(reinterpret_cast<const char*>(asn1_end->data),
asn1_end->length, &end, 0);
+ if (ret == -1) {
+ return {};
+ }
+ return std::chrono::system_clock::from_time_t(std::mktime(&end));
Review Comment:
I think `ASN1_time_parse()` returns a UTC timestamp, but `mktime()` parses
it as local time. We have `utils::mkgmtime()` to parse a UTC timestamp.
##########
libminifi/src/controllers/SSLContextService.cpp:
##########
@@ -175,51 +176,31 @@ bool SSLContextService::configure_ssl_context(SSL_CTX
*ctx) {
}
bool SSLContextService::addP12CertificateToSSLContext(SSL_CTX* ctx) const {
- const auto fp_deleter = [](BIO* ptr) { BIO_free(ptr); };
- std::unique_ptr<BIO, decltype(fp_deleter)> fp(BIO_new(BIO_s_file()),
fp_deleter);
- if (fp == nullptr) {
- core::logging::LOG_ERROR(logger_) << "Failed create new file BIO, " <<
getLatestOpenSSLErrorString();
- return false;
- }
- if (BIO_read_filename(fp.get(), certificate_.c_str()) <= 0) {
- core::logging::LOG_ERROR(logger_) << "Failed to read certificate file " <<
certificate_ << ", " << getLatestOpenSSLErrorString();
- return false;
- }
- const auto p12_deleter = [](PKCS12* ptr) { PKCS12_free(ptr); };
- std::unique_ptr<PKCS12, decltype(p12_deleter)> p12(d2i_PKCS12_bio(fp.get(),
nullptr), p12_deleter);
- if (p12 == nullptr) {
- core::logging::LOG_ERROR(logger_) << "Failed to DER decode certificate
file " << certificate_ << ", " << getLatestOpenSSLErrorString();
- return false;
- }
-
- EVP_PKEY* pkey = nullptr;
- X509* cert = nullptr;
- STACK_OF(X509)* ca = nullptr;
- if (!PKCS12_parse(p12.get(), passphrase_.c_str(), &pkey, &cert, &ca)) {
- core::logging::LOG_ERROR(logger_) << "Failed to parse certificate file "
<< certificate_ << " as PKCS#12, " << getLatestOpenSSLErrorString();
- return false;
- }
- utils::tls::EVP_PKEY_unique_ptr pkey_ptr{pkey};
- utils::tls::X509_unique_ptr cert_ptr{cert};
- const auto ca_deleter = gsl::finally([ca] { sk_X509_pop_free(ca, X509_free);
});
-
- if (SSL_CTX_use_certificate(ctx, cert) != 1) {
- core::logging::LOG_ERROR(logger_) << "Failed to set certificate from " <<
certificate_ << ", " << getLatestOpenSSLErrorString();
- return false;
- }
- while (ca != nullptr && sk_X509_num(ca) > 0) {
- utils::tls::X509_unique_ptr cacert{sk_X509_pop(ca)};
- if (SSL_CTX_add_extra_chain_cert(ctx, cacert.get()) != 1) {
- core::logging::LOG_ERROR(logger_) << "Failed to set additional
certificate from " << certificate_ << ", " << getLatestOpenSSLErrorString();
- return false;
+ auto error = utils::tls::processP12Certificate(certificate_, passphrase_, {
+ .cert_cb = [&] (auto& cert) -> std::optional<std::string> {
+ if (SSL_CTX_use_certificate(ctx, cert.get()) != 1) {
+ return utils::StringUtils::join_pack("Failed to set certificate from
", certificate_, ", ", getLatestOpenSSLErrorString());
Review Comment:
Most of these callbacks just log an error and return `nullopt`, but a few
(like this one) return a string and the caller will log the error. Why do we
do this in two different ways?
##########
libminifi/include/utils/tls/CertificateUtils.h:
##########
@@ -43,14 +50,53 @@ struct X509_deleter {
};
using X509_unique_ptr = std::unique_ptr<X509, X509_deleter>;
+struct BIO_deleter {
+ void operator()(BIO* bio) const { BIO_free(bio); }
+};
+using BIO_unique_ptr = std::unique_ptr<BIO, BIO_deleter>;
+
+struct PKCS12_deleter {
+ void operator()(PKCS12* cert) const { PKCS12_free(cert); }
+};
+using PKCS12_unique_ptr = std::unique_ptr<PKCS12, PKCS12_deleter>;
+
#ifdef WIN32
+class WindowsCertStore {
+ public:
+ WindowsCertStore(const WindowsCertStoreLocation& loc, const std::string&
cert_store);
+
+ bool isOpen() const;
+
+ PCCERT_CONTEXT nextCert();
+
+ ~WindowsCertStore();
+
+ private:
+ HCERTSTORE store_ptr_;
+ PCCERT_CONTEXT cert_ctx_ptr_ = nullptr;
+};
+
// Returns nullptr on errors
X509_unique_ptr convertWindowsCertificate(PCCERT_CONTEXT certificate);
// Returns nullptr if the certificate has no associated private key, or the
private key could not be extracted
EVP_PKEY_unique_ptr extractPrivateKey(PCCERT_CONTEXT certificate);
#endif // WIN32
+std::string getLatestOpenSSLErrorString();
+
+std::optional<std::chrono::system_clock::time_point>
getCertificateExpiration(const X509_unique_ptr& cert);
+
+struct CertHandler {
+ std::function<std::optional<std::string>(const X509_unique_ptr& cert)>
cert_cb;
+ std::function<std::optional<std::string>(X509_unique_ptr cert)>
chain_cert_cb;
+ std::function<std::optional<std::string>(const EVP_PKEY_unique_ptr&
priv_key)> priv_key_cb;
Review Comment:
Why is the signature of `chain_cert_cb` different? And could all these
functions take a `T*` or `const T&` instead of a `unique_ptr<T>`?
##########
libminifi/src/controllers/SSLContextService.cpp:
##########
@@ -563,6 +559,89 @@ void SSLContextService::initializeProperties() {
setSupportedProperties(supportedProperties);
}
+void SSLContextService::verifyCertificateExpiration() {
+ auto verify = [&] (const std::string& cert_file, const
utils::tls::X509_unique_ptr& cert) {
+ if (auto end_date = utils::tls::getCertificateExpiration(cert)) {
+ std::string end_date_str =
getTimeStr(std::chrono::duration_cast<std::chrono::milliseconds>(end_date->time_since_epoch()).count());
+ if (end_date.value() < std::chrono::system_clock::now()) {
+ core::logging::LOG_ERROR(logger_) << "Certificate in '" << cert_file
<< "' expired at " << end_date_str;
+ } else if (auto diff = end_date.value() -
std::chrono::system_clock::now(); diff < std::chrono::months{3}) {
+ core::logging::LOG_WARN(logger_) << "Certificate in '" << cert_file <<
"' will expire at " << end_date_str;
+ } else {
+ core::logging::LOG_DEBUG(logger_) << "Certificate in '" << cert_file
<< "' will expire at " << end_date_str;
+ }
+ } else {
+ core::logging::LOG_ERROR(logger_) << "Could not determine expiration
date for certificate in '" << certificate_ << "'";
Review Comment:
I think this should be
```suggestion
core::logging::LOG_ERROR(logger_) << "Could not determine expiration
date for certificate in '" << cert_file << "'";
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
