Fix for broken_promise exception observed on connection shutdown #106

graphcareful · 2024-04-12T17:41:28Z

REVIEWERS: Please review #115 instead

A broken_promise exception was observed when calling close() due to the fact that the input stream was called while it was waiting on a pending operation to complete.

This occurred on call to close because of how do_shutdown() is implemented. do_shutdown() acquires the output semaphore, but not the input, then attempts to wait_for_input(). If there is already a waiter on the read path, then the broken_promise exception will be observed when do_shutdown() calls do_get().

The fix is to simply modify do_shutdown() to call wait_for_eof() instead of wait_for_input(),however this would be incomplete because there are other instances in the implementation where out of band operations on the read / write path come from opposite paths.

To resolve the above concern new methods have been developed (maybe_wait_for_data and maybe_send_data) which attempt to acquire the in/out semaphore before performing the desired operation. If the semaphore is already acquired, then the method just returns to the caller, which would lead to a retry until the operation can be performed.

Fixes: CORE-539

graphcareful · 2024-04-12T17:52:33Z

Changes in force-push

Split formatting commit out

graphcareful · 2024-04-12T20:45:04Z

Changes in force-push

Removed accidental duplicate call wrapping do_get() in context of semaphore
Slightly reworked logic in wait_for_eof() , previously this depended on waiting on _in_sem a deadlock was detected in a unit test. Its been since refactored to use wait_for_input() instead

src/net/ossl.cc

graphcareful · 2024-04-18T18:09:37Z

Changes in force-push

Modified implementation to not use 2 additional semaphores and instead retry if semaphore is already taken , otherwise take lease and perform work.
Ensure read lock is taken within wait_for_eof

michael-redpanda

Just one clean up suggestion

michael-redpanda · 2024-04-22T13:27:34Z

src/net/ossl.cc

+        static const auto max_len = 4096;
+        char buffer[max_len];
+        memset(buffer, 0, max_len);
+        BIO_read(out.get(), buffer, max_len - 1);
+        return sstring(buffer);


Suggested change

static const auto max_len = 4096;

char buffer[max_len];

memset(buffer, 0, max_len);

BIO_read(out.get(), buffer, max_len - 1);

return sstring(buffer);

char * bio_ptr = nullptr;

auto len = BIO_get_mem_data(out.get(), &bio_ptr);

return sstring(bio_ptr, len);

char * bio_ptr = nullptr; auto len = BIO_get_mem_data(out.get(), &bio_ptr); return sstring(bio_ptr, len);

BIO_get_mem_data() returns the total number of bytes available on success, 0 if b is NULL, or a negative value in case of other errors.

Looks like the error condition should be checked?

BenPope · 2024-04-23T10:24:47Z

src/net/ossl.cc

