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

tls: Introduce OpenSSL #2569

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open
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
22 changes: 14 additions & 8 deletions .github/workflows/test.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,11 @@ on:
type: string
default: ''
required: false
crypto_provider:
description: 'cryptographic provider to use'
type: string
default: 'GnuTLS'
required: false

jobs:
test:
Expand Down Expand Up @@ -83,14 +88,15 @@ jobs:
if ${{ inputs.enable-ccache }}; then
MAYBE_CCACHE_OPT="--ccache"
fi
./configure.py \
--c++-standard ${{ inputs.standard }} \
--compiler ${{ inputs.compiler }} \
--c-compiler $CC \
--mode ${{ inputs.mode }} \
$MAYBE_CCACHE_OPT \
${{ inputs.options }} \
${{ inputs.enables }}
./configure.py \
--c++-standard ${{ inputs.standard }} \
--compiler ${{ inputs.compiler }} \
--c-compiler $CC \
--mode ${{ inputs.mode }} \
$MAYBE_CCACHE_OPT \
${{ inputs.options }} \
${{ inputs.enables }} \
--crypto-provider ${{ inputs.crypto_provider }}

- name: Build
run: cmake --build build/${{inputs.mode}}
Expand Down
21 changes: 18 additions & 3 deletions .github/workflows/tests.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -11,20 +11,22 @@ concurrency:

jobs:
regular_test:
name: "Test (${{ matrix.compiler }}, C++${{ matrix.standard}}, ${{ matrix.mode }})"
name: "Test (${{ matrix.compiler }}, C++${{ matrix.standard}}, ${{ matrix.mode }}, ${{ matrix.crypto_provider }})"
uses: ./.github/workflows/test.yaml
strategy:
fail-fast: false
matrix:
compiler: [clang++, g++]
standard: [20, 23]
mode: [dev, debug, release]
crypto_provider: [GnuTLS, OpenSSL]
with:
compiler: ${{ matrix.compiler }}
standard: ${{ matrix.standard }}
mode: ${{ matrix.mode }}
enables: ${{ matrix.enables }}
options: ${{ matrix.options }}
crypto_provider: ${{ matrix.crypto_provider }}
build_with_dpdk:
name: "Test with DPDK enabled"
uses: ./.github/workflows/test.yaml
Expand All @@ -36,8 +38,8 @@ jobs:
mode: release
enables: --enable-dpdk
options: --cook dpdk
build_with_cxx_modules:
name: "Test with C++20 modules enabled"
build_with_cxx_modules_gnutls:
name: "Test with C++20 modules enabled (GnuTLS)"
uses: ./.github/workflows/test.yaml
strategy:
fail-fast: false
Expand All @@ -47,3 +49,16 @@ jobs:
mode: debug
enables: --enable-cxx-modules
enable-ccache: false
crypto_provider: GnuTLS
build_with_cxx_modules_openssl:
name: "Test with C++20 modules enabled (OpenSSL)"
uses: ./.github/workflows/test.yaml
strategy:
fail-fast: false
with:
compiler: clang++
standard: 23
mode: debug
enables: --enable-cxx-modules
enable-ccache: false
crypto_provider: OpenSSL
Copy link
Contributor

Choose a reason for hiding this comment

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

piggyback the tests with OpenSSL / GnuTLS might not be a great idea. because, we only enable the test step in

- name: Test
if: ${{ ! contains(inputs.enables, 'cxx-modules') }}
run: ./test.py --mode=${{ inputs.mode }}
if "cxx-modules" is not enabled. in other words, these two jobs do not run any of the unit tests. so i'd suggest adding a dedicated job in tests.yaml for building with the OpenSSL backend, and keep the existing job of build_with_cxx_modules intact.

please allow me to provide more context here: because C++20 modules is currently an experimental feature, not all Seastar facilities are exposed as in the "seastar" C++20 module at this moment, and we only have a single "hello_cxx_module" test for testing the build with C++20 module. none of the unit tests is built with the "seastar" C++20 module at the time of writing.

24 changes: 22 additions & 2 deletions CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,16 @@ if (NOT Seastar_SCHEDULING_GROUPS_COUNT MATCHES "^[1-9][0-9]*")
message(FATAL_ERROR "Seastar_SCHEDULING_GROUPS_COUNT must be a positive number (${Seastar_SCHEDULING_GROUPS_COUNT})")
endif ()

option (Seastar_USE_OPENSSL
"Use OpenSSL rather than GnuTLS for cryptographic operations, including TLS"
OFF)

