tls: Introduce OpenSSL #2569

michael-redpanda · 2024-12-09T13:29:42Z

Introduces OpenSSL as an alternative TLS implementation to GnuTLS. This is a build-time configuration controlled by the CMake variable Seastar_USE_OPENSSL. The configure.py script has been updated to now have a --crypto-provider option. Valid arguments to that are OpenSSL and GnuTLS.

This implementation was released in Redpanda v24.2 on July 31st, and has been running on production clusters since.

Redpanda implemented these changes in order to provide a FIPS-compliant build to customers that require it (such as those wishing to undergo FedRAMP evaluation). OpenSSL was selected as it allows implementors to maintain the validation of the cryptographic module even when it's built from source.

OpenSSL's security policy
- See appendix A for instructions on how to build OpenSSL for FIPS
FIPS 140-2 Implementation Guidance
- See section G.5 Maintaining validation compliance of software or firmware cryptographic
  modules

No changes have been introduced to enable the FIPS provider for Seastar. It is up to the implementor to enable and use the FIPS cryptographic module if desired.

Fixes: #698

avikivity · 2024-12-09T14:14:36Z

@elcallio please review

Are there any functional differences?

Is hot reload of certificates supported?

Should we support gnutls and openssl in parallel?

michael-redpanda · 2024-12-09T14:39:58Z

Are there any functional differences?

The only major difference between the OpenSSL vs GnuTLS implementation is how the implementation is configured. With OpenSSL, there are 5 new methods that can be used to control it (compared to the single set_priority_string with GnuTLS):

set_cipher_string - used for controlling which ciphers are available in TLS1.2 and below (OpenSSL SSL_CTX_set_cihper_list)
set_ciphersuites - used for controlling which cipher suites are available in TLS1.3 (same doc page as above)
enable_server_precedence
set_minimum_tls_version - defaults to 1.0
set_maximum_tls_version - defaults to 1.3

There may be some other subtler differences, such as OpenSSL may be stricter about certificate contents (e.g. see 1744b66 - this required the CA cert to have CA:True)

Is hot reload of certificates supported?

Yes

elcallio

This is quite a big change. I've mostly added nits and some form comments here.
My biggest gripe with it is that library/session config is somewhat different between OSSL/gnutls. The latter uses a single string for config, OSSL is more complicated.
A caller needs to know which impl is used, and adjust his code accordingly.
It makes for more potential bugs when a dev somewhere changes things in one impl, but misses to do it in the other. I.e. we need to make sure CI tests both properly.

elcallio · 2024-12-09T14:33:35Z

src/seastar.cc

@@ -335,6 +335,7 @@ module : private;
 #include <seastar/net/virtio.hh>

 #include "net/native-stack-impl.hh"
+#include "net/tls-impl.hh"


Why is this included here?

This is necessary for when compiling with modules enabled

elcallio · 2024-12-09T14:34:26Z

src/net/tls-impl.hh

+public:
+    static std::unique_ptr<connected_socket_impl> get(connected_socket s) {
+        return std::move(s._csi);
+    }


Seems to me this should be just declaration, with impl in impl.cc

This was just moved from tls.cc to this file.

Yes. But that was a cc. This is a header.

elcallio · 2024-12-09T14:34:48Z

src/net/tls-impl.hh

+        if (_session && _session.use_count() == 1) {
+            _session->close();
+        }
+    }


Same goes for things like this...

Same as above. Code should not be in headers if it can be avoided (even internal, not very included ones)

elcallio · 2024-12-09T14:40:03Z

include/seastar/net/tls.hh

