diff --git a/src/net/tls_openssl.cc b/src/net/tls_openssl.cc index 1e40af9ea6e..1beb82a0150 100644 --- a/src/net/tls_openssl.cc +++ b/src/net/tls_openssl.cc @@ -1144,9 +1144,13 @@ class openssl_session : public enable_shared_from_this, public return make_ready_future<>(); } - if (contains_openssl_error(error_codes, ERR_LIB_SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE) || - contains_openssl_error(error_codes, ERR_LIB_SSL, SSL_R_CERTIFICATE_VERIFY_FAILED) || - contains_openssl_error(error_codes, ERR_LIB_SSL, SSL_R_NO_CERTIFICATES_RETURNED)) { + // Consult verify() when the peer certificate is at fault, + // keyed on the verification result rather than on + // version-dependent reason codes. The no-peer-cert codes + // leave that result at X509_V_OK, so check them explicitly. + if (SSL_get_verify_result(_ssl.get()) != X509_V_OK + || contains_openssl_error(error_codes, ERR_LIB_SSL, SSL_R_PEER_DID_NOT_RETURN_A_CERTIFICATE) + || contains_openssl_error(error_codes, ERR_LIB_SSL, SSL_R_NO_CERTIFICATES_RETURNED)) { try { verify(); } catch (...) { @@ -2527,8 +2531,13 @@ auto fmt::formatter::format(seastar::openssl_errc error, // Buffer passed to ERR_error_string must be at least 256 bytes large // https://www.openssl.org/docs/man3.0/man3/ERR_error_string_n.html std::array buf{}; - ERR_error_string_n( - static_cast(error), buf.data(), buf.size()); + const auto code = static_cast(error); + ERR_error_string_n(code, buf.data(), buf.size()); // ERR_error_string_n does include the terminating null character + if (ERR_GET_LIB(code) == ERR_LIB_SSL && ERR_GET_REASON(code) >= SSL_AD_REASON_OFFSET) { + // Reason codes >= SSL_AD_REASON_OFFSET are alerts received from the + // peer; the default wording obscures that direction. + return fmt::format_to(ctx.out(), "Received TLS alert from peer: {}", buf.data()); + } return fmt::format_to(ctx.out(), "{}", buf.data()); } diff --git a/tests/unit/tls_test.cc b/tests/unit/tls_test.cc index d902874e99a..4dc3403f345 100644 --- a/tests/unit/tls_test.cc +++ b/tests/unit/tls_test.cc @@ -2627,3 +2627,104 @@ 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"); +}