if (Seastar_USE_OPENSSL)
set(Seastar_USE_GNUTLS OFF)
else()
set(Seastar_USE_GNUTLS ON)
Comment on lines +97 to +99
Copy link
Contributor

Choose a reason for hiding this comment

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

could you please add space after set and else? just to be consistent with the rest of this CMake script.

endif ()

#
# Add a dev build type.
#
Expand Down Expand Up @@ -753,13 +763,16 @@ add_library (seastar
src/net/native-stack-impl.hh
src/net/native-stack.cc
src/net/net.cc
$<$<BOOL:${Seastar_USE_OPENSSL}>:src/net/ossl.cc>
src/net/packet.cc
src/net/posix-stack.cc
src/net/proxy.cc
src/net/socket_address.cc
src/net/stack.cc
src/net/tcp.cc
src/net/tls.cc
$<$<BOOL:${Seastar_USE_GNUTLS}>:src/net/tls.cc>
src/net/tls-impl.cc
src/net/tls-impl.hh
src/net/udp.cc
src/net/unix_address.cc
src/net/virtio.cc
Expand All @@ -778,6 +791,9 @@ add_library (seastar
src/util/tmp_file.cc
src/util/short_streams.cc
src/websocket/server.cc
src/websocket/base64.hh
$<$<BOOL:${Seastar_USE_GNUTLS}>:src/websocket/base64-gnutls.cc>
$<$<BOOL:${Seastar_USE_OPENSSL}>:src/websocket/base64-openssl.cc>
)

add_library (Seastar::seastar ALIAS seastar)
Expand Down Expand Up @@ -856,7 +872,9 @@ target_link_libraries (seastar
SourceLocation::source_location
PRIVATE
${CMAKE_DL_LIBS}
GnuTLS::gnutls
$<$<BOOL:${Seastar_USE_GNUTLS}>:GnuTLS::gnutls>
$<$<BOOL:${Seastar_USE_OPENSSL}>:OpenSSL::SSL>
$<$<BOOL:${Seastar_USE_OPENSSL}>:OpenSSL::Crypto>
StdAtomic::atomic
lksctp-tools::lksctp-tools
protobuf::libprotobuf
Expand Down Expand Up @@ -913,6 +931,8 @@ include (CTest)
target_compile_definitions(seastar
PUBLIC
SEASTAR_API_LEVEL=${Seastar_API_LEVEL}
$<$<BOOL:${Seastar_USE_OPENSSL}>:SEASTAR_USE_OPENSSL>
$<$<BOOL:${Seastar_USE_GNUTLS}>:SEASTAR_USE_GNUTLS>
$<$<BOOL:${BUILD_SHARED_LIBS}>:SEASTAR_BUILD_SHARED_LIBS>)

target_compile_features(seastar
Expand Down
7 changes: 6 additions & 1 deletion cmake/SeastarDependencies.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -91,6 +91,7 @@ macro (seastar_find_dependencies)
GnuTLS
LibUring
LinuxMembarrier
OpenSSL
# Protobuf is searched manually.
Sanitizers
SourceLocation
Expand Down Expand Up @@ -124,8 +125,12 @@ macro (seastar_find_dependencies)
VERSION 8.1.1)
seastar_set_dep_args (lz4 REQUIRED
VERSION 1.7.3)
seastar_set_dep_args (OpenSSL
VERSION 3.0.0
OPTION ${Seastar_USE_OPENSSL})
seastar_set_dep_args (GnuTLS REQUIRED
VERSION 3.3.26)
VERSION 3.3.26
OPTION ${Seastar_USE_GNUTLS})
seastar_set_dep_args (LibUring
VERSION 2.0
OPTION ${Seastar_IO_URING})
Expand Down
7 changes: 7 additions & 0 deletions configure.py
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,9 @@ def standard_supported(standard, compiler='g++'):
arg_parser.add_argument('--verbose', dest='verbose', action='store_true', help='Make configure output more verbose.')
arg_parser.add_argument('--scheduling-groups-count', action='store', dest='scheduling_groups_count', default='16',
help='Number of available scheduling groups in the reactor')
arg_parser.add_argument('--crypto-provider', dest='crypto_provider', choices=seastar_cmake.SUPPORTED_CRYPTO_PROVIDERS,
default='GnuTLS', help='The cryptographic provider ot use')
arg_parser.add_argument('--openssl-root-dir', dest='openssl_root_dir', help="Root directory for OpenSSL library")

add_tristate(
arg_parser,
Expand Down Expand Up @@ -191,6 +194,7 @@ def configure_mode(mode):
'-DBUILD_SHARED_LIBS={}'.format('yes' if mode in ('debug', 'dev') else 'no'),
'-DSeastar_API_LEVEL={}'.format(args.api_level),
'-DSeastar_SCHEDULING_GROUPS_COUNT={}'.format(args.scheduling_groups_count),
'-DSeastar_USE_OPENSSL={}'.format('yes' if args.crypto_provider == 'OpenSSL' else 'no'),
tr(args.exclude_tests, 'EXCLUDE_TESTS_FROM_ALL'),
tr(args.exclude_apps, 'EXCLUDE_APPS_FROM_ALL'),
tr(args.exclude_demos, 'EXCLUDE_DEMOS_FROM_ALL'),
Expand All @@ -211,6 +215,9 @@ def configure_mode(mode):
tr(args.debug_shared_ptr, 'DEBUG_SHARED_PTR', value_when_none='default'),
]

if args.openssl_root_dir is not None:
TRANSLATED_ARGS.appen(f'-DOPENSSL_ROOT_DIR={args.openssl_root_dir}')

ingredients_to_cook = set(args.cook)

if args.dpdk:
Expand Down
36 changes: 36 additions & 0 deletions include/seastar/net/tcp.hh
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,12 @@
#include <random>
#include <stdexcept>
#include <system_error>
#ifdef SEASTAR_USE_OPENSSL
#include <openssl/evp.h>
#else
#include <gnutls/crypto.h>
#endif
#endif
#include <seastar/core/shared_ptr.hh>
#include <seastar/core/queue.hh>
#include <seastar/core/semaphore.hh>
Expand All @@ -42,6 +46,7 @@
#include <seastar/net/ip.hh>
#include <seastar/net/const.hh>
#include <seastar/net/packet-util.hh>
#include <seastar/util/defer.hh>
#include <seastar/util/std-compat.hh>

namespace seastar {
Expand Down Expand Up @@ -2084,6 +2089,36 @@ tcp_seq tcp<InetTraits>::tcb::get_isn() {
// ISN = M + F(localip, localport, remoteip, remoteport, secretkey)
// M is the 4 microsecond timer
using namespace std::chrono;
#ifdef SEASTAR_USE_OPENSSL
uint32_t hash[8];
hash[0] = _local_ip.ip;
hash[1] = _foreign_ip.ip;
hash[2] = (_local_port << 16) + _foreign_port;
unsigned int hash_size = sizeof(hash);

// Why SHA-256 for OpenSSL vs MD5?
// MD5 may be disabled if OpenSSL is in FIPS mode, also some bench testing
// has shown that the SHA-256 performance is equivalent or better than MD5
// as SHA256 is hardware accelerated on most modern CPU architectures
auto md_ptr = EVP_MD_fetch(nullptr, "SHA256", nullptr);
assert(md_ptr);
auto free_md_ptr = defer([&]() noexcept { EVP_MD_free(md_ptr); });
assert(hash_size == static_cast<unsigned int>(EVP_MD_get_size(md_ptr)));
auto md_ctx = EVP_MD_CTX_new();
assert(md_ctx);
auto free_md_ctx = defer([&]() noexcept { EVP_MD_CTX_free(md_ctx); });
auto res = EVP_DigestInit(md_ctx, md_ptr);
assert(1 == res);
res = EVP_DigestUpdate(
md_ctx, hash, 3 * sizeof(hash[0]));
assert(1 == res);
res = EVP_DigestUpdate(
md_ctx, _isn_secret.key, sizeof(_isn_secret.key));
assert(1 == res);
res = EVP_DigestFinal_ex(
md_ctx, reinterpret_cast<unsigned char *>(hash), &hash_size);
assert(1 == res);
#else
uint32_t hash[4];
hash[0] = _local_ip.ip;
hash[1] = _foreign_ip.ip;
Expand All @@ -2096,6 +2131,7 @@ tcp_seq tcp<InetTraits>::tcb::get_isn() {
// reuse "hash" for the output of digest
assert(sizeof(hash) == gnutls_hash_get_len(GNUTLS_DIG_MD5));
gnutls_hash_deinit(md5_hash_handle, hash);
#endif
auto seq = hash[0];
auto m = duration_cast<microseconds>(clock_type::now().time_since_epoch());
seq += m.count() / 4;
Expand Down
Loading
Loading