-            return ossl_str;
+    static std::optional<sstring> get_dn_string(X509_NAME* name) {
+        auto out = bio_ptr(BIO_new(BIO_s_mem()));
+        if (!X509_NAME_print_ex(out.get(), name, 0, ASN1_STRFLGS_RFC2253 | XN_FLAG_SEP_COMMA_PLUS |


X509_NAME_print_ex() and X509_NAME_print_ex_fp() return 1 on success or 0 on error if the XN_FLAG_COMPAT is set, which is the same as X509_NAME_print(). Otherwise, it returns -1 on error or other values on success.

Should the check be -1 == X509_NAME_print_ex?

BenPope · 2024-04-23T10:27:24Z

src/net/ossl.cc

+        static const auto max_len = 4096;
+        char buffer[max_len];
+        memset(buffer, 0, max_len);
+        BIO_read(out.get(), buffer, max_len - 1);
+        return sstring(buffer);


char * bio_ptr = nullptr; auto len = BIO_get_mem_data(out.get(), &bio_ptr); return sstring(bio_ptr, len);

BIO_get_mem_data() returns the total number of bytes available on success, 0 if b is NULL, or a negative value in case of other errors.

Looks like the error condition should be checked?

BenPope · 2024-04-23T10:30:14Z

tests/unit/tls_test.cc

@@ -1444,6 +1444,10 @@ SEASTAR_THREAD_TEST_CASE(test_dn_name_handling) {
        fout.get();

        auto dn = fdn.get();
+        BOOST_REQUIRE(dn.has_value());


Are there tests for the failure modes?

Nope, i believe these unit tests could be enhanced a bit more

(cherry picked from commit 5466b77)

- Now that code has been moved around, the session class' methods need to be known to consumers for example tls_connected_socket_impl. - Solution is to create a new interface, called session_impl that tls::session inherits from. session_ref now stores a pointer to this interface instead of the concrete instance. - This will be useful for defining other session types in future commits (cherry picked from commit 0ffb0fc)

- Now that code has been moved around the private _impl within the certificate credentials class cannot be accessed externally due to the fact that impl is forward defined in tls.hh - Solution is to create new method on the certificate_credentials class that will forward the call to the underlying impl (cherry picked from commit d44bf62)

- Also disables websockets (cherry picked from commit eb2cfd5)

- The implementation of the struct isn't neccessary to get up and running with a simple server. However the methods must have implementations or the code won't compile. (cherry picked from commit 173b1a6)

(cherry picked from commit 789669a)

(cherry picked from commit 1cf20ec)

(cherry picked from commit 1298e57)

(cherry picked from commit 0e8cfae)

(cherry picked from commit 10c083a)

- Caching the x509 certs as they are in the process of being verified is neccessary because openssl does not provide an API to get certificates that have failed verification. - Examining failed certificates for the reason they failed will be implemented in future commits. (cherry picked from commit 5e14b14)

(cherry picked from commit b453dd7)

(cherry picked from commit 699444a)

(cherry picked from commit 62f6a66)

(cherry picked from commit dfb03e3)

- Differences in dn name handling will cause issues in projects that expected a subject or issuer to be in a particular format. This is the case for features like mtls principal mapping within redpanda. - The method X509_NAME_oneline printed a nicely human readable version of the distinguished name parameters however this differed from what GnuTLS would print. - The change swaps out the use of X509_NAME_oneline in favor of X509_NAME_print_ex which accepts an integer for which can be used to control how the distinguished name is formatted. (cherry picked from commit c09cd25)

graphcareful · 2024-05-06T14:22:04Z

Note to reviewers, base has been modified to rebase-feature-ossl which is the feature-ossl branch with the seastar 24.2.x changes rebased onto it.

- x509_verify was never the correct function to verify the signature of a private key, replacing with x509_check_private_key instead. Fixes: CORE-2025

- Flush() aquires the _out_sem and so does do_shutdown, a deadlock was observed due to this fact. Flush would wait for shutdown to complete, which was being held by wait_for_eof. After a 10s timeout, eof would manually be set and the stall would be resolved. - The fix is to not call wait_for_eof within the context of the _out_sem.

- With this behavior redpanda will not log disconnects as ERRORs during shutdown

- This line will cause redpanda to crash as its currently set to pass GnuTLS specific priority strings.

BenPope · 2024-05-13T13:02:59Z

src/net/ossl.cc

@@ -824,7 +850,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
                f = pull_encrypted_and_send();


Should this by maybe_send_data()?

BenPope · 2024-05-14T11:39:23Z

src/net/ossl.cc

@@ -1204,7 +1227,8 @@ class session : public enable_shared_from_this<session>, public session_impl {
    future<> server_handshake() {
        return wait_for_input().then([this]{
            if (eof()) {
-                return make_exception_future<>(std::runtime_error("EOF observed during handshake"));
+                _error = std::make_exception_ptr(std::system_error(ENOTCONN, std::system_category()));


Could there be an existing error that should be preferred/not overwritten?

BenPope · 2024-05-14T11:39:28Z

src/net/ossl.cc

    }

    future<> client_handshake() {
        return do_handshake(SSL_connect, [this]{
            return pull_encrypted_and_send().then([this]{
                return wait_for_input().then([this]{
                    if (eof()) {
-                        return make_exception_future<>(std::runtime_error("EOF observed during handshake"));
+                        _error = std::make_exception_ptr(std::system_error(ENOTCONN, std::system_category()));


Could there be an existing error that should be preferred/not overwritten?

Is it possible to add a test?

The commit message talks about Redpanda, but I think it would be more appropriate to discuss this in terms of compatibility with existing behaviour in GnuTLS.

The commit message talks about Redpanda, but I think it would be more appropriate to discuss this in terms of compatibility with existing behaviour in GnuTLS.

In this situation, GnuTLS will return an error code which turns into a system_error which Redpanda does parse to see if it's a reconnect type error.

BenPope · 2024-05-14T11:40:21Z

src/net/ossl.cc

        if (!ext) {
            return {};
        }
-        auto names = general_names_ptr((GENERAL_NAMES*)X509V3_EXT_d2i(ext.get()));
+        auto names = general_names_ptr((GENERAL_NAMES*)X509V3_EXT_d2i(ext));


Prefer static_cast.

BenPope · 2024-05-14T11:44:30Z

cmake/SeastarDependencies.cmake

+  seastar_set_dep_args (OpenSSL REQUIRED
+    VERSION 3.0.0)


Is this version correct? Is it an exact match? A minimum?

It's a minimum.

BenPope · 2024-05-14T12:04:46Z

src/net/ossl.cc

+                if (!_options.wait_for_eof_on_shutdown) {
+                    return make_ready_future<>();
+                }


Can you add a note about why this is "sidestepped".

This matches original behavior in the GnuTLS implementation

BenPope · 2024-05-14T12:12:29Z

src/net/ossl.cc

@@ -315,11 +315,9 @@ class tls::certificate_credentials::impl {
        if (!pkey) {
            throw ossl_error("Error attempting to parse private key");
        }
-#if 0 // https://github.com/redpanda-data/core-internal/issues/1233
-        if (!X509_verify(x509_cert.get(), pkey.get())) {
+        if (!X509_check_private_key(x509_cert.get(), pkey.get())) {


Can a test be added?

Seastar's TLS unit tests do verify the coherency of the key and cert

BenPope · 2024-05-14T12:14:23Z

src/net/ossl.cc

@@ -828,7 +828,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
    }

    future<> do_shutdown() {
-        if(_error || !connected() || eof()) {
+        if(_error || !connected()) {


Can a test be added?

There are a number of shutdown unit tests in Seastar that verify the shutdown behavior

michael-redpanda · 2024-05-17T18:50:05Z

Leaving this in draft, but will close. Please review #115

graphcareful requested review from dotnwat, BenPope and michael-redpanda April 12, 2024 17:41

graphcareful force-pushed the ossl-shutdown-bug branch 2 times, most recently from c3a5aca to 403239d Compare April 12, 2024 17:52

graphcareful force-pushed the ossl-shutdown-bug branch from 403239d to 5c76161 Compare April 12, 2024 20:43

graphcareful force-pushed the ossl-shutdown-bug branch from d47468a to d76a31a Compare April 16, 2024 20:16

michael-redpanda reviewed Apr 17, 2024

View reviewed changes

src/net/ossl.cc Outdated Show resolved Hide resolved

src/net/ossl.cc Outdated Show resolved Hide resolved

graphcareful force-pushed the ossl-shutdown-bug branch from d76a31a to c09cd25 Compare April 18, 2024 16:40

graphcareful requested a review from michael-redpanda April 18, 2024 16:41

michael-redpanda reviewed Apr 22, 2024

View reviewed changes

BenPope reviewed Apr 23, 2024

View reviewed changes

graphcareful added 15 commits May 3, 2024 10:34

net: Move some to be shared code out of tls.cc

c19e9ea

(cherry picked from commit 5466b77)

build: Conditionally build with openssl

3f59727

- Also disables websockets (cherry picked from commit eb2cfd5)

net: Stub ossl version of tls::dh_params struct

9423b31

- The implementation of the struct isn't neccessary to get up and running with a simple server. However the methods must have implementations or the code won't compile. (cherry picked from commit 173b1a6)

net: openssl impl of cert_credentials::impl

e0bba65

(cherry picked from commit 789669a)

net: Stubs for new method get_x509_info

b358ef3

(cherry picked from commit 1cf20ec)

net: openssl impl of tls::session class

b78f688

(cherry picked from commit 1298e57)

demos: Print exceptions in tls echo server

5f9ae4f

(cherry picked from commit 0e8cfae)

demos: Enable a dn verification callback

c05b066

(cherry picked from commit 10c083a)

net: Implement verify() callback for ossl impl

74580e4

(cherry picked from commit b453dd7)

net: Fix SSL_WANT_WRITE case during handshake

49f92f7

(cherry picked from commit 699444a)

net: Implement session::get_distinguished_name

dd025ac

(cherry picked from commit 62f6a66)

net: Implement get_alt_name_information

a5e33c2

(cherry picked from commit dfb03e3)

graphcareful added 2 commits May 3, 2024 10:53

net: Sidestep normal shutdown on no wait_for_eof

5809ab4

graphcareful changed the base branch from feature-ossl to rebase-feature-ossl May 6, 2024 14:19

graphcareful changed the base branch from rebase-feature-ossl to feature-ossl May 6, 2024 14:20

graphcareful changed the base branch from feature-ossl to rebase-feature-ossl May 6, 2024 14:20

graphcareful force-pushed the ossl-shutdown-bug branch from c09cd25 to 5809ab4 Compare May 6, 2024 14:21

michael-redpanda requested review from michael-redpanda and BenPope May 7, 2024 11:59

graphcareful added 6 commits May 8, 2024 14:25

cmake: Add OpenSSL to SeastarDependencies

b13bc63

net: Throw EIO exception upon failure to write

ab945ee

net: Replace X509_verify for X509_checkprivate_key

afef826

- x509_verify was never the correct function to verify the signature of a private key, replacing with x509_check_private_key instead. Fixes: CORE-2025

net: Throw system_error ENOTCONN on handshake EOF

0a391eb

- With this behavior redpanda will not log disconnects as ERRORs during shutdown

REMOVE BEFORE SUBMITTING

c33efd7

- This line will cause redpanda to crash as its currently set to pass GnuTLS specific priority strings.

graphcareful force-pushed the ossl-shutdown-bug branch from 9512dd9 to c33efd7 Compare May 13, 2024 03:13

BenPope reviewed May 14, 2024

View reviewed changes

michael-redpanda force-pushed the rebase-feature-ossl branch 2 times, most recently from 58f6053 to 83fae91 Compare May 17, 2024 17:07

michael-redpanda marked this pull request as draft May 17, 2024 18:49

michael-redpanda requested review from BenPope and michael-redpanda and removed request for dotnwat, michael-redpanda and BenPope May 17, 2024 18:50

michael-redpanda closed this May 27, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for broken_promise exception observed on connection shutdown #106

Fix for broken_promise exception observed on connection shutdown #106

graphcareful commented Apr 12, 2024 •

edited by michael-redpanda

graphcareful commented Apr 12, 2024

graphcareful commented Apr 12, 2024

graphcareful commented Apr 18, 2024

michael-redpanda left a comment

michael-redpanda Apr 22, 2024 •

edited

BenPope Apr 23, 2024

BenPope Apr 23, 2024

BenPope Apr 23, 2024

BenPope Apr 23, 2024

graphcareful May 3, 2024

graphcareful commented May 6, 2024

BenPope May 13, 2024

BenPope May 14, 2024

BenPope May 14, 2024

BenPope May 14, 2024

michael-redpanda May 14, 2024

BenPope May 14, 2024

BenPope May 14, 2024

michael-redpanda May 14, 2024

BenPope May 14, 2024

michael-redpanda May 17, 2024

BenPope May 14, 2024

michael-redpanda May 17, 2024

BenPope May 14, 2024

michael-redpanda May 17, 2024

michael-redpanda commented May 17, 2024

		@@ -824,7 +850,7 @@ class session : public enable_shared_from_this<session>, public session_impl {
		f = pull_encrypted_and_send();

Fix for broken_promise exception observed on connection shutdown #106

Fix for broken_promise exception observed on connection shutdown #106

Conversation

graphcareful commented Apr 12, 2024 • edited by michael-redpanda

graphcareful commented Apr 12, 2024

graphcareful commented Apr 12, 2024

graphcareful commented Apr 18, 2024

michael-redpanda left a comment

Choose a reason for hiding this comment

michael-redpanda Apr 22, 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

graphcareful commented May 6, 2024

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

michael-redpanda commented May 17, 2024

graphcareful commented Apr 12, 2024 •

edited by michael-redpanda

michael-redpanda Apr 22, 2024 •

edited