@@ -202,13 +212,53 @@ namespace tls {

        // TODO add methods for certificate verification

+#ifndef SEASTAR_USE_OPENSSL


Bit of a negative check. Should this not be SEASTAR_USE_GNUTLS?

That's actually not set as a compile definitions, but I think it should be. Needs to be added to target_compile_definitions in CMakeLists.txt. I'll make that update.

elcallio · 2024-12-09T14:51:20Z

src/net/ossl.cc

+
+        // This call is required to lower SSL's security level to permit TLSv1.0 and TLSv1.1
+        // See https://www.openssl.org/docs/man3.0/man3/SSL_CTX_set_security_level.html
+        SSL_CTX_set_security_level(ssl_ctx.get(), 0);


Should this be unconditional? In the gnutls impl, allowing or not allowing TLS versions is controlled by the prio string. You check min/max version above, does the level always need lowering?

I'll experiment with this and get back to you

Did a little bit of research on this function and what the level controls changes depending on the version of OpenSSL.

For OpenSSL 3.0, setting security level to 3 disables TLS1.0 and below, and setting it to 4 disables TLS1.1 and below (ref).

Starting with OpenSSL 3.1, level 1 disables TLS1.1 and below (ref).

elcallio · 2024-12-09T14:57:12Z

src/net/tls-impl.cc

+#endif
+
+#ifdef SEASTAR_USE_OPENSSL
+void tls::credentials_builder::set_cipher_string(const sstring& cipher_string) {


Why not place these in the openssl impl file?

That's a good idea, I'll move the definitions to the correct source file

elcallio · 2024-12-09T15:05:19Z

tests/unit/tls_test.cc

@@ -161,7 +163,29 @@ SEASTAR_TEST_CASE(test_x509_client_with_builder_system_trust_multiple) {
    });
 }

