From 9ca488bc63ac7c6b83cb342e750b7d10fbbba94b Mon Sep 17 00:00:00 2001 From: Vivek Saini Date: Thu, 3 Mar 2022 13:59:07 -0500 Subject: [PATCH] TLS Name Validation (#48) Added TLS Name Validation --- keydb.conf | 17 +++++ src/config.cpp | 5 ++ src/server.h | 4 ++ src/tls.cpp | 91 ++++++++++++++++++++++- tests/test_helper.tcl | 1 + tests/unit/tls-name-validation.tcl | 111 +++++++++++++++++++++++++++++ utils/gen-test-certs.sh | 33 +++++++-- 7 files changed, 257 insertions(+), 5 deletions(-) create mode 100644 tests/unit/tls-name-validation.tcl diff --git a/keydb.conf b/keydb.conf index 677303f86..4ac16f73a 100644 --- a/keydb.conf +++ b/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 ... +# +# or place then all on their own seperate line (or a combination of the two): +# +# tls-whitelist +# tls-whitelist +# ... +# +# 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. diff --git a/src/config.cpp b/src/config.cpp index a1afdbba0..870d2b6c3 100644 --- a/src/config.cpp +++ b/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); diff --git a/src/server.h b/src/server.h index 4dee3d07c..4a0b9f55c 100644 --- a/src/server.h +++ b/src/server.h @@ -63,6 +63,7 @@ #include #include #include +#include #ifdef __cplusplus extern "C" { #include @@ -2613,6 +2614,9 @@ struct redisServer { int tls_replication; int tls_auth_clients; int tls_rotation; + + int tls_whitelist_enabled; + std::unordered_set tls_whitelist; redisTLSContextConfig tls_ctx_config; /* cpu affinity */ diff --git a/src/tls.cpp b/src/tls.cpp index ba6351cde..fda7e2afc 100644 --- a/src/tls.cpp +++ b/src/tls.cpp @@ -40,8 +40,12 @@ #include #include #include +#include +#include +#include #include +#include #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(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(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(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(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) @@ -691,7 +775,12 @@ void tlsHandleEvent(tls_connection *conn, int mask) { /* If not handled, it's an error */ conn->c.state = CONN_STATE_ERROR; } else { - conn->c.state = CONN_STATE_CONNECTED; + /* Validate name */ + if (g_pserver->tls_whitelist_enabled && !tlsValidateCertificateName(conn)){ + conn->c.state = CONN_STATE_ERROR; + } else { + conn->c.state = CONN_STATE_CONNECTED; + } } { diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 2436c1ae0..77fc11a24 100644 --- a/tests/test_helper.tcl +++ b/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 diff --git a/tests/unit/tls-name-validation.tcl b/tests/unit/tls-name-validation.tcl new file mode 100644 index 000000000..2f3e34d53 --- /dev/null +++ b/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 + } +} diff --git a/utils/gen-test-certs.sh b/utils/gen-test-certs.sh index 265255c42..c4643276b 100755 --- a/utils/gen-test-certs.sh +++ b/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