[GitHub] [nifi-minifi-cpp] szaszm commented on a diff in pull request #1336: MINIFICPP-1826 - Warn on expiring certificate

[GitBox <gi...@apache.org>]

szaszm commented on code in PR #1336:
URL: https://github.com/apache/nifi-minifi-cpp/pull/1336#discussion_r876065172


##########
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;

Review Comment:
   I find this a bit excessive. WARN is enough for the last 2 (or maybe 4) weeks.
   
   Additionally, I would avoid comparing a precise duration to months, because months are variable length. I'm a bit surprised that this even compiles. (end_date - N months) < now() would be better, because then we know exactly which months to subtract and how long they are. If you change this to 2 weeks, which is plenty IMO, it will of course no longer be a problem.



##########
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;
+};
+
+std::optional<std::string> processP12Certificate(const std::string& cert_file, const std::string& passphrase, const CertHandler& handler);
+
+std::optional<std::string> processPEMCertificate(const std::string& cert_file, const std::optional<std::string>& passphrase, const CertHandler& handler);

Review Comment:
   I think a `std::error_code` return type with an internal openssl error category would better communicate the meaning and it would be similarly easy to use. See `DNS.cpp` for an example error_category definition.



##########
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:
   About `GetLastError()`: The WINAPI way of converting it to string is complex and messy. This (copied from DNS.cpp) works for non-winsock errors as well, and it has a message member:
   ```
   std::error_code{WSAGetLastError(), std::system_category()}
   ```



-- 
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: issues-unsubscribe@nifi.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org