From ed9c2a0b78a424105cd75d1d5c21d9fa94b0223f Mon Sep 17 00:00:00 2001 From: Calle Wilund Date: Tue, 1 Apr 2025 09:25:22 +0000 Subject: [PATCH] tls: Make session resume key shared across credentials builders creds Fixes #2708 Session resume key (if enabled) was only generated/kept on credentials level. This means that access patterns where we create a new such object per shard, using same builder, would have different session keys, and thus not be able to resume each others sessions. This moves key generation to builder level and re-uses this for all generated server credentials. Note that a reload of any file (in reloadable) will re-create the key (though it should become invalid anyway, but...). Add tests for this, and a hefty warning that session keys are shared, should anyone try to re-use a builder for disparate credentials. v2: * Make gnutls_datum more container-like (i.e. encapsulated sized alloc) --- include/seastar/net/tls.hh | 8 +++ src/net/tls.cc | 27 ++++++- tests/unit/tls_test.cc | 144 ++++++++++++++++++++++++++++++++++++- 3 files changed, 173 insertions(+), 6 deletions(-) diff --git a/include/seastar/net/tls.hh b/include/seastar/net/tls.hh index cdef2402284..a2262b860c5 100644 --- a/include/seastar/net/tls.hh +++ b/include/seastar/net/tls.hh @@ -315,6 +315,13 @@ namespace tls { future<> set_system_trust(); void set_client_auth(client_auth); void set_priority_string(const sstring&); + /** + * Sets session resume mode to be applied to all created server credential sets + * Note: setting this will generate a session key that will be reused across all + * built server credentials, i.e. they will share resume key. + * If you wish to reuse a builder to create disparate server crendentials, + * simply call this method again to regenerate the key. + */ void set_session_resume_mode(session_resume_mode); void apply_to(certificate_credentials&) const; @@ -339,6 +346,7 @@ namespace tls { client_auth _client_auth = client_auth::NONE; session_resume_mode _session_resume_mode = session_resume_mode::NONE; sstring _priority; + std::vector _session_resume_key; }; using session_data = std::vector; diff --git a/src/net/tls.cc b/src/net/tls.cc index 3aebe81ab1d..e5d40b833be 100644 --- a/src/net/tls.cc +++ b/src/net/tls.cc @@ -339,6 +339,13 @@ future tls::x509_cert::from_file( // wrapper for gnutls_datum, with raii free struct gnutls_datum : public gnutls_datum_t { + gnutls_datum(size_t s) { + data = reinterpret_cast(gnutls_malloc(s)); + if (data == nullptr) { + throw std::bad_alloc(); + } + size = s; + } gnutls_datum() { data = nullptr; size = 0; @@ -436,12 +443,17 @@ class tls::certificate_credentials::impl: public gnutlsobj { client_auth get_client_auth() const { return _client_auth; } - void set_session_resume_mode(session_resume_mode m) { + void set_session_resume_mode(session_resume_mode m, std::span key = {}) { _session_resume_mode = m; // (re-)generate session key if (m != session_resume_mode::NONE) { _session_resume_key = {}; - gnutls_session_ticket_key_generate(&_session_resume_key); + if (key.empty()) { + gtls_chk(gnutls_session_ticket_key_generate(&_session_resume_key)); + } else { + _session_resume_key = gnutls_datum(key.size()); + std::copy(key.begin(), key.end(), _session_resume_key.data); + } } } session_resume_mode get_session_resume_mode() const { @@ -710,6 +722,11 @@ void tls::credentials_builder::set_priority_string(const sstring& prio) { void tls::credentials_builder::set_session_resume_mode(session_resume_mode m) { _session_resume_mode = m; + if (m != session_resume_mode::NONE) { + gnutls_datum key; + gtls_chk(gnutls_session_ticket_key_generate(&key)); + _session_resume_key.assign(key.data, key.data + key.size); + } } template @@ -760,7 +777,7 @@ void tls::credentials_builder::apply_to(certificate_credentials& creds) const { creds._impl->set_client_auth(_client_auth); // Note: this causes server session key rotation on cert reload - creds._impl->set_session_resume_mode(_session_resume_mode); + creds._impl->set_session_resume_mode(_session_resume_mode, std::span{_session_resume_key.begin(), _session_resume_key.end()}); } shared_ptr tls::credentials_builder::build_certificate_credentials() const { @@ -925,6 +942,10 @@ class tls::reloadable_credentials_base { return; } try { + // force rebuilding session resume mode key if + // enabled. should not reuse sessions across certificate + // change (should not work anyway) + set_session_resume_mode(_session_resume_mode); if (_creds) { _creds->rebuild(*this); } diff --git a/tests/unit/tls_test.cc b/tests/unit/tls_test.cc index 3e45089ea60..8d99cb33852 100644 --- a/tests/unit/tls_test.cc +++ b/tests/unit/tls_test.cc @@ -1554,19 +1554,153 @@ SEASTAR_THREAD_TEST_CASE(test_skip_wait_for_eof) { } } +static void do_test_tls13_session_tickets(bool reset_server) { + tls::credentials_builder b; + + b.set_x509_key_file(certfile("test.crt"), certfile("test.key"), tls::x509_crt_format::PEM).get(); + b.set_x509_trust_file(certfile("catest.pem"), tls::x509_crt_format::PEM).get(); + b.set_session_resume_mode(tls::session_resume_mode::TLS13_SESSION_TICKET); + b.set_priority_string("SECURE128:+SECURE192:-VERS-TLS-ALL:+VERS-TLS1.3"); + + auto creds = b.build_certificate_credentials(); + auto serv = b.build_server_credentials(); + + ::listen_options opts; + opts.reuse_address = true; + opts.set_fixed_cpu(this_shard_id()); + + auto addr = ::make_ipv4_address( {0x7f000001, 4712}); + auto server = tls::listen(serv, addr, opts); + + tls::session_data sess_data; + + { + auto sa = server.accept(); + auto c = tls::connect(creds, addr).get(); + auto s = sa.get(); + + auto in = s.connection.input(); + auto cin = c.input(); + output_stream out(c.output().detach(), 1024); + output_stream sout(s.connection.output().detach(), 1024); + + // write data in both directions. Required for session data to + // become available. + out.write("nils").get(); + auto fin = in.read(); + auto fout = out.flush(); + + fout.get(); + fin.get(); + + sout.write("banan").get(); + fin = cin.read(); + fout = sout.flush(); + + fout.get(); + fin.get(); + + BOOST_REQUIRE(!tls::check_session_is_resumed(c).get()); // no resume data + + // get ticket data + sess_data = tls::get_session_resume_data(c).get(); + BOOST_REQUIRE(!sess_data.empty()); + + in.close().get(); + out.close().get(); + + s.connection.shutdown_input(); + s.connection.shutdown_output(); + + c.shutdown_input(); + c.shutdown_output(); + } + + if (reset_server) { + server = {}; + // rebuild creds + serv = b.build_server_credentials(); + server = tls::listen(serv, addr, opts); + } + + { + auto sa = server.accept(); + + // tell client to try resuming. + tls::tls_options tls_opts; + tls_opts.session_resume_data = sess_data; + + auto c = tls::connect(creds, addr, tls_opts).get(); + auto s = sa.get(); + + // This is ok. Will force a handshake. + auto f = tls::check_session_is_resumed(c); + + // But we need to force some IO to make the + // handshake actually happen. + auto in = s.connection.input(); + output_stream out(c.output().detach(), 1024); + + auto fin = in.read(); + + out.write("nils").get(); + auto fout = out.flush(); + + fout.get(); + fin.get(); + + BOOST_REQUIRE(f.get()); // Should work + + in.close().get(); + out.close().get(); + + s.connection.shutdown_input(); + s.connection.shutdown_output(); + + c.shutdown_input(); + c.shutdown_output(); + } + +} + /** * Test TLS13 session ticket support. */ SEASTAR_THREAD_TEST_CASE(test_tls13_session_tickets) { + do_test_tls13_session_tickets(false); +} + +SEASTAR_THREAD_TEST_CASE(test_tls13_session_tickets_retain_session_key) { + do_test_tls13_session_tickets(true); +} + +SEASTAR_THREAD_TEST_CASE(test_tls13_session_tickets_invalidated_by_reload) { tls::credentials_builder b; + tmpdir tmp; - b.set_x509_key_file(certfile("test.crt"), certfile("test.key"), tls::x509_crt_format::PEM).get(); + namespace fs = std::filesystem; + + // copy the wrong certs. We don't trust these + // blocking calls, but this is a test and seastar does not have a copy + // util and I am lazy... + fs::copy_file(certfile("test.crt"), tmp.path() / "test.crt"); + fs::copy_file(certfile("test.key"), tmp.path() / "test.key"); + + auto cert = (tmp.path() / "test.crt").native(); + auto key = (tmp.path() / "test.key").native(); + promise<> p; + + b.set_x509_key_file(cert, key, tls::x509_crt_format::PEM).get(); b.set_x509_trust_file(certfile("catest.pem"), tls::x509_crt_format::PEM).get(); b.set_session_resume_mode(tls::session_resume_mode::TLS13_SESSION_TICKET); b.set_priority_string("SECURE128:+SECURE192:-VERS-TLS-ALL:+VERS-TLS1.3"); auto creds = b.build_certificate_credentials(); - auto serv = b.build_server_credentials(); + auto serv = b.build_reloadable_server_credentials([&p](const std::unordered_set&, std::exception_ptr) { + p.set_value(); + }).get(); + + auto reloaded = p.get_future(); ::listen_options opts; opts.reuse_address = true; @@ -1619,6 +1753,10 @@ SEASTAR_THREAD_TEST_CASE(test_tls13_session_tickets) { c.shutdown_output(); } + BOOST_REQUIRE(!reloaded.available()); + + fs::copy_file(certfile("test.crt"), tmp.path() / "test.crt", fs::copy_options::overwrite_existing); + reloaded.get(); { auto sa = server.accept(); @@ -1646,7 +1784,7 @@ SEASTAR_THREAD_TEST_CASE(test_tls13_session_tickets) { fout.get(); fin.get(); - BOOST_REQUIRE(f.get()); // Should work + BOOST_REQUIRE(!f.get()); // Should NOT work. Keys should have been replaced in.close().get(); out.close().get();