Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

Closed
wants to merge 1 commit into from
Closed

MINIFICPP-1070 - Fixed handling of PKCS12 certificates #666

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 46 additions & 29 deletions libminifi/src/controllers/SSLContextService.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,8 @@
#include "core/Property.h"
#include "io/validation.h"
#include "properties/Configure.h"
#include "utils/ScopeGuard.h"

namespace org {
namespace apache {
namespace nifi {
Expand Down Expand Up @@ -55,39 +57,46 @@ bool SSLContextService::configure_ssl_context(SSL_CTX *ctx) {
logging::LOG_ERROR(logger_) << "Failed create new file BIO, " << getLatestOpenSSLErrorString();
return false;
}
utils::ScopeGuard fp_guard([fp]() { BIO_free(fp); });
if (BIO_read_filename(fp, certificate.c_str()) <= 0) {
logging::LOG_ERROR(logger_) << "Failed to read certificate file " << certificate << ", " << getLatestOpenSSLErrorString();
BIO_free(fp);
return false;
}
PKCS12* p12 = d2i_PKCS12_bio(fp, nullptr);
BIO_free(fp);
if (p12 == nullptr) {
logging::LOG_ERROR(logger_) << "Failed to DER decode certificate file " << certificate << ", " << getLatestOpenSSLErrorString();
return false;
}
utils::ScopeGuard p12_guard([p12]() { PKCS12_free(p12); });
EVP_PKEY* pkey = nullptr;
X509* cert = nullptr;
if (!PKCS12_parse(p12, passphrase_.c_str(), &pkey, &cert, nullptr /*ca*/)) {
STACK_OF(X509)* ca = nullptr;
if (!PKCS12_parse(p12, passphrase_.c_str(), &pkey, &cert, &ca)) {
logging::LOG_ERROR(logger_) << "Failed to parse certificate file " << certificate << " as PKCS#12, " << getLatestOpenSSLErrorString();
PKCS12_free(p12);
return false;
}
PKCS12_free(p12);
if (SSL_CTX_use_certificate(ctx, cert) != 1) {
logging::LOG_ERROR(logger_) << "Failed to set certificate from " << certificate << ", " << getLatestOpenSSLErrorString();
utils::ScopeGuard certs_guard([pkey, cert, ca]() {
EVP_PKEY_free(pkey);
X509_free(cert);
Copy link
Contributor

@arpadboda arpadboda Oct 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feeling says that whenever we do this we leak the page pointed by "ca".
PKCS12_parse allocates that as well, so I think we need to free it.

I would also prefer using scope guards for the resources here to avoid leaks.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed the code to use scope guards.

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) {
X509 *cacert = sk_X509_pop(ca);
utils::ScopeGuard cacert_guard([cacert]() { X509_free(cacert); });
if (SSL_CTX_add_extra_chain_cert(ctx, cacert) != 1) {
logging::LOG_ERROR(logger_) << "Failed to set additional certificate from " << certificate << ", " << getLatestOpenSSLErrorString();
return false;
}
cacert_guard.disable();
}
if (SSL_CTX_use_PrivateKey(ctx, pkey) != 1) {
logging::LOG_ERROR(logger_) << "Failed to set private key from " << certificate << ", " << getLatestOpenSSLErrorString();
EVP_PKEY_free(pkey);
X509_free(cert);
return false;
}
EVP_PKEY_free(pkey);
X509_free(cert);
} else {
if (SSL_CTX_use_certificate_chain_file(ctx, certificate.c_str()) <= 0) {
logging::LOG_ERROR(logger_) << "Could not create load certificate " << certificate << ", " << getLatestOpenSSLErrorString();
Expand Down Expand Up @@ -192,39 +201,47 @@ void SSLContextService::onEnable() {

logger_->log_trace("onEnable()");

if (getProperty(property.getName(), certificate) && getProperty(privKey.getName(), private_key_)) {
bool has_certificate_property = getProperty(property.getName(), certificate);
if (has_certificate_property) {
std::ifstream cert_file(certificate);
std::ifstream priv_file(private_key_);
if (!cert_file.good()) {
logger_->log_info("%s not good", certificate);
logger_->log_warn("Cannot open certificate file %s", certificate);
std::string test_cert = default_dir + certificate;
std::ifstream cert_file_test(test_cert);
if (cert_file_test.good()) {
certificate = test_cert;
logger_->log_debug("%s now good", certificate);
logger_->log_info("Using certificate file %s", certificate);
} else {
logger_->log_warn("%s still not good", test_cert);
logger_->log_error("Cannot open certificate file %s", test_cert);
valid_ = false;
}
cert_file_test.close();
}

if (!priv_file.good()) {
std::string test_priv = default_dir + private_key_;
std::ifstream private_file_test(test_priv);
if (private_file_test.good()) {
private_key_ = test_priv;
} else {
valid_ = false;
}
private_file_test.close();
}
cert_file.close();
priv_file.close();

} else {
logger_->log_debug("Certificate empty");
}
if (has_certificate_property && !isFileTypeP12(certificate)) {
if (getProperty(privKey.getName(), private_key_)) {
std::ifstream priv_file(private_key_);
if (!priv_file.good()) {
logger_->log_warn("Cannot open private key file %s", private_key_);
std::string test_priv = default_dir + private_key_;
std::ifstream private_file_test(test_priv);
if (private_file_test.good()) {
private_key_ = test_priv;
logger_->log_info("Using private key file %s", private_key_);
} else {
logger_->log_error("Cannot open private key file %s", test_priv);
valid_ = false;
}
private_file_test.close();
}
priv_file.close();
} else {
logger_->log_debug("Private key empty");
}
}
if (!getProperty(passphrase_prop.getName(), passphrase_)) {
logger_->log_debug("No pass phrase for %s", certificate);
} else {
Expand Down
39 changes: 17 additions & 22 deletions libminifi/src/io/tls/TLSSocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,40 +80,35 @@ int16_t TLSContext::initialize(bool server_method) {
std::string caCertificate;

if (ssl_service_ != nullptr) {
certificate = ssl_service_->getCertificateFile();
privatekey = ssl_service_->getPrivateKeyFile();
caCertificate = ssl_service_->getCACertificate();
passphrase = ssl_service_->getPassphrase();
} else {
if (!(configure_->get(Configure::nifi_security_client_certificate, certificate) && configure_->get(Configure::nifi_security_client_private_key, privatekey))) {
if (!ssl_service_->configure_ssl_context(ctx)) {
error_value = TLS_ERROR_CERT_ERROR;
return error_value;
}
return 0;
}

if (!(configure_->get(Configure::nifi_security_client_certificate, certificate) && configure_->get(Configure::nifi_security_client_private_key, privatekey))) {
logger_->log_error("Certificate and Private Key PEM file not configured, error: %s.", std::strerror(errno));
error_value = TLS_ERROR_PEM_MISSING;
return error_value;
}
}
// load certificates and private key in PEM format
if (SSL_CTX_use_certificate_chain_file(ctx, certificate.c_str()) <= 0) {
logger_->log_error("Could not load certificate %s, for %X and %X error : %s", certificate, this, ctx, std::strerror(errno));
error_value = TLS_ERROR_CERT_MISSING;
return error_value;
}
if (ssl_service_ != nullptr) {
// if the private key has passphase
if (configure_->get(Configure::nifi_security_client_pass_phrase, passphrase)) {
std::ifstream file(passphrase.c_str(), std::ifstream::in);
if (file.good()) {
// if we have been given a file copy that, otherwise treat the passphrase as a password
std::string password;
password.assign((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>());
file.close();
passphrase = password;
}
SSL_CTX_set_default_passwd_cb(ctx, io::tls::pemPassWordCb);
SSL_CTX_set_default_passwd_cb_userdata(ctx, &passphrase);
} else {
if (configure_->get(Configure::nifi_security_client_pass_phrase, passphrase)) {
std::ifstream file(passphrase.c_str(), std::ifstream::in);
if (file.good()) {
// if we have been given a file copy that, otherwise treat the passphrase as a password
std::string password;
password.assign((std::istreambuf_iterator<char>(file)), std::istreambuf_iterator<char>());
file.close();
passphrase = password;
}
SSL_CTX_set_default_passwd_cb(ctx, io::tls::pemPassWordCb);
SSL_CTX_set_default_passwd_cb_userdata(ctx, &passphrase);
}
}

int retp = SSL_CTX_use_PrivateKey_file(ctx, privatekey.c_str(), SSL_FILETYPE_PEM);
Expand Down