lordgamez commented on a change in pull request #947:
URL: https://github.com/apache/nifi-minifi-cpp/pull/947#discussion_r537570225
##########
File path: libminifi/src/controllers/SSLContextService.cpp
##########
@@ -128,16 +149,234 @@ bool SSLContextService::configure_ssl_context(SSL_CTX
*ctx) {
}
SSL_CTX_set_verify(ctx, SSL_VERIFY_PEER, nullptr);
- int retp = SSL_CTX_load_verify_locations(ctx, ca_certificate_.c_str(), 0);
- if (retp == 0) {
- logging::LOG_ERROR(logger_) << "Can not load CA certificate, Exiting, " <<
getLatestOpenSSLErrorString();
+ if (!IsNullOrEmpty(ca_certificate_)) {
+ if (SSL_CTX_load_verify_locations(ctx, ca_certificate_.c_str(), 0) == 0) {
+ logging::LOG_ERROR(logger_) << "Cannot load CA certificate, exiting, "
<< getLatestOpenSSLErrorString();
+ return false;
+ }
+ }
+
+ if (use_system_cert_store_ && IsNullOrEmpty(certificate_)) {
+ if (!addClientCertificateFromSystemStoreToSSLContext(ctx)) {
+ return false;
+ }
+ }
+
+ if (use_system_cert_store_ && IsNullOrEmpty(ca_certificate_)) {
+ if (!addServerCertificatesFromSystemStoreToSSLContext(ctx)) {
+ return false;
+ }
+ }
+
+ return true;
+}
+
+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) {
+ logging::LOG_ERROR(logger_) << "Failed create new file BIO, " <<
getLatestOpenSSLErrorString();
+ return false;
+ }
+ if (BIO_read_filename(fp.get(), certificate_.c_str()) <= 0) {
+ 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) {
+ 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)) {
+ 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) {
+ 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) {
+ logging::LOG_ERROR(logger_) << "Failed to set additional certificate
from " << certificate_ << ", " << getLatestOpenSSLErrorString();
+ return false;
+ }
+ cacert.release(); // a successful SSL_CTX_add_extra_chain_cert() takes
ownership of cacert
+ }
+ if (SSL_CTX_use_PrivateKey(ctx, pkey) != 1) {
+ logging::LOG_ERROR(logger_) << "Failed to set private key from " <<
certificate_ << ", " << getLatestOpenSSLErrorString();
return false;
}
return true;
}
-#endif
+
+bool SSLContextService::addPemCertificateToSSLContext(SSL_CTX* ctx) const {
+ if (SSL_CTX_use_certificate_chain_file(ctx, certificate_.c_str()) <= 0) {
+ logging::LOG_ERROR(logger_) << "Could not load client certificate " <<
certificate_ << ", " << getLatestOpenSSLErrorString();
+ return false;
+ }
+
+ if (!IsNullOrEmpty(passphrase_)) {
+ void* passphrase = const_cast<std::string*>(&passphrase_);
+ SSL_CTX_set_default_passwd_cb_userdata(ctx, passphrase);
+ SSL_CTX_set_default_passwd_cb(ctx, minifi::utils::tls::pemPassWordCb);
+ }
+
+ if (!IsNullOrEmpty(private_key_)) {
+ int retp = SSL_CTX_use_PrivateKey_file(ctx, private_key_.c_str(),
SSL_FILETYPE_PEM);
+ if (retp != 1) {
+ logging::LOG_ERROR(logger_) << "Could not load private key, " << retp <<
" on " << private_key_ << ", " << getLatestOpenSSLErrorString();
+ return false;
+ }
+ }
+
+ return true;
+}
+
+bool
SSLContextService::addClientCertificateFromSystemStoreToSSLContext(SSL_CTX*
ctx) const {
+#ifdef WIN32
+ utils::tls::WindowsCertStoreLocation store_location{cert_store_location_};
+ HCERTSTORE hCertStore = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0, NULL,
+ CERT_STORE_OPEN_EXISTING_FLAG |
CERT_STORE_READONLY_FLAG | store_location.getBitfieldValue(),
+ client_cert_store_.data());
+ if (!hCertStore) {
+ logger_->log_error("Could not open system certificate store %s/%s (client
certificates)", cert_store_location_, client_cert_store_);
+ return false;
+ }
+ const auto store_close = gsl::finally([hCertStore](){
CertCloseStore(hCertStore, 0); });
+
+ logger_->log_debug("Looking for client certificate in sytem store %s/%s",
cert_store_location_, client_cert_store_);
+
+ PCCERT_CONTEXT pCertContext = nullptr;
+ while (pCertContext = CertEnumCertificatesInStore(hCertStore, pCertContext))
{
+ bool certificateIsAcceptableAndWasSuccessfullyAddedToSSLContext =
useClientCertificate(ctx, pCertContext);
+ if (certificateIsAcceptableAndWasSuccessfullyAddedToSSLContext) {
+ CertFreeCertificateContext(pCertContext);
+ return true;
+ }
+ }
+
+ logger_->log_error("Could not find any suitable client certificate in sytem
store %s/%s", cert_store_location_, client_cert_store_);
+ return false;
+#else
+ logger_->log_error("Getting client certificate from the system store is only
supported on Windows");
+ return false;
+#endif // WIN32
+}
+
+#ifdef WIN32
+bool SSLContextService::useClientCertificate(SSL_CTX* ctx, PCCERT_CONTEXT
certificate) const {
+ utils::tls::X509_unique_ptr x509_cert =
utils::tls::convertWindowsCertificate(certificate);
+ if (!x509_cert) {
+ logger_->log_error("Failed to convert system store client certificate to
X.509 format");
+ return false;
+ }
+
+ utils::tls::EVP_PKEY_unique_ptr private_key =
utils::tls::extractPrivateKey(certificate);
+ if (!private_key) {
+ logger_->log_debug("Skipping client certificate %s because it has no
exportable private key", x509_cert->name);
+ return false;
+ }
+
+ if (!client_cert_cn_.empty()) {
+ utils::tls::DistinguishedName dn =
utils::tls::DistinguishedName::fromSlashSeparated(x509_cert->name);
+ utils::optional<std::string> cn = dn.getCN();
+ if (!cn || *cn != client_cert_cn_) {
+ logger_->log_debug("Skipping client certificate %s because it doesn't
match CN=%s", x509_cert->name, client_cert_cn_);
+ return false;
+ }
+ }
+
+ utils::tls::EXTENDED_KEY_USAGE_unique_ptr
key_usage{static_cast<EXTENDED_KEY_USAGE*>(X509_get_ext_d2i(x509_cert.get(),
NID_ext_key_usage, nullptr, nullptr))};
+ if (!key_usage) {
+ logger_->log_error("Skipping client certificate %s because it has no
extended key usage", x509_cert->name);
+ return false;
+ }
+
+ if
(!(client_cert_key_usage_.isSubsetOf(utils::tls::ExtendedKeyUsage{*key_usage})))
{
+ logger_->log_debug("Skipping client certificate %s because its extended
key usage set does not contain all usages specified in %s",
+ x509_cert->name,
Configuration::nifi_security_windows_client_cert_key_usage);
+ return false;
+ }
+
+ if (SSL_CTX_use_certificate(ctx, x509_cert.get()) != 1) {
+ logger_->log_error("Failed to set certificate from %s, %s",
x509_cert->name, getLatestOpenSSLErrorString);
+ return false;
+ }
+
+ if (SSL_CTX_use_PrivateKey(ctx, private_key.get()) != 1) {
+ logger_->log_error("Failed to use private key %s, %s", x509_cert->name,
getLatestOpenSSLErrorString());
+ return false;
+ }
+
+ logger_->log_debug("Found client certificate %s", x509_cert->name);
+
+ return true;
+}
+#endif // WIN32
+
+bool
SSLContextService::addServerCertificatesFromSystemStoreToSSLContext(SSL_CTX*
ctx) const {
+#ifdef WIN32
+ utils::tls::WindowsCertStoreLocation store_location{cert_store_location_};
+ HCERTSTORE hCertStore = CertOpenStore(CERT_STORE_PROV_SYSTEM_A, 0, NULL,
+ CERT_STORE_OPEN_EXISTING_FLAG |
CERT_STORE_READONLY_FLAG | store_location.getBitfieldValue(),
+ server_cert_store_.data());
+ if (!hCertStore) {
+ logger_->log_error("Could not open system certificate store %s/%s (server
certificates)", cert_store_location_, server_cert_store_);
+ return false;
+ }
+ const auto store_close = gsl::finally([hCertStore](){
CertCloseStore(hCertStore, 0); });
+
+ X509_STORE* ssl_store = SSL_CTX_get_cert_store(ctx);
+ if (!ssl_store) {
+ logger_->log_error("Could not get handle to SSL certificate store");
+ return false;
+ }
+
+ logger_->log_debug("Adding server certificates from system store %s/%s",
cert_store_location_, server_cert_store_);
+
+ PCCERT_CONTEXT pCertContext = nullptr;
+ while (pCertContext = CertEnumCertificatesInStore(hCertStore, pCertContext))
{
+ addServerCertificateToSSLStore(ssl_store, pCertContext);
+ }
+
+ return true;
+#else
+ SSL_CTX_set_default_verify_paths(ctx);
+ return true;
+#endif // WIN32
+}
+
+#ifdef WIN32
+void SSLContextService::addServerCertificateToSSLStore(X509_STORE* ssl_store,
PCCERT_CONTEXT certificate) const {
+ utils::tls::X509_unique_ptr x509_cert =
utils::tls::convertWindowsCertificate(certificate);
+ if (!x509_cert) {
+ logger_->log_error("Failed to convert system store server certificate to
X.509 format");
+ return;
+ }
+
+ int success = X509_STORE_add_cert(ssl_store, x509_cert.get());
+ if (success == 1) {
+ logger_->log_debug("Added server certificate %s from the system store to
the SSL store", x509_cert->name);
+ } else if (ERR_peek_last_error() == X509_R_CERT_ALREADY_IN_HASH_TABLE) {
+ logger_->log_debug("Ignoring duplicate server certificate %s",
x509_cert->name);
+ } else {
+ logger_->log_error("Failed to add server certificate %s to the SSL store;
error: %s", x509_cert->name, getLatestOpenSSLErrorString());
+ }
Review comment:
I tested the latest change, but unfortunately it does not change the
logging. The `X509_R_CERT_ALREADY_IN_HASH_TABLE` we are trying to check has the
value `101` but after I log the `ERR_peek_last_error` it returns `201322597` so
it is not the error code we were looking for. As checking further the error
code in binary is `1011 1111 1111 1111 0000 0110 0101` where the last byte's
value is actually `101` in decimal.
According to the [OpenSSL
documentation](https://www.openssl.org/docs/man1.0.2/man3/ERR_GET_FUNC.html)
the returned value of `ERR_peek_last_error` consists of a library number,
function code and reason code and there are separate macros defined to retrieve
these. We need to use `ERR_GET_REASON` to retrieve the error code.
I tested the following change and it seems to be working.
```suggestion
} else {
auto err = ERR_peek_last_error();
if (ERR_GET_REASON(err) == X509_R_CERT_ALREADY_IN_HASH_TABLE) {
logger_->log_debug("Ignoring duplicate server certificate %s",
x509_cert->name);
} else {
logger_->log_error("Failed to add server certificate %s to the SSL
store; error: %s", x509_cert->name, getLatestOpenSSLErrorString());
}
}
```
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
