Skip to content

Name the CA file or directory when loading it fails #584

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

Open
iliaal wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Name the CA file or directory when loading it fails #584

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
59 changes: 37 additions & 22 deletions clickhouse/base/sslsocket.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,10 +31,20 @@ std::string getCertificateInfo(X509* cert)

void throwSSLError(SSL * ssl, int error, const char * /*location*/, const char * /*statement*/, const std::string prefix = "OpenSSL error: ") {
const auto detail_error = ERR_get_error();
auto reason = ERR_reason_error_string(detail_error);
reason = reason ? reason : "Unknown SSL error";
std::string reason_str;
if (const auto reason = ERR_reason_error_string(detail_error)) {
reason_str = reason;
} else if (detail_error != 0) {
// OpenSSL 3 has no static reason string for system errors (errno from fopen and the like).
char buf[256];
ERR_error_string_n(detail_error, buf, sizeof(buf));
reason_str = buf;
} else {
reason_str = "Unknown SSL error";
}
// Leftover entries would be reported as the reason of a later, unrelated failure.
ERR_clear_error();

std::string reason_str = reason;
if (ssl) {
// Print certificate only if handshake isn't completed
if (auto ssl_session = SSL_get_session(ssl); ssl_session && SSL_get_state(ssl) != TLS_ST_OK)
Expand Down Expand Up @@ -129,16 +139,20 @@ SSL_CTX * prepareSSLContext(const clickhouse::SSLParams & context_params) {

if (context_params.use_default_ca_locations)
HANDLE_SSL_CTX_ERROR(SSL_CTX_set_default_verify_paths(ctx.get()));
const auto load_verify_locations = [&ctx](const char* file, const char* dir, const std::string& what) {
// Otherwise an entry queued by unrelated earlier work would be reported as the reason.
ERR_clear_error();
if (!SSL_CTX_load_verify_locations(ctx.get(), file, dir))
throwSSLError(nullptr, static_cast<int>(ERR_peek_error()), LOCATION, "SSL_CTX_load_verify_locations",
"Failed to load " + what + ": ");
};
if (!context_params.path_to_ca_directory.empty())
HANDLE_SSL_CTX_ERROR(
SSL_CTX_load_verify_locations(
ctx.get(),
nullptr,
context_params.path_to_ca_directory.c_str())
);
load_verify_locations(nullptr, context_params.path_to_ca_directory.c_str(),
"CA directory '" + context_params.path_to_ca_directory + "'");

for (const auto & f : context_params.path_to_ca_files)
HANDLE_SSL_CTX_ERROR(SSL_CTX_load_verify_locations(ctx.get(), f.c_str(), nullptr));
for (const auto& f : context_params.path_to_ca_files) {
load_verify_locations(f.c_str(), nullptr, "CA file '" + f + "'");
}

if (context_params.context_options != -1)
SSL_CTX_set_options(ctx.get(), context_params.context_options);
Expand Down Expand Up @@ -255,17 +269,18 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SocketTimeoutParams& time
+ "\nServer certificate: " + getCertificateInfo(SSL_get_peer_certificate(ssl)));
}

// Host name verification is done by OpenSSL itself, however if we are connecting to an ip-address,
// no verification is made, so we have to do it manually.
// Just in case if this is ever required, leave it here commented out.
// if (ip_addr) {
// // if hostname is actually an IP address
// HANDLE_SSL_ERROR(ssl, X509_check_ip(
// SSL_get_peer_certificate(ssl),
// ASN1_STRING_get0_data(ip_addr.get()),
// ASN1_STRING_length(ip_addr.get()),
// 0));
// }
// SSL_set1_host() above makes OpenSSL verify the peer name. Since OpenSSL 3.0 it also
// recognizes IP literals and checks them against the certificate's IP SANs; older
// releases match an IP literal only as a DNS name. The manual IP check below is kept
// for reference in case a build ever needs it.
// if (ip_addr) {
// // if hostname is actually an IP address
// HANDLE_SSL_ERROR(ssl, X509_check_ip(
// SSL_get_peer_certificate(ssl),
// ASN1_STRING_get0_data(ip_addr.get()),
// ASN1_STRING_length(ip_addr.get()),
// 0));
// }
}

void SSLSocket::validateParams(const SSLParams & ssl_params) {
Expand Down
28 changes: 23 additions & 5 deletions ut/ssl_ut.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,16 @@
/** Collection of integration tests that validate TLS-connectivity to a CH server
*/
#include "readonly_client_test.h"
#include "connection_failed_client_test.h"
#include "utils.h"

#include <openssl/tls1.h>
#include <clickhouse/base/sslsocket.h>
#include <openssl/ssl2.h>
#include <openssl/ssl3.h>
#include <openssl/tls1.h>

#include <cerrno>
#include <cstring>

#include "connection_failed_client_test.h"
#include "readonly_client_test.h"
#include "utils.h"

namespace {
using namespace clickhouse;
Expand Down Expand Up @@ -123,6 +127,20 @@ TEST(OpenSSLConfiguration, InValidValues) {
}
}

TEST(SSLContext, MissingCAFileErrorNamesFileAndReason) {
const std::string path = "/nonexistent/clickhouse-cpp-ut/ca.pem";
SSLParams params{};
params.path_to_ca_files = {path};

try {
SSLContext context(params);
FAIL() << "Expected OpenSSLError for a missing CA file";
} catch (const OpenSSLError& e) {
const std::string what = e.what();
EXPECT_NE(what.find(path), std::string::npos) << what;
EXPECT_NE(what.find(std::strerror(ENOENT)), std::string::npos) << what;
}
}

//INSTANTIATE_TEST_SUITE_P(
// Remote_GH_API_TLS_WithStringConfig, ReadonlyClientTest,
Expand Down
Loading