From 6fc2a85011228019a5bfa8343ab8afd0d0cffedd Mon Sep 17 00:00:00 2001 From: Mateusz Najda Date: Wed, 12 Aug 2026 14:49:30 +0200 Subject: [PATCH] net/tls: name the peer as the source of received TLS alerts OpenSSL words a received alert as "tlsv1 alert unknown ca", which reads as though the local side rejected a certificate. It is the opposite: the peer rejected the certificate we presented. The reason code alone cannot settle the direction, since the state machine also raises alert-range reasons for alerts it is about to send. Record the descriptions of alerts actually read from the peer with an SSL_CB_READ_ALERT info callback, and prefix "Received TLS alert from peer: " only on the handshake error codes that match one. OpenSSL's own text is kept verbatim, so log matchers keep working. Add tests for the three cases: a client certificate from an untrusted CA must still be named in the error, a received alert must not be displaced by "no certificate presented by peer", and a handshake failure with no alert read must not be attributed to the peer. (cherry picked from commit 4c8930e7672ec70f35d0f1da48b24f2b22529fbd) --- src/net/tls_openssl.cc | 62 +++++++++++++++++- tests/unit/tls_test.cc | 140 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 201 insertions(+), 1 deletion(-) diff --git a/src/net/tls_openssl.cc b/src/net/tls_openssl.cc index 1e40af9ea6e..4ac9a9ed66d 100644 --- a/src/net/tls_openssl.cc +++ b/src/net/tls_openssl.cc @@ -41,6 +41,7 @@ #include #include #include +#include #include #include #include @@ -187,6 +188,21 @@ bool contains_openssl_error(const std::vector & error_codes, int l }); } +// OpenSSL encodes a TLS alert as an ERR_LIB_SSL reason of SSL_AD_REASON_OFFSET +// plus the one-byte alert description. ERR_GET_REASON() keeps the ERR_RFLAG_* +// bits, so flagged reasons exceed the offset without being alerts. +static std::optional tls_alert_of_reason(openssl_errc error) { + const auto code = static_cast(error); + if (ERR_GET_LIB(code) != ERR_LIB_SSL) { + return std::nullopt; + } + const auto alert_description = ERR_GET_REASON(code) - SSL_AD_REASON_OFFSET; + if (!std::in_range(alert_description)) { + return std::nullopt; + } + return static_cast(alert_description); +} + template sstring asn1_str_to_str(T* asn1) { const auto len = ASN1_STRING_length(asn1); @@ -843,6 +859,21 @@ class openssl_session : public enable_shared_from_this, public if (1 != SSL_set_ex_data(_ssl.get(), SSL_EX_DATA_SESSION, this)) { throw make_openssl_error("Failed to set EX data for SSL session"); } + // Record alerts read from the peer, so a failure caused by a received + // alert can be attributed to the peer when it is reported. The reason + // code alone cannot make that call: the state machine also raises + // alert-range reasons for alerts the local side is about to send. + SSL_set_info_callback(_ssl.get(), [](const SSL* ssl, int where, int ret) { + if ((where & SSL_CB_READ_ALERT) != SSL_CB_READ_ALERT) { + return; + } + auto* session = static_cast( + SSL_get_ex_data(ssl, SSL_EX_DATA_SESSION)); + SEASTAR_ASSERT(session != nullptr); + // The description is ret's low byte, the part + // SSL_alert_desc_string_long() reads; the high byte is the level. + session->_alert_read = static_cast(ret); + }); bio_ptr in_bio(BIO_new(get_method())); bio_ptr out_bio(BIO_new(get_method())); if (!in_bio || !out_bio) { @@ -1107,6 +1138,10 @@ class openssl_session : public enable_shared_from_this, public tls_log.trace("{} do_handshake: already connected", *this); return make_ready_future<>(); } + // Scope the recorded alert to this handshake attempt: a renegotiation + // clears SSL_is_init_finished(), so get() can drive a second one. + // WANT_READ and WANT_WRITE retries stay inside the loop below. + _alert_read.reset(); return do_until( [this] { return connected() || eof(); }, [this] { @@ -1160,7 +1195,7 @@ class openssl_session : public enable_shared_from_this, public } // Verify did not throw, fall through and make a generic error } - auto err = make_openssl_error("Failed to establish SSL handshake", std::move(error_codes)); + auto err = make_handshake_error(std::move(error_codes)); return handle_output_error(std::move(err)); } default: @@ -1379,6 +1414,28 @@ class openssl_session : public enable_shared_from_this, public } } + // Like make_openssl_error(), but labels a code carrying an alert read from + // the peer as such: OpenSSL words a received alert as though the local side + // found the fault. + std::system_error make_handshake_error(std::vector error_codes) { + static const std::string msg = "Failed to establish SSL handshake"; + const auto is_peer_alert = [this](openssl_errc code) { + return _alert_read.has_value() && tls_alert_of_reason(code) == _alert_read; + }; + if (std::ranges::none_of(error_codes, is_peer_alert)) { + return make_openssl_error(msg, std::move(error_codes)); + } + auto rendered = error_codes | std::views::transform([&](openssl_errc code) { + return is_peer_alert(code) + ? fmt::format("Received TLS alert from peer: {}", code) + : fmt::format("{}", code); + }); + return std::system_error( + static_cast(error_codes.front()), + openssl_error_cat(), + fmt::format("{}: [{}]", msg, fmt::join(rendered, ", "))); + } + void verify() { tls_log.trace("{} verify", *this); if (!_creds->_enable_certificate_verification) { @@ -2163,6 +2220,9 @@ class openssl_session : public enable_shared_from_this, public session_type _type; bool _eof = false; bool _shutdown = false; + // Description of the last TLS alert read from the peer, recorded by the + // info callback registered in the constructor. + std::optional _alert_read; friend int bio_write_ex(BIO* b, const char * data, size_t dlen, size_t * written); friend int bio_read_ex(BIO* b, char * data, size_t dlen, size_t *readbytes); diff --git a/tests/unit/tls_test.cc b/tests/unit/tls_test.cc index d902874e99a..834ebde818c 100644 --- a/tests/unit/tls_test.cc +++ b/tests/unit/tls_test.cc @@ -2627,3 +2627,143 @@ SEASTAR_THREAD_TEST_CASE(test_output_pending_exception_on_destroy) { uint64_t after = engine().abandoned_failed_futures(); BOOST_REQUIRE_EQUAL(after, before); } + +static void require_contains(const sstring& text, std::string_view expected) { + BOOST_REQUIRE_MESSAGE(text.find(expected) != sstring::npos, + "expected to find \"" << expected << "\" in \"" << text << "\""); +} + +static void require_not_contains(const sstring& text, std::string_view unexpected) { + BOOST_REQUIRE_MESSAGE(text.find(unexpected) == sstring::npos, + "expected not to find \"" << unexpected << "\" in \"" << text << "\""); +} + +// Runs a handshake expected to fail over a loopback socket pair and returns the +// server-side exception message. The client's own failure is ignored. +static sstring failed_handshake_server_error( + ::shared_ptr server_creds, + ::shared_ptr client_creds) { + auto b1 = ::make_lw_shared(nullptr, loopback_buffer::type::SERVER_TX); + auto b2 = ::make_lw_shared(nullptr, loopback_buffer::type::CLIENT_TX); + + auto ssi = std::make_unique(b1, b2); + auto csi = std::make_unique(b2, b1); + + auto ss = tls::wrap_server(server_creds, connected_socket(std::move(ssi))).get(); + auto cs = tls::wrap_client(client_creds, connected_socket(std::move(csi)), + tls::tls_options{.server_name = "test.scylladb.org"}).get(); + + auto strms = ::make_lw_shared(std::move(cs)); + auto client_loop = strms->out.write(message) + .then([strms] { return strms->out.flush(); }) + .then([strms] { return strms->in.read().discard_result(); }) + .handle_exception([](std::exception_ptr) {}); + + sstring server_error; + try { + ss.input().read().get(); + BOOST_FAIL("Expected the server side of the handshake to fail"); + } catch (const std::exception& e) { + server_error = e.what(); + } + client_loop.get(); + + BOOST_TEST_MESSAGE(fmt::format("server side error: {}", server_error).c_str()); + return server_error; +} + +// A client cert signed by a CA the server does not trust must be reported with +// the offending certificate's DN, so an operator can tell which client +// misbehaved. OpenSSL backend only; GnuTLS words these errors differently. +SEASTAR_THREAD_TEST_CASE(test_x509_server_rejects_client_cert_from_unknown_ca) { + if (using_gnutls()) { + return; + } + + tls::credentials_builder cb; + // other.crt is signed by caother, which the server does not trust. + cb.set_x509_key_file(certfile("other.crt"), certfile("other.key"), tls::x509_crt_format::PEM).get(); + cb.set_x509_trust_file(certfile("catest.pem"), tls::x509_crt_format::PEM).get(); + auto client_creds = cb.build_certificate_credentials(); + + tls::credentials_builder sb; + sb.set_x509_key_file(certfile("test.crt"), certfile("test.key"), tls::x509_crt_format::PEM).get(); + sb.set_x509_trust_file(certfile("catest.pem"), tls::x509_crt_format::PEM).get(); + sb.set_client_auth(tls::client_auth::REQUIRE); + sb.set_dh_level(); + auto server_creds = sb.build_server_credentials(); + + auto err = failed_handshake_server_error(server_creds, client_creds); + + require_contains(err, "Issuer"); + require_contains(err, "Subject"); + require_contains(err, "other.apa.org"); +} + +// A client that rejects the server's certificate chain aborts with an +// unknown_ca(48) alert, before sending a certificate of its own. The server-side +// error must name that received alert - not local certificate state such as +// "no certificate presented by peer", which is a consequence of the abort. +// OpenSSL backend only; GnuTLS words these errors differently. +SEASTAR_THREAD_TEST_CASE(test_server_handshake_error_preserves_ssl_error_detail) { + if (using_gnutls()) { + return; + } + + tls::credentials_builder cb; + // A trust store that does not contain the server's CA. + cb.set_x509_trust_file(certfile("tls-ca-bundle.pem"), tls::x509_crt_format::PEM).get(); + auto client_creds = cb.build_certificate_credentials(); + + tls::credentials_builder sb; + sb.set_x509_key_file(certfile("test.crt"), certfile("test.key"), tls::x509_crt_format::PEM).get(); + sb.set_x509_trust_file(certfile("catest.pem"), tls::x509_crt_format::PEM).get(); + sb.set_client_auth(tls::client_auth::REQUIRE); + sb.set_dh_level(); + auto server_creds = sb.build_server_credentials(); + + auto err = failed_handshake_server_error(server_creds, client_creds); + + require_contains(err, "Received TLS alert from peer"); + require_contains(err, "unknown ca"); + require_not_contains(err, "no certificate presented by peer"); +} + +// A failure the server diagnoses itself, with no alert arriving from the peer, +// must not be reported as a received alert. +// OpenSSL backend only; GnuTLS words these errors differently. +SEASTAR_THREAD_TEST_CASE(test_handshake_error_without_alert_is_not_from_peer) { + if (using_gnutls()) { + return; + } + + tls::credentials_builder sb; + sb.set_x509_key_file(certfile("test.crt"), certfile("test.key"), tls::x509_crt_format::PEM).get(); + auto server_creds = sb.build_server_credentials(); + + auto b1 = ::make_lw_shared(nullptr, loopback_buffer::type::SERVER_TX); + auto b2 = ::make_lw_shared(nullptr, loopback_buffer::type::CLIENT_TX); + auto ssi = std::make_unique(b1, b2); + auto csi = std::make_unique(b2, b1); + + auto ss = tls::wrap_server(server_creds, connected_socket(std::move(ssi))).get(); + + // Plain HTTP to a TLS server: the server diagnoses the failure itself. + ::connected_socket cs(std::move(csi)); + output_stream out(cs.output().detach(), 1024); + out.write(sstring("GET / HTTP/1.1\r\n\r\n")).get(); + out.flush().get(); + + sstring err; + try { + ss.input().read().get(); + BOOST_FAIL("Expected the server side of the handshake to fail"); + } catch (const std::exception& e) { + err = e.what(); + } + BOOST_TEST_MESSAGE(fmt::format("server side error: {}", err).c_str()); + + require_contains(err, "Failed to establish SSL handshake"); + require_contains(err, "Received HTTP request on HTTPS server"); + require_not_contains(err, "Received TLS alert from peer"); +}