Merge pull request #40 from Snapchat/tls_cert_rotation

Added support for TLS certificate rotation.

.github/workflows/ci.yml

@@ -31,3 +31,6 @@ jobs:
     - name: module tests
       run: |
           ./runtest-moduleapi
+    - name: rotation test
+      run: |
+          ./runtest-rotation

.gitignore

@@ -48,7 +48,7 @@ src/nodes.conf
 deps/lua/src/lua
 deps/lua/src/luac
 deps/lua/src/liblua.a
-tests/tls/*
+tests/tls*/*
 .make-*
 .prerequisites
 *.dSYM

README.md

@@ -122,13 +122,6 @@ installed):
     % ./runtest --tls
 
 
-If TLS is built, running the tests with TLS enabled (you will need `tcl-tls`
-installed):
-
-    % ./utils/gen-test-certs.sh
-    % ./runtest --tls
-
-
 Fixing build problems with dependencies or cached build options
 ---------
 

keydb.conf

@@ -257,6 +257,11 @@ tcp-keepalive 300
 #
 # tls-session-cache-timeout 60
 
+# Allow the server to monitor the filesystem and rotate out TLS certificates if 
+# they change on disk, defaults to no.
+# 
+# tls-rotation no
+
 ################################# GENERAL #####################################
 
 # By default KeyDB does not run as a daemon. Use 'yes' if you need it.

runtest-rotation

@@ -0,0 +1,31 @@
+#!/bin/sh
+TCL_VERSIONS="8.5 8.6"
+TCLSH=""
+
+export ASAN_OPTIONS=allocator_may_return_null=1  $ASAN_OPTIONS
+
+for VERSION in $TCL_VERSIONS; do
+	TCL=`which tclsh$VERSION 2>/dev/null` && TCLSH=$TCL
+done
+
+if [ -z $TCLSH ]
+then
+    echo "You need tcl 8.5 or newer in order to run the KeyDB test"
+    exit 1
+fi
+if [ ! -r tests/tls ] || [ ! -r tests/tls_1 ] || [ ! -r tests/tls_2 ];
+then
+    echo "Generating neccessary certificates for TLS rotation testing."
+    rm -rf tests/tls tests/tls_1 tests/tls_2
+
+    utils/gen-test-certs.sh
+    mv tests/tls tests/tls_1
+    utils/gen-test-certs.sh
+    mv tests/tls tests/tls_2
+    utils/gen-test-certs.sh
+fi
+$TCLSH tests/test_helper.tcl \
+--single unit/tls-rotation \
+--tls \
+--config server-threads 3 \
+"${@}"

src/config.cpp

