sec: Adjust flag validation for TLS.

src/server/protocol_client.cc

@@ -56,17 +56,26 @@ static ProtocolClient::SSL_CTX* CreateSslClientCntx() {
   const auto& tls_key_file = GetFlag(FLAGS_tls_key_file);
   unsigned mask = SSL_VERIFY_PEER | SSL_VERIFY_FAIL_IF_NO_PEER_CERT;
 
-  CHECK_EQ(1, SSL_CTX_use_PrivateKey_file(ctx, tls_key_file.c_str(), SSL_FILETYPE_PEM));
-  const auto& tls_cert_file = GetFlag(FLAGS_tls_cert_file);
+  // Load client certificate if given.
+  if (!tls_key_file.empty()) {
+    CHECK_EQ(1, SSL_CTX_use_PrivateKey_file(ctx, tls_key_file.c_str(), SSL_FILETYPE_PEM));
+    // We checked that the flag is non empty in ValidateClientTlsFlags.
+    const auto& tls_cert_file = GetFlag(FLAGS_tls_cert_file);
 
-  CHECK_EQ(1, SSL_CTX_use_certificate_chain_file(ctx, tls_cert_file.c_str()));
+    CHECK_EQ(1, SSL_CTX_use_certificate_chain_file(ctx, tls_cert_file.c_str()));
+  }
 
+  // Load custom certificate validation if given.
   const auto& tls_ca_cert_file = GetFlag(FLAGS_tls_ca_cert_file);
   const auto& tls_ca_cert_dir = GetFlag(FLAGS_tls_ca_cert_dir);
 
   const auto* file = tls_ca_cert_file.empty() ? nullptr : tls_ca_cert_file.data();
   const auto* dir = tls_ca_cert_dir.empty() ? nullptr : tls_ca_cert_dir.data();
-  CHECK_EQ(1, SSL_CTX_load_verify_locations(ctx, file, dir));
+  if (file || dir) {
+    CHECK_EQ(1, SSL_CTX_load_verify_locations(ctx, file, dir));
+  } else {
+    CHECK_EQ(1, SSL_CTX_set_default_verify_paths(ctx));
+  }
 
   CHECK_EQ(1, SSL_CTX_set_cipher_list(ctx, "DEFAULT"));
   SSL_CTX_set_min_proto_version(ctx, TLS1_2_VERSION);
@@ -142,22 +151,32 @@ std::string ProtocolClient::ServerContext::Description() const {
   return absl::StrCat(host, ":", port);
 }
 
-void ProtocolClient::ValidateTlsFlags() const {
-  if (absl::GetFlag(FLAGS_tls_cert_file).empty()) {
-    LOG(ERROR) << "tls_cert_file flag should be set";
-    exit(1);
+void ValidateClientTlsFlags() {
+  if (!absl::GetFlag(FLAGS_tls_replication)) {
+    return;
+  }
+
+  bool has_auth = false;
+
+  if (!absl::GetFlag(FLAGS_tls_key_file).empty()) {
+    if (absl::GetFlag(FLAGS_tls_cert_file).empty()) {
+      LOG(ERROR) << "tls_cert_file flag should be set";
+      exit(1);
+    }
+    has_auth = true;
   }
 
-  if (absl::GetFlag(FLAGS_tls_ca_cert_file).empty() &&
-      absl::GetFlag(FLAGS_tls_ca_cert_dir).empty()) {
-    LOG(ERROR) << "Either or both tls_ca_cert_file or tls_ca_cert_dir flags must be set";
+  if (!absl::GetFlag(FLAGS_masterauth).empty())
+    has_auth = true;
+
+  if (!has_auth) {
+    LOG(ERROR) << "No authentication method configured!";
     exit(1);
   }
 }
 
 void ProtocolClient::MaybeInitSslCtx() {
   if (absl::GetFlag(FLAGS_tls_replication)) {
-    ValidateTlsFlags();
     ssl_ctx_ = CreateSslClientCntx();
   }
 }

src/server/protocol_client.h

@@ -32,6 +32,8 @@ class ConnectionContext;
 class JournalExecutor;
 struct JournalReader;
 
+void ValidateClientTlsFlags();
+
 // A helper class for implementing a Redis client that talks to a redis server.
 // This class should be inherited from.
 class ProtocolClient {
@@ -130,12 +132,14 @@ class ProtocolClient {
   std::string last_resp_;
 
   uint64_t last_io_time_ = 0;  // in ns, monotonic clock.
+
 #ifdef DFLY_USE_SSL
-  void ValidateTlsFlags() const;
 
   void MaybeInitSslCtx();
 
   SSL_CTX* ssl_ctx_{nullptr};
+#else
+  void* ssl_ctx_{nullptr};
 #endif
 };
 

src/server/server_family.cc

@@ -36,6 +36,7 @@ extern "C" {
 #include "server/journal/journal.h"
 #include "server/main_service.h"
 #include "server/memory_cmd.h"
+#include "server/protocol_client.h"
 #include "server/rdb_load.h"
 #include "server/rdb_save.h"
 #include "server/script_mgr.h"
@@ -68,6 +69,9 @@ ABSL_FLAG(int, epoll_file_threads, 0,
 ABSL_DECLARE_FLAG(uint32_t, port);
 ABSL_DECLARE_FLAG(bool, cache_mode);
 ABSL_DECLARE_FLAG(uint32_t, hz);
+ABSL_DECLARE_FLAG(bool, tls);
+ABSL_DECLARE_FLAG(string, tls_ca_cert_file);
+ABSL_DECLARE_FLAG(string, tls_ca_cert_dir);
 
 namespace dfly {
 
@@ -419,6 +423,32 @@ void SlowLog(CmdArgList args, ConnectionContext* cntx) {
   (*cntx)->SendError(UnknownSubCmd(sub_cmd, "SLOWLOG"), kSyntaxErrType);
 }
 
+// Check that if TLS is used at least one form of client authentication is
+// enabled. That means either using a password or giving a root
+// certificate for authenticating client certificates which will
+// be required.
+void ValidateServerTlsFlags() {
+  if (!absl::GetFlag(FLAGS_tls)) {
+    return;
+  }
+
+  bool has_auth = false;
+
+  if (!dfly::GetPassword().empty()) {
+    has_auth = true;
+  }
+
+  if (!(absl::GetFlag(FLAGS_tls_ca_cert_file).empty() &&
+        absl::GetFlag(FLAGS_tls_ca_cert_dir).empty())) {
+    has_auth = true;
+  }
+
+  if (!has_auth) {
+    LOG(ERROR) << "TLS configured but no authentication method is used!";
+    exit(1);
+  }
+}
+
 }  // namespace
 
 std::optional<SnapshotSpec> ParseSaveSchedule(string_view time) {
@@ -488,6 +518,9 @@ ServerFamily::ServerFamily(Service* service) : service_(*service) {
     LOG(ERROR) << ec.Format();
     exit(1);
   }
+
+  ValidateServerTlsFlags();
+  ValidateClientTlsFlags();
 }
 
 ServerFamily::~ServerFamily() {

tests/dragonfly/__init__.py

@@ -24,6 +24,10 @@ class DflyParams:
     env: any
 
 
+class DflyStartException(Exception):
+    pass
+
+
 class DflyInstance:
     """
     Represents a runnable and stoppable Dragonfly instance
@@ -81,7 +85,7 @@ def _check_status(self):
         if not self.params.existing_port:
             return_code = self.proc.poll()
             if return_code is not None:
-                raise Exception(f"Failed to start instance, return code {return_code}")
+                raise DflyStartException(f"Failed to start instance, return code {return_code}")
 
     def __getitem__(self, k):
         return self.args.get(k)

tests/dragonfly/tls_conf_test.py

@@ -0,0 +1,50 @@
+import pytest
+import redis
+from .utility import *
+from . import DflyStartException
+
+
+def test_tls_no_auth(df_factory, with_tls_server_args):
+    # Needs some authentication
+    server = df_factory.create(port=1111, **with_tls_server_args)
+    with pytest.raises(DflyStartException):
+        server.start()
+
+
+def test_tls_no_key(df_factory):
+    # Needs a private key and certificate.
+    server = df_factory.create(port=1112, tls=None, requirepass="XXX")
+    with pytest.raises(DflyStartException):
+        server.start()
+
+
+def test_tls_password(df_factory, with_tls_server_args):
+    server = df_factory.create(port=1113, requirepass="XXX", **with_tls_server_args)
+    server.start()
+    server.stop()
+
+
+def test_tls_client_certs(df_factory, with_ca_tls_server_args):
+    server = df_factory.create(port=1114, **with_ca_tls_server_args)
+    server.start()
+    server.stop()
+
+
+def test_client_tls_no_auth(df_factory):
+    server = df_factory.create(port=1115, tls_replication=None)
+    with pytest.raises(DflyStartException):
+        server.start()
+
+
+def test_client_tls_password(df_factory):
+    server = df_factory.create(port=1116, tls_replication=None, masterauth="XXX")
+    server.start()
+    server.stop()
+
+
+def test_client_tls_cert(df_factory, with_tls_server_args):
+    key_args = with_tls_server_args.copy()
+    key_args.pop("tls")
+    server = df_factory.create(port=1117, tls_replication=None, **key_args)
+    server.start()
+    server.stop()