TLS Name Validation (#48)

Added TLS Name Validation

keydb.conf

@@ -262,6 +262,23 @@ tcp-keepalive 300
 # 
 # tls-rotation no
 
+# Setup a whitelist 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 whitelist 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:
+#
+# tls-whitelist <dns1> <dns2> <dns3> ...
+# 
+# or place then all on their own seperate line (or a combination of the two):
+#
+# tls-whitelist <dns1>
+# tls-whitelist <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"
+
 ################################# GENERAL #####################################
 
 # By default KeyDB does not run as a daemon. Use 'yes' if you need it.

src/config.cpp

@@ -737,6 +737,11 @@ void loadServerConfigFromString(char *config) {
                 g_pserver->fActiveReplica = CONFIG_DEFAULT_ACTIVE_REPLICA;
                 err = "argument must be 'yes' or 'no'"; goto loaderr;
             }
+        } else if (!strcasecmp(argv[0], "tls-whitelist")) {
+            if (!g_pserver->tls_whitelist_enabled) 
+                g_pserver->tls_whitelist_enabled = true;
+            for (int i = 1; i < argc; i++)
+                g_pserver->tls_whitelist.insert(zstrdup(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);

src/server.h

@@ -63,6 +63,7 @@
 #include <map>
 #include <string>
 #include <mutex>
+#include <unordered_set>
 #ifdef __cplusplus
 extern "C" {
 #include <lua.h>
@@ -2613,6 +2614,9 @@ struct redisServer {
     int tls_replication;
     int tls_auth_clients;
     int tls_rotation;
+
+    int tls_whitelist_enabled;
+    std::unordered_set<char *> tls_whitelist;
     redisTLSContextConfig tls_ctx_config;
 
     /* cpu affinity */

src/tls.cpp

@@ -40,8 +40,12 @@
 #include <openssl/err.h>
 #include <openssl/rand.h>
 #include <openssl/pem.h>
+#include <openssl/x509.h>
+#include <openssl/x509v3.h>
+#include <cstring>
 
 #include <sys/stat.h>
+#include <arpa/inet.h>
 
 #define REDIS_TLS_PROTO_TLSv1       (1<<0)
 #define REDIS_TLS_PROTO_TLSv1_1     (1<<1)
@@ -474,6 +478,86 @@ typedef struct tls_connection {
     aeEventLoop *el;
 } tls_connection;
 
+/* Check to see if a given client name matches against our whitelist.
+ * Return true if it does */
+bool tlsCheckAgainstWhitelist(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_whitelist){
+        if (stringmatchlen(client_pattern, strlen(client_pattern), client, strlen(client), 1))
+            return true;
+    }
+    return false;
+}
+
+bool tlsValidateCertificateName(tls_connection* conn){
+    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));
+    const char * commonName = reinterpret_cast<const char*>(ASN1_STRING_get0_data(X509_NAME_ENTRY_get_data(ne)));
+    
+    if (tlsCheckAgainstWhitelist(commonName))
+        return true;
+
+    /* If that fails, check through the subject alternative names (SANs) as well */
+    GENERAL_NAMES* subjectAltNames = (GENERAL_NAMES*)X509_get_ext_d2i(cert, NID_subject_alt_name, NULL, NULL);
+    if (subjectAltNames != nullptr){
+        for (int i = 0; i < sk_GENERAL_NAME_num(subjectAltNames); i++)
+        {
+            GENERAL_NAME* generalName = sk_GENERAL_NAME_value(subjectAltNames, i);
+            /* Short circuit if one of the SANs match. 
+             * We only support DNS, EMAIL, URI, and IP (specifically IPv4) */
+            switch (generalName->type)
+            {
+                case GEN_EMAIL:
+                    if (tlsCheckAgainstWhitelist(reinterpret_cast<const char*>(ASN1_STRING_get0_data(generalName->d.rfc822Name)))){
+                        sk_GENERAL_NAME_pop_free(subjectAltNames, GENERAL_NAME_free);
+                        return true;
+                    }
+                    break;
+                case GEN_DNS:
+                    if (tlsCheckAgainstWhitelist(reinterpret_cast<const char*>(ASN1_STRING_get0_data(generalName->d.dNSName)))){
+                        sk_GENERAL_NAME_pop_free(subjectAltNames, GENERAL_NAME_free);
+                        return true;
+                    }
+                    break;
+                case GEN_URI:
+                    if (tlsCheckAgainstWhitelist(reinterpret_cast<const char*>(ASN1_STRING_get0_data(generalName->d.uniformResourceIdentifier)))){
+                        sk_GENERAL_NAME_pop_free(subjectAltNames, GENERAL_NAME_free);
+                        return true;
+                    }
+                    break;
+                case GEN_IPADD:
+                    {
+                        int ipLen = ASN1_STRING_length(generalName->d.iPAddress);
+                        if (ipLen == 4){ //IPv4 case
+                            char addr[INET_ADDRSTRLEN];
+                            inet_ntop(AF_INET, ASN1_STRING_get0_data(generalName->d.iPAddress), addr, INET_ADDRSTRLEN);
+                            if (tlsCheckAgainstWhitelist(addr)){
+                                sk_GENERAL_NAME_pop_free(subjectAltNames, GENERAL_NAME_free);
+                                return true;
+                            }
+                        } else if (ipLen == 16) { // IPv6 case
+                            /* We don't support IPv6 at the moment */
+                        }
+                    }
+                    break;     
+                default:
+                    break;
+            }
+        }
+        sk_GENERAL_NAME_pop_free(subjectAltNames, GENERAL_NAME_free);
+    }
+
+    /* If neither the CN nor the SANs match, update the SSL error and return false */
+    conn->c.last_errno = 0;
+    if (conn->ssl_error) zfree(conn->ssl_error);
+    conn->ssl_error = (char*)zmalloc(512);
+    snprintf(conn->ssl_error, 512, "Client CN (%s) and SANs not found in whitelist.", commonName);
+    return false;
+}
+
 static connection *createTLSConnection(int client_side) {
     SSL_CTX *ctx = redis_tls_ctx;
     if (client_side && redis_tls_client_ctx)
@@ -690,9 +774,14 @@ void tlsHandleEvent(tls_connection *conn, int mask) {
 
                 /* If not handled, it's an error */
                 conn->c.state = CONN_STATE_ERROR;
+            } else {
+                /* Validate name */
+                if (g_pserver->tls_whitelist_enabled && !tlsValidateCertificateName(conn)){
+                    conn->c.state = CONN_STATE_ERROR;
                 } else {
                     conn->c.state = CONN_STATE_CONNECTED;
                 }
+            }
 
             {
             AeLocker locker;

tests/test_helper.tcl

@@ -79,6 +79,7 @@ set ::all_tests {
     unit/wait
     unit/pendingquerybuf
     unit/tls
+    unit/tls-name-validation
     unit/tracking
     unit/oom-score-adj
     unit/shutdown

tests/unit/tls-name-validation.tcl

@@ -0,0 +1,111 @@
+test {TLS: Able to connect with no whitelist} {
+    start_server {tags {"tls"}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect with whitelist '*'} {
+    start_server {tags {"tls"} overrides {tls-whitelist *}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect with matching CN} {
+    start_server {tags {"tls"} overrides {tls-whitelist client.keydb.dev}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect with matching SAN} {
+    start_server {tags {"tls"} overrides {tls-whitelist san1.keydb.dev}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect with matching CN with wildcard} {
+    start_server {tags {"tls"} overrides {tls-whitelist client*.dev}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect with matching SAN with wildcard} {
+    start_server {tags {"tls"} overrides {tls-whitelist san*.dev}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect while with CN having a comprehensive list} {
+    start_server {tags {"tls"} overrides {tls-whitelist {dummy.keydb.dev client.keydb.dev other.keydb.dev}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect while with SAN having a comprehensive list} {
+    start_server {tags {"tls"} overrides {tls-whitelist {dummy.keydb.dev san2.keydb.dev other.keydb.dev}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect while with CN having a comprehensive list with wildcards} {
+    start_server {tags {"tls"} overrides {tls-whitelist {dummy.* client*.dev other.*}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to connect while with SAN having a comprehensive list with wildcards} {
+    start_server {tags {"tls"} overrides {tls-whitelist {dummy.* san*.dev other.*}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Not matching CN or SAN rejected} {
+    start_server {tags {"tls"} overrides {tls-whitelist {client.keydb.dev}}} {
+        catch {set r2 [redis_client_tls -keyfile "$::tlsdir/client2.key" -certfile "$::tlsdir/client2.crt" -require 1 -cafile "$::tlsdir/ca.crt"]} e
+        assert_match {*I/O error reading reply*} $e
+    }
+}
+
+test {TLS: Able to match against DNS SAN} {
+    start_server {tags {"tls"} overrides {tls-whitelist {san1.keydb.dev}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to match against email SAN} {
+    start_server {tags {"tls"} overrides {tls-whitelist {someone@keydb.dev}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to match against IPv4 SAN} {
+    start_server {tags {"tls"} overrides {tls-whitelist {192.168.0.1}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to match against IPv4 with a wildcard} {
+    start_server {tags {"tls"} overrides {tls-whitelist {192.*}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}
+
+test {TLS: Able to match against URI SAN} {
+    start_server {tags {"tls"} overrides {tls-whitelist {https://keydb.dev}}} {
+        catch {r PING} e
+        assert_match {PONG} $e
+    }
+}

utils/gen-test-certs.sh

@@ -16,10 +16,11 @@ generate_cert() {
     local keyfile=tests/tls/${name}.key
     local certfile=tests/tls/${name}.crt
 
-    [ -f $keyfile ] || openssl genrsa -out $keyfile 2048
+    [ -f $keyfile ] || openssl genrsa -out $keyfile 4096
     openssl req \
         -new -sha256 \
         -subj "/O=KeyDB Test/CN=$cn" \
+        -config "tests/tls/openssl.cnf" \
         -key $keyfile | \
         openssl x509 \
             -req -sha256 \
@@ -42,6 +43,29 @@ openssl req \
     -out tests/tls/ca.crt
 
 cat > tests/tls/openssl.cnf <<_END_
+[ req ]
+default_bits       = 4096
+distinguished_name = req_distinguished_name
+req_extensions     = req_ext
+
+[req_distinguished_name]
+
+[req_ext]
+subjectAltName = @alt_names
+
+[alt_names]
+DNS.1=san1.keydb.dev
+DNS.2=san2.keydb.dev
+DNS.3=san3.keydb.dev
+IP.1=192.168.0.1
+IP.2=8.8.8.8
+IP.3=2001:0db8:15::8a2e:0370:7334
+email.1=someone@keydb.dev
+email.2=someone_else@keydb.dev
+URI.1=https://keydb.dev
+URI.2=https://google.com
+
+
 [ server_cert ]
 keyUsage = digitalSignature, keyEncipherment
 nsCertType = server
@@ -51,8 +75,9 @@ keyUsage = digitalSignature, keyEncipherment
 nsCertType = client
 _END_
 
-generate_cert server "Server-only" "-extfile tests/tls/openssl.cnf -extensions server_cert"
-generate_cert client "Client-only" "-extfile tests/tls/openssl.cnf -extensions client_cert"
-generate_cert keydb "Generic-cert"
+generate_cert server "server.keydb.dev" "-extfile tests/tls/openssl.cnf -extensions server_cert -extensions req_ext"
+generate_cert client "client.keydb.dev" "-extfile tests/tls/openssl.cnf -extensions client_cert -extensions req_ext"
+generate_cert client2 "client2.keydb.dev" "-extfile tests/tls/openssl.cnf -extensions client_cert -extensions req_ext"
+generate_cert keydb "generic.keydb.dev" "-extfile tests/tls/openssl.cnf -extensions req_ext"
 
 [ -f tests/tls/keydb.dh ] || openssl dhparam -out tests/tls/keydb.dh 2048