Add OpenSSL Library Context Service #16927

michael-redpanda · 2024-03-07T01:42:35Z

Fixes: https://github.com/redpanda-data/core-internal/issues/1143

Backports Required

Release Notes

none

vbotbuildovich · 2024-03-07T03:54:02Z

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45770#018e16c4-73d5-47fc-b484-66d128498dcd

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45770#018e16c4-73d2-474f-b56c-6f845761e7e6

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45770#018e16ec-9f37-4b6c-9e88-f3a8c3cf7f2b

ducktape was retried in https://buildkite.com/redpanda/redpanda/builds/45989#018e2eb6-89c6-4365-b461-2db5f7e67d1d

oleiman

meat and potatoes lgtm. still need to go through the big unit testing diff in a bit more detail.

oleiman · 2024-03-07T04:00:57Z

src/v/crypto/ossl_context_service.cc

+template<class R>
+using initialize_result = result<R, std::string>;
+
+ss::logger lg("ossl-library-context-service");


Would it make sense to break this out into it's own compilation unit in case upcoming additions to crypto need logging facilities as well?

I originally did actually... but there are some shared items between the two... open to going back to that idea

idk, up to you. I don't have a reliable heuristic for deciding logger scope

oleiman · 2024-03-07T04:08:52Z

src/v/crypto/ossl_context_service.cc

+    }
+
+    if (fips_mode) {
+        if (!EVP_set_default_properties(ctx, "fips=yes")) {


"fips=yes"

😐

src/v/crypto/ossl_context_service.cc

oleiman · 2024-03-07T05:12:09Z

src/v/crypto/test/test_values.h

+ * by the Apache License, Version 2.0
+ */
+
+#pragma once


nit: would it be a good idea to namespace these as a matter of course?

yeah probably a good idea

rockwotj · 2024-03-07T02:26:41Z

src/v/README.md

@@ -13,6 +13,7 @@ compat                 | Compatibility checks |
 compression            | utilities supporting compression/decompression of many types |
 config                 | Redpanda cluster level and node level configuration options |
 container              | Generic Redpanda specific containers and data structures |
+crypto                 | Middelware library used to perform cryptographic operations |


typo with Middleware

rockwotj · 2024-03-07T02:28:15Z

src/v/crypto/include/crypto/ossl_context_service.h

+     * @param thread_worker Thread worker used to perform I/O operations outside
+     * of the Seastar context
+     * @param config_file Path to the OpenSSL config file
+     * @param module_path Path to the directory that contains the FIPS module


should this be optional? Then you may not really need the fips mode bit...

Which one? The config_file? Sorry GH is showing me 4 lines :)

The module_path is only valid if fips_mode::yes right?

If we wanted to use the legacy provider we would need it... but I don't think we would (no need for MD2 or RC4)

In fact the config file really is only needed in FIPS mode (I have tests that show that starting up in non FIPS mode without a config works fine).

I'll come up with some thing

maybe there is just std::optional<fips_config>?

I kind of like keeping the flag. Then I can check that both config file and module path are provided when in FIPS mode and error out if they're not provided.

Yeah this is sort of an awkward thing to express in the typesystem in C++ and std::variant<no_fips_tag, fips_config> is awkward. I'll stop trying to be pedantic about the typesystem enforcing you can't do silly things (so you don't even need to handle errors of the required config not being present because the typesystem doesn't allow you to set the flag true without giving values).

Gotcha, I appreciate where you're coming from =). I guess part of me is trying to future proof for an event where we do need module_path or conf_File even when not in FIPS mode. This may come up when trying to add support for MD5 (which the FIPS module doens't provide but the base one does)

rockwotj · 2024-03-07T02:51:18Z

src/v/crypto/ssl_utils.h