@@ -2863,6 +2863,7 @@ standardConfig configs[] = {
     createEnumConfig("tls-auth-clients", NULL, MODIFIABLE_CONFIG, tls_auth_clients_enum, g_pserver->tls_auth_clients, TLS_CLIENT_AUTH_YES, NULL, NULL),
     createBoolConfig("tls-prefer-server-ciphers", NULL, MODIFIABLE_CONFIG, g_pserver->tls_ctx_config.prefer_server_ciphers, 0, NULL, updateTlsCfgBool),
     createBoolConfig("tls-session-caching", NULL, MODIFIABLE_CONFIG, g_pserver->tls_ctx_config.session_caching, 1, NULL, updateTlsCfgBool),
+    createBoolConfig("tls-rotation", NULL, MODIFIABLE_CONFIG, g_pserver->tls_rotation, 0, NULL, updateTlsCfgBool),
     createStringConfig("tls-cert-file", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, g_pserver->tls_ctx_config.cert_file, NULL, NULL, updateTlsCfg),
     createStringConfig("tls-key-file", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, g_pserver->tls_ctx_config.key_file, NULL, NULL, updateTlsCfg),
     createStringConfig("tls-key-file-pass", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, g_pserver->tls_ctx_config.key_file_pass, NULL, NULL, updateTlsCfg),

src/sentinel.cpp

@@ -31,7 +31,7 @@
 #include "server.h"
 #include "hiredis.h"
 #ifdef USE_OPENSSL
-#include "openssl/ssl.h"
+#include <openssl/ssl.h>
 extern "C" {
 #include "hiredis_ssl.h"
 }

src/server.cpp

@@ -2578,6 +2578,13 @@ int serverCron(struct aeEventLoop *eventLoop, long long id, void *clientData) {
         fastlock_auto_adjust_waits();
     }
 
+    /* Reload the TLS cert if neccessary. This effectively rotates the 
+     * cert if a change has been made on disk, but the KeyDB server hasn't
+     * been notified. */
+    run_with_period(1000){
+        tlsReload();
+    }
+
     /* Resize tracking keys table if needed. This is also done at every
      * command execution, but we want to be sure that if the last command
      * executed changes the value via CONFIG SET, the server will perform

src/server.h

@@ -1915,6 +1915,12 @@ typedef struct redisTLSContextConfig {
     int session_caching;
     int session_cache_size;
     int session_cache_timeout;
+    time_t cert_file_last_modified;
+    time_t key_file_last_modified;
+    time_t client_cert_file_last_modified;
+    time_t client_key_file_last_modified;
+    time_t ca_cert_file_last_modified;
+    time_t ca_cert_dir_last_modified;
 } redisTLSContextConfig;
 
 /*-----------------------------------------------------------------------------
@@ -2606,6 +2612,7 @@ struct redisServer {
     int tls_cluster;
     int tls_replication;
     int tls_auth_clients;
+    int tls_rotation;
     redisTLSContextConfig tls_ctx_config;
 
     /* cpu affinity */
@@ -3881,7 +3888,7 @@ void tlsInit(void);
 void tlsInitThread();
 void tlsCleanup(void);
 int tlsConfigure(redisTLSContextConfig *ctx_config);
-
+void tlsReload(void);
 
 
 class ShutdownException

src/tls.cpp

@@ -35,13 +35,14 @@
 #include <mutex>
 
 #ifdef USE_OPENSSL
-
 #include <openssl/conf.h>
 #include <openssl/ssl.h>
 #include <openssl/err.h>
 #include <openssl/rand.h>
 #include <openssl/pem.h>
 
+#include <sys/stat.h>
+
 #define REDIS_TLS_PROTO_TLSv1       (1<<0)
 #define REDIS_TLS_PROTO_TLSv1_1     (1<<1)
 #define REDIS_TLS_PROTO_TLSv1_2     (1<<2)
@@ -206,6 +207,18 @@ static int tlsPasswordCallback(char *buf, int size, int rwflag, void *u) {
     return (int) pass_len;
 }
 
+/* Given a path to a file, return the last time it was accessed (in seconds) */
+time_t getLastModifiedTime(const char* path){
+    struct stat path_stat;
+    stat(path, &path_stat);
+
+#ifdef __APPLE__
+    return path_stat.st_mtimespec.tv_sec;
+#else
+    return path_stat.st_mtime;
+#endif
+}
+
 /* Create a *base* SSL_CTX using the SSL configuration provided. The base context
  * includes everything that's common for both client-side and server-side connections.
  */
@@ -310,6 +323,14 @@ int tlsConfigure(redisTLSContextConfig *ctx_config) {
         goto error;
     }
 
+    /* Update the last modified times for the TLS elements */
+    ctx_config->key_file_last_modified = getLastModifiedTime(ctx_config->key_file);
+    ctx_config->cert_file_last_modified = getLastModifiedTime(ctx_config->cert_file);
+    ctx_config->client_cert_file_last_modified = getLastModifiedTime(ctx_config->client_cert_file);
+    ctx_config->client_key_file_last_modified = getLastModifiedTime(ctx_config->client_key_file);
+    ctx_config->ca_cert_dir_last_modified = getLastModifiedTime(ctx_config->ca_cert_dir);
+    ctx_config->ca_cert_file_last_modified = getLastModifiedTime(ctx_config->ca_cert_file);
+
     protocols = parseProtocolsConfig(ctx_config->protocols);
     if (protocols == -1) goto error;
 
@@ -385,6 +406,31 @@ error:
     return C_ERR;
 }
 
