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.

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

OpenSSL updates: allow to configure or skip verification #152

Merged
merged 1 commit into from
Feb 15, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
122 changes: 90 additions & 32 deletions clickhouse/base/sslsocket.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -27,17 +27,16 @@ std::string getCertificateInfo(X509* cert)
return std::string(data, len);
}

void throwSSLError(SSL * ssl, int error, const char * /*location*/, const char * /*statement*/) {
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 = reason;
if (ssl) {
// TODO: maybe print certificate only if handshake isn't completed (SSL_get_state(ssl) != TLS_ST_OK)
if (auto ssl_session = SSL_get_session(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)
reason_str += "\nServer certificate: " + getCertificateInfo(SSL_SESSION_get0_peer(ssl_session));
}
}

// std::cerr << "!!! SSL error at " << location
Expand All @@ -46,7 +45,44 @@ void throwSSLError(SSL * ssl, int error, const char * /*location*/, const char *
// << "\n\t last err: " << ERR_peek_last_error()
// << std::endl;

throw std::runtime_error(std::string("OpenSSL error: ") + std::to_string(error) + " : " + reason_str);
throw std::runtime_error(prefix + std::to_string(error) + " : " + reason_str);
}

void configureSSL(const clickhouse::SSLParams::ConfigurationType & configuration, SSL * ssl, SSL_CTX * context = nullptr) {
std::unique_ptr<SSL_CONF_CTX, decltype(&SSL_CONF_CTX_free)> conf_ctx_holder(SSL_CONF_CTX_new(), SSL_CONF_CTX_free);
auto conf_ctx = conf_ctx_holder.get();

// To make both cmdline and flag file commands start with no prefix.
SSL_CONF_CTX_set1_prefix(conf_ctx, "");
// Allow all set of client commands, also turn on proper error reporting to reuse throwSSLError().
SSL_CONF_CTX_set_flags(conf_ctx, SSL_CONF_FLAG_CMDLINE | SSL_CONF_FLAG_FILE | SSL_CONF_FLAG_CLIENT | SSL_CONF_FLAG_SHOW_ERRORS | SSL_CONF_FLAG_CERTIFICATE );
if (ssl)
SSL_CONF_CTX_set_ssl(conf_ctx, ssl);
else if (context)
SSL_CONF_CTX_set_ssl_ctx(conf_ctx, context);

for (const auto & kv : configuration) {
const int err = SSL_CONF_cmd(conf_ctx, kv.first.c_str(), (kv.second ? kv.second->c_str() : nullptr));
// From the documentation:
// 2 - both key and value used
// 1 - only key used
// 0 - error during processing
// -2 - key not recodnized
// -3 - missing value
const bool value_present = !!kv.second;
if (err == 2 || (err == 1 && !value_present))
continue;
else if (err == 0)
throwSSLError(ssl, SSL_ERROR_NONE, nullptr, nullptr, "Failed to configure OpenSSL with command '" + kv.first + "' ");
else if (err == 1 && value_present)
throw std::runtime_error("Failed to configure OpenSSL: command '" + kv.first + "' needs no value");
else if (err == -2)
throw std::runtime_error("Failed to cofigure OpenSSL: unknown command '" + kv.first + "'");
else if (err == -3)
throw std::runtime_error("Failed to cofigure OpenSSL: command '" + kv.first + "' requires a value");
else
throw std::runtime_error("Failed to cofigure OpenSSL: command '" + kv.first + "' unknown error: " + std::to_string(err));
}
}

#define STRINGIFY_HELPER(x) #x
Expand Down Expand Up @@ -101,16 +137,28 @@ SSL_CTX * prepareSSLContext(const clickhouse::SSLParams & context_params) {
#undef HANDLE_SSL_CTX_ERROR
}

auto convertConfiguration(const decltype(clickhouse::ClientOptions::SSLOptions::configuration) & configuration)
{
auto result = decltype(clickhouse::SSLParams::configuration){};
for (const auto & cv : configuration)
result.push_back({cv.command, cv.value});

return result;
}

clickhouse::SSLParams GetSSLParams(const clickhouse::ClientOptions& opts) {
const auto& ssl_options = opts.ssl_options;
const auto& ssl_options = *opts.ssl_options;
return clickhouse::SSLParams{
ssl_options.path_to_ca_files,
ssl_options.path_to_ca_directory,
ssl_options.use_default_ca_locations,
ssl_options.context_options,
ssl_options.min_protocol_version,
ssl_options.max_protocol_version,
ssl_options.use_sni
ssl_options.use_sni,
ssl_options.skip_verification,
ssl_options.host_flags,
convertConfiguration(ssl_options.configuration)
};
}

Expand Down Expand Up @@ -141,7 +189,7 @@ SSL_CTX * SSLContext::getContext() {
} \
else \
return ret_code; \
}()
} ()

