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

CORE-4263 Removed any use of GnuTLS #20129

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
1 change: 1 addition & 0 deletions cmake/dependencies.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ fetch_dep(fmt
set(Seastar_TESTING ON CACHE BOOL "" FORCE)
set(Seastar_API_LEVEL 6 CACHE STRING "" FORCE)
set(Seastar_CXX_FLAGS -Wno-error)
set(Seastar_WITH_OSSL ON CACHE BOOL "" FORCE)
set(CMAKE_CXX_STANDARD
"${CMAKE_CXX_STANDARD}"
CACHE
Expand Down
4 changes: 0 additions & 4 deletions install-dependencies.sh
Original file line number Diff line number Diff line change
Expand Up @@ -31,11 +31,9 @@ deb_deps=(
clang-tidy
cmake
git
gnutls-dev
golang
libboost-all-dev
libc-ares-dev
libcrypto++-dev
libgssapi-krb5-2
libkrb5-dev
liblz4-dev
Expand Down Expand Up @@ -69,9 +67,7 @@ fedora_deps=(
clang-tools-extra
cmake
compiler-rt
cryptopp-devel
git
gnutls-devel
golang
hwloc-devel
krb5-devel
Expand Down
4 changes: 0 additions & 4 deletions licenses/third_party.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,8 @@ please keep this up to date with every new library use.
| CRoaring | Apache License 2 |
| clang | Apache License 2 |
| crc32c | BSD 3 |
| cryptopp | Public Domain |
| DPDK | BSD |
| fmt | BSD |
| gmp | LGPL v3 |
| gnutls | LGPL v2.1 |
Copy link
Member

Choose a reason for hiding this comment

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

should nettle, gmp, etc... also be removed from this list?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yup

| HdrHistogram | BSD 2 |
| hwloc | BSD |
| krb5 | MIT |
Expand All @@ -30,7 +27,6 @@ please keep this up to date with every new library use.
| libpciaccess | MIT |
| lksctp-tools | LGPL v2.1 |
| lz4 | BSD 2 |
| nettle | LGPL v3 |
| OpenSSL v3 | Apache License 2 |
| protobuf | Apache License 2 |
| rapidjson | MIT |
Expand Down
9 changes: 0 additions & 9 deletions src/CMakeLists.txt
Original file line number Diff line number Diff line change
@@ -1,14 +1,5 @@
find_package(Seastar REQUIRED)

# normalize the name of the gnutls dependency. when seastar is imported via a
# normal installation arrange for the library name given by seastar's find
# module to match the name from the system cmake find module for gnutls.
if (TARGET GnuTLS::gnutls)
add_library(GnuTLS::GnuTLS ALIAS GnuTLS::gnutls)
else()
find_package(GnuTLS)
endif()

find_package(Boost REQUIRED
COMPONENTS
iostreams
Expand Down
2 changes: 1 addition & 1 deletion src/v/cloud_roles/refresh_credentials.cc
Original file line number Diff line number Diff line change
Expand Up @@ -361,7 +361,7 @@ ss::future<> refresh_credentials::impl::init_tls_certs(ss::sstring name) {
co_await b.set_x509_trust_file(
ca_file.value(), ss::tls::x509_crt_format::PEM);
} else {
vlog(clrl_log.info, "Using GnuTLS default");
vlog(clrl_log.info, "Using system default");
co_await b.set_system_trust();
}

Expand Down
2 changes: 1 addition & 1 deletion src/v/cloud_roles/tests/signature_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -226,7 +226,7 @@ SEASTAR_THREAD_TEST_CASE(test_abs_signature_computation_many_query_params) {

/// Test is based on this example
/// https://docs.aws.amazon.com/general/latest/gr/sigv4-calculate-signature.html
SEASTAR_THREAD_TEST_CASE(test_gnutls) {
SEASTAR_THREAD_TEST_CASE(test_sig_gen) {
std::string ksecret = "wJalrXUtnFEMI/K7MDENG+bPxRfiCYEXAMPLEKEY";
std::string date = "20150830";
std::string region = "us-east-1";
Expand Down
12 changes: 2 additions & 10 deletions src/v/cloud_storage_clients/configuration.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,27 +26,19 @@ build_tls_credentials(
std::optional<cloud_storage_clients::ca_trust_file> 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());
co_await cred_builder.set_x509_trust_file(
file().string(), ss::tls::x509_crt_format::PEM);
} else {
// Use GnuTLS defaults, might not work on all systems
// Use system defaults, might not work on all systems
auto ca_file = co_await net::find_ca_file();
if (ca_file) {
vlog(
Expand All @@ -58,7 +50,7 @@ build_tls_credentials(
} else {
vlog(
log.info,
"Trust file can't be detected automatically, using GnuTLS "
"Trust file can't be detected automatically, using system "
"default");
co_await cred_builder.set_system_trust();
}
Expand Down
1 change: 0 additions & 1 deletion src/v/cloud_storage_clients/s3_client.cc
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,6 @@
#include <boost/beast/http/status.hpp>
#include <boost/property_tree/ptree.hpp>
#include <boost/property_tree/xml_parser.hpp>
#include <gnutls/crypto.h>

#include <bit>
#include <exception>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
#include <boost/optional/optional.hpp>
#include <boost/outcome/detail/value_storage.hpp>
#include <boost/system/system_error.hpp>
#include <gnutls/gnutls.h>

#include <chrono>
#include <exception>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,6 @@
#include <boost/optional/optional.hpp>
#include <boost/outcome/detail/value_storage.hpp>
#include <boost/system/system_error.hpp>
#include <gnutls/gnutls.h>

#include <chrono>
#include <exception>
Expand Down
4 changes: 0 additions & 4 deletions src/v/config/tls_config.cc
Original file line number Diff line number Diff line change
Expand Up @@ -26,15 +26,11 @@ 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);
Expand Down
4 changes: 2 additions & 2 deletions src/v/crypto/tests/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ rp_test(
BINARY_NAME crypto_bench
SOURCES crypto_bench.cc
LIBRARIES
Seastar::seastar_perf_testing v::crypto v::random GnuTLS::GnuTLS
Seastar::seastar_perf_testing v::crypto v::random
LABELS crypto
ENV "OPENSSL_CONF=${CMAKE_CURRENT_BINARY_DIR}/test/openssl_conf.cnf;MODULE_DIR=${REDPANDA_DEPS_INSTALL_DIR}/lib/ossl-modules"
)
Expand All @@ -72,7 +72,7 @@ rp_test(
DEFINITIONS
PERF_FIPS_MODE
LIBRARIES
Seastar::seastar_perf_testing v::crypto v::random GnuTLS::GnuTLS
Seastar::seastar_perf_testing v::crypto v::random
LABELS crypto
ENV "OPENSSL_CONF=${CMAKE_CURRENT_BINARY_DIR}/test/openssl_conf.cnf;MODULE_DIR=${REDPANDA_DEPS_INSTALL_DIR}/lib/ossl-modules"
)
Expand Down
163 changes: 0 additions & 163 deletions src/v/crypto/tests/crypto_bench.cc
Original file line number Diff line number Diff line change
Expand Up @@ -17,129 +17,8 @@
#include <seastar/core/sleep.hh>
#include <seastar/testing/perf_tests.hh>

#include <gnutls/crypto.h>
#include <gnutls/gnutls.h>

static constexpr size_t inner_iters = 1000;

template<gnutls_mac_algorithm_t Algo, size_t DigestSize>
class hmac {
static_assert(DigestSize > 0, "digest cannot be zero length");
Comment on lines -25 to -27
Copy link
Member

Choose a reason for hiding this comment

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

Question about this removal, mostly for my own benefit - so in effect this validation is already done to our satisfaction and we can remove this? Or there's more work to do there and we'll build off an old commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm good with the bench results. Just left it in until we fully pulled gnutls

Copy link
Member

Choose a reason for hiding this comment

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

Cool, yeah. I just haven't been following along I guess 😕


public:
// silence clang-tidy about _handle being uninitialized
// NOLINTNEXTLINE(hicpp-member-init, cppcoreguidelines-pro-type-member-init)
explicit hmac(std::string_view key)
: hmac(key.data(), key.size()) {}

// silence clang-tidy about _handle being uninitialized
// NOLINTNEXTLINE(hicpp-member-init, cppcoreguidelines-pro-type-member-init)
explicit hmac(bytes_view key)
: hmac(key.data(), key.size()) {}

hmac(const hmac&) = delete;
hmac& operator=(const hmac&) = delete;
hmac(hmac&&) = delete;
hmac& operator=(hmac&&) = delete;

~hmac() noexcept { gnutls_hmac_deinit(_handle, nullptr); }

void update(std::string_view data) { update(data.data(), data.size()); }
void update(bytes_view data) { update(data.data(), data.size()); }

template<std::size_t Size>
void update(const std::array<char, Size>& data) {
update(data.data(), Size);
}

/**
* Return the current output and reset.
*/
std::array<char, DigestSize> reset() {
std::array<char, DigestSize> digest;
gnutls_hmac_output(_handle, digest.data());
return digest;
}

private:
// silence clang-tidy about _handle being uninitialized
// NOLINTNEXTLINE(hicpp-member-init, cppcoreguidelines-pro-type-member-init)
hmac(const void* key, size_t size) {
int ret = gnutls_hmac_init(&_handle, Algo, key, size);
if (unlikely(ret)) {
throw std::runtime_error(gnutls_strerror(ret));
}

ret = gnutls_hmac_get_len(Algo);
if (unlikely(ret != DigestSize)) {
throw std::runtime_error("invalid digest length");
}
}

void update(const void* data, size_t size) {
int ret = gnutls_hmac(_handle, data, size);
if (unlikely(ret)) {
throw std::runtime_error(gnutls_strerror(ret));
}
}

gnutls_hmac_hd_t _handle;
};

template<gnutls_digest_algorithm_t Algo, size_t DigestSize>
class hash {
public:
static constexpr auto digest_size = DigestSize;
using digest_type = std::array<char, DigestSize>;

hash() {
int ret = gnutls_hash_init(&_handle, Algo);
if (unlikely(ret)) {
throw std::runtime_error("hash init failed");
}

ret = gnutls_hash_get_len(Algo);
if (unlikely(ret != DigestSize)) {
throw std::runtime_error("BOO");
}
}

hash(const hash&) = delete;
hash& operator=(const hash&) = delete;
hash(hash&&) = delete;
hash& operator=(hash&&) = delete;

~hash() noexcept { gnutls_hash_deinit(_handle, nullptr); }

void update(std::string_view data) { update(data.data(), data.size()); }
void update(bytes_view data) { update(data.data(), data.size()); }

/**
* Return the current output and reset.
*/
digest_type reset() {
std::array<char, DigestSize> digest;
gnutls_hash_output(_handle, digest.data());
return digest;
}

private:
void update(const void* data, size_t size) {
int ret = gnutls_hash(_handle, data, size);
if (unlikely(ret)) {
throw std::runtime_error("blah update");
}
}

gnutls_hash_hd_t _handle;
};

using hmac_sha256 = hmac<GNUTLS_MAC_SHA256, 32>;
using hmac_sha512 = hmac<GNUTLS_MAC_SHA512, 64>;
using hash_sha256 = hash<GNUTLS_DIG_SHA256, 32>;
using hash_sha512 = hash<GNUTLS_DIG_SHA512, 64>;
using hash_md5 = hash<GNUTLS_DIG_MD5, 16>;

template<typename F>
static size_t test_body(size_t msg_len, F n) {
auto buffer = random_generators::gen_alphanum_string(msg_len);
Expand Down Expand Up @@ -209,30 +88,6 @@ PERF_TEST_F(openssl_perf, sha512_1k) {
});
}

PERF_TEST(gnutls, md5_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
hash_md5 md5{};
md5.update(buffer);
return md5.reset();
});
}

PERF_TEST(gnutls, sha256_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
hash_sha256 sha256{};
sha256.update(buffer);
return sha256.reset();
});
}

PERF_TEST(gnutls, sha512_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
hash_sha512 sha512{};
sha512.update(buffer);
return sha512.reset();
});
}

PERF_TEST_F(openssl_perf, hmac_sha256_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
auto key = random_generators::gen_alphanum_string(32);
Expand All @@ -246,21 +101,3 @@ PERF_TEST_F(openssl_perf, hmac_sha512_1k) {
return crypto::hmac(crypto::digest_type::SHA512, key, buffer);
});
}

PERF_TEST(gnutls, hmac_sha256_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
auto key = random_generators::gen_alphanum_string(32);
hmac_sha256 hmac{key};
hmac.update(buffer);
return hmac.reset();
});
}