+/* Reload TLS certificate from disk, effectively rotating it */
+void tlsReload(void){
+    /* We will only bother checking keys and certs if TLS is enabled, otherwise we would be calling 'stat' for no reason */
+    if (g_pserver->tls_rotation && (g_pserver->tls_port || g_pserver->tls_replication || g_pserver->tls_cluster)){
+
+        bool cert_file_modified = getLastModifiedTime(g_pserver->tls_ctx_config.cert_file) != g_pserver->tls_ctx_config.cert_file_last_modified;
+        bool key_file_modified = getLastModifiedTime(g_pserver->tls_ctx_config.key_file) != g_pserver->tls_ctx_config.key_file_last_modified;
+
+        bool client_cert_file_modified = g_pserver->tls_ctx_config.client_cert_file != NULL && getLastModifiedTime(g_pserver->tls_ctx_config.client_cert_file) != g_pserver->tls_ctx_config.client_cert_file_last_modified;
+        bool client_key_file_modified = g_pserver->tls_ctx_config.client_key_file != NULL && getLastModifiedTime(g_pserver->tls_ctx_config.client_key_file) != g_pserver->tls_ctx_config.client_key_file_last_modified;
+
+        bool ca_cert_file_modified = g_pserver->tls_ctx_config.ca_cert_file != NULL && getLastModifiedTime(g_pserver->tls_ctx_config.ca_cert_file) != g_pserver->tls_ctx_config.ca_cert_file_last_modified;
+        bool ca_cert_dir_modified = g_pserver->tls_ctx_config.ca_cert_dir != NULL && getLastModifiedTime(g_pserver->tls_ctx_config.ca_cert_dir) != g_pserver->tls_ctx_config.ca_cert_dir_last_modified;
+
+        if (cert_file_modified || key_file_modified || ca_cert_file_modified || ca_cert_dir_modified || client_cert_file_modified || client_key_file_modified){
+            serverLog(LL_NOTICE, "TLS certificates changed on disk, attempting to rotate.");
+            if (tlsConfigure(&g_pserver->tls_ctx_config) == C_ERR) {
+                serverLog(LL_NOTICE, "Error trying to rotate TLS certificates, TLS credentials remain unchanged.");
+            } else {
+                serverLog(LL_NOTICE, "TLS certificates rotated successfully.");
+            }
+        }
+    }
+}
+
 #ifdef TLS_DEBUGGING
 #define TLSCONN_DEBUG(fmt, ...) \
     serverLog(LL_DEBUG, "TLSCONN: " fmt, __VA_ARGS__)
@@ -1093,6 +1139,9 @@ int tlsConfigure(redisTLSContextConfig *ctx_config) {
     return C_OK;
 }
 
+void tlsReload(void){
+}
+
 connection *connCreateTLS(void) { 
     return NULL;
 }

tests/test_helper.tcl

@@ -242,6 +242,26 @@ proc redis_client {args} {
     return $client
 }
 
