From 96fad948b500a4b6af85a4e8397f94d45a824bb8 Mon Sep 17 00:00:00 2001 From: David Yu Date: Mon, 21 Sep 2026 22:18:21 -0700 Subject: [PATCH] tls: verify the peer certificate against server_name on OpenSSL, opt-in MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The GnuTLS backend passes server_name to gnutls_certificate_verify_peers3, so a certificate that chains to a trusted CA but is issued for a different host (or a different IP SAN) fails verification. The OpenSSL backend only ever checked the chain: SSL_VERIFY_PEER with no expected host, so any trusted certificate was accepted for any server_name. Add tls_options::verify_server_name. On OpenSSL it registers the expected identity on the session's X509_VERIFY_PARAM — an IP literal (brackets and zone stripped) via X509_VERIFY_PARAM_set1_ip_asc, a DNS name via SSL_set1_host without partial wildcards — so the mismatch surfaces through SSL_get_verify_result and the existing verify() path. It defaults to false: clients that connect by address to servers whose certificates carry no IP SAN keep working until they opt in. GnuTLS keeps its always-on check; the header documents the difference. The test certificate gains an IP SAN for ::1 next to 127.0.0.1, and test_alt_names asserts both values. New cases run the echo test with the flag set: matching DNS name and IP literals (127.0.0.1, ::1, [::1]) pass, a wrong DNS name and wrong addresses raise verification_error. --- include/seastar/net/tls.hh | 12 +++++- src/net/tls_gnutls.cc | 5 ++- src/net/tls_openssl.cc | 20 ++++++++++ tests/unit/CMakeLists.txt | 3 ++ tests/unit/cert.cfg.in | 2 +- tests/unit/tls_test.cc | 78 +++++++++++++++++++++++++++++++++++--- 6 files changed, 110 insertions(+), 10 deletions(-) diff --git a/include/seastar/net/tls.hh b/include/seastar/net/tls.hh index 3136d57aeed..8f001c1129e 100644 --- a/include/seastar/net/tls.hh +++ b/include/seastar/net/tls.hh @@ -445,13 +445,23 @@ namespace tls { public: /// \brief whether to wait for EOF from server on session termination deprecated_wait_for_eof_on_shutdown wait_for_eof_on_shutdown; - /// \brief server name to be used for the SNI TLS extension + /// \brief the server being connected to: a DNS name, or an IP literal + /// (brackets allowed) which is not sent as SNI (RFC 6066 §3). Also the + /// name verification checks, see verify_server_name. sstring server_name = {}; /// \brief whether server certificate should be verified. May be set to false /// in test environments. bool verify_certificate = true; + /// \brief whether the peer certificate must be issued for server_name + /// (a DNS SAN for names, an IP SAN for literals) as well as chaining to + /// a trusted CA. GnuTLS always checks this when server_name is set; + /// OpenSSL only when this is true. Off by default, so clients + /// connecting by address to certificates without an IP SAN keep + /// working until they opt in. + bool verify_server_name = false; + /// \brief Optional session resume data. Must be retrieved via /// get_session_resume_data below. session_data session_resume_data; diff --git a/src/net/tls_gnutls.cc b/src/net/tls_gnutls.cc index 66b052b3563..7ee88012080 100644 --- a/src/net/tls_gnutls.cc +++ b/src/net/tls_gnutls.cc @@ -749,8 +749,9 @@ class session : public enable_shared_from_this, public tls::session_imp } unsigned int status; - auto res = gnutls_certificate_verify_peers3(*this, _type != type::CLIENT || _options.server_name.empty() - ? nullptr : _options.server_name.c_str(), &status); + auto name = verification_name(_options.server_name); + auto res = gnutls_certificate_verify_peers3(*this, _type != type::CLIENT || name.empty() + ? nullptr : name.c_str(), &status); if (res == GNUTLS_E_NO_CERTIFICATE_FOUND && _type != type::CLIENT && _creds->get_client_auth() != client_auth::REQUIRE) { return; } diff --git a/src/net/tls_openssl.cc b/src/net/tls_openssl.cc index 75a0a41db55..bfcc8fd5df2 100644 --- a/src/net/tls_openssl.cc +++ b/src/net/tls_openssl.cc @@ -845,6 +845,9 @@ class openssl_session : public enable_shared_from_this, public SSL_set_tlsext_host_name( _ssl.get(), _options.server_name.c_str()); } + if (_options.verify_server_name && !_options.server_name.empty()) { + expect_peer_name(_options.server_name); + } SSL_set_connect_state(_ssl.get()); } @@ -867,6 +870,23 @@ class openssl_session : public enable_shared_from_this, public tls_options options = {}) : openssl_session(t, std::move(creds), net::get_impl::get(std::move(sock)), options) {} + // OpenSSL then reports a mismatch through SSL_get_verify_result, so + // verify() sees it like any other chain error. + void expect_peer_name(std::string_view name) { + auto host = verification_name(name); + auto* param = SSL_get0_param(_ssl.get()); + if (is_ip_literal(host)) { + if (1 != X509_VERIFY_PARAM_set1_ip_asc(param, host.c_str())) { + throw make_openssl_error("Failed to set the expected peer address"); + } + return; + } + X509_VERIFY_PARAM_set_hostflags(param, X509_CHECK_FLAG_NO_PARTIAL_WILDCARDS); + if (1 != SSL_set1_host(_ssl.get(), host.c_str())) { + throw make_openssl_error("Failed to set the expected peer name"); + } + } + ~openssl_session() { SEASTAR_ASSERT(_output_pending.available()); } diff --git a/tests/unit/CMakeLists.txt b/tests/unit/CMakeLists.txt index 061780beafc..19c2d5f6717 100644 --- a/tests/unit/CMakeLists.txt +++ b/tests/unit/CMakeLists.txt @@ -632,6 +632,9 @@ function(seastar_add_certgen name) if (NOT CERT_ALT_IP_1) set(CERT_ALT_IP_1 127.0.0.1) endif() + if (NOT CERT_ALT_IP_2) + set(CERT_ALT_IP_2 ::1) + endif() if (NOT CERT_ALT_DNS) set(CERT_ALT_DNS ${CERT_COMMON}) endif() diff --git a/tests/unit/cert.cfg.in b/tests/unit/cert.cfg.in index f4b894418d1..e48768bb894 100644 --- a/tests/unit/cert.cfg.in +++ b/tests/unit/cert.cfg.in @@ -23,4 +23,4 @@ basicConstraints = CA:FALSE keyUsage = nonRepudiation, digitalSignature, keyEncipherment [req_ext] -subjectAltName=email:@CERT_ALT_EMAIL_1@,email:@CERT_ALT_EMAIL_2@,IP:@CERT_ALT_IP_1@,DNS:@CERT_ALT_DNS@ +subjectAltName=email:@CERT_ALT_EMAIL_1@,email:@CERT_ALT_EMAIL_2@,IP:@CERT_ALT_IP_1@,IP:@CERT_ALT_IP_2@,DNS:@CERT_ALT_DNS@ diff --git a/tests/unit/tls_test.cc b/tests/unit/tls_test.cc index 31a1a0bc478..462652f8596 100644 --- a/tests/unit/tls_test.cc +++ b/tests/unit/tls_test.cc @@ -49,6 +49,7 @@ #include +#include "ipv6_support.hh" #include "loopback_socket.hh" #include "tmpdir.hh" @@ -745,9 +746,10 @@ static future<> echo_client_session(::shared_ptr msg, socket_address addr, const sstring& name, int loops, - bool do_read) + bool do_read, + bool verify_server_name = false) { - auto s = co_await tls::connect(certs, addr, tls::tls_options{.server_name = name}); + auto s = co_await tls::connect(certs, addr, tls::tls_options{.server_name = name, .verify_server_name = verify_server_name}); auto strms = ::make_lw_shared(std::move(s)); auto echo = [strms, msg, loops]() -> future<> { @@ -779,7 +781,9 @@ static future<> run_echo_test(sstring message, sstring client_key = {}, bool do_read = true, bool use_dh_params = true, - tls::dn_callback distinguished_name_callback = {} + tls::dn_callback distinguished_name_callback = {}, + bool verify_server_name = false, + std::optional listen_addr = {} ) { static const auto port = 4711; @@ -787,7 +791,7 @@ static future<> run_echo_test(sstring message, auto msg = ::make_shared(std::move(message)); auto certs = ::make_shared(); auto server = ::make_shared>(); - auto addr = ::make_ipv4_address( {0x7f000001, port}); + auto addr = listen_addr.value_or(::make_ipv4_address( {0x7f000001, port})); SEASTAR_ASSERT(do_read || loops == 1); @@ -806,7 +810,7 @@ static future<> run_echo_test(sstring message, server_trust = trust; } co_await server->invoke_on_all(&echoserver::listen, addr, crt, key, ca, server_trust); - co_await echo_client_session(msg, certs, addr, name, loops, do_read); + co_await echo_client_session(msg, certs, addr, name, loops, do_read, verify_server_name); }().finally([server] { return server->stop(); }); @@ -878,6 +882,57 @@ SEASTAR_TEST_CASE(test_x509_client_server_cert_validation_fail_name) { }); } +// run_echo_test against the local server with verify_server_name set; +// test.crt is issued for test.scylladb.org with IP SANs 127.0.0.1 and ::1. +static future<> run_verified_echo_test(sstring name, std::optional addr = {}) { + return run_echo_test(message, 1, certfile("catest.pem"), std::move(name), certfile("test.crt"), certfile("test.key"), + tls::client_auth::NONE, {}, {}, true, true, {}, true, std::move(addr)); +} + +static future<> expect_verification_error(future<> f) { + return f.then([] { + BOOST_FAIL("Should have gotten validation error"); + }).handle_exception([](auto ep) { + try { + std::rethrow_exception(ep); + } catch (tls::verification_error&) { + // ok. + } catch (...) { + BOOST_FAIL(fmt::format("Unexpected exception: {}", std::current_exception())); + } + }); +} + +SEASTAR_TEST_CASE(test_verify_server_name_dns_match) { + return run_verified_echo_test("test.scylladb.org"); +} + +SEASTAR_TEST_CASE(test_verify_server_name_dns_mismatch) { + // trusted CA, wrong name: the only thing standing between the client and + // this server is the name check + return expect_verification_error(run_verified_echo_test("nils.holgersson.gov")); +} + +SEASTAR_TEST_CASE(test_verify_server_name_ip_san_ipv4) { + return run_verified_echo_test("127.0.0.1"); +} + +SEASTAR_TEST_CASE(test_verify_server_name_ip_san_mismatch) { + return expect_verification_error(run_verified_echo_test("127.0.0.2")); +} + +SEASTAR_TEST_CASE(test_verify_server_name_ip_san_ipv6) { + if (!seastar::testing::ipv6_available_or_skip()) { + return make_ready_future<>(); + } + auto addr = socket_address(ipv6_addr("::1", 4711)); + return run_verified_echo_test("::1", addr).then([addr] { + return run_verified_echo_test("[::1]", addr); + }).then([addr] { + return expect_verification_error(run_verified_echo_test("::2", addr)); + }); +} + SEASTAR_TEST_CASE(test_large_message_x509_client_server) { // Make sure we load our own auth trust pem file, otherwise our certs // will not validate @@ -1621,9 +1676,20 @@ SEASTAR_THREAD_TEST_CASE(test_alt_names) { BOOST_FAIL("Missing " + std::to_string(min_count) + " alt name attributes of type " + std::to_string(int(type))); }; - ensure_alt_name(tls::subject_alt_name_type::ipaddress, 1); + ensure_alt_name(tls::subject_alt_name_type::ipaddress, 2); ensure_alt_name(tls::subject_alt_name_type::rfc822name, 2); ensure_alt_name(tls::subject_alt_name_type::dnsname, 1); + + // and the IP SANs come back as addresses of the right family + std::vector ips; + for (auto& v : alt_names) { + if (v.type == tls::subject_alt_name_type::ipaddress) { + ips.push_back(std::get(v.value)); + } + } + BOOST_REQUIRE_EQUAL(ips.size(), 2u); + BOOST_REQUIRE(std::ranges::find(ips, net::inet_address("127.0.0.1")) != ips.end()); + BOOST_REQUIRE(std::ranges::find(ips, net::inet_address("::1")) != ips.end()); } }