diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index e98e0d0ca..b50d41e6f 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -31,3 +31,6 @@ jobs: - name: module tests run: | ./runtest-moduleapi + - name: rotation test + run: | + ./runtest-rotation diff --git a/.gitignore b/.gitignore index a4a96d1bd..cb2ee29cf 100644 --- a/.gitignore +++ b/.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 diff --git a/README.md b/README.md index 2c31df298..aba0cb8fa 100644 --- a/README.md +++ b/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 --------- diff --git a/keydb.conf b/keydb.conf index 247bcb9ad..677303f86 100644 --- a/keydb.conf +++ b/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. diff --git a/runtest-rotation b/runtest-rotation new file mode 100755 index 000000000..7151f752d --- /dev/null +++ b/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 \ +"${@}" diff --git a/src/config.cpp b/src/config.cpp index 0ff434a8a..a1afdbba0 100644 --- a/src/config.cpp +++ b/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), diff --git a/src/sentinel.cpp b/src/sentinel.cpp index 42bbdc396..d45757daa 100644 --- a/src/sentinel.cpp +++ b/src/sentinel.cpp @@ -31,7 +31,7 @@ #include "server.h" #include "hiredis.h" #ifdef USE_OPENSSL -#include "openssl/ssl.h" +#include extern "C" { #include "hiredis_ssl.h" } diff --git a/src/server.cpp b/src/server.cpp index 34fa4bc93..119f91bb4 100644 --- a/src/server.cpp +++ b/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 diff --git a/src/server.h b/src/server.h index 5542f1077..4dee3d07c 100644 --- a/src/server.h +++ b/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 diff --git a/src/tls.cpp b/src/tls.cpp index 3ad43e0a0..ba6351cde 100644 --- a/src/tls.cpp +++ b/src/tls.cpp @@ -35,13 +35,14 @@ #include #ifdef USE_OPENSSL - #include #include #include #include #include +#include + #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; } diff --git a/tests/test_helper.tcl b/tests/test_helper.tcl index 768abc19a..2436c1ae0 100644 --- a/tests/test_helper.tcl +++ b/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"} } diff --git a/tests/unit/tls-rotation.tcl b/tests/unit/tls-rotation.tcl new file mode 100644 index 000000000..75e70e077 --- /dev/null +++ b/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] + } + } +} diff --git a/tests/unit/tls.tcl b/tests/unit/tls.tcl index 7dff47eb4..1e2b505bf 100644 --- a/tests/unit/tls.tcl +++ b/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