From bf6c002005fcc0b4320bc2bc3208f3de87f5c876 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Fri, 10 Jul 2020 11:33:47 +0300 Subject: [PATCH] TLS: Session caching configuration support. (#7420) * TLS: Session caching configuration support. * TLS: Remove redundant config initialization. Former-commit-id: d3834c50699bc4f31f381d6d03d4c1b022380895 --- TLS.md | 2 -- keydb.conf | 16 ++++++++++++++++ src/aelocker.h | 5 ++++- src/config.cpp | 11 ++++++++++- src/networking.cpp | 11 ++++++----- src/server.cpp | 11 ++--------- src/server.h | 5 ++++- src/tls.cpp | 11 +++++++++-- tests/unit/introspection.tcl | 28 ++++++++++++++++++---------- 9 files changed, 69 insertions(+), 31 deletions(-) diff --git a/TLS.md b/TLS.md index ae1a066db..1afa2e8fa 100644 --- a/TLS.md +++ b/TLS.md @@ -56,8 +56,6 @@ Note that unlike Redis, KeyDB fully supports multithreading of TLS connections. To-Do List ---------- -- [ ] Add session caching support. Check if/how it's handled by clients to - assess how useful/important it is. - [ ] redis-benchmark support. The current implementation is a mix of using hiredis for parsing and basic networking (establishing connections), but directly manipulating sockets for most actions. This will need to be cleaned diff --git a/keydb.conf b/keydb.conf index 8979ee47c..53afa107a 100644 --- a/keydb.conf +++ b/keydb.conf @@ -199,6 +199,22 @@ tcp-keepalive 300 # # tls-prefer-server-ciphers yes +# By default, TLS session caching is enabled to allow faster and less expensive +# reconnections by clients that support it. Use the following directive to disable +# caching. +# +# tls-session-caching no + +# Change the default number of TLS sessions cached. A zero value sets the cache +# to unlimited size. The default size is 20480. +# +# tls-session-cache-size 5000 + +# Change the default timeout of cached TLS sessions. The default timeout is 300 +# seconds. +# +# tls-session-cache-timeout 60 + ################################# GENERAL ##################################### # By default KeyDB does not run as a daemon. Use 'yes' if you need it. diff --git a/src/aelocker.h b/src/aelocker.h index ef757d2d2..e854f907b 100644 --- a/src/aelocker.h +++ b/src/aelocker.h @@ -9,11 +9,14 @@ public: { } - void arm(client *c) // if a client is passed, then the client is already locked + void arm(client *c, bool fIfNeeded = false) // if a client is passed, then the client is already locked { if (m_fArmed) return; + if (fIfNeeded && aeThreadOwnsLock()) + return; + serverAssertDebug(!GlobalLocksAcquired()); if (c != nullptr) diff --git a/src/config.cpp b/src/config.cpp index 75360c4b7..c0b8ff87e 100644 --- a/src/config.cpp +++ b/src/config.cpp @@ -2180,7 +2180,7 @@ static int updateTlsCfg(char *val, char *prev, const char **err) { UNUSED(prev); UNUSED(err); if (tlsConfigure(&g_pserver->tls_ctx_config) == C_ERR) { - *err = "Unable to configure tls-cert-file. Check server logs."; + *err = "Unable to update TLS configuration. Check server logs."; return 0; } return 1; @@ -2190,6 +2190,12 @@ static int updateTlsCfgBool(int val, int prev, const char **err) { UNUSED(prev); return updateTlsCfg(NULL, NULL, err); } + +static int updateTlsCfgInt(long long val, long long prev, const char **err) { + UNUSED(val); + UNUSED(prev); + return updateTlsCfg(NULL, NULL, err); +} #endif /* USE_OPENSSL */ int fDummy = false; @@ -2324,10 +2330,13 @@ standardConfig configs[] = { #ifdef USE_OPENSSL createIntConfig("tls-port", NULL, IMMUTABLE_CONFIG, 0, 65535, g_pserver->tls_port, 0, INTEGER_CONFIG, NULL, NULL), /* TCP port. */ + createIntConfig("tls-session-cache-size", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, g_pserver->tls_ctx_config.session_cache_size, 20*1024, INTEGER_CONFIG, NULL, updateTlsCfgInt), + createIntConfig("tls-session-cache-timeout", NULL, MODIFIABLE_CONFIG, 0, INT_MAX, g_pserver->tls_ctx_config.session_cache_timeout, 300, INTEGER_CONFIG, NULL, updateTlsCfgInt), createBoolConfig("tls-cluster", NULL, MODIFIABLE_CONFIG, g_pserver->tls_cluster, 0, NULL, NULL), createBoolConfig("tls-replication", NULL, MODIFIABLE_CONFIG, g_pserver->tls_replication, 0, NULL, NULL), createBoolConfig("tls-auth-clients", NULL, MODIFIABLE_CONFIG, g_pserver->tls_auth_clients, 1, 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), 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-dh-params-file", NULL, MODIFIABLE_CONFIG, EMPTY_STRING_IS_NULL, g_pserver->tls_ctx_config.dh_params_file, NULL, NULL, updateTlsCfg), diff --git a/src/networking.cpp b/src/networking.cpp index d5b3ef0b3..666d2fdae 100644 --- a/src/networking.cpp +++ b/src/networking.cpp @@ -2308,8 +2308,9 @@ void commandProcessed(client *c) { int processCommandAndResetClient(client *c, int flags) { int deadclient = 0; serverTL->current_client = c; - AeLocker locker; - if (processCommand(c, flags, locker) == C_OK) { + serverAssert(GlobalLocksAcquired()); + + if (processCommand(c, flags) == C_OK) { commandProcessed(c); } if (serverTL->current_client == NULL) deadclient = 1; @@ -2465,7 +2466,8 @@ void readQueryFromClient(connection *conn) { void processClients() { - aeAcquireLock(); + serverAssert(GlobalLocksAcquired()); + for (client *c : serverTL->vecclientsProcess) { /* There is more data in the client input buffer, continue parsing it * in case to check if there is a full command to execute. */ @@ -2477,7 +2479,6 @@ void processClients() { ProcessPendingAsyncWrites(); } - aeReleaseLock(); serverTL->vecclientsProcess.clear(); } @@ -3437,7 +3438,7 @@ void processEventsWhileBlocked(int iel) { aeReleaseLock(); - serverAssertDebug(!GlobalLocksAcquired()); + serverAssert(!GlobalLocksAcquired()); try { while (iterations--) { diff --git a/src/server.cpp b/src/server.cpp index 458a0f48f..faab9341d 100644 --- a/src/server.cpp +++ b/src/server.cpp @@ -3636,12 +3636,12 @@ void call(client *c, int flags) { * If C_OK is returned the client is still alive and valid and * other operations can be performed by the caller. Otherwise * if C_ERR is returned the client was destroyed (i.e. after QUIT). */ -int processCommand(client *c, int callFlags, AeLocker &locker) { +int processCommand(client *c, int callFlags) { AssertCorrectThread(c); + serverAssert(GlobalLocksAcquired()); if (moduleHasCommandFilters()) { - locker.arm(c); moduleCallCommandFilters(c); } @@ -3693,9 +3693,6 @@ int processCommand(client *c, int callFlags, AeLocker &locker) { /* Check if the user can run this command according to the current * ACLs. */ - if (c->puser && !(c->puser->flags & USER_FLAG_ALLCOMMANDS)) - locker.arm(c); // ACLs require the lock - int acl_keypos; int acl_retval = ACLCheckCommandPerm(c,&acl_keypos); if (acl_retval != ACL_OK) { @@ -3723,7 +3720,6 @@ int processCommand(client *c, int callFlags, AeLocker &locker) { !(c->cmd->getkeys_proc == NULL && c->cmd->firstkey == 0 && c->cmd->proc != execCommand)) { - locker.arm(c); int hashslot; int error_code; clusterNode *n = getNodeByQuery(c,c->cmd,c->argv,c->argc, @@ -3738,9 +3734,6 @@ int processCommand(client *c, int callFlags, AeLocker &locker) { return C_OK; } } - - if (!locker.isArmed()) - locker.arm(c); incrementMvccTstamp(); diff --git a/src/server.h b/src/server.h index d51d2354a..8d2c7b929 100644 --- a/src/server.h +++ b/src/server.h @@ -1528,6 +1528,9 @@ typedef struct redisTLSContextConfig { char *ciphers; char *ciphersuites; int prefer_server_ciphers; + int session_caching; + int session_cache_size; + int session_cache_timeout; } redisTLSContextConfig; /*----------------------------------------------------------------------------- @@ -2564,7 +2567,7 @@ int getMaxmemoryState(size_t *total, size_t *logical, size_t *tofree, float *lev size_t freeMemoryGetNotCountedMemory(); int freeMemoryIfNeeded(void); int freeMemoryIfNeededAndSafe(void); -int processCommand(client *c, int callFlags, class AeLocker &locker); +int processCommand(client *c, int callFlags); void setupSignalHandlers(void); struct redisCommand *lookupCommand(sds name); struct redisCommand *lookupCommandByCString(const char *s); diff --git a/src/tls.cpp b/src/tls.cpp index 25ca0bd31..5a128b596 100644 --- a/src/tls.cpp +++ b/src/tls.cpp @@ -150,8 +150,6 @@ void tlsInit(void) { serverLog(LL_WARNING, "OpenSSL: Failed to seed random number generator."); } - /* Server configuration */ - g_pserver->tls_auth_clients = 1; /* Secure by default */ tlsInitThread(); } @@ -193,6 +191,15 @@ int tlsConfigure(redisTLSContextConfig *ctx_config) { SSL_CTX_set_options(ctx, SSL_OP_DONT_INSERT_EMPTY_FRAGMENTS); #endif + if (ctx_config->session_caching) { + SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_SERVER); + SSL_CTX_sess_set_cache_size(ctx, ctx_config->session_cache_size); + SSL_CTX_set_timeout(ctx, ctx_config->session_cache_timeout); + SSL_CTX_set_session_id_context(ctx, (const unsigned char*) "KeyDB", 5); + } else { + SSL_CTX_set_session_cache_mode(ctx, SSL_SESS_CACHE_OFF); + } + protocols = parseProtocolsConfig(ctx_config->protocols); if (protocols == -1) goto error; diff --git a/tests/unit/introspection.tcl b/tests/unit/introspection.tcl index c7121e848..8ca84bf2f 100644 --- a/tests/unit/introspection.tcl +++ b/tests/unit/introspection.tcl @@ -78,17 +78,8 @@ start_server {tags {"introspection"}} { syslog-facility databases port - io-threads tls-port - tls-prefer-server-ciphers - tls-cert-file - tls-key-file - tls-dh-params-file - tls-ca-cert-file - tls-ca-cert-dir - tls-protocols - tls-ciphers - tls-ciphersuites + io-threads logfile unixsocketperm slaveof @@ -101,6 +92,23 @@ start_server {tags {"introspection"}} { bgsave_cpulist } + if {!$::tls} { + append skip_configs { + tls-prefer-server-ciphers + tls-session-cache-timeout + tls-session-cache-size + tls-session-caching + tls-cert-file + tls-key-file + tls-dh-params-file + tls-ca-cert-file + tls-ca-cert-dir + tls-protocols + tls-ciphers + tls-ciphersuites + } + } + set configs {} foreach {k v} [r config get *] { if {[lsearch $skip_configs $k] != -1} {