Introduce extra mutex to avoid need for unlock guard

Also add instructions on how to regenerate keys/certs for tests.

3 files changed, 59 insertions, 29 deletions

diff --git a/vespalib/src/tests/net/tls/auto_reloading_tls_crypto_engine/auto_reloading_tls_crypto_engine_test.cpp b/vespalib/src/tests/net/tls/auto_reloading_tls_crypto_engine/auto_reloading_tls_crypto_engine_test.cpp
index 7926f71e812..034dc99b72c 100644
--- a/vespalib/src/tests/net/tls/auto_reloading_tls_crypto_engine/auto_reloading_tls_crypto_engine_test.cpp
+++ b/vespalib/src/tests/net/tls/auto_reloading_tls_crypto_engine/auto_reloading_tls_crypto_engine_test.cpp
@@ -15,6 +15,48 @@ using namespace vespalib;
 using namespace vespalib::net::tls;
 using namespace std::chrono_literals;
 
+/*
+
+Keys and certificates used for these tests generated with commands:
+
+openssl ecparam -name prime256v1 -genkey -noout -out ca.key
+
+# test_ca.pem:
+openssl req -new -x509 -nodes -key ca.key \
+    -sha256 -out test_ca.pem \
+    -subj '/C=US/L=LooneyVille/O=ACME/OU=ACME test CA/CN=acme.example.com' \
+    -days 10000
+
+openssl ecparam -name prime256v1 -genkey -noout -out test_key.pem
+
+openssl req -new -key test_key.pem -out host1.csr \
+    -subj '/C=US/L=LooneyVille/O=Wile. E. Coyote, Ltd./CN=wile.example.com' \
+    -sha256
+
+# cert1_pem:
+openssl x509 -req -in host1.csr \
+    -CA ca.pem \
+    -CAkey ca.key \
+    -CAcreateserial \
+    -out cert1.pem \
+    -days 10000 \
+    -sha256
+
+openssl req -new -key test_key.pem -out host2.csr \
+    -subj '/C=US/L=LooneyVille/O=Wile. E. Coyote, Ltd./CN=wile.example.com' \
+    -sha256
+
+# cert2_pem:
+openssl x509 -req -in host2.csr \
+    -CA ca.pem \
+    -CAkey ca.key \
+    -CAcreateserial \
+    -out cert2.pem \
+    -days 10000 \
+    -sha256
+
+*/
+
 constexpr const char* cert1_pem = R"(-----BEGIN CERTIFICATE-----
 MIIBszCCAVgCCQCXsYrXQWS0bzAKBggqhkjOPQQDAjBkMQswCQYDVQQGEwJVUzEU
 MBIGA1UEBwwLTG9vbmV5VmlsbGUxDTALBgNVBAoMBEFDTUUxFTATBgNVBAsMDEFD
diff --git a/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.cpp b/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.cpp
index 665b19cdaaf..1b84c98a46d 100644
--- a/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.cpp
+++ b/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.cpp
@@ -31,23 +31,13 @@ std::shared_ptr<TlsCryptoEngine> try_create_engine_from_tls_config(const vespali
     }
 }
 
-// Unlocks on construction, re-locks on destruction.
-struct UnlockGuard {
-    std::unique_lock<std::mutex>& _lock;
-    explicit UnlockGuard(std::unique_lock<std::mutex>& lock) : _lock(lock) {
-        _lock.unlock();
-    }
-    ~UnlockGuard() {
-        _lock.lock();
-    }
-};
-
 } // anonymous namespace
 
 AutoReloadingTlsCryptoEngine::AutoReloadingTlsCryptoEngine(vespalib::string config_file_path,
                                                            TimeInterval reload_interval)
-    : _mutex(),
-      _cond(),
+    : _thread_mutex(),
+      _thread_cond(),
+      _engine_mutex(),
       _shutdown(false),
       _config_file_path(std::move(config_file_path)),
       _current_engine(tls_engine_from_config_file(_config_file_path)),
@@ -58,9 +48,9 @@ AutoReloadingTlsCryptoEngine::AutoReloadingTlsCryptoEngine(vespalib::string conf
 
 AutoReloadingTlsCryptoEngine::~AutoReloadingTlsCryptoEngine() {
     {
-        std::unique_lock lock(_mutex);
+        std::lock_guard lock(_thread_mutex);
         _shutdown = true;
-        _cond.notify_all();
+        _thread_cond.notify_all();
     }
     _reload_thread.join();
 }
@@ -70,30 +60,27 @@ std::chrono::steady_clock::time_point AutoReloadingTlsCryptoEngine::make_future_
 }
 
 void AutoReloadingTlsCryptoEngine::run_reload_loop() {
-    std::unique_lock lock(_mutex);
+    std::unique_lock lock(_thread_mutex);
     auto reload_at_time = make_future_reload_time_point();
     while (!_shutdown) {
-        if (_cond.wait_until(lock, reload_at_time) == std::cv_status::timeout) {
+        if (_thread_cond.wait_until(lock, reload_at_time) == std::cv_status::timeout) {
             LOG(debug, "TLS config reload time reached, reloading file '%s'", _config_file_path.c_str());
-            try_replace_current_engine(lock);
+            try_replace_current_engine();
             reload_at_time = make_future_reload_time_point();
         } // else: spurious wakeup or shutdown
     }
 }
 
-void AutoReloadingTlsCryptoEngine::try_replace_current_engine(std::unique_lock<std::mutex>& held_lock) {
-    std::shared_ptr<TlsCryptoEngine> new_engine;
-    {
-        UnlockGuard guard(held_lock);
-        new_engine = try_create_engine_from_tls_config(_config_file_path);
-    }
+void AutoReloadingTlsCryptoEngine::try_replace_current_engine() {
+    std::shared_ptr<TlsCryptoEngine> new_engine = try_create_engine_from_tls_config(_config_file_path);
     if (new_engine) {
+        std::lock_guard guard(_engine_mutex);
         _current_engine = std::move(new_engine);
     }
 }
 
 AutoReloadingTlsCryptoEngine::EngineSP AutoReloadingTlsCryptoEngine::acquire_current_engine() const {
-    std::lock_guard guard(_mutex);
+    std::lock_guard guard(_engine_mutex);
     return _current_engine;
 }
 
diff --git a/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.h b/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.h
index 327179dbd17..4e3a4074b03 100644
--- a/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.h
+++ b/vespalib/src/vespa/vespalib/net/tls/auto_reloading_tls_crypto_engine.h
@@ -17,16 +17,17 @@ public:
     using EngineSP     = std::shared_ptr<TlsCryptoEngine>;
     using TimeInterval = std::chrono::steady_clock::duration;
 private:
-    mutable std::mutex      _mutex;
-    std::condition_variable _cond;
+    mutable std::mutex      _thread_mutex;
+    std::condition_variable _thread_cond;
+    mutable std::mutex      _engine_mutex;
     bool                    _shutdown;
     const vespalib::string  _config_file_path;
-    EngineSP                _current_engine; // Access must be under _mutex
+    EngineSP                _current_engine; // Access must be under _engine_mutex
     TimeInterval            _reload_interval;
     std::thread             _reload_thread;
 
     void run_reload_loop();
-    void try_replace_current_engine(std::unique_lock<std::mutex>& held_lock);
+    void try_replace_current_engine();
     std::chrono::steady_clock::time_point make_future_reload_time_point() const noexcept;
 
 public: