Skip to content

tls: Make session resume key shared across credentials builders creds #2709

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

tls: Make session resume key shared across credentials builders creds #2709

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions include/seastar/net/tls.hh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<uint8_t> _session_resume_key;
};

using session_data = std::vector<uint8_t>;
Expand Down
27 changes: 24 additions & 3 deletions src/net/tls.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -339,6 +339,13 @@ future<tls::x509_cert> 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<unsigned char*>(gnutls_malloc(s));
if (data == nullptr) {
throw std::bad_alloc();
}
size = s;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While this is internal, it's still a dangerous constructor (esp. the non-explicit conversion from any pointer).

Please change to std::span<std::byte>.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better solution is to make the constructor only take a size and do the alloc internally, as the memory must be allocated by gnutls_malloc. Better to enforce.

}
gnutls_datum() {
data = nullptr;
size = 0;
Expand Down Expand Up @@ -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<const uint8_t> 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 {
Expand Down Expand Up @@ -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);
}
}
Comment on lines 723 to 730
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just an idea, If we want to tls renegotiation to work even when node is being restarted we need _session_resume_key to be persisted either in the config or as a file or any other way.

Which means we will need another API that could feed pregenerated _session_resume_key to the credentials builder.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is unnecessary. If you have node restarts so frequent it impacts TLS session management, you have a bigger problem.
Should the need arise, get/set API:s for this can be added, but for now I'd say it is overreach.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that is unnecessary. If you have node restarts so frequent it impacts TLS session management, you have a bigger problem. Should the need arise, get/set API:s for this can be added, but for now I'd say it is overreach.

It is not about frequency, it is about of amount of clients that are trying to reconnect at the same time.
When client is reconnecting it establishes TLS connection first and the go thru CQL initialization process, CPU pressure of having thousands of clients reconnecting at the same time could be so high that it impedes CQL initialization process, as result client breaks connection and reconnects again.
Though, on second reconnect, it should have TLS ticket learned, so it should go smoother.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The point is that a restart should be infrequent. Connections can use session resume across the nodes uptime or the ticket expiry, whichever comes first (lets hope the latter). But also remember that storing the session key across restarts would more or less require them to be clean restarts (because we do want to refresh the key at least on cert reload etc), which, in the case of crashes, makes it even less useful.
Also remember that using the same key for to long is a security concern, and already the cross-session usage here is somewhat dubious (but since we can treat cross-shard instances as equivalent to a "normal" server single session ok).


template<typename Blobs, typename Visitor>
Expand Down Expand Up @@ -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::certificate_credentials> tls::credentials_builder::build_certificate_credentials() const {
Expand Down Expand Up @@ -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);
}
Expand Down
144 changes: 141 additions & 3 deletions tests/unit/tls_test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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<char> out(c.output().detach(), 1024);
output_stream<char> 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<char> 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<sstring>&, std::exception_ptr) {
p.set_value();
}).get();

auto reloaded = p.get_future();

::listen_options opts;
opts.reuse_address = true;
Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand Down