+proc redis_client_tls {args} {
+    set level 0
+    if {[llength $args] > 0 && [string is integer [lindex $args 0]]} {
+        set level [lindex $args 0]
+        set args [lrange $args 1 end]
+    }
+
+    set tlsoptions ""
+    if {[llength $args] > 0 && ![string is integer [lindex $args 0]]} {
+        set tlsoptions [lrange $args 0 end]
+    }
+
+    # create client that takes in custom tls options
+    set client [redis [srv $level "host"] [srv $level "port"] 0 $::tls $tlsoptions]
+
+    # # select the right db and read the response (OK)
+    $client select 9
+    return $client
+}
+
 # Provide easy access to INFO properties. Same semantic as "proc r".
 proc s {args} {
     set level 0
@@ -273,6 +293,7 @@ proc cleanup {} {
     flush stdout
     catch {exec rm -rf {*}[glob tests/tmp/redis.conf.*]}
     catch {exec rm -rf {*}[glob tests/tmp/server.*]}
+    catch {exec rm -rf {*}[glob tests/tmp/tlscerts.*]}
     if {!$::quiet} {puts "OK"}
 }
 

tests/unit/tls-rotation.tcl

@@ -0,0 +1,109 @@
+start_server {tags {"tls-rotation"} overrides {tls-rotation yes}} {
+    if {$::tls} {
+        package require tls
+
+        test {TLS: Create temporary location for certificate rotation} {
+            set rootdir [tmpdir tlscerts]
+
+            file copy "tests/tls" $rootdir
+            file copy "tests/tls_1" $rootdir
+            file copy "tests/tls_2" $rootdir
+
+            set serverdir [format "%s/$rootdir/tls" [pwd]]
+            set clientdir1 [format "%s/$rootdir/tls_1" [pwd]]
+            set clientdir2 [format "%s/$rootdir/tls_2" [pwd]]
+        }
+
+        test {TLS: Update server config to point to temporary location } {
+            r config set tls-key-file "$serverdir/server.key"
+            r config set tls-cert-file "$serverdir/server.crt"
+            r config set tls-ca-cert-file "$serverdir/ca.crt"
+        }
+
+        test {TLS: Connect client to server} {
+            set r2 [redis_client_tls -keyfile "$serverdir/client.key" -certfile "$serverdir/client.crt" -require 1 -cafile "$serverdir/ca.crt"]
+            $r2 set x 50
+            assert_equal {50} [$r2 get x]
+        }
+
+        test {TLS: Rotate all TLS certificates} {
+            file delete -force -- $serverdir 
+            file copy $clientdir1 $serverdir
+            after 1000
+        }
+
+        test {TLS: Already connected clients do not lose connection post certificate rotation} {
+            $r2 incrby x 50
+            assert_equal {100} [$r2 get x]
+        }
+
+        test {TLS: Clients with outdated credentials cannot connect} {
+            catch {set r2 [redis_client_tls -keyfile "$clientdir2/client.key" -certfile "$clientdir2/client.crt" -require 1 -cafile "$clientdir2/ca.crt"]} e
+            assert_no_match {*::redis::redisHandle*} $e
+        }
+
+        test {TLS: Clients with correct certifcates can cannect to server post rotation} {
+            set r2 [redis_client_tls -keyfile "$clientdir1/client.key" -certfile "$clientdir1/client.crt" -require 1 -cafile "$clientdir1/ca.crt"]
+            $r2 incrby x 50
+            assert_equal {150} [$r2 get x]
+        }
+
+        test {TLS: Rotate only server key causing key/cert mismatch} {
+            file copy -force -- "$clientdir2/server.key" $serverdir
+            after 1000
+        }
+
+        test {TLS: Already connected clients do not lose connection despite mismatch} {
+            $r2 incrby x 50
+            assert_equal {200} [$r2 get x]
+        }
+
+        test {TLS: Clients with old credentials can still connect} {
+            set r2 [redis_client_tls -keyfile "$clientdir1/client.key" -certfile "$clientdir1/client.crt" -require 1 -cafile "$clientdir1/ca.crt"]
+            $r2 incrby x 50
+            assert_equal {250} [$r2 get x]
+        }
+
+        test {TLS: Rotate corresponding cert fixing key/cert mismatch} {
+            file copy -force -- "$clientdir2/server.crt" $serverdir
+            after 1000
+        }
+
+        test {TLS: Check that old client is still connected post rotation} {
+            $r2 incrby x 50
+            assert_equal {300} [$r2 get x]
+        }
+
+        test {TLS: Clients with outdated credentials cannot connect} {
+            catch {set r2 [redis_client_tls -keyfile "$clientdir1/client.key" -certfile "$clientdir1/client.crt" -require 1 -cafile "$clientdir1/ca.crt"]} e
+            assert_no_match {*::redis::redisHandle*} $e
+        }
+
+        test {TLS: Clients with correct certifcates can cannect to server post rotation} {
+            set r2 [redis_client_tls -keyfile "$clientdir1/client.key" -certfile "$clientdir1/client.crt" -require 1 -cafile "$clientdir2/ca.crt"]
+            $r2 incrby x 50
+            assert_equal {350} [$r2 get x]
+        }
+
+        test {TLS: Rotate only CA cert} {
+            file copy -force -- "$clientdir2/ca.crt" $serverdir
+            after 1000
+        }
+
+        test {TLS: Check that old client is still connected} {
+            $r2 incrby x 50
+            assert_equal {400} [$r2 get x]
+        }
+
+        test {TLS: Check that client with old credentials won't connect} {
+            catch {set r2 [redis_client_tls -keyfile "$clientdir1/client.key" -certfile "$clientdir1/client.crt" -require 1 -cafile "$clientdir2/ca.crt"]} e
+            assert_no_match {*::redis::redisHandle*} $e
+        }
+
+        test {TLS: Check that client with updated credentials will connect} {
+            catch {set r2 [redis_client_tls -keyfile "$clientdir2/client.key" -certfile "$clientdir2/client.crt" -require 1 -cafile "$clientdir2/ca.crt"]} e
+            $r2 incrby x 50
+            assert_equal {450} [$r2 get x]
+        }
+    }
+}

tests/unit/tls.tcl

@@ -1,4 +1,4 @@
-start_server {tags {"tls"}} {
+start_server {tags {"tls"} overrides {tls-rotation yes}} {
     if {$::tls} {
         package require tls