PERF_TEST(gnutls, hmac_sha512_1k) {
return test_body(1024, [](const ss::sstring& buffer) {
auto key = random_generators::gen_alphanum_string(32);
hmac_sha512 hmac{key};
hmac.update(buffer);
return hmac.reset();
});
}
6 changes: 3 additions & 3 deletions src/v/net/include/net/tls.h
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,14 @@ namespace net {
/// Historically, different linux distributions use different locations to
/// store certificates for their private key infrastructure. This is just a
/// convention and can't be queried by the application code. The application
/// is required to 'know' where to find the certs. In case of GnuTLS the
/// is required to 'know' where to find the certs. In case of OpenSSL the
/// location is configured during build time. It depend on distribution on
/// which GnuTLS is built. This approach doesn't work for Redpanda because
/// which OpenSSL is built. This approach doesn't work for Redpanda because
/// single Redpanda binary can be executed on any linux distro. So the default
/// option only work on some distributions. The rest require the location to
/// be explicitly specified. This function does different thing. It probes
/// the set of default locations for different distributions untill it finds
/// the one that exists. This path is then passed to GnuTLS.
/// the one that exists. This path is then passed to OpenSSL.
ss::future<std::optional<ss::sstring>> find_ca_file();

} // namespace net
3 changes: 2 additions & 1 deletion src/v/storage/key_offset_map.h
Original file line number Diff line number Diff line change
Expand Up @@ -174,7 +174,8 @@ class hash_key_offset_map : public key_offset_map {

/**
* hash the compaction key. this helper will catch exceptions and reset the
* hashing object which is reused to avoid reinitialization of gnutls state.
* hashing object which is reused to avoid reinitialization of OpenSSL
* state.
*/
hash_type::digest_type hash_key(const compaction_key&) const;

Expand Down