More parameters for tls layer in protocols #63985

ianton-ru · 2024-05-16T17:04:21Z

Changelog category (leave one):

Improvement

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

In composable protocols TLS layer accepted only certificateFile and privateKeyFile parameters.
https://clickhouse.com/docs/en/operations/settings/composable-protocols

This PR adds other parameters:

caConfig
verificationMode
verificationDepth
loadDefaultCAFile
cipherList/cypherList
requireTLSv1, requireTLSv1_1, requireTLSv1_2
dhParamsFile
ecdhCurve
disableProtocols
extendedVerification
preferServerCiphers

For backwards compatibility parameters used only when both certificateFile and privateKeyFile present, caConfig by default from defaultServerContext, other parameters empty by default.

Also added reusing Context for additional ports instead of recreate on each request and support in CertificateReloader.

robot-ch-test-poll4 · 2024-05-16T20:34:26Z

This is an automated comment for commit 983fa64 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
A Sync	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	⏳ pending
CI running	A meta-check that indicates the running CI. Normally, it's in success or pending state. The failed status indicates some problems with the PR	⏳ pending
Mergeable Check	Checks if all other necessary checks are successful	❌ failure
Stateful tests	Runs stateful functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure
Stateless tests	Runs stateless functional tests for ClickHouse binaries built in various configurations -- release, debug, with sanitizers, etc	❌ failure
Stress test	Runs stateless functional tests concurrently from several clients to detect concurrency-related errors	❌ failure
Upgrade check	Runs stress tests on server version from last release and then tries to upgrade it to the version from the PR. It checks if the new server can successfully startup without any errors, crashes or sanitizer asserts	❌ failure

Successful checks

Check name	Description	Status
AST fuzzer	Runs randomly generated queries to catch program errors. The build type is optionally given in parenthesis. If it fails, ask a maintainer for help	✅ success
ClickBench	Runs [ClickBench](https://github.com/ClickHouse/ClickBench/) with instant-attach table	✅ success
ClickHouse build check	Builds ClickHouse in various configurations for use in further steps. You have to fix the builds that fail. Build logs often has enough information to fix the error, but you might have to reproduce the failure locally. The cmake options can be found in the build log, grepping for cmake. Use these options and follow the general build process	✅ success
Compatibility check	Checks that clickhouse binary runs on distributions with old libc versions. If it fails, ask a maintainer for help	✅ success
Docker keeper image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docker server image	The check to build and optionally push the mentioned image to docker hub	✅ success
Docs check	Builds and tests the documentation	✅ success
Fast test	Normally this is the first check that is ran for a PR. It builds ClickHouse and runs most of stateless functional tests, omitting some. If it fails, further checks are not started until it is fixed. Look at the report to see which tests fail, then reproduce the failure locally as described here	✅ success
Flaky tests	Checks if new added or modified tests are flaky by running them repeatedly, in parallel, with more randomization. Functional tests are run 100 times with address sanitizer, and additional randomization of thread scheduling. Integration tests are run up to 10 times. If at least once a new test has failed, or was too long, this check will be red. We don't allow flaky tests, read the doc	✅ success
Install packages	Checks that the built packages are installable in a clear environment	✅ success
Integration tests	The integration tests report. In parenthesis the package type is given, and in square brackets are the optional part/total tests	✅ success
PR Check	There's no description for the check yet, please add it to tests/ci/ci_config.py:CHECK_DESCRIPTIONS	✅ success
Performance Comparison	Measure changes in query performance. The performance test report is described in detail here. In square brackets are the optional part/total tests	✅ success
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	✅ success
Unit tests	Runs the unit tests for different release types	✅ success

ianton-ru · 2024-05-17T12:01:09Z

src/Server/TLSHandler.cpp

+    , stack_data(stack_data_)
+{
+#if USE_SSL
+    params.privateKeyFile = config.getString(prefix + SSLManager::CFG_PRIV_KEY_FILE, "");


The same logic as in Poco::Net::SSLManager::initDefaultContext method.

Possible that better use settings for default context everywhere as default (as for caLocation), but this breaks backwards compatibility for everyone who already uses tls protocol.

ianton-ru · 2024-05-17T12:04:29Z

base/poco/NetSSL_OpenSSL/include/Poco/Net/SSLManager.h

@@ -258,6 +258,40 @@ namespace Net
        static const std::string CFG_SERVER_PREFIX;
        static const std::string CFG_CLIENT_PREFIX;

+        static const std::string CFG_PRIV_KEY_FILE;


Move from private to public section to use same names in TLSHandler.

ianton-ru · 2024-05-17T12:05:46Z

tests/integration/test_composable_protocols/test.py

@@ -84,6 +101,49 @@ def test_connections():

    assert execute_query_https(server.ip_address, 8444, "SELECT 1") == "1\n"

+    warnings.filterwarnings("ignore", category=DeprecationWarning)


Suppress python warning about deprecated tls versions (TLSv1_1 and older)

ianton-ru · 2024-05-17T12:06:29Z

tests/integration/test_composable_protocols/test.py

@@ -84,6 +101,49 @@ def test_connections():

    assert execute_query_https(server.ip_address, 8444, "SELECT 1") == "1\n"

+    warnings.filterwarnings("ignore", category=DeprecationWarning)
+


I don't check how OpenSSL works, only that it gets new parameters.

ianton-ru · 2024-05-17T12:48:10Z

Can't understand, what wrong with style in marked lines.
https://github.com/ClickHouse/ClickHouse/actions/runs/9128220719/job/25100164305?pr=63985

UPD. Tabs instead of spaces...

ianton-ru · 2024-05-22T16:24:33Z

src/Server/CertificateReloader.cpp

-    auto* ctx = Poco::Net::SSLManager::instance().defaultServerContext()->sslContext();
-    SSL_CTX_set_cert_cb(ctx, callSetCertificate, nullptr);
-    init_was_not_made = false;
+    SSL_CTX_set_cert_cb(ctx, callSetCertificate, reinterpret_cast<void *>(pdata));


MultiData object must not moved after creation, so pointer can be passed as callback parameter.

ianton-ru · 2024-05-22T16:28:28Z

src/Server/CertificateReloader.cpp

+void CertificateReloader::tryReloadAll(const Poco::Util::AbstractConfiguration & config)
+{
+    std::unique_lock<std::mutex> lock(data_mutex);
+    for (auto & item : data_index)


Default server context is keeped here with openSSL.server. as key. Customs contexts - with protocols.<name>. names.

More parameters for tls layer in protocols

92df0f9

antaljanosbenjamin added the can be tested Allows running workflows for external contributors label May 16, 2024

robot-ch-test-poll4 added the pr-improvement Pull request with some product improvements label May 16, 2024

Fix test style

aa61102

ianton-ru commented May 17, 2024

View reviewed changes

Fix variable name style

20b23b0

ianton-ru added 6 commits May 17, 2024 14:53

Fix tabs instead of spaces

183223b

Fix non-SSL build

222dbe3

Fix non-SSL build 2

7228561

Fix non-SSL build 3

f4afaf7

Fix non-SSL build 4

f01386f

Merge master

b838992

jiebinn mentioned this pull request May 20, 2024

Limit the array index of FixedHashTable by min/max #62746

Merged

30 tasks

ianton-ru added 3 commits May 20, 2024 14:38

Merge master

b752f2b

Fix tidy build

40eef7f

Use CertificateReloader in TLSHandler

ca45acd

ianton-ru force-pushed the protocols-tls-extend-params branch from d2fa977 to e5fe443 Compare May 22, 2024 11:49

Fix style

298b19c

ianton-ru force-pushed the protocols-tls-extend-params branch 3 times, most recently from 5bf6534 to a946f4f Compare May 22, 2024 16:18

ianton-ru commented May 22, 2024

View reviewed changes

ianton-ru force-pushed the protocols-tls-extend-params branch 2 times, most recently from ac6d792 to ddad7cf Compare May 23, 2024 12:33

Fix CertificateReloader usage for custom protocols

983fa64

ianton-ru force-pushed the protocols-tls-extend-params branch from ddad7cf to 983fa64 Compare May 23, 2024 15:53

CheSema self-assigned this May 24, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More parameters for tls layer in protocols #63985

More parameters for tls layer in protocols #63985

ianton-ru commented May 16, 2024 •

edited

robot-ch-test-poll4 commented May 16, 2024 •

edited by robot-clickhouse-ci-1

ianton-ru May 17, 2024 •

edited

ianton-ru May 17, 2024

ianton-ru May 17, 2024

ianton-ru May 17, 2024

ianton-ru commented May 17, 2024 •

edited

ianton-ru May 22, 2024

ianton-ru May 22, 2024 •

edited

		@@ -84,6 +101,49 @@ def test_connections():

		assert execute_query_https(server.ip_address, 8444, "SELECT 1") == "1\n"

		warnings.filterwarnings("ignore", category=DeprecationWarning)

More parameters for tls layer in protocols #63985

Are you sure you want to change the base?

More parameters for tls layer in protocols #63985

Conversation

ianton-ru commented May 16, 2024 • edited

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

robot-ch-test-poll4 commented May 16, 2024 • edited by robot-clickhouse-ci-1

ianton-ru May 17, 2024 • edited

Choose a reason for hiding this comment

ianton-ru May 17, 2024

Choose a reason for hiding this comment

ianton-ru May 17, 2024

Choose a reason for hiding this comment

ianton-ru May 17, 2024

Choose a reason for hiding this comment

ianton-ru commented May 17, 2024 • edited

ianton-ru May 22, 2024

Choose a reason for hiding this comment

ianton-ru May 22, 2024 • edited

Choose a reason for hiding this comment

ianton-ru commented May 16, 2024 •

edited

robot-ch-test-poll4 commented May 16, 2024 •

edited by robot-clickhouse-ci-1

ianton-ru May 17, 2024 •

edited

ianton-ru commented May 17, 2024 •

edited

ianton-ru May 22, 2024 •

edited