+            fn(ptr);
+        } else if constexpr (std::is_invocable_r_v<int, decltype(fn), T*>) {
+            if (1 != fn(ptr)) {
+                throw ossl_error();


This is going to throw in a destructor, which I am pretty sure is a bad thing to do...

yeah I guess i'll just have to ignore it then?

I would suggest asserting if they fail, but if it's actually expected they can fail at runtime, then I would suggest logging? I don't know.

How where you expecting to handle these exceptions?

Right now the only one that needs it is the OSSL_PROVIDER_unload which only happens at shut down. Maybe I'll wrap this up in a function that logs a warning if it fails

rockwotj · 2024-03-07T02:51:56Z

src/v/crypto/test/CMakeLists.txt

+    v::gtest_main
+  LABELS crypto
+  ARGS "-- -c 2"
+  ENV "OPENSSL_CONF=${CMAKE_CURRENT_BINARY_DIR}/test/openssl_conf.cnf;MODULE_DIR=${REDPANDA_DEPS_INSTALL_DIR}/lib64/ossl-modules"


This second bit doesn't work for the open source build does it?

I guess it won't. But then again running in FIPS mode isn't really supported for the open source build. The user would have to build the FIPS module and configure it for it to work.

I check to see if the directory exists and if it doesn't skip the FIPS tests? Do we have any other unit tests that do not run in the oss build?

Do we have any other unit tests that do not run in the oss build?

Not that I know of - FWIW I don't know if our unit test cmake wrapper stuff works well in the open source build at all, but we could just disable these tests (i.e. not build them) in non OSS build with some CMake flag

This still need resolving right? You can use GTEST_SKIP() to skip tests in the setup phase I think.

rockwotj · 2024-03-07T14:29:29Z

src/v/crypto/ossl_context_service.cc

+        _shard_ctx.reset();
+        _shard_ctx = nullptr;


FWIW these two lines do the same thing

whoops meant to replace 228 with 227 and not have both :)

rockwotj · 2024-03-07T14:29:45Z

src/v/crypto/ossl_context_service.cc

+            _thread_worker_ctx.reset();
+            _thread_worker_ctx = nullptr;


These do the same thing.

double whammy of goodness

rockwotj · 2024-03-07T14:31:14Z

src/v/crypto/include/crypto/ossl_context_service.h

+ * These contexts will be fetched by the OpenSSL library when performing
+ * cryptographic operations.
+ */
+class ossl_context_service final {


What is the lifecycle of this service? Is it bound to the full application lifecycle? I'm assuming it exists on every core? Should we mention the special responsibilities of core0?

I'll add that to the comment, but yes full application lifecycle

rockwotj · 2024-03-07T14:31:49Z

src/v/crypto/include/crypto/ossl_context_service.h

+/*
+ * 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
+ */


What stuff is going to be RCL vs BSL?

That... is an interesting question. Because FIPS mode is definitely an enterprise feature, but the use of OpenSSL is not. I guess I"d argue this is BSL since it's available in the main product regardless of FIPS or not?

Fair I wonder if it's worth while to have an implementation that is "fips mode" that is RCL, but if that's just passing a flag to openssl, I guess there isn't anything interesting there...

graphcareful · 2024-03-07T14:17:27Z

src/v/crypto/include/crypto/ossl_context_service.h

+    is_fips_mode fips_mode() const;
+
+private:
+    class impl;


nit: is a private impl neccessary here? I can't think of a future instance there could be a second inner implementation of this class

nit: is a private impl neccessary here? I can't think of a future instance there could be a second inner implementation of this class

I assume is is for a compiler firewall; to hide #include <openssl/*> from the header.

Yes, hides away all the annoying OpenSSL stuff. Pretty much why I did the same pattern for the crypto library.

oh right, sweet definitely a great idea

graphcareful · 2024-03-07T14:23:22Z

src/v/crypto/ossl_context_service.cc

+    impl& operator=(impl&&) noexcept = delete;
+
+    ss::future<> start() {
+        lg.trace("Starting service...");


since this log occurs once i think its fine to be at a higher level, also it could print the service name.

Service name should be printed as the logger name, yes?

graphcareful · 2024-03-07T14:26:32Z

src/v/crypto/ossl_context_service.cc

+            });
+
+            if (init_resp.has_error()) {
+                throw exception(init_resp.assume_error());


throw ossl_error ?

This was constructed from ossl_error::build_error() which steps through the OpenSSL error stack to build up the message. I didn't want to throw from the functions that were running on the alien thread. Also ossl_error inherits from crypto::exception.

graphcareful · 2024-03-07T14:29:33Z

src/v/crypto/ssl_utils.h

+        if constexpr (std::is_invocable_v<decltype(fn), T*>) {
+            fn(ptr);
+        } else if constexpr (std::is_invocable_r_v<int, decltype(fn), T*>) {
+            if (1 != fn(ptr)) {


do all of the deleters return 1 on success?

graphcareful · 2024-03-07T14:46:36Z

src/v/crypto/ossl_context_service.cc

+    return msg + internal::ossl_error::build_error();
+}
+
+initialize_result<initialize_return> initialize_openssl(


nit:

using inititalize_result = result<initialize_return, std::string>

the two initialize functions return different structures

graphcareful · 2024-03-07T14:47:06Z

src/v/crypto/ossl_context_service.cc

+  std::string_view conf_file,
+  is_fips_mode fips_mode) {
+    if (!conf_file.empty()) {
+        lg.debug("Attempting to load config file {}", conf_file);


Change to ossl config file

graphcareful · 2024-03-07T14:49:05Z

src/v/crypto/ossl_context_service.cc

+            // On main shard, load the 'null' provider to the default context
+            // This prevents the default context from performing any
+            // cryptographic operation
+            _defctxnull = internal::OSSL_PROVIDER_ptr(


question: This means _defctxnull is also expected to always be nullptr ?

no _defctxnull is not a nullptr. It loads a null provider into the global context so the global context can not be used to perform any crypto operations.

graphcareful · 2024-03-07T14:51:50Z

src/v/crypto/ossl_context_service.cc

+            // perform
+            _thread_worker_ctx = internal::OSSL_LIB_CTX_ptr(OSSL_LIB_CTX_new());
+            auto init_resp = co_await _thread_worker.submit([this] {
+                return initialize_worker_thread(


question: Could the context made on shard0 clash with the shard0 context _shard_ctx since ossl will be looking for one context per shard

The _thread_worker_ctx will get assigned to the worker thread's context via the call to OSSL_LIB_CTX_set0_default

graphcareful · 2024-03-07T15:31:40Z

src/v/crypto/ossl_context_service.cc

+          "Created new shard context for {} replacing {}",
+          fmt::ptr(_shard_ctx.get()),
+          fmt::ptr(_old_context));
+        auto init_resp = co_await _thread_worker.submit([this] {


question: How does this intended to work? The memory for the contexts is allocated on the correct shard but then the work to initialize and set them is dispatched to thread worker which may not run the function on the calling shard.

Memory allocated in an alien thread uses the system allocator, not the seastar allocator. The seastar allocator has a slow path for freeing memory not allocated on that shard, but it's generally not recommended.

+1 , i was referring to _shard_ctx which is allocated within a seastar shards context, due to the call being a few lines above the call to .submit

To further clarify my initial comment, I would assume we would want to allocate and initialize the OSSLContext on all shards , allocation and initialization occurring in each respective shard.

Allocation and assignment of the OpenSSL library context happens per shard. The initialization of the library context needs to happen in an alien thread because there's a bunch of system call stuff that OpenSSL does under the hood (namely dlloading the providers and opening and reading files, etc).

Chatted with @graphcareful , I'll add some comments that hopefully will clear this up :)

BenPope · 2024-03-07T15:53:30Z

src/v/README.md

@@ -13,6 +13,7 @@ compat                 | Compatibility checks |
 compression            | utilities supporting compression/decompression of many types |
 config                 | Redpanda cluster level and node level configuration options |
 container              | Generic Redpanda specific containers and data structures |
+crypto                 | Middelware library used to perform cryptographic operations |


Also missing license: https://github.com/redpanda-data/redpanda/blob/dev/licenses/third_party.md

BenPope · 2024-03-07T15:58:21Z

src/v/crypto/include/crypto/ossl_context_service.h

+    is_fips_mode fips_mode() const;
+
+private:
+    class impl;


nit: is a private impl neccessary here? I can't think of a future instance there could be a second inner implementation of this class

I assume is is for a compiler firewall; to hide #include <openssl/*> from the header.

BenPope · 2024-03-07T16:04:53Z

src/v/crypto/ossl_context_service.cc

+    internal::OSSL_PROVIDER_ptr base_provider;
+};
+
+std::string make_ssl_error_response(const std::string& msg) {


nitpick: Feels strange that initialising libcrypto provider has ssl in the name. I think we should stick to "ossl" for openssl (libcrypto and libssl).

Are you referring to OSSL_PROVIDER_ptr? Sorry not following the context :)

michael-redpanda · 2024-03-07T21:17:22Z

Force push 8d35f8c:

Updates from PR comments

michael-redpanda · 2024-03-07T21:19:08Z

licenses/third_party.md

I think my vscode linter decided to lint this file... if people object to the changes I'll revert them

rockwotj · 2024-03-07T21:21:12Z

licenses/third_party.md

@@ -32,12 +31,13 @@ please keep this up to date with every new library use.
 | lksctp-tools    | LGPL v2.1                          |
 | lz4             | BSD 2                              |
 | nettle          | LGPL v3                            |
+| OpenSSL         | Apache License 2                   |


Do we want to specify v3? I know older versions had different licensing.

rockwotj · 2024-03-07T21:24:02Z

src/v/crypto/test/CMakeLists.txt

+    v::gtest_main
+  LABELS crypto
+  ARGS "-- -c 2"
+  ENV "OPENSSL_CONF=${CMAKE_CURRENT_BINARY_DIR}/test/openssl_conf.cnf;MODULE_DIR=${REDPANDA_DEPS_INSTALL_DIR}/lib64/ossl-modules"


This still need resolving right? You can use GTEST_SKIP() to skip tests in the setup phase I think.

michael-redpanda · 2024-03-11T16:48:22Z

Force push c466590:

Add support for new flag in build (see https://github.com/redpanda-data/vtools/pull/2559)

Signed-off-by: Michael Boquard <michael@redpanda.com>

Added service to support loading shard local library contexts Signed-off-by: Michael Boquard <michael@redpanda.com>

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda · 2024-03-11T16:49:45Z

Force push 2275bb4:

Specified OpenSSL version 3 in license file

github-actions bot added the area/redpanda label Mar 7, 2024

michael-redpanda requested review from BenPope, oleiman, rockwotj, pgellert and graphcareful March 7, 2024 01:42

michael-redpanda self-assigned this Mar 7, 2024

michael-redpanda added area/security and removed area/redpanda labels Mar 7, 2024

oleiman reviewed Mar 7, 2024

View reviewed changes

rockwotj reviewed Mar 7, 2024

View reviewed changes

graphcareful reviewed Mar 7, 2024

View reviewed changes

BenPope reviewed Mar 7, 2024

View reviewed changes

michael-redpanda force-pushed the add_ossl_lib_ctx branch from 53797f0 to 8d35f8c Compare March 7, 2024 21:16

michael-redpanda requested review from emaxerrno and dswang as code owners March 7, 2024 21:16

github-actions bot added the area/redpanda label Mar 7, 2024

michael-redpanda requested review from BenPope, oleiman, rockwotj and graphcareful and removed request for emaxerrno March 7, 2024 21:17

michael-redpanda commented Mar 7, 2024

View reviewed changes

rockwotj reviewed Mar 7, 2024

View reviewed changes

michael-redpanda force-pushed the add_ossl_lib_ctx branch from 8d35f8c to c466590 Compare March 11, 2024 16:47

michael-redpanda requested a review from rockwotj March 11, 2024 16:47

redpanda: Added missing line to src/v/README.md

d739d60

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda added 5 commits March 11, 2024 12:49

test: Added ASSERT_NO_THROW_CORO

78728a3

Signed-off-by: Michael Boquard <michael@redpanda.com>

ossl_context_service: Added OpenSSL context service

9ff4574

Added service to support loading shard local library contexts Signed-off-by: Michael Boquard <michael@redpanda.com>

crypto/tests: Cleaned up error conditions in tests

2116081

Signed-off-by: Michael Boquard <michael@redpanda.com>

crypto/tests: Moved test values to own file for reuse

6744da5

Signed-off-by: Michael Boquard <michael@redpanda.com>

crypto/tests: Added tests for OSSL context service

2275bb4

Signed-off-by: Michael Boquard <michael@redpanda.com>

michael-redpanda force-pushed the add_ossl_lib_ctx branch from c466590 to 2275bb4 Compare March 11, 2024 16:49

rockwotj approved these changes Mar 11, 2024

View reviewed changes

michael-redpanda merged commit 6b7f878 into redpanda-data:dev Mar 11, 2024
16 checks passed

Add OpenSSL Library Context Service #16927

Add OpenSSL Library Context Service #16927

Conversation

michael-redpanda commented Mar 7, 2024

Backports Required

Release Notes

vbotbuildovich commented Mar 7, 2024 • edited

oleiman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

graphcareful Mar 7, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-redpanda commented Mar 11, 2024

michael-redpanda commented Mar 11, 2024

vbotbuildovich commented Mar 7, 2024 •

edited

graphcareful Mar 7, 2024 •

edited