/* // debug macro for tracing SSL state
#define LOG_SSL_STATE() std::cerr << "!!!!" << LOCATION << " @" << __FUNCTION__ \
Expand All @@ -158,49 +206,59 @@ SSLSocket::SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params,
if (!ssl)
throw std::runtime_error("Failed to create SSL instance");

std::unique_ptr<ASN1_OCTET_STRING, decltype(&ASN1_OCTET_STRING_free)> ip_addr(a2i_IPADDRESS(addr.Host().c_str()), &ASN1_OCTET_STRING_free);

HANDLE_SSL_ERROR(ssl, SSL_set_fd(ssl, handle_));
if (ssl_params.use_SNI)
Copy link
Collaborator

Choose a reason for hiding this comment

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

sidenote: maybe that is not needed at all, it sounds safe to call SSL_set_tlsext_host_name always.

HANDLE_SSL_ERROR(ssl, SSL_set_tlsext_host_name(ssl, addr.Host().c_str()));

if (ssl_params.host_flags != -1)
SSL_set_hostflags(ssl, ssl_params.host_flags);
HANDLE_SSL_ERROR(ssl, SSL_set1_host(ssl, addr.Host().c_str()));

// DO NOT use SSL_set_verify(ssl, SSL_VERIFY_PEER, nullptr), since
// we check verification result later, and that provides better error message.

if (ssl_params.configuration.size() > 0)
configureSSL(ssl_params.configuration, ssl);

SSL_set_connect_state(ssl);
HANDLE_SSL_ERROR(ssl, SSL_connect(ssl));
HANDLE_SSL_ERROR(ssl, SSL_set_mode(ssl, SSL_MODE_AUTO_RETRY));
auto peer_certificate = SSL_get_peer_certificate(ssl);

if (!peer_certificate)
throw std::runtime_error("Failed to verify SSL connection: server provided no certificate.");

if (const auto verify_result = SSL_get_verify_result(ssl); verify_result != X509_V_OK) {
if (const auto verify_result = SSL_get_verify_result(ssl); !ssl_params.skip_verification && verify_result != X509_V_OK) {
auto error_message = X509_verify_cert_error_string(verify_result);
throw std::runtime_error("Failed to verify SSL connection, X509_v error: "
+ std::to_string(verify_result)
+ " " + error_message
+ "\nServer certificate: " + getCertificateInfo(peer_certificate));
+ "\nServer certificate: " + getCertificateInfo(SSL_get_peer_certificate(ssl)));
}

if (ssl_params.use_SNI) {
auto hostname = addr.Host();
char * out_name = nullptr;

std::unique_ptr<ASN1_OCTET_STRING, decltype(&ASN1_OCTET_STRING_free)> addr(a2i_IPADDRESS(hostname.c_str()), &ASN1_OCTET_STRING_free);
if (addr) {
// if hostname is actually an IP address
HANDLE_SSL_ERROR(ssl, X509_check_ip(
peer_certificate,
ASN1_STRING_get0_data(addr.get()),
ASN1_STRING_length(addr.get()),
0));
} else {
HANDLE_SSL_ERROR(ssl, X509_check_host(peer_certificate, hostname.c_str(), hostname.length(), 0, &out_name));
}
}
// 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.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, makes sense to have some safe defaults

// 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) {
// We need either SSL or SSL_CTX to properly validate configuration, so create a temporary one.
std::unique_ptr<SSL_CTX, decltype(&SSL_CTX_free)> ctx(SSL_CTX_new(TLS_client_method()), &SSL_CTX_free);
configureSSL(ssl_params.configuration, nullptr, ctx.get());
}


SSLSocketFactory::SSLSocketFactory(const ClientOptions& opts)
: NonSecureSocketFactory()
, ssl_params_(GetSSLParams(opts)) {
if (opts.ssl_options.ssl_context) {
ssl_context_ = std::make_unique<SSLContext>(*opts.ssl_options.ssl_context);
if (opts.ssl_options->ssl_context) {
ssl_context_ = std::make_unique<SSLContext>(*opts.ssl_options->ssl_context);
} else {
ssl_context_ = std::make_unique<SSLContext>(ssl_params_);
}
Expand Down
10 changes: 8 additions & 2 deletions clickhouse/base/sslsocket.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@
#include "socket.h"

#include <memory>
#include <optional>
#include <vector>

typedef struct ssl_ctx_st SSL_CTX;
typedef struct ssl_st SSL;
Expand All @@ -18,6 +20,10 @@ struct SSLParams
int min_protocol_version;
int max_protocol_version;
bool use_SNI;
bool skip_verification;
int host_flags;
using ConfigurationType = std::vector<std::pair<std::string, std::optional<std::string>>>;
ConfigurationType configuration;
};

class SSLContext
Expand All @@ -42,8 +48,7 @@ class SSLContext

class SSLSocket : public Socket {
public:
explicit SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params,
SSLContext& context);
explicit SSLSocket(const NetworkAddress& addr, const SSLParams & ssl_params, SSLContext& context);
SSLSocket(SSLSocket &&) = default;
~SSLSocket() override = default;

Expand All @@ -53,6 +58,7 @@ class SSLSocket : public Socket {
std::unique_ptr<InputStream> makeInputStream() const override;
std::unique_ptr<OutputStream> makeOutputStream() const override;

static void validateParams(const SSLParams & ssl_params);
private:
std::unique_ptr<SSL, void (*)(SSL *s)> ssl_;
};
Expand Down
17 changes: 13 additions & 4 deletions clickhouse/client.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -63,8 +63,8 @@ std::ostream& operator<<(std::ostream& os, const ClientOptions& opt) {
<< " compression_method:"
<< (opt.compression_method == CompressionMethod::LZ4 ? "LZ4" : "None");
#if defined(WITH_OPENSSL)
if (opt.ssl_options.use_ssl) {
const auto & ssl_options = opt.ssl_options;
if (opt.ssl_options) {
const auto & ssl_options = *opt.ssl_options;
os << " SSL ("
<< " ssl_context: " << (ssl_options.ssl_context ? "provided by user" : "created internally")
<< " use_default_ca_locations: " << ssl_options.use_default_ca_locations
Expand All @@ -80,13 +80,23 @@ std::ostream& operator<<(std::ostream& os, const ClientOptions& opt) {
return os;
}

ClientOptions& ClientOptions::SetSSLOptions(ClientOptions::SSLOptions options)
{
#ifdef WITH_OPENSSL
ssl_options = options;
return *this;
#else
(void)options;
throw std::runtime_error("Library was built with no SSL support");
#endif
}

namespace {

std::unique_ptr<SocketFactory> GetSocketFactory(const ClientOptions& opts) {
(void)opts;
#if defined(WITH_OPENSSL)
if (opts.ssl_options.use_ssl)
if (opts.ssl_options)
return std::make_unique<SSLSocketFactory>(opts);
else
#endif
Expand All @@ -95,7 +105,6 @@ std::unique_ptr<SocketFactory> GetSocketFactory(const ClientOptions& opts) {

}


class Client::Impl {
public:
Impl(const ClientOptions& opts);
Expand Down
46 changes: 37 additions & 9 deletions clickhouse/client.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,10 +20,9 @@
#include <memory>
#include <ostream>
#include <string>
#include <optional>

#if defined(WITH_OPENSSL)
typedef struct ssl_ctx_st SSL_CTX;
#endif

namespace clickhouse {

Expand Down Expand Up @@ -105,10 +104,7 @@ struct ClientOptions {
*/
DECLARE_FIELD(max_compression_chunk_size, unsigned int, SetMaxCompressionChunkSize, 65535);

#if defined(WITH_OPENSSL)
struct SSLOptions {
bool use_ssl = true; // not expected to be set manually.

/** There are two ways to configure an SSL connection:
* - provide a pre-configured SSL_CTX, which is not modified and not owned by the Client.
* - provide a set of options and allow the Client to create and configure SSL_CTX by itself.
Expand All @@ -118,6 +114,9 @@ struct ClientOptions {
* If NOT null client DONES NOT take ownership of context and it must be valid for client lifetime.
* If null client initlaizes OpenSSL and creates his own context, initializes it using
* other options, like path_to_ca_files, path_to_ca_directory, use_default_ca_locations, etc.
*
* Either way context is used to create an SSL-connection, which is then configured with
* whatever was provided as `configuration`, `host_flags`, `skip_verification` and `use_sni`.
*/
SSL_CTX * ssl_context = nullptr;
auto & SetExternalSSLContext(SSL_CTX * new_ssl_context) {
Expand Down Expand Up @@ -147,16 +146,45 @@ struct ClientOptions {
*/
DECLARE_FIELD(context_options, int, SetContextOptions, DEFAULT_VALUE);

/** Use SNI at ClientHello and verify that certificate is issued to the hostname we are trying to connect to
/** Use SNI at ClientHello
*/
DECLARE_FIELD(use_sni, bool, SetUseSNI, true);

/** Skip SSL session verification (server's certificate, etc).
*
* WARNING: settig to true will bypass all SSL session checks, which
* is dangerous, but can be used against self-signed certificates, e.g. for testing purposes.
*/
DECLARE_FIELD(skip_verification, bool, SetSkipVerification, false);

/** Mode of verifying host ssl certificate against name of the host, set with SSL_set_hostflags.
* For details see https://www.openssl.org/docs/man1.1.1/man3/SSL_set_hostflags.html
Enmk marked this conversation as resolved.
Show resolved Hide resolved
*/
DECLARE_FIELD(host_flags, int, SetHostVerifyFlags, DEFAULT_VALUE);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Test? For example maybe we can trigger X509_CHECK_FLAG_NO_WILDCARDS? I guess it should make the github.demo.trial.altinity.cloud stop working, because it uses wildcard cert:

depth=0 CN = *.demo.trial.altinity.cloud

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Rn, it is too hard to set up any complex tests with the tools we have

But I already asked QA team to do the in-depth tests with various tricky ssl (mis)configurations. They'll do it once they have resources.


struct CommandAndValue {
std::string command;
std::optional<std::string> value = std::nullopt;
};
/** Extra configuration options, set with SSL_CONF_cmd.
* For deatils see https://www.openssl.org/docs/man1.1.1/man3/SSL_CONF_cmd.html
*
* Takes multiple pairs of command-value strings, all commands are supported,
* and prefix is empty.
* i.e. pass `sigalgs` or `SignatureAlgorithms` instead of `-sigalgs`.
*
* Rewrites any other options/flags if set in other ways.
*/
DECLARE_FIELD(configuration, std::vector<CommandAndValue>, SetConfiguration, {});

static const int DEFAULT_VALUE = -1;
};

// By default SSL is turned off, hence the `{false}`
DECLARE_FIELD(ssl_options, SSLOptions, SetSSLOptions, {false});
#endif
// By default SSL is turned off.
std::optional<SSLOptions> ssl_options = std::nullopt;

// Will throw an exception if client was built without SSL support.
ClientOptions& SetSSLOptions(SSLOptions options);

#undef DECLARE_FIELD
};
Expand Down
1 change: 1 addition & 0 deletions tests/simple/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
ADD_EXECUTABLE (simple-test
../../ut/utils.cpp
main.cpp
)

Expand Down
Loading