+static void set_priority_string(tls::credentials_builder & b, const sstring & prio, [[maybe_unused]] bool is_tls_v13 = false) {


Please break this into two (or three) distinct functions and adjust call sites instead. Calling this set_prio more amplifies the issue with library configuration not being equivalent between openssl and gnutls, and is more confusing (imho) at the call sites than just doing different things to accomplish roughly the same.

elcallio · 2024-12-09T15:08:46Z

src/net/tls-impl.hh

+/*
+ * Copyright 2015 Cloudius Systems
+ */
+#pragma once


Why is this file in public header dir? Seems to me it should only be required by internal compilation units, so should probably be under src.

It's not in the public header directory, it's in src/net

Doh, sorry, github confused me.

elcallio · 2024-12-09T15:12:14Z

src/net/ossl.cc

    // This function waits for the _output_pending future to resolve
    // If an error occurs, it is saved off into _error and returned
    future<> wait_for_output() {
+        _logger.trace("wait_for_output");


Why invent a new logger type when you could just add a formatter for the session object that pretty-prints the local/remote etc, and then just use tls_log::trace("{} wait_for_output")?

Thanks for the suggestion, great idea!

elcallio · 2024-12-09T15:13:17Z

tests/unit/tls_test.cc

+                // during the handshake before the client has fully
+                // closed its connection, then the get() call will
+                // succeed by return an empty buffer indicating EOF
+                BOOST_REQUIRE(res.size() == 0);


Seems like a good change, but not related to the rest of the PR

This test was being flaky when using OpenSSL. It would pass reliably in release but not in debug.

nit, might want to use BOOST_REQUIRE_EQUAL() for better postmortem debugging experience. as Boost.Test prints out the lhs and rhs of the comparison f the check fails.

avikivity · 2024-12-09T15:32:46Z

The different configuration options mean one of two:

side-by-side support, at least for a transitional period
emulate priority strings for openssl (far from the focus of Seastar)

So I think we should choose one implementation, and deprecate the other after a transition period.

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

mykaul · 2024-12-09T16:31:02Z

GnuTLS is not FIPS enabled, if compiled with it? (I see https://www.gnutls.org/manual/html_node/FIPS140_002d2-mode.html ) - what's missing?

michael-redpanda · 2024-12-09T20:18:35Z

GnuTLS is not FIPS enabled, if compiled with it? (I see https://www.gnutls.org/manual/html_node/FIPS140_002d2-mode.html ) - what's missing?

The FIPS flag for GnuTLS means that GnuTLS will work in a FIPS compliant way (e.g. rejecting any non FIPS approved crypto like DES or GOST), however that doesn't mean that its implementation was validated. GnuTLS doesn't provide a path to build it and maintain validation, OpenSSL does (see above links to OpenSSL's security policy).

elcallio · 2024-12-10T11:38:04Z

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

While I can somewhat sympathize with the sentiment, this would effectively mean dropping the way we handle TLS config for clients/applications (i.e. usage of the prio string). Since at least one database application I know of exposes this in its customer config, you'd effectively be asking to require changing all customer TLS configs where such a prio is applied. Not sure how many they are and how complicated the configs are. You said a transition period, but not sure how to handle this, nor enforce a config migration with clients?

avikivity · 2024-12-10T12:12:33Z

gnutls was chosen due to funky licensing and an unstable API from openssl, but I think that's behind us now. Given that, it's better to use the leader in the field rather than a follower.

While I can somewhat sympathize with the sentiment, this would effectively mean dropping the way we handle TLS config for clients/applications (i.e. usage of the prio string). Since at least one database application I know of exposes this in its customer config, you'd effectively be asking to require changing all customer TLS configs where such a prio is applied. Not sure how many they are and how complicated the configs are. You said a transition period, but not sure how to handle this, nor enforce a config migration with clients?

I would not want to force a config migration.

Claude says this:

The gnutls-cli tool includes a "--priority-debug" option that can show detailed information about a priority string's settings, but there isn't a direct tool to convert between GnuTLS priority strings and OpenSSL configuration formats.

You can analyze the GnuTLS priority string components with gnutls-cli --priority-debug "PRIORITY STRING" and then manually map those to equivalent OpenSSL configurations based on the cipher suites and protocols shown.

For more control, you could write a script that parses the gnutls-cli output and maps the components to OpenSSL's configuration syntax, though you'd need to account for the differences in how each library names and groups their ciphers and protocols.

elcallio · 2024-12-10T12:39:13Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc.
As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

avikivity · 2024-12-10T12:44:00Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc. As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

I agree with that.

@tzach do you have any insight about priority string configuration across our fleet? Do we ever diverge from the default?

mykaul · 2024-12-10T12:48:04Z

Yes, but I am honestly very nervous about writing/maintaining a prio string translator. The mapping is not just cipher to cipher etc, it is a state machine in itself, disabling and adding ciphers, exchange modes etc. As for using cli tool and manually map - that is what a customer would have to do if we changed the config approach.

I agree with that.

@tzach do you have any insight about priority string configuration across our fleet? Do we ever diverge from the default?

Since we did not disable TLSv1.1 by default (not sure why), there's a good chance users do it - https://enterprise.docs.scylladb.com/stable/operating-scylla/security/client-node-encryption.html#priority-string-and-tlsv1-2-1-3-support

michael-redpanda · 2024-12-12T01:06:54Z

Force push fc616ee:

Added compile definition SEASTAR_USE_GNUTLS for when GnuTLS is the cryptographic provder
Updated #ifndef SEASTAR_USE_OPENSSL to instead be #ifdef SEASTAR_USE_GNUTLS
Move implementation specific credential builder methods to the implementation specific compilation units
Removed tls_session_logger and added a pretty-print function for the OpenSSL implementation of seastar::tls::session
Dynamically setting minimum security level passed to SSL_CTX_set_security_level

michael-redpanda · 2024-12-12T13:46:27Z

Force push 1a53361:

Added missing compile definition for SEASTAR_USE_GNUTLS when compiled to use modules
Added missing header include for fmt/ostream.hh when compiled to use modules

elcallio

Code looks ok, but again, I doubt I can really verify it just by looking. :-(
One huge downside with the openssl code is that imho it seems to make the interface code even cludgier - perhaps partially an aspect of the code it needs to emulate is from a gnutls universe, but I would still argue that a lot of the gnutls interfaces are a bit nicer.
Thus I would worry a little about maintenance here.

elcallio · 2024-12-16T13:27:02Z

src/net/tls-impl.hh

+public:
+    static std::unique_ptr<connected_socket_impl> get(connected_socket s) {
+        return std::move(s._csi);
+    }


Yes. But that was a cc. This is a header.

elcallio · 2024-12-16T13:27:35Z

src/net/tls-impl.hh

+        if (_session && _session.use_count() == 1) {
+            _session->close();
+        }
+    }


Same as above. Code should not be in headers if it can be avoided (even internal, not very included ones)

elcallio · 2024-12-16T13:28:07Z

include/seastar/net/tls.hh

        void set_session_resume_mode(session_resume_mode);
+        void set_priority_string(const sstring&);


Seems like a gratuitous change?

elcallio · 2024-12-16T13:28:18Z

include/seastar/net/tls.hh

@@ -235,6 +247,12 @@ namespace tls {
        template<typename Base>
        friend class reloadable_credentials;
        shared_ptr<impl> _impl;
+
+        // The following methods are provided so classes that inherity from


Nit: inherit

elcallio · 2024-12-16T13:35:25Z

src/net/ossl.cc

+            auto dn = extract_dn_information();
+            if (dn) {
+                std::string_view stat_str_view{stat_str};
+                if (stat_str_view.ends_with(" ")) {


Nit: I assume this is an openssl quirk of some sort? Maybe a while instead (instead of assuming just one space), or maybe even a proper back and front whitespace strip?

elcallio · 2024-12-16T13:39:18Z

src/net/ossl.cc

+        }
+
+        auto & min_tls_version = _creds->minimum_tls_version();
+        auto & max_tls_version = _creds->maximum_tls_version();


Nit: space between type and ref.

elcallio · 2024-12-16T13:41:09Z

src/net/ossl.cc

+            }
+        }
+
+        auto get_min_level = [&ssl_ctx]() {


Nit: Should be "max level" really, as we should consider us setting the max level we can use given the tls version bounds.

elcallio · 2024-12-16T13:44:40Z

src/net/ossl.cc

+BIO_METHOD* get_method() {
+    static thread_local bio_method_ptr method_ptr = [] {
+        auto ptr = tls::create_bio_method();
+        if (!ptr) {


Seems redundant - all paths in create throws if failed...

elcallio · 2024-12-16T13:47:01Z

src/websocket/server.cc

@@ -126,6 +130,30 @@ future<> connection::process() {

 static std::string sha1_base64(std::string_view source) {
    unsigned char hash[20];
+
+#ifdef SEASTAR_USE_OPENSSL


Could we instead make the function an external/internal-header-decl, and move the full impl into gnutls/openssl compilation units? This just adds to an already bad separation of concern/api exposure.

Created tls-impl.cc and tls-impl.h which contains common structures and definitions that are not dependent on the underlying TLS mechanism. These changes set the stage for implementing other TLS providers. Signed-off-by: Michael Boquard <[email protected]>

This commit adds support for using OpenSSL, instead of GnuTLS, as the TLS provider within Seastar. To support this change, the configure script has been updated to allow users to select which cryptographic provider should be used by supply `--crypto-provider` and specificying either `OpenSSL` or `GnuTLS`. The OpenSSL implementation mirrors the GnuTLS implementation. Instead of using callbacks, a custom BIO was created to handle moving data on/off of the OpenSSL SSL session into the Seastar TLS session data sinks. When compiled for OpenSSL, the `certificate_credentials::set_priority_string` method is compiled out and replaced with the following: * `set_cipher_string` * `set_ciphersuites` * `enable_server_precedence` * `set_minimum_tls_version` * `set_maximum_tls_version` These methods are specific to OpenSSL. The github actions have been updated to run the full suite of tests against both cryptographic providers. `src/net/tcp.hh` and `src/websocket/server.cc` have been updated to use OpenSSL instead of GnuTLS, depending upon the build configuration. Signed-off-by: Michael Boquard <[email protected]>

Added pretty-print capabilities to seastar::tls::session for OpenSSL and added a number of log statements that may be helpful if debugging the implementation. Signed-off-by: Michael Boquard <[email protected]>

More recent versions of OpenSSL requrire CA certificates to have CA:true Signed-off-by: Michael Boquard <[email protected]>

Now handling situations where the get() call doesn't throw but does return an empty buffer indicating EOF. Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda · 2024-12-20T15:22:49Z

Force push 2f935ca:

Addressed issues from comments
Created GnuTLS & OpenSSL implementations for SHA1-Base64 used by websocket
Fixed nits

tchaikov · 2024-12-23T02:23:24Z

CMakeLists.txt

+  set(Seastar_USE_GNUTLS OFF)
+else()
+  set(Seastar_USE_GNUTLS ON)


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

src/net/tls-impl.cc

+ * under the License.
+ */
+/*
+ * Copyright 2015 Cloudius Systems


tchaikov · 2024-12-23T02:39:53Z

src/net/ossl.cc

@@ -79,6 +79,8 @@ module seastar;
 #include "net/tls-impl.hh"
 #endif

+template <> struct fmt::formatter<seastar::tls::session> : fmt::ostream_formatter {};


is it possible to implement the specialization of fmt::fornmatter<seastar::tls::session> without the operator<<? it would be better if we can avoid adding more operator<<, for two reasons:

avoid adding the unused operator<<

avoid the overhead of using a temporary ostream for formatting seastar::tls::session.

tchaikov · 2024-12-23T02:54:50Z

src/net/ossl.cc

+}
+
+template<>
+struct fmt::formatter<seastar::ossl_errc> : public fmt::formatter<std::string_view> {


instead of inheriting from fmt::formatter<std::string_view> , i'd suggest inheriting from fmt::formatter<string_view>. please see scylladb/scylladb@168ade7 for more details. we switched from this approach to the proposed one in the referenced commit in scylladb a while ago.

tchaikov · 2024-12-23T03:00:18Z

src/net/ossl.cc

@@ -770,6 +772,8 @@ class session : public enable_shared_from_this<session>, public session_impl {
    session(session_type t, shared_ptr<tls::certificate_credentials> creds,
            std::unique_ptr<net::connected_socket_impl> sock, tls_options options = {})
      : _sock(std::move(sock))
+      , _local_address(fmt::format("{}", _sock->local_address()))


nit, could use fmt::to_string(_sock->local_address()) instead. simpler this way. and slightly more performant.

tchaikov · 2024-12-23T03:00:26Z

src/net/ossl.cc

@@ -770,6 +772,8 @@ class session : public enable_shared_from_this<session>, public session_impl {
    session(session_type t, shared_ptr<tls::certificate_credentials> creds,
            std::unique_ptr<net::connected_socket_impl> sock, tls_options options = {})
      : _sock(std::move(sock))
+      , _local_address(fmt::format("{}", _sock->local_address()))
+      , _remote_address(fmt::format("{}", _sock->remote_address()))


tchaikov · 2024-12-23T03:01:32Z

src/net/ossl.cc

+                   session.get_type_string(),
+                   session.local_address(),
+                   session.remote_address());
+        return os;


as explained above, i'd prefer avoid adding more operator<< overloads unless they are actually necessary and used.

tchaikov · 2024-12-23T03:06:45Z

src/net/ossl.cc

            return make_ready_future();
        }
        return _in.get()
          .then([this](buf_type buf) {
              // Set EOF if it's empty
+              tls_log.debug("{} wait_for_input: buffer {}empty", *this, buf.empty() ? "is ": "");


i don't really understand this logging message:

if buf is empty, we print "wait_for_input: buffer is empty" -- looks good

if buf is not empty, we print "wait_for_input: buffer empty" -- what does this mean? shall we print "wait_for_input: buffer is not empty" or "wait_for_input: buffer not empty" instead?

tchaikov · 2024-12-23T03:25:43Z

.github/workflows/tests.yaml

+      mode: debug
+      enables: --enable-cxx-modules
+      enable-ccache: false
+      crypto_provider: OpenSSL


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

seastar/.github/workflows/test.yaml

Lines 110 to 112 in 613d8b3

- 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.

tchaikov · 2024-12-23T03:32:06Z

src/net/ossl.cc

+        auto err_code = static_cast<unsigned long>(error_codes.front());
+        if (ERR_LIB_SYS == ERR_GET_LIB(err_code)) {
+            // If the error code belongs to ERR_LIB_SYS, then the error is a system error
+            // Extract the errno using ERR_GET_REASON and throw a std::generic_category
+            return std::system_error(
+                ERR_GET_REASON(err_code),
+                std::generic_category(),
+                fmt::format("{}: {}", msg, error_codes));
+        }
+        return std::system_error(
+            static_cast<int>(err_code),
+            tls::error_category(),
+            fmt::format("{}: {}", msg, error_codes));


nit, could reduce the indent level by removing the "else" clause, because of the early return above.

tchaikov · 2024-12-23T03:39:19Z

src/net/ossl.cc

+
+    impl() : _creds([] {
+        auto store = X509_STORE_new();
+        if(store == nullptr) {


could you please add a space after if?

michael-redpanda mentioned this pull request Dec 9, 2024

tls: Introduce OpenSSL michael-redpanda/seastar#1

Closed

elcallio reviewed Dec 9, 2024

View reviewed changes

michael-redpanda force-pushed the add-openssl-implementation branch from 5c709c2 to fc616ee Compare December 12, 2024 01:03

michael-redpanda requested a review from elcallio December 12, 2024 01:07

michael-redpanda force-pushed the add-openssl-implementation branch from fc616ee to 1a53361 Compare December 12, 2024 13:45

elcallio reviewed Dec 16, 2024

View reviewed changes

michael-redpanda added 5 commits December 20, 2024 09:04

tls: Create tls-impl

2b7397f

Created tls-impl.cc and tls-impl.h which contains common structures and definitions that are not dependent on the underlying TLS mechanism. These changes set the stage for implementing other TLS providers. Signed-off-by: Michael Boquard <[email protected]>

tls: Added tls_logger and log statements

0a974b2

Added pretty-print capabilities to seastar::tls::session for OpenSSL and added a number of log statements that may be helpful if debugging the implementation. Signed-off-by: Michael Boquard <[email protected]>

test/tls: Using CA extension for generating CA

98a2d0f

More recent versions of OpenSSL requrire CA certificates to have CA:true Signed-off-by: Michael Boquard <[email protected]>

tls/test: Updated test_reload_certificates

2f935ca

Now handling situations where the get() call doesn't throw but does return an empty buffer indicating EOF. Signed-off-by: Michael Boquard <[email protected]>

michael-redpanda force-pushed the add-openssl-implementation branch from 1a53361 to 2f935ca Compare December 20, 2024 15:20

michael-redpanda requested a review from elcallio December 20, 2024 15:20

tchaikov reviewed Dec 23, 2024

View reviewed changes

src/net/tls-impl.cc

* under the License.

*/

/*

* Copyright 2015 Cloudius Systems

This comment was marked as resolved.

Sign in to view

tchaikov reviewed Dec 23, 2024

View reviewed changes

		@@ -202,13 +212,53 @@ namespace tls {

		// TODO add methods for certificate verification

		#ifndef SEASTAR_USE_OPENSSL

		void set_session_resume_mode(session_resume_mode);
		void set_priority_string(const sstring&);

	- name: Test
	if: ${{ ! contains(inputs.enables, 'cxx-modules') }}
	run: ./test.py --mode=${{ inputs.mode }}

tls: Introduce OpenSSL #2569

Are you sure you want to change the base?

tls: Introduce OpenSSL #2569

Conversation

michael-redpanda commented Dec 9, 2024 • edited Loading

avikivity commented Dec 9, 2024

michael-redpanda commented Dec 9, 2024 • edited Loading

elcallio 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

avikivity commented Dec 9, 2024

mykaul commented Dec 9, 2024

michael-redpanda commented Dec 9, 2024

elcallio commented Dec 10, 2024

avikivity commented Dec 10, 2024

elcallio commented Dec 10, 2024

avikivity commented Dec 10, 2024

mykaul commented Dec 10, 2024

michael-redpanda commented Dec 12, 2024

michael-redpanda commented Dec 12, 2024

elcallio 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

michael-redpanda commented Dec 20, 2024

Choose a reason for hiding this comment

This comment was marked as resolved.

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

michael-redpanda commented Dec 9, 2024 •

edited

Loading

michael-redpanda commented Dec 9, 2024 •

edited

Loading