Support dynamic runtime modification of tls-allowlist, and add config rewrite support

keydb.conf

@@ -265,16 +265,12 @@ tcp-keepalive 300
 # Setup a allowlist of allowed Common Names (CNs)/Subject Alternative Names (SANs)
 # that are allowed to connect to this server. This includes both normal clients as
 # well as other servers connected for replication/clustering purposes. If nothing is
-# specified, then no allowlist is used. Supports IPv4, DNS, RFC822, and URI SAN types.
-# You can opt to either put all of the names on one line as follows:
+# specified, then no allowlist is used and all certificates are accepted. 
+# Supports IPv4, DNS, RFC822, and URI SAN types.
+# You can put multiple names on one line as follows:
 #
 # tls-allowlist <dns1> <dns2> <dns3> ...
 # 
-# or place then all on their own seperate line (or a combination of the two):
-#
-# tls-allowlist <dns1>
-# tls-allowlist <dns2>
-# ... 
 #
 # This configuration also allows for wildcard characters with glob style formatting
 # i.e. "*.com" would allow all clients to connect with a CN/SAN that ends with ".com"
@@ -2073,4 +2069,4 @@ server-threads 2
 # this to 1.  Very write heavy workloads may benefit from higher numbers.
 #
 # By default KeyDB sets this to 2.
-replica-weighting-factor 2
+replica-weighting-factor 2

src/config.cpp

@@ -742,10 +742,14 @@ void loadServerConfigFromString(char *config) {
                 err = "argument must be 'yes' or 'no'"; goto loaderr;
             }
         } else if (!strcasecmp(argv[0], "tls-allowlist")) {
-            if (!g_pserver->tls_allowlist_enabled) 
-                g_pserver->tls_allowlist_enabled = true;
+            if (argc < 2) {
+                err = "must supply at least one element in the allow list"; goto loaderr;
+            }
+            if (!g_pserver->tls_allowlist.empty()) {
+                err = "tls-allowlist may only be set once"; goto loaderr;
+            }
             for (int i = 1; i < argc; i++)
-                g_pserver->tls_allowlist.insert(zstrdup(argv[i]));
+                g_pserver->tls_allowlist.emplace(argv[i], strlen(argv[i]));
         } else if (!strcasecmp(argv[0], "version-override") && argc == 2) {
             KEYDB_SET_VERSION = zstrdup(argv[1]);
             serverLog(LL_WARNING, "Warning version is overriden to: %s\n", KEYDB_SET_VERSION);
@@ -866,8 +870,18 @@ void configSetCommand(client *c) {
     int err;
     const char *errstr = NULL;
     serverAssertWithInfo(c,c->argv[2],sdsEncodedObject(c->argv[2]));
-    serverAssertWithInfo(c,c->argv[3],sdsEncodedObject(c->argv[3]));
-    o = c->argv[3];
+
+    if (c->argc < 4 || c->argc > 4) {
+        o = nullptr;
+        // Variadic set is only supported for tls-allowlist
+        if (strcasecmp(szFromObj(c->argv[2]), "tls-allowlist")) {
+            addReplySubcommandSyntaxError(c);
+            return;
+        }
+    } else {
+        o = c->argv[3];
+        serverAssertWithInfo(c,c->argv[3],sdsEncodedObject(c->argv[3]));
+    }
 
     /* Iterate the configs that are standard */
     for (standardConfig *config = configs; config->name != NULL; config++) {
@@ -1029,6 +1043,12 @@ void configSetCommand(client *c) {
             enableWatchdog(ll);
         else
             disableWatchdog();
+    } config_set_special_field("tls-allowlist") {
+        g_pserver->tls_allowlist.clear();
+        for (int i = 3; i < c->argc; ++i) {
+            robj *val = c->argv[i];
+            g_pserver->tls_allowlist.emplace(szFromObj(val), sdslen(szFromObj(val)));
+        }
     /* Everything else is an error... */
     } config_set_else {
         addReplyErrorFormat(c,"Unsupported CONFIG parameter: %s",
@@ -1232,6 +1252,14 @@ void configGetCommand(client *c) {
         addReplyBulkCString(c, g_pserver->fActiveReplica ? "yes" : "no");
         matches++;
     }
+    if (stringmatch(pattern, "tls-allowlist", 1)) {
+        addReplyBulkCString(c,"tls-allowlist");
+        addReplyArrayLen(c, (long)g_pserver->tls_allowlist.size());
+        for (auto &elem : g_pserver->tls_allowlist) {
+            addReplyBulkCBuffer(c, elem.get(), elem.size()); // addReplyBulkSds will free which we absolutely don't want
+        }
+        matches++;
+    }
 
     setDeferredMapLen(c,replylen,matches);
 }
@@ -1901,6 +1929,20 @@ int rewriteConfig(char *path, int force_all) {
     rewriteConfigStringOption(state, "version-override",KEYDB_SET_VERSION,KEYDB_REAL_VERSION);
     rewriteConfigOOMScoreAdjValuesOption(state);
 
+    if (!g_pserver->tls_allowlist.empty()) {
+        sds conf = sdsnew("tls-allowlist ");
+        for (auto &elem : g_pserver->tls_allowlist) {
+            conf = sdscatsds(conf, (sds)elem.get());
+            conf = sdscat(conf, " ");
+        }
+        // trim the trailing space
+        sdsrange(conf, 0, -1);
+        rewriteConfigRewriteLine(state,"tls-allowlist",conf,1 /*force*/);
+        // note: conf is owned by rewriteConfigRewriteLine - no need to free
+    } else {
+        rewriteConfigMarkAsProcessed(state, "tls-allowlist"); // ensure the line is removed if it existed
+    }
+
     /* Rewrite Sentinel config if in Sentinel mode. */
     if (g_pserver->sentinel_mode) rewriteConfigSentinelOption(state);
 
@@ -2907,7 +2949,7 @@ NULL
         };
 
         addReplyHelp(c, help);
-    } else if (!strcasecmp(szFromObj(c->argv[1]),"set") && c->argc == 4) {
+    } else if (!strcasecmp(szFromObj(c->argv[1]),"set") && c->argc >= 3) {
         configSetCommand(c);
     } else if (!strcasecmp(szFromObj(c->argv[1]),"get") && c->argc == 3) {
         configGetCommand(c);

src/server.h

@@ -2612,8 +2612,7 @@ struct redisServer {
     int tls_auth_clients;
     int tls_rotation;
 
-    int tls_allowlist_enabled;
-    std::unordered_set<char *> tls_allowlist;
+    std::set<sdsstring> tls_allowlist;
     redisTLSContextConfig tls_ctx_config;
 
     /* cpu affinity */

src/tls.cpp

@@ -484,14 +484,17 @@ bool tlsCheckAgainstAllowlist(const char * client){
     /* Because of wildcard matching, we need to iterate over the entire set.
      * If we were doing simply straight matching, we could just directly 
      * check to see if the client name is in the set in O(1) time */
-    for (char * client_pattern: g_pserver->tls_allowlist){
-        if (stringmatchlen(client_pattern, strlen(client_pattern), client, strlen(client), 1))
+    for (auto &client_pattern: g_pserver->tls_allowlist){
+        if (stringmatchlen(client_pattern.get(), client_pattern.size(), client, strlen(client), 1))
             return true;
     }
     return false;
 }
 
 bool tlsValidateCertificateName(tls_connection* conn){
+    if (g_pserver->tls_allowlist.empty())
+        return true;    // Empty list implies acceptance of all
+
     X509 * cert = SSL_get_peer_certificate(conn->ssl);
     /* Check the common name (CN) of the certificate first */
     X509_NAME_ENTRY * ne = X509_NAME_get_entry(X509_get_subject_name(cert), X509_NAME_get_index_by_NID(X509_get_subject_name(cert), NID_commonName, -1));
@@ -776,7 +779,7 @@ void tlsHandleEvent(tls_connection *conn, int mask) {
                 conn->c.state = CONN_STATE_ERROR;
             } else {
                 /* Validate name */
-                if (g_pserver->tls_allowlist_enabled && !tlsValidateCertificateName(conn)){
+                if (!tlsValidateCertificateName(conn)){
                     conn->c.state = CONN_STATE_ERROR;
                 } else {
                     conn->c.state = CONN_STATE_CONNECTED;