From 3c47f3df4b11b6824aec87a9cffb51c97683cccb Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 13 Sep 2024 13:34:21 +0200 Subject: [PATCH 1/3] Support more advanced SSL options for Keeper internal communication --- .../poco/Crypto/include/Poco/Crypto/EVPPKey.h | 5 +- .../NetSSL_OpenSSL/include/Poco/Net/Context.h | 10 + base/poco/NetSSL_OpenSSL/src/Context.cpp | 5 + src/Coordination/KeeperServer.cpp | 60 +++++- src/Server/CertificateReloader.cpp | 12 +- src/Server/CertificateReloader.h | 3 + .../configs/WithPassPhrase.crt | 20 ++ .../configs/WithPassPhrase.key | 30 +++ .../{server.crt => WithoutPassPhrase.crt} | 0 .../{server.key => WithoutPassPhrase.key} | 0 .../configs/enable_secure_keeper1.xml | 1 + .../configs/enable_secure_keeper2.xml | 1 + .../configs/enable_secure_keeper3.xml | 1 + .../configs/ssl_conf.xml | 15 -- .../configs/ssl_conf.yml | 10 + .../configs/ssl_conf_password.yml | 14 ++ .../test_keeper_internal_secure/ssl_conf.yml | 0 .../test_keeper_internal_secure/test.py | 190 +++++++++++++----- 18 files changed, 303 insertions(+), 74 deletions(-) create mode 100644 tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.crt create mode 100644 tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.key rename tests/integration/test_keeper_internal_secure/configs/{server.crt => WithoutPassPhrase.crt} (100%) rename tests/integration/test_keeper_internal_secure/configs/{server.key => WithoutPassPhrase.key} (100%) delete mode 100644 tests/integration/test_keeper_internal_secure/configs/ssl_conf.xml create mode 100644 tests/integration/test_keeper_internal_secure/configs/ssl_conf.yml create mode 100644 tests/integration/test_keeper_internal_secure/configs/ssl_conf_password.yml create mode 100644 tests/integration/test_keeper_internal_secure/ssl_conf.yml diff --git a/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h b/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h index acc79ec92b2..c33e0ae847f 100644 --- a/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h +++ b/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h @@ -188,8 +188,9 @@ namespace Crypto pFile = fopen(keyFile.c_str(), "r"); if (pFile) { - pem_password_cb * pCB = pass.empty() ? (pem_password_cb *)0 : &passCB; - void * pPassword = pass.empty() ? (void *)0 : (void *)pass.c_str(); + pem_password_cb * pCB = &passCB; + static constexpr char * no_password = ""; + void * pPassword = pass.empty() ? (void *)no_password : (void *)pass.c_str(); if (readFunc(pFile, &pKey, pCB, pPassword)) { fclose(pFile); diff --git a/base/poco/NetSSL_OpenSSL/include/Poco/Net/Context.h b/base/poco/NetSSL_OpenSSL/include/Poco/Net/Context.h index c19eecf5c73..2c56875835e 100644 --- a/base/poco/NetSSL_OpenSSL/include/Poco/Net/Context.h +++ b/base/poco/NetSSL_OpenSSL/include/Poco/Net/Context.h @@ -248,6 +248,9 @@ namespace Net SSL_CTX * sslContext() const; /// Returns the underlying OpenSSL SSL Context object. + SSL_CTX * takeSslContext(); + /// Takes ownership of the underlying OpenSSL SSL Context object. + Usage usage() const; /// Returns whether the context is for use by a client or by a server /// and whether TLSv1 is required. @@ -401,6 +404,13 @@ namespace Net return _pSSLContext; } + inline SSL_CTX * Context::takeSslContext() + { + auto * result = _pSSLContext; + _pSSLContext = nullptr; + return result; + } + inline bool Context::extendedCertificateVerificationEnabled() const { diff --git a/base/poco/NetSSL_OpenSSL/src/Context.cpp b/base/poco/NetSSL_OpenSSL/src/Context.cpp index da1c121286b..69c88eef63a 100644 --- a/base/poco/NetSSL_OpenSSL/src/Context.cpp +++ b/base/poco/NetSSL_OpenSSL/src/Context.cpp @@ -106,6 +106,11 @@ Context::Context( Context::~Context() { + if (_pSSLContext == nullptr) + { + return; + } + try { SSL_CTX_free(_pSSLContext); diff --git a/src/Coordination/KeeperServer.cpp b/src/Coordination/KeeperServer.cpp index f09ea56391a..8bfb6629fee 100644 --- a/src/Coordination/KeeperServer.cpp +++ b/src/Coordination/KeeperServer.cpp @@ -28,6 +28,15 @@ #include #include +#if USE_SSL +# include +# include +# include +# include +# include +# include +#endif + #include #include #include @@ -48,6 +57,7 @@ namespace ErrorCodes extern const int SUPPORT_IS_DISABLED; extern const int LOGICAL_ERROR; extern const int INVALID_CONFIG_PARAMETER; + extern const int BAD_ARGUMENTS; } using namespace std::chrono_literals; @@ -56,6 +66,16 @@ namespace { #if USE_SSL + +int callSetCertificate(SSL * ssl, void * arg) +{ + if (!arg) + return -1; + + const CertificateReloader::Data * data = reinterpret_cast(arg); + return setCertificateCallback(ssl, data, getLogger("SSLContext")); +} + void setSSLParams(nuraft::asio_service::options & asio_opts) { const Poco::Util::LayeredConfiguration & config = Poco::Util::Application::instance().config(); @@ -69,18 +89,42 @@ void setSSLParams(nuraft::asio_service::options & asio_opts) if (!config.has(private_key_file_property)) throw Exception(ErrorCodes::NO_ELEMENTS_IN_CONFIG, "Server private key file is not set."); - asio_opts.enable_ssl_ = true; - asio_opts.server_cert_file_ = config.getString(certificate_file_property); - asio_opts.server_key_file_ = config.getString(private_key_file_property); + Poco::Net::Context::Params params; + params.certificateFile = config.getString(certificate_file_property); + if (params.certificateFile.empty()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Server certificate file in config '{}' is empty", certificate_file_property); + + params.privateKeyFile = config.getString(private_key_file_property); + if (params.privateKeyFile.empty()) + throw Exception(ErrorCodes::BAD_ARGUMENTS, "Server key file in config '{}' is empty", private_key_file_property); + + auto pass_phrase = config.getString("openSSL.server.privateKeyPassphraseHandler.options.password", ""); + auto certificate_data = std::make_shared(params.certificateFile, params.privateKeyFile, pass_phrase); if (config.has(root_ca_file_property)) - asio_opts.root_cert_file_ = config.getString(root_ca_file_property); + params.caLocation = config.getString(root_ca_file_property); - if (config.getBool("openSSL.server.loadDefaultCAFile", false)) - asio_opts.load_default_ca_file_ = true; + params.loadDefaultCAs = config.getBool("openSSL.server.loadDefaultCAFile", false); + params.verificationMode = Poco::Net::Utility::convertVerificationMode(config.getString("openSSL.server.verificationMode", "none")); - if (config.getString("openSSL.server.verificationMode", "none") == "none") - asio_opts.skip_verification_ = true; + asio_opts.ssl_context_provider_server_ = [ctx_params = params, certificate_data] + { + Poco::Net::Context context(Poco::Net::Context::Usage::TLSV1_2_SERVER_USE, ctx_params); + SSL_CTX * ssl_ctx = context.takeSslContext(); + uint64_t options = 0; + options |= SSL_OP_ALL; + options |= SSL_OP_NO_SSLv2; + options |= SSL_OP_SINGLE_DH_USE; + SSL_CTX_set_options(ssl_ctx, options); + SSL_CTX_set_cert_cb(ssl_ctx, callSetCertificate, reinterpret_cast(certificate_data.get())); + return ssl_ctx; + }; + + asio_opts.ssl_context_provider_client_ = [ctx_params = std::move(params)] + { + Poco::Net::Context context(Poco::Net::Context::Usage::TLSV1_2_CLIENT_USE, ctx_params); + return context.takeSslContext(); + }; } #endif diff --git a/src/Server/CertificateReloader.cpp b/src/Server/CertificateReloader.cpp index df7b6e7fbd7..5b981fc7a87 100644 --- a/src/Server/CertificateReloader.cpp +++ b/src/Server/CertificateReloader.cpp @@ -34,8 +34,12 @@ int CertificateReloader::setCertificate(SSL * ssl, const CertificateReloader::Mu auto current = pdata->data.get(); if (!current) return -1; + return setCertificateCallback(ssl, current.get(), log); +} - if (current->certs_chain.empty()) +int setCertificateCallback(SSL * ssl, const CertificateReloader::Data * current_data, LoggerPtr log) +{ + if (current_data->certs_chain.empty()) return -1; if (auto err = SSL_clear_chain_certs(ssl); err != 1) @@ -43,12 +47,12 @@ int CertificateReloader::setCertificate(SSL * ssl, const CertificateReloader::Mu LOG_ERROR(log, "Clear certificates {}", Poco::Net::Utility::getLastError()); return -1; } - if (auto err = SSL_use_certificate(ssl, const_cast(current->certs_chain[0].certificate())); err != 1) + if (auto err = SSL_use_certificate(ssl, const_cast(current_data->certs_chain[0].certificate())); err != 1) { LOG_ERROR(log, "Use certificate {}", Poco::Net::Utility::getLastError()); return -1; } - for (auto cert = current->certs_chain.begin() + 1; cert != current->certs_chain.end(); cert++) + for (auto cert = current_data->certs_chain.begin() + 1; cert != current_data->certs_chain.end(); cert++) { if (auto err = SSL_add1_chain_cert(ssl, const_cast(cert->certificate())); err != 1) { @@ -56,7 +60,7 @@ int CertificateReloader::setCertificate(SSL * ssl, const CertificateReloader::Mu return -1; } } - if (auto err = SSL_use_PrivateKey(ssl, const_cast(static_cast(current->key))); err != 1) + if (auto err = SSL_use_PrivateKey(ssl, const_cast(static_cast(current_data->key))); err != 1) { LOG_ERROR(log, "Use private key {}", Poco::Net::Utility::getLastError()); return -1; diff --git a/src/Server/CertificateReloader.h b/src/Server/CertificateReloader.h index 7472d2f6baa..28737988fdd 100644 --- a/src/Server/CertificateReloader.h +++ b/src/Server/CertificateReloader.h @@ -104,6 +104,9 @@ private: mutable std::mutex data_mutex; }; +/// A callback for OpenSSL +int setCertificateCallback(SSL * ssl, const CertificateReloader::Data * current_data, LoggerPtr log); + } #endif diff --git a/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.crt b/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.crt new file mode 100644 index 00000000000..cabc53fa809 --- /dev/null +++ b/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.crt @@ -0,0 +1,20 @@ +-----BEGIN CERTIFICATE----- +MIIDPDCCAiQCFBXNOvsLA+dqmX/TkYG9JXdD5m72MA0GCSqGSIb3DQEBCwUAMFox +CzAJBgNVBAYTAkFVMRMwEQYDVQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRl +cm5ldCBXaWRnaXRzIFB0eSBMdGQxEzARBgNVBAMMCmNsaWNraG91c2UwIBcNMjIw +NDIxMTAzNDU1WhgPMjEyMjAzMjgxMDM0NTVaMFkxCzAJBgNVBAYTAkFVMRMwEQYD +VQQIDApTb21lLVN0YXRlMSEwHwYDVQQKDBhJbnRlcm5ldCBXaWRnaXRzIFB0eSBM +dGQxEjAQBgNVBAMMCWxvY2FsaG9zdDCCASIwDQYJKoZIhvcNAQEBBQADggEPADCC +AQoCggEBAKaXz596N4NC2zZdIqdwZbSYAtNdBCsBVPt5YT9F640aF5zOogPZyxGP +ENyOZwABi/7HhwFbH657xyRvi8lTau8dZL+0tbakyoIn1Tw6j+/3GXTjLduJSy6C +mOf4OzsrFC8mYgU+7p5ijvWVlO9h5NMbLdAPSIB5WSHhmSORH5LgjoK6oMOYdRod +GmfHqSbwPVwy3Li5SXlniCQmJsM0zl64LFbJ/NU+13qETmhBiDgmh0Svi+wzSzqZ +q1PIX92T3k44IXNZbvF7lKbUOS9Xb3BoxA4cDqRcTx4x73xRDwodSmqiuQOC99HI +A0C/tZJ25VNAGjLKukPSHqYscq2PAsUCAwEAATANBgkqhkiG9w0BAQsFAAOCAQEA +IDQwjf/ja3TfOXrz+Gn1eErSKnWS3asjRT9rYWQsy3tzVUkMIcszrG+FqTR16g5H +ZWyuEOi6KIRmda3SYKdLKmtQLrgx6/d/jvH5TQ0LTFZrp6vh0lo3pV+L6fLo1ZRD +V1i8jW/7HHNyqJamUXOjwA0DpPOMkdtwuyV+rJ+2bTG1ZSK33O4Ae2CY5+dad6zy +YI6b1c9flWfDznuNEMH7jDDjKgXwjZGeU53FiuuhHiNyRchsr/B9eIBsom8oykiD +kch4cnAxx2E+m3hLYzupkXHOVQ5CNpVk8PGUCIGcyqDxPt+fOj1WbDQ9laEcfhmV +kR+vHmzOqWZnHU4QeMqDig== +-----END CERTIFICATE----- diff --git a/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.key b/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.key new file mode 100644 index 00000000000..1e29a4c8fa1 --- /dev/null +++ b/tests/integration/test_keeper_internal_secure/configs/WithPassPhrase.key @@ -0,0 +1,30 @@ +-----BEGIN RSA PRIVATE KEY----- +Proc-Type: 4,ENCRYPTED +DEK-Info: AES-256-CBC,4E14FF586022476CD22AAFB662BB0E40 + +dpJZKq5k+fMuC7XECfTSRjPeOEl9wNuVtZkcjEWaHN8ky4umND7ARyRyuU1Nk7cy +fpCFlFKOqDfCkT5zVK/fB6pF32wqAI7sqeSuYPfQY0+L77yRdwM6L46WslzVKZYE +lXD1AmqKT/LgF3+eBY5slkAAJo10zYDgKEwnoQVBp31YW2/+6oAGaY/O6x3p7aTG +dw9CP+SFc0o8lPl1lsSovdNXDUiVCftvClog7hwyDv8AhHyGgynw3UJXX8UlyWu+ +Zz5zpgrvB2gvDLeoZZ6qjMGvtpEwlYBh4de9ZOsvQUpXEEfkQFtJV0j613OCQune +LoTxKpYV1V/mZX4HPaJ1oC0OJ7XcliAOSS9K49YobTuz7Kg5Wg3bVGo9xRfYDjch +rVeMP4u5RXSGuHL23FPMfK6EqcldrFyRwLaY/IV1Yl6UNUMKAphn/WMtWVuT3TiT +QMCI2VRt7ItwZwwFn5RgyDweWdFf5v3AmN/lOhATDBqosahkPxDZ1VZ6OBPoJLPM +UrDWH/lqrByeEjtAOwr5UsWKwLuJ8qUxQ4TchHwFKOwy6VsrRwMQ3ZWi2govPF9I +W0sfLj5Ulfjx6zHdqnF48a1Elit4JH6inBEWFuj7bmlOotq+PHoeT61zAwW+gnrG +3JTo3XnaE2WwRDpqvKYHWLv/J218rq8PtIaq9gjr55odPfIt8lkJ1XzF4WQ21rIJ +GNWZ3xz4fxpvrKnQyAKGu0ZcdjA1nqs16oiVr+UnJoXmkM5yBCic4fZYwPTSQHYS +ZxwaTzEjfeGxrSeLrN9CgoweucvogOvUjJOBcW/py80du8vWz0YyzMhg3o0YeGME +C+Kts/YWxmyfw4DaWt8RtWCKl85hEmz8RODvkMLGtLzvVoSyLQWqp1NhGIlFtzXs +7sPLulUeyD2avTC/RB/Pu9Nk80c0368BxCoeYbiFWZpaN70SJmCUE5H59J2d0olw +5v2RVjLBi8wqnzoa0+2L8wnG7IQGadS97dj0eBR+JXNtoJhVrurS80RJ6B0bNxdu +gX8otfnJYsZyK5hbEhcQqLdnyGhDEE8YHe7Hv9stWwLAFOfOMzyzC06lFS1eNiw4 +FJyXJUhDieb8EqetouAC8dNVXz4Q1zOTlGuAbGoKm5v0U5IhCQap9GUSW5QiUgOQ +AEMs9aGfd91R+IcDf19mZptsQLYA6MGBN6fm+3O2iZImKIbF+ZZo0S6liFFmn6lm +M+diTzaoiqgEkiXOuRhdQUMaiGV8BMZxv8qUH6/vyC3gSueoTio0f9PfASDYfvXD +A3GuI87P6LF1it2UlN6ssFoXTZdfQQZwRmNuqOqw+BJOJHrR6trcXOCZOQ77Qnvd +M5a348gIzluVUkExAPGCsySQWMx4Of5NBF28jEC3+TAwkRqBV2ZHmfGLWnvwaB+A +YUeKtpWblfG1lsrDAdwL2dilU95oby+35sExX7M2dCrL9Y2P5oTCW3u12//ZSLeL +Yhi1Rzol6LAuesZCVF0Zv/YYDhzAckJfT/qXK5B5pz9saswxCUBEpiKlLpVsjOFJ +2bHm8NgOMD5b3cdh1kvts4wZe+giry7LHsn46f+9VqN+gA6XxeVsPyb4uO1KW3SN +-----END RSA PRIVATE KEY----- diff --git a/tests/integration/test_keeper_internal_secure/configs/server.crt b/tests/integration/test_keeper_internal_secure/configs/WithoutPassPhrase.crt similarity index 100% rename from tests/integration/test_keeper_internal_secure/configs/server.crt rename to tests/integration/test_keeper_internal_secure/configs/WithoutPassPhrase.crt diff --git a/tests/integration/test_keeper_internal_secure/configs/server.key b/tests/integration/test_keeper_internal_secure/configs/WithoutPassPhrase.key similarity index 100% rename from tests/integration/test_keeper_internal_secure/configs/server.key rename to tests/integration/test_keeper_internal_secure/configs/WithoutPassPhrase.key diff --git a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper1.xml b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper1.xml index 986b503ebe3..dabf280bc36 100644 --- a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper1.xml +++ b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper1.xml @@ -1,5 +1,6 @@ + 0 9181 1 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper2.xml b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper2.xml index 652b1992f46..21d8f1e0eb3 100644 --- a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper2.xml +++ b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper2.xml @@ -1,5 +1,6 @@ + 0 9181 2 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper3.xml b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper3.xml index 6507f97473b..5d8cfb8b3e7 100644 --- a/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper3.xml +++ b/tests/integration/test_keeper_internal_secure/configs/enable_secure_keeper3.xml @@ -1,5 +1,6 @@ + 0 9181 3 /var/lib/clickhouse/coordination/log diff --git a/tests/integration/test_keeper_internal_secure/configs/ssl_conf.xml b/tests/integration/test_keeper_internal_secure/configs/ssl_conf.xml deleted file mode 100644 index 37eb2624b1b..00000000000 --- a/tests/integration/test_keeper_internal_secure/configs/ssl_conf.xml +++ /dev/null @@ -1,15 +0,0 @@ - - - - - /etc/clickhouse-server/config.d/server.crt - /etc/clickhouse-server/config.d/server.key - /etc/clickhouse-server/config.d/rootCA.pem - true - none - true - sslv2,sslv3 - true - - - diff --git a/tests/integration/test_keeper_internal_secure/configs/ssl_conf.yml b/tests/integration/test_keeper_internal_secure/configs/ssl_conf.yml new file mode 100644 index 00000000000..a444122b09d --- /dev/null +++ b/tests/integration/test_keeper_internal_secure/configs/ssl_conf.yml @@ -0,0 +1,10 @@ +openSSL: + server: + certificateFile: '/etc/clickhouse-server/config.d/WithoutPassPhrase.crt' + privateKeyFile: '/etc/clickhouse-server/config.d/WithoutPassPhrase.key' + caConfig: '/etc/clickhouse-server/config.d/rootCA.pem' + loadDefaultCAFile: true + verificationMode: 'none' + cacheSessions: true + disableProtocols: 'sslv2,sslv3' + preferServerCiphers: true diff --git a/tests/integration/test_keeper_internal_secure/configs/ssl_conf_password.yml b/tests/integration/test_keeper_internal_secure/configs/ssl_conf_password.yml new file mode 100644 index 00000000000..51b65c5253a --- /dev/null +++ b/tests/integration/test_keeper_internal_secure/configs/ssl_conf_password.yml @@ -0,0 +1,14 @@ +openSSL: + server: + certificateFile: '/etc/clickhouse-server/config.d/WithoutPassPhrase.crt' + privateKeyFile: '/etc/clickhouse-server/config.d/WithoutPassPhrase.key' + privateKeyPassphraseHandler: + name: KeyFileHandler + options: + password: 'PASSWORD' + caConfig: '/etc/clickhouse-server/config.d/rootCA.pem' + loadDefaultCAFile: true + verificationMode: 'none' + cacheSessions: true + disableProtocols: 'sslv2,sslv3' + preferServerCiphers: true diff --git a/tests/integration/test_keeper_internal_secure/ssl_conf.yml b/tests/integration/test_keeper_internal_secure/ssl_conf.yml new file mode 100644 index 00000000000..e69de29bb2d diff --git a/tests/integration/test_keeper_internal_secure/test.py b/tests/integration/test_keeper_internal_secure/test.py index 2d45e95e4ff..af511a60636 100644 --- a/tests/integration/test_keeper_internal_secure/test.py +++ b/tests/integration/test_keeper_internal_secure/test.py @@ -2,44 +2,55 @@ import pytest from helpers.cluster import ClickHouseCluster -import random -import string +import helpers.keeper_utils as ku +from multiprocessing.dummy import Pool import os -import time +CURRENT_TEST_DIR = os.path.dirname(os.path.abspath(__file__)) cluster = ClickHouseCluster(__file__) -node1 = cluster.add_instance( - "node1", - main_configs=[ - "configs/enable_secure_keeper1.xml", - "configs/ssl_conf.xml", - "configs/server.crt", - "configs/server.key", - "configs/rootCA.pem", - ], -) -node2 = cluster.add_instance( - "node2", - main_configs=[ - "configs/enable_secure_keeper2.xml", - "configs/ssl_conf.xml", - "configs/server.crt", - "configs/server.key", - "configs/rootCA.pem", - ], -) -node3 = cluster.add_instance( - "node3", - main_configs=[ - "configs/enable_secure_keeper3.xml", - "configs/ssl_conf.xml", - "configs/server.crt", - "configs/server.key", - "configs/rootCA.pem", - ], -) +nodes = [ + cluster.add_instance( + "node1", + main_configs=[ + "configs/enable_secure_keeper1.xml", + "configs/ssl_conf.yml", + "configs/WithoutPassPhrase.crt", + "configs/WithoutPassPhrase.key", + "configs/WithPassPhrase.crt", + "configs/WithPassPhrase.key", + "configs/rootCA.pem", + ], + stay_alive=True, + ), + cluster.add_instance( + "node2", + main_configs=[ + "configs/enable_secure_keeper2.xml", + "configs/ssl_conf.yml", + "configs/WithoutPassPhrase.crt", + "configs/WithoutPassPhrase.key", + "configs/WithPassPhrase.crt", + "configs/WithPassPhrase.key", + "configs/rootCA.pem", + ], + stay_alive=True, + ), + cluster.add_instance( + "node3", + main_configs=[ + "configs/enable_secure_keeper3.xml", + "configs/ssl_conf.yml", + "configs/WithoutPassPhrase.crt", + "configs/WithoutPassPhrase.key", + "configs/WithPassPhrase.crt", + "configs/WithPassPhrase.key", + "configs/rootCA.pem", + ], + stay_alive=True, + ), +] -from kazoo.client import KazooClient, KazooState +from kazoo.client import KazooClient @pytest.fixture(scope="module") @@ -61,23 +72,112 @@ def get_fake_zk(nodename, timeout=30.0): return _fake_zk_instance -def test_secure_raft_works(started_cluster): +def run_test(): + node_zks = [] try: - node1_zk = get_fake_zk("node1") - node2_zk = get_fake_zk("node2") - node3_zk = get_fake_zk("node3") + for node in nodes: + node_zks.append(get_fake_zk(node.name)) - node1_zk.create("/test_node", b"somedata1") - node2_zk.sync("/test_node") - node3_zk.sync("/test_node") + node_zks[0].create("/test_node", b"somedata1") + node_zks[1].sync("/test_node") + node_zks[2].sync("/test_node") - assert node1_zk.exists("/test_node") is not None - assert node2_zk.exists("/test_node") is not None - assert node3_zk.exists("/test_node") is not None + for node_zk in node_zks: + assert node_zk.exists("/test_node") is not None finally: try: - for zk_conn in [node1_zk, node2_zk, node3_zk]: + for zk_conn in node_zks: + if zk_conn is None: + continue zk_conn.stop() zk_conn.close() except: pass + + +def setupSsl(node, filename, password): + if password is None: + node.copy_file_to_container( + os.path.join(CURRENT_TEST_DIR, "configs/ssl_conf.yml"), + "/etc/clickhouse-server/config.d/ssl_conf.yml", + ) + + node.replace_in_config( + "/etc/clickhouse-server/config.d/ssl_conf.yml", + "WithoutPassPhrase", + filename, + ) + return + + node.copy_file_to_container( + os.path.join(CURRENT_TEST_DIR, "configs/ssl_conf_password.yml"), + "/etc/clickhouse-server/config.d/ssl_conf.yml", + ) + + node.replace_in_config( + "/etc/clickhouse-server/config.d/ssl_conf.yml", + "WithoutPassPhrase", + filename, + ) + + node.replace_in_config( + "/etc/clickhouse-server/config.d/ssl_conf.yml", + "PASSWORD", + password, + ) + + +def stop_all_clickhouse(): + for node in nodes: + node.stop_clickhouse() + + for node in nodes: + node.exec_in_container(["rm", "-rf", "/var/lib/clickhouse/coordination"]) + + +def start_clickhouse(node): + node.start_clickhouse() + + +def start_all_clickhouse(): + p = Pool(3) + waiters = [] + + for node in nodes: + waiters.append(p.apply_async(start_clickhouse, args=(node,))) + + for waiter in waiters: + waiter.wait() + + for node in nodes: + ku.wait_until_connected(cluster, node) + + +def check_valid_configuration(filename, password): + stop_all_clickhouse() + for node in nodes: + setupSsl(node, filename, password) + start_all_clickhouse() + run_test() + + +def test_secure_raft_works(started_cluster): + check_valid_configuration("WithoutPassPhrase", None) + + +def test_secure_raft_works_with_password(started_cluster): + def check_invalid_configuration(filename, password): + stop_all_clickhouse() + for node in nodes: + setupSsl(node, filename, password) + + nodes[0].start_clickhouse(expected_to_fail=True) + nodes[0].contains_in_log( + "OpenSSLException: EVPKey::loadKey(string): error:0480006C:PEM routines::no start line" + ) + + check_valid_configuration("WithoutPassPhrase", "unusedpassword") + check_invalid_configuration("WithPassPhrase", "wrongpassword") + check_invalid_configuration("WithPassPhrase", "") + check_valid_configuration("WithPassPhrase", "test") + check_invalid_configuration("WithPassPhrase", None) From 9a31fc385d6335d2f21adfbfef2bb609044510ad Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Fri, 13 Sep 2024 15:58:17 +0200 Subject: [PATCH 2/3] Fixes --- src/Coordination/KeeperServer.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/Coordination/KeeperServer.cpp b/src/Coordination/KeeperServer.cpp index 48c377a0c0b..e0a94b1a00c 100644 --- a/src/Coordination/KeeperServer.cpp +++ b/src/Coordination/KeeperServer.cpp @@ -35,6 +35,7 @@ # include # include # include +# include #endif #include @@ -107,15 +108,28 @@ void setSSLParams(nuraft::asio_service::options & asio_opts) params.loadDefaultCAs = config.getBool("openSSL.server.loadDefaultCAFile", false); params.verificationMode = Poco::Net::Utility::convertVerificationMode(config.getString("openSSL.server.verificationMode", "none")); - asio_opts.ssl_context_provider_server_ = [ctx_params = params, certificate_data] + std::string disabled_protocols_list = config.getString("openSSL.server.disableProtocols", ""); + Poco::StringTokenizer dp_tok(disabled_protocols_list, ";,", Poco::StringTokenizer::TOK_TRIM | Poco::StringTokenizer::TOK_IGNORE_EMPTY); + int disabled_protocols = 0; + for (const auto & token : dp_tok) + { + if (token == "sslv2") + disabled_protocols |= Poco::Net::Context::PROTO_SSLV2; + else if (token == "sslv3") + disabled_protocols |= Poco::Net::Context::PROTO_SSLV3; + else if (token == "tlsv1") + disabled_protocols |= Poco::Net::Context::PROTO_TLSV1; + else if (token == "tlsv1_1") + disabled_protocols |= Poco::Net::Context::PROTO_TLSV1_1; + else if (token == "tlsv1_2") + disabled_protocols |= Poco::Net::Context::PROTO_TLSV1_2; + } + + asio_opts.ssl_context_provider_server_ = [ctx_params = params, certificate_data, disabled_protocols] { Poco::Net::Context context(Poco::Net::Context::Usage::TLSV1_2_SERVER_USE, ctx_params); + context.disableProtocols(disabled_protocols); SSL_CTX * ssl_ctx = context.takeSslContext(); - uint64_t options = 0; - options |= SSL_OP_ALL; - options |= SSL_OP_NO_SSLv2; - options |= SSL_OP_SINGLE_DH_USE; - SSL_CTX_set_options(ssl_ctx, options); SSL_CTX_set_cert_cb(ssl_ctx, callSetCertificate, reinterpret_cast(certificate_data.get())); return ssl_ctx; }; From 8cdcc431fe5b3cb001523b697d1348abe111d88c Mon Sep 17 00:00:00 2001 From: Antonio Andelic Date: Mon, 16 Sep 2024 09:56:31 +0200 Subject: [PATCH 3/3] Fix --- .../poco/Crypto/include/Poco/Crypto/EVPPKey.h | 14 +++++++++++++ src/Coordination/KeeperServer.cpp | 4 ++-- tests/integration/helpers/cluster.py | 2 +- .../test_keeper_internal_secure/ssl_conf.yml | 0 .../test_keeper_internal_secure/test.py | 21 ++++++++++--------- 5 files changed, 28 insertions(+), 13 deletions(-) delete mode 100644 tests/integration/test_keeper_internal_secure/ssl_conf.yml diff --git a/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h b/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h index c33e0ae847f..6e44d9f45b7 100644 --- a/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h +++ b/base/poco/Crypto/include/Poco/Crypto/EVPPKey.h @@ -226,6 +226,13 @@ namespace Crypto error: if (pFile) fclose(pFile); + if (*ppKey) + { + if constexpr (std::is_same_v) + EVP_PKEY_free(*ppKey); + else + EC_KEY_free(*ppKey); + } throw OpenSSLException("EVPKey::loadKey(string)"); } @@ -287,6 +294,13 @@ namespace Crypto error: if (pBIO) BIO_free(pBIO); + if (*ppKey) + { + if constexpr (std::is_same_v) + EVP_PKEY_free(*ppKey); + else + EC_KEY_free(*ppKey); + } throw OpenSSLException("EVPKey::loadKey(stream)"); } diff --git a/src/Coordination/KeeperServer.cpp b/src/Coordination/KeeperServer.cpp index e0a94b1a00c..2eada508e22 100644 --- a/src/Coordination/KeeperServer.cpp +++ b/src/Coordination/KeeperServer.cpp @@ -125,9 +125,9 @@ void setSSLParams(nuraft::asio_service::options & asio_opts) disabled_protocols |= Poco::Net::Context::PROTO_TLSV1_2; } - asio_opts.ssl_context_provider_server_ = [ctx_params = params, certificate_data, disabled_protocols] + asio_opts.ssl_context_provider_server_ = [params, certificate_data, disabled_protocols] { - Poco::Net::Context context(Poco::Net::Context::Usage::TLSV1_2_SERVER_USE, ctx_params); + Poco::Net::Context context(Poco::Net::Context::Usage::TLSV1_2_SERVER_USE, params); context.disableProtocols(disabled_protocols); SSL_CTX * ssl_ctx = context.takeSslContext(); SSL_CTX_set_cert_cb(ssl_ctx, callSetCertificate, reinterpret_cast(certificate_data.get())); diff --git a/tests/integration/helpers/cluster.py b/tests/integration/helpers/cluster.py index 821bb887435..4ef2699ea3b 100644 --- a/tests/integration/helpers/cluster.py +++ b/tests/integration/helpers/cluster.py @@ -4093,7 +4093,7 @@ class ClickHouseInstance: exclusion_substring="", ): if from_host: - # We check fist file exists but want to look for all rotated logs as well + # We check first file exists but want to look for all rotated logs as well result = subprocess_check_call( [ "bash", diff --git a/tests/integration/test_keeper_internal_secure/ssl_conf.yml b/tests/integration/test_keeper_internal_secure/ssl_conf.yml deleted file mode 100644 index e69de29bb2d..00000000000 diff --git a/tests/integration/test_keeper_internal_secure/test.py b/tests/integration/test_keeper_internal_secure/test.py index af511a60636..8cab03b6e2d 100644 --- a/tests/integration/test_keeper_internal_secure/test.py +++ b/tests/integration/test_keeper_internal_secure/test.py @@ -161,21 +161,22 @@ def check_valid_configuration(filename, password): run_test() +def check_invalid_configuration(filename, password): + stop_all_clickhouse() + for node in nodes: + setupSsl(node, filename, password) + + nodes[0].start_clickhouse(expected_to_fail=True) + nodes[0].wait_for_log_line( + "OpenSSLException: EVPKey::loadKey.*error:0480006C:PEM routines::no start line", + ) + + def test_secure_raft_works(started_cluster): check_valid_configuration("WithoutPassPhrase", None) def test_secure_raft_works_with_password(started_cluster): - def check_invalid_configuration(filename, password): - stop_all_clickhouse() - for node in nodes: - setupSsl(node, filename, password) - - nodes[0].start_clickhouse(expected_to_fail=True) - nodes[0].contains_in_log( - "OpenSSLException: EVPKey::loadKey(string): error:0480006C:PEM routines::no start line" - ) - check_valid_configuration("WithoutPassPhrase", "unusedpassword") check_invalid_configuration("WithPassPhrase", "wrongpassword") check_invalid_configuration("WithPassPhrase", "")