From f6d30ee4588de0d80ee3ca3772a6d5f302e6118a Mon Sep 17 00:00:00 2001 From: Michael Boquard Date: Sat, 1 Jun 2024 14:04:24 -0400 Subject: [PATCH 1/3] config: Split out tls_config implementation Created tls_config.cc to hold implementation of TLS config Signed-off-by: Michael Boquard --- src/v/config/CMakeLists.txt | 1 + src/v/config/tls_config.cc | 172 ++++++++++++++++++++++++++++++++++++ src/v/config/tls_config.h | 151 ++----------------------------- src/v/rpc/rpc_utils.cc | 2 + 4 files changed, 184 insertions(+), 142 deletions(-) create mode 100644 src/v/config/tls_config.cc diff --git a/src/v/config/CMakeLists.txt b/src/v/config/CMakeLists.txt index 808b60df8642c..ef59eb05c3032 100644 --- a/src/v/config/CMakeLists.txt +++ b/src/v/config/CMakeLists.txt @@ -10,6 +10,7 @@ v_cc_library( rjson_serialization.cc validators.cc throughput_control_group.cc + tls_config.cc DEPS v::json v::model diff --git a/src/v/config/tls_config.cc b/src/v/config/tls_config.cc new file mode 100644 index 0000000000000..aed4e31fcf4aa --- /dev/null +++ b/src/v/config/tls_config.cc @@ -0,0 +1,172 @@ +/* + * Copyright 2024 Redpanda Data, Inc. + * + * Use of this software is governed by the Business Source License + * included in the file licenses/BSL.md + * + * As of the Change Date specified in that file, in accordance with + * the Business Source License, use of this software will be governed + * by the Apache License, Version 2.0 + */ + +#include "tls_config.h" + +#include "config/convert.h" +#include "utils/to_string.h" + +#include +#include + +#include + +namespace config { +ss::future> +tls_config::get_credentials_builder() const& { + if (_enabled) { + return ss::do_with( + ss::tls::credentials_builder{}, + [this](ss::tls::credentials_builder& builder) { + builder.set_priority_string("PERFORMANCE:%SERVER_PRECEDENCE"); + builder.set_dh_level(ss::tls::dh_params::level::MEDIUM); + if (_require_client_auth) { + builder.set_client_auth(ss::tls::client_auth::REQUIRE); + } + + auto f = _truststore_file ? builder.set_x509_trust_file( + *_truststore_file, ss::tls::x509_crt_format::PEM) + : builder.set_system_trust(); + + if (_crl_file) { + f = f.then([this, &builder] { + return builder.set_x509_crl_file( + *_crl_file, ss::tls::x509_crt_format::PEM); + }); + } + + if (_key_cert) { + f = f.then([this, &builder] { + return builder.set_x509_key_file( + (*_key_cert).cert_file, + (*_key_cert).key_file, + ss::tls::x509_crt_format::PEM); + }); + } + + return f.then([&builder]() { + return std::make_optional(std::move(builder)); + }); + }); + } + + return ss::make_ready_future>( + std::nullopt); +} + +ss::future> +tls_config::get_credentials_builder() && { + auto ptr = ss::make_lw_shared(std::move(*this)); + return ptr->get_credentials_builder().finally([ptr] {}); +} + +std::optional tls_config::validate(const tls_config& c) { + if (c.get_require_client_auth() && !c.get_truststore_file()) { + return "Trust store is required when client authentication is " + "enabled"; + } + + return std::nullopt; +} + +std::ostream& operator<<(std::ostream& o, const config::key_cert& c) { + o << "{ " + << "key_file: " << c.key_file << " " + << "cert_file: " << c.cert_file << " }"; + return o; +} + +std::ostream& operator<<(std::ostream& o, const config::tls_config& c) { + o << "{ " + << "enabled: " << c.is_enabled() << " " + << "key/cert files: " << c.get_key_cert_files() << " " + << "ca file: " << c.get_truststore_file() << " " + << "crl file: " << c.get_crl_file() << " " + << "client_auth_required: " << c.get_require_client_auth() << "" + << " }"; + return o; +} +} // namespace config + +namespace YAML { + +inline ss::sstring to_absolute(const ss::sstring& path) { + namespace fs = std::filesystem; + if (path.empty()) { + return path; + } + return fs::absolute(fs::path(path)).native(); +} + +inline std::optional +to_absolute(const std::optional& path) { + if (path) { + return to_absolute(*path); + } + return std::nullopt; +} + +Node convert::encode(const config::tls_config& rhs) { + Node node; + + node["enabled"] = rhs.is_enabled(); + node["require_client_auth"] = rhs.get_require_client_auth(); + + if (rhs.get_key_cert_files()) { + node["cert_file"] = (*rhs.get_key_cert_files()).key_file; + node["key_file"] = (*rhs.get_key_cert_files()).cert_file; + } + + if (rhs.get_truststore_file()) { + node["truststore_file"] = *rhs.get_truststore_file(); + } + + return node; +} + +std::optional convert::read_optional( + const Node& node, const ss::sstring& key) { + if (node[key]) { + return node[key].as(); + } + return std::nullopt; +} + +bool convert::decode( + const Node& node, config::tls_config& rhs) { + // either both true or both false + if ( + static_cast(node["key_file"]) + ^ static_cast(node["cert_file"])) { + return false; + } + auto enabled = node["enabled"] && node["enabled"].as(); + if (!enabled) { + rhs = config::tls_config( + false, std::nullopt, std::nullopt, std::nullopt, false); + } else { + auto key_cert = node["key_file"] ? std::make_optional( + config::key_cert{ + to_absolute(node["key_file"].as()), + to_absolute(node["cert_file"].as())}) + : std::nullopt; + rhs = config::tls_config( + enabled, + key_cert, + to_absolute(read_optional(node, "truststore_file")), + to_absolute(read_optional(node, "crl_file")), + node["require_client_auth"] + && node["require_client_auth"].as()); + } + return true; +} + +} // namespace YAML diff --git a/src/v/config/tls_config.h b/src/v/config/tls_config.h index 6a4181083e76f..131cf288279f2 100644 --- a/src/v/config/tls_config.h +++ b/src/v/config/tls_config.h @@ -12,21 +12,14 @@ #pragma once #include "base/seastarx.h" -#include "config/convert.h" -#include "utils/to_string.h" -#include #include -#include #include -#include #include -#include #include #include -#include namespace config { @@ -38,12 +31,7 @@ struct key_cert { return key_file == rhs.key_file && cert_file == rhs.cert_file; } - friend std::ostream& operator<<(std::ostream& o, const key_cert& c) { - o << "{ " - << "key_file: " << c.key_file << " " - << "cert_file: " << c.cert_file << " }"; - return o; - } + friend std::ostream& operator<<(std::ostream& o, const key_cert& c); }; class tls_config { @@ -80,75 +68,18 @@ class tls_config { bool get_require_client_auth() const { return _require_client_auth; } ss::future> - get_credentials_builder() const& { - if (_enabled) { - return ss::do_with( - ss::tls::credentials_builder{}, - [this](ss::tls::credentials_builder& builder) { - builder.set_priority_string("PERFORMANCE:%SERVER_PRECEDENCE"); - builder.set_dh_level(ss::tls::dh_params::level::MEDIUM); - if (_require_client_auth) { - builder.set_client_auth(ss::tls::client_auth::REQUIRE); - } - - auto f = _truststore_file ? builder.set_x509_trust_file( - *_truststore_file, ss::tls::x509_crt_format::PEM) - : builder.set_system_trust(); - - if (_crl_file) { - f = f.then([this, &builder] { - return builder.set_x509_crl_file( - *_crl_file, ss::tls::x509_crt_format::PEM); - }); - } - - if (_key_cert) { - f = f.then([this, &builder] { - return builder.set_x509_key_file( - (*_key_cert).cert_file, - (*_key_cert).key_file, - ss::tls::x509_crt_format::PEM); - }); - } - - return f.then([&builder]() { - return std::make_optional(std::move(builder)); - }); - }); - } - - return ss::make_ready_future< - std::optional>(std::nullopt); - } + + get_credentials_builder() const&; ss::future> - get_credentials_builder() && { - auto ptr = ss::make_lw_shared(std::move(*this)); - return ptr->get_credentials_builder().finally([ptr] {}); - } + get_credentials_builder() &&; - static std::optional validate(const tls_config& c) { - if (c.get_require_client_auth() && !c.get_truststore_file()) { - return "Trust store is required when client authentication is " - "enabled"; - } - - return std::nullopt; - } + static std::optional validate(const tls_config& c); bool operator==(const tls_config& rhs) const = default; friend std::ostream& - operator<<(std::ostream& o, const config::tls_config& c) { - o << "{ " - << "enabled: " << c.is_enabled() << " " - << "key/cert files: " << c.get_key_cert_files() << " " - << "ca file: " << c.get_truststore_file() << " " - << "crl file: " << c.get_crl_file() << " " - << "client_auth_required: " << c.get_require_client_auth() << "" - << " }"; - return o; - } + operator<<(std::ostream& o, const config::tls_config& c); private: bool _enabled{false}; @@ -162,78 +93,14 @@ class tls_config { namespace YAML { -inline ss::sstring to_absolute(const ss::sstring& path) { - namespace fs = std::filesystem; - if (path.empty()) { - return path; - } - return fs::absolute(fs::path(path)).native(); -} - -inline std::optional -to_absolute(const std::optional& path) { - if (path) { - return to_absolute(*path); - } - return std::nullopt; -} - template<> struct convert { - static Node encode(const config::tls_config& rhs) { - Node node; - - node["enabled"] = rhs.is_enabled(); - node["require_client_auth"] = rhs.get_require_client_auth(); - - if (rhs.get_key_cert_files()) { - node["cert_file"] = (*rhs.get_key_cert_files()).key_file; - node["key_file"] = (*rhs.get_key_cert_files()).cert_file; - } - - if (rhs.get_truststore_file()) { - node["truststore_file"] = *rhs.get_truststore_file(); - } - - return node; - } + static Node encode(const config::tls_config& rhs); static std::optional - read_optional(const Node& node, const ss::sstring& key) { - if (node[key]) { - return node[key].as(); - } - return std::nullopt; - } + read_optional(const Node& node, const ss::sstring& key); - static bool decode(const Node& node, config::tls_config& rhs) { - // either both true or both false - if ( - static_cast(node["key_file"]) - ^ static_cast(node["cert_file"])) { - return false; - } - auto enabled = node["enabled"] && node["enabled"].as(); - if (!enabled) { - rhs = config::tls_config( - false, std::nullopt, std::nullopt, std::nullopt, false); - } else { - auto key_cert - = node["key_file"] - ? std::make_optional(config::key_cert{ - to_absolute(node["key_file"].as()), - to_absolute(node["cert_file"].as())}) - : std::nullopt; - rhs = config::tls_config( - enabled, - key_cert, - to_absolute(read_optional(node, "truststore_file")), - to_absolute(read_optional(node, "crl_file")), - node["require_client_auth"] - && node["require_client_auth"].as()); - } - return true; - } + static bool decode(const Node& node, config::tls_config& rhs); }; } // namespace YAML diff --git a/src/v/rpc/rpc_utils.cc b/src/v/rpc/rpc_utils.cc index 9444a6734def2..845f12df15f25 100644 --- a/src/v/rpc/rpc_utils.cc +++ b/src/v/rpc/rpc_utils.cc @@ -11,6 +11,8 @@ #include "rpc/rpc_utils.h" +#include "base/vlog.h" + namespace rpc { void log_certificate_reload_event( From 0a94a73116bc7e36f99842e6648f403f29176f29 Mon Sep 17 00:00:00 2001 From: Michael Boquard Date: Thu, 23 May 2024 15:59:03 -0400 Subject: [PATCH 2/3] tls: Added support for OpenSSL cipher strings When compiling with OpenSSL in Seastar, switch to a new set of calls to Seastar to enable a faster set of ciphers than what OpenSSL selects by default. Signed-off-by: Michael Boquard --- src/v/cloud_storage_clients/configuration.cc | 10 ++++++++++ src/v/config/tls_config.cc | 8 ++++++++ src/v/config/tls_config.h | 8 ++++++++ 3 files changed, 26 insertions(+) diff --git a/src/v/cloud_storage_clients/configuration.cc b/src/v/cloud_storage_clients/configuration.cc index c4537017afa76..53d80b2c445fb 100644 --- a/src/v/cloud_storage_clients/configuration.cc +++ b/src/v/cloud_storage_clients/configuration.cc @@ -12,6 +12,7 @@ #include "cloud_storage_clients/logger.h" #include "config/configuration.h" +#include "config/tls_config.h" #include "net/tls.h" #include "net/tls_certificate_probe.h" @@ -25,11 +26,20 @@ build_tls_credentials( std::optional trust_file, ss::logger& log) { ss::tls::credentials_builder cred_builder; +#ifdef SEASTAR_WITH_TLS_OSSL + cred_builder.set_cipher_string( + {config::tlsv1_2_cipher_string.data(), + config::tlsv1_2_cipher_string.size()}); + cred_builder.set_ciphersuites( + {config::tlsv1_3_ciphersuites.data(), + config::tlsv1_3_ciphersuites.size()}); +#else // NOTE: this is a pre-defined gnutls priority string that // picks the ciphersuites with 128-bit ciphers which // leads to up to 10x improvement in upload speed, compared // to 256-bit ciphers cred_builder.set_priority_string("PERFORMANCE"); +#endif if (trust_file.has_value()) { auto file = trust_file.value(); vlog(log.info, "Use non-default trust file {}", file()); diff --git a/src/v/config/tls_config.cc b/src/v/config/tls_config.cc index aed4e31fcf4aa..bbe3e2a5c8aff 100644 --- a/src/v/config/tls_config.cc +++ b/src/v/config/tls_config.cc @@ -26,7 +26,15 @@ tls_config::get_credentials_builder() const& { return ss::do_with( ss::tls::credentials_builder{}, [this](ss::tls::credentials_builder& builder) { +#ifdef SEASTAR_WITH_TLS_OSSL + builder.enable_server_precedence(); + builder.set_cipher_string( + {tlsv1_2_cipher_string.data(), tlsv1_2_cipher_string.size()}); + builder.set_ciphersuites( + {tlsv1_3_ciphersuites.data(), tlsv1_3_ciphersuites.size()}); +#else builder.set_priority_string("PERFORMANCE:%SERVER_PRECEDENCE"); +#endif builder.set_dh_level(ss::tls::dh_params::level::MEDIUM); if (_require_client_auth) { builder.set_client_auth(ss::tls::client_auth::REQUIRE); diff --git a/src/v/config/tls_config.h b/src/v/config/tls_config.h index 131cf288279f2..618c264ad0001 100644 --- a/src/v/config/tls_config.h +++ b/src/v/config/tls_config.h @@ -34,6 +34,14 @@ struct key_cert { friend std::ostream& operator<<(std::ostream& o, const key_cert& c); }; +inline constexpr std::string_view tlsv1_2_cipher_string + = "ECDHE-RSA-AES128-GCM-SHA256:AES128-GCM-SHA256:ECDHE-RSA-AES256-GCM-SHA384:" + "AES256-GCM-SHA384:ECDHE-RSA-CHACHA20-POLY1305:ECDHE-RSA-AES128-SHA:AES128-" + "SHA:AES128-CCM:ECDHE-RSA-AES256-SHA:AES256-SHA:AES256-CCM"; + +inline constexpr std::string_view tlsv1_3_ciphersuites + = "TLS_AES_128_GCM_SHA256:TLS_AES_256_GCM_SHA384:TLS_CHACHA20_POLY1305_" + "SHA256:TLS_AES_128_CCM_SHA256"; class tls_config { public: tls_config() From e578efe1a08919829e132d51692ccd531248aabe Mon Sep 17 00:00:00 2001 From: Michael Boquard Date: Tue, 4 Jun 2024 16:34:36 -0400 Subject: [PATCH 3/3] dt: Fix up CRL tests * Updated certs to use cRLSign extension * Updated RFC test to start node with TLS enabled * Updated error condition searchs for OpenSSL responses Signed-off-by: Michael Boquard --- tests/rptest/services/tls.py | 6 +++--- tests/rptest/tests/crl_test.py | 20 ++++++++++++++------ 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/tests/rptest/services/tls.py b/tests/rptest/services/tls.py index 00e4d28809eea..7f1cc988aa2a5 100644 --- a/tests/rptest/services/tls.py +++ b/tests/rptest/services/tls.py @@ -42,7 +42,7 @@ commonName = Redpanda Test CA [ extensions ] -keyUsage = critical,digitalSignature,nonRepudiation,keyEncipherment,keyCertSign +keyUsage = critical,digitalSignature,nonRepudiation,keyEncipherment,keyCertSign,cRLSign basicConstraints = critical,CA:true,pathlen:1 # Common policy for nodes and users. @@ -52,12 +52,12 @@ # Used to sign node certificates. [ signing_node_req ] -keyUsage = critical,digitalSignature,keyEncipherment +keyUsage = critical,digitalSignature,keyEncipherment,cRLSign extendedKeyUsage = serverAuth,clientAuth # Used to sign client certificates. [ signing_client_req ] -keyUsage = critical,digitalSignature,keyEncipherment +keyUsage = critical,digitalSignature,keyEncipherment,cRLSign extendedKeyUsage = clientAuth """ diff --git a/tests/rptest/tests/crl_test.py b/tests/rptest/tests/crl_test.py index e256be64c4638..d895419d456d2 100644 --- a/tests/rptest/tests/crl_test.py +++ b/tests/rptest/tests/crl_test.py @@ -20,7 +20,7 @@ from rptest.services.admin import Admin from rptest.services.redpanda import RedpandaService -from rptest.clients.rpk import RpkTool, ClusterAuthorizationError, RpkException +from rptest.clients.rpk import RpkTool, RpkException from rptest.services.redpanda import (SecurityConfig, TLSProvider, SchemaRegistryConfig, PandaproxyConfig) from rptest.services import tls @@ -185,14 +185,19 @@ def get_topics(node: ClusterNode): self.tls.revoke_cert(self.user_cert) self.redpanda.write_crl_file(node, self.tls.ca) - with expect_exception(requests.exceptions.ConnectionError, - lambda e: "Connection aborted" in str(e)): + with expect_exception((requests.exceptions.SSLError, + requests.exceptions.ConnectionError), + lambda e: "certificate revoked" in str( + e) or "Connection aborted" in str(e)): get_topics(node) with get_topics(self.redpanda.nodes[1]) as res: assert res.status_code == 200, f"Bad status: {res.status_code}" - @cluster(num_nodes=3) + @cluster(num_nodes=3, + log_allow_list=[ + "certificate revoked", "The certificate is NOT trusted" + ]) def test_rpc(self): node = self.redpanda.nodes[0] @@ -242,9 +247,12 @@ def cluster_health(node: ClusterNode, assert node.account.hostname in broker_cert.crt, f"Cert order mismatch: {broker_cert.crt}" self.tls.revoke_cert(broker_cert) - self.redpanda.write_crl_file(node, self.tls.ca) - self.redpanda.restart_nodes([node]) + self.redpanda.write_crl_file(self.redpanda.nodes[1], self.tls.ca) + self.redpanda.write_crl_file(self.redpanda.nodes[2], self.tls.ca) + + self.redpanda.restart_nodes( + [node], override_cfg_params={"rpc_server_tls": RPC_TLS_CONFIG}) other_node = self.redpanda.nodes[1] self.logger.debug(