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.

Author: Calle Wilund

Diff for: 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<uint8_t> _session_resume_key;
     };
 
     using session_data = std::vector<uint8_t>;

Diff for: src/net/tls.cc

@@ -339,9 +339,9 @@ 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() {
-        data = nullptr;
-        size = 0;
+    gnutls_datum(void* d = nullptr, size_t s = 0) {
+        data = reinterpret_cast<unsigned char*>(d);
+        size = s;
     }
     gnutls_datum(const gnutls_datum&) = delete;
     gnutls_datum& operator=(gnutls_datum&& other) {
@@ -436,12 +436,20 @@ 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, const std::vector<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()) {
+                gnutls_session_ticket_key_generate(&_session_resume_key);
+            } else {
+                _session_resume_key = gnutls_datum(gnutls_malloc(key.size()), key.size());
+                if (_session_resume_key.data == nullptr) {
+                    throw std::bad_alloc();
+                }
+                std::copy(key.begin(), key.end(), _session_resume_key.data);
+            }
         }
     }
     session_resume_mode get_session_resume_mode() const {
@@ -710,6 +718,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;
+        gnutls_session_ticket_key_generate(&key);
+        _session_resume_key.assign(key.data, key.data + key.size);
+    }
 }
 
 template<typename Blobs, typename Visitor>
@@ -760,7 +773,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, _session_resume_key);
 }
 
 shared_ptr<tls::certificate_credentials> tls::credentials_builder::build_certificate_credentials() const {
@@ -925,6 +938,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 for: 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<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;
@@ -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();