From: Justin Dossey Date: Mon, 22 Sep 2025 15:25:46 +0000 (-0700) Subject: Fix errors found by @vstakhov in #5626 X-Git-Tag: 3.13.1~11^2~2 X-Git-Url: http://git.ipfire.org/?a=commitdiff_plain;h=90b894715c1087655eed845487582ecec85e37b4;p=thirdparty%2Frspamd.git Fix errors found by @vstakhov in #5626 - remove redundant `ensure_ssl_inited` function and calls. Core SSL init should suffice. - Refactor TLS initiation into `redis_pool_elt::initiate_tls(...)` to eliminate duplication - Switch TLS flags to `bool` in the public struct - Fix ephemeral string usage in lua by duplicating the values into locals and freeing after connect. Flags are boolean. (it's not super likely that Lua will GC the strings before we connect to Redis, but this ensures that it won't be a problem) - Remove the redis TLS options propagation unit test Build succeeds and C++ unit tests pass. --- diff --git a/src/libserver/redis_pool.cxx b/src/libserver/redis_pool.cxx index 7fcd7824cb..f8aa8c31e0 100644 --- a/src/libserver/redis_pool.cxx +++ b/src/libserver/redis_pool.cxx @@ -233,6 +233,26 @@ public: } private: + auto initiate_tls(redisAsyncContext *ctx) const -> bool + { + redisSSLContextError ssl_err = REDIS_SSL_CTX_NONE; + redisSSLOptions opts{}; + opts.cacert_filename = ca_file.empty() ? nullptr : ca_file.c_str(); + opts.capath = ca_dir.empty() ? nullptr : ca_dir.c_str(); + opts.cert_filename = cert_file.empty() ? nullptr : cert_file.c_str(); + opts.private_key_filename = key_file.empty() ? nullptr : key_file.c_str(); + opts.server_name = sni.empty() ? nullptr : sni.c_str(); + opts.verify_mode = no_ssl_verify ? REDIS_SSL_VERIFY_NONE : REDIS_SSL_VERIFY_PEER; + auto *ssl_ctx = redisCreateSSLContextWithOptions(&opts, &ssl_err); + if (!ssl_ctx || redisInitiateSSLWithContext(&ctx->c, ssl_ctx) != REDIS_OK) { + msg_err("cannot start TLS for redis %s:%d: %s", ip.c_str(), port, ctx->errstr); + if (ssl_ctx) redisFreeSSLContext(ssl_ctx); + return false; + } + redisFreeSSLContext(ssl_ctx); + return true; + } + auto redis_async_new() -> redisAsyncContext * { struct redisAsyncContext *ctx; @@ -498,20 +518,7 @@ redis_pool_connection::redis_pool_connection(redis_pool *_pool, } } -static bool ensure_ssl_inited() -{ - static bool inited = false; - if (!inited) { - if (redisInitOpenSSL() == REDIS_OK) { - inited = true; - } - else { - /* still try, hiredis might work if already inited elsewhere */ - inited = true; - } - } - return inited; -} +/* OpenSSL is initialised by core before Redis pool is used */ auto redis_pool_elt::new_connection() -> redisAsyncContext * { @@ -558,30 +565,13 @@ auto redis_pool_elt::new_connection() -> redisAsyncContext * conn->ctx->errstr, ip.c_str(), port, nctx); if (nctx) { - /* If TLS is configured for this element, initiate it now */ - if (use_tls && !is_unix) { - if (ensure_ssl_inited()) { - redisSSLContextError ssl_err = REDIS_SSL_CTX_NONE; - redisSSLOptions opts{}; - opts.cacert_filename = ca_file.empty() ? nullptr : ca_file.c_str(); - opts.capath = ca_dir.empty() ? nullptr : ca_dir.c_str(); - opts.cert_filename = cert_file.empty() ? nullptr : cert_file.c_str(); - opts.private_key_filename = key_file.empty() ? nullptr : key_file.c_str(); - opts.server_name = sni.empty() ? nullptr : sni.c_str(); - opts.verify_mode = no_ssl_verify ? REDIS_SSL_VERIFY_NONE : REDIS_SSL_VERIFY_PEER; - - auto *ssl_ctx = redisCreateSSLContextWithOptions(&opts, &ssl_err); - if (!ssl_ctx || redisInitiateSSLWithContext(&nctx->c, ssl_ctx) != REDIS_OK) { - msg_err("cannot start TLS for redis %s:%d: %s", ip.c_str(), port, nctx->errstr); - if (ssl_ctx) redisFreeSSLContext(ssl_ctx); + /* If TLS is configured for this element, initiate it now */ + if (use_tls && !is_unix) { + if (!initiate_tls(nctx)) { redisAsyncFree(nctx); nctx = nullptr; } - else { - redisFreeSSLContext(ssl_ctx); - } } - } if (nctx) { active.emplace_front(std::make_unique(pool, this, @@ -597,30 +587,13 @@ auto redis_pool_elt::new_connection() -> redisAsyncContext * auto *nctx = redis_async_new(); if (nctx) { - /* If TLS is configured for this element, initiate it now */ - if (use_tls && !is_unix) { - if (ensure_ssl_inited()) { - redisSSLContextError ssl_err = REDIS_SSL_CTX_NONE; - redisSSLOptions opts{}; - opts.cacert_filename = ca_file.empty() ? nullptr : ca_file.c_str(); - opts.capath = ca_dir.empty() ? nullptr : ca_dir.c_str(); - opts.cert_filename = cert_file.empty() ? nullptr : cert_file.c_str(); - opts.private_key_filename = key_file.empty() ? nullptr : key_file.c_str(); - opts.server_name = sni.empty() ? nullptr : sni.c_str(); - opts.verify_mode = no_ssl_verify ? REDIS_SSL_VERIFY_NONE : REDIS_SSL_VERIFY_PEER; - - auto *ssl_ctx = redisCreateSSLContextWithOptions(&opts, &ssl_err); - if (!ssl_ctx || redisInitiateSSLWithContext(&nctx->c, ssl_ctx) != REDIS_OK) { - msg_err("cannot start TLS for redis %s:%d: %s", ip.c_str(), port, nctx->errstr); - if (ssl_ctx) redisFreeSSLContext(ssl_ctx); + /* If TLS is configured for this element, initiate it now */ + if (use_tls && !is_unix) { + if (!initiate_tls(nctx)) { redisAsyncFree(nctx); nctx = nullptr; } - else { - redisFreeSSLContext(ssl_ctx); - } } - } if (nctx) { active.emplace_front(std::make_unique(pool, this, @@ -788,9 +761,9 @@ rspamd_redis_pool_connect_ext(void *p, g_assert(p != NULL); auto *pool = reinterpret_cast(p); - if (tls && tls->use_tls) { - return pool->new_connection_ext(db, username, password, ip, port, - true, tls->no_ssl_verify != 0, + if (tls && tls->use_tls) { + return pool->new_connection_ext(db, username, password, ip, port, + true, tls->no_ssl_verify, tls->ca_file, tls->ca_dir, tls->cert_file, tls->key_file, tls->sni); diff --git a/src/libserver/redis_pool.h b/src/libserver/redis_pool.h index 71eaa17088..ed648abece 100644 --- a/src/libserver/redis_pool.h +++ b/src/libserver/redis_pool.h @@ -25,8 +25,8 @@ struct rspamd_config; struct redisAsyncContext; struct ev_loop; struct rspamd_redis_tls_opts { - int use_tls; /* 0/1 */ - int no_ssl_verify; /* 0/1 */ + bool use_tls; /* enable TLS */ + bool no_ssl_verify; /* disable peer verify */ const char *ca_file; /* optional */ const char *ca_dir; /* optional */ const char *cert_file; /* optional */ diff --git a/src/lua/lua_redis.c b/src/lua/lua_redis.c index d46a23d2a7..fbe65bacee 100644 --- a/src/lua/lua_redis.c +++ b/src/lua/lua_redis.c @@ -889,6 +889,9 @@ rspamd_lua_redis_prepare_connection(lua_State *L, int *pcbref, gboolean is_async const char *host = NULL; const char *username = NULL, *password = NULL, *dbname = NULL, *log_tag = NULL; struct rspamd_redis_tls_opts tls_opts; + /* Duplicated TLS strings to ensure lifetime beyond Lua stack */ + char *dup_ca_file = NULL, *dup_ca_dir = NULL, *dup_cert_file = NULL, + *dup_key_file = NULL, *dup_sni = NULL; int cbref = -1; struct rspamd_config *cfg = NULL; struct rspamd_async_session *session = NULL; @@ -896,7 +899,7 @@ rspamd_lua_redis_prepare_connection(lua_State *L, int *pcbref, gboolean is_async gboolean ret = FALSE; unsigned int flags = 0; - memset(&tls_opts, 0, sizeof(tls_opts)); + memset(&tls_opts, 0, sizeof(tls_opts)); if (lua_istable(L, 1)) { /* Table version */ @@ -1013,55 +1016,61 @@ rspamd_lua_redis_prepare_connection(lua_State *L, int *pcbref, gboolean is_async } lua_pop(L, 1); - /* TLS options (optional) */ - lua_pushstring(L, "ssl"); - lua_gettable(L, -2); - if (!!lua_toboolean(L, -1)) { - tls_opts.use_tls = 1; - } - lua_pop(L, 1); - - lua_pushstring(L, "no_ssl_verify"); - lua_gettable(L, -2); - if (!!lua_toboolean(L, -1)) { - tls_opts.no_ssl_verify = 1; - } - lua_pop(L, 1); - - lua_pushstring(L, "ssl_ca"); - lua_gettable(L, -2); - if (lua_type(L, -1) == LUA_TSTRING) { - tls_opts.ca_file = lua_tostring(L, -1); - } - lua_pop(L, 1); - - lua_pushstring(L, "ssl_ca_dir"); - lua_gettable(L, -2); - if (lua_type(L, -1) == LUA_TSTRING) { - tls_opts.ca_dir = lua_tostring(L, -1); - } - lua_pop(L, 1); - - lua_pushstring(L, "ssl_cert"); - lua_gettable(L, -2); - if (lua_type(L, -1) == LUA_TSTRING) { - tls_opts.cert_file = lua_tostring(L, -1); - } - lua_pop(L, 1); - - lua_pushstring(L, "ssl_key"); - lua_gettable(L, -2); - if (lua_type(L, -1) == LUA_TSTRING) { - tls_opts.key_file = lua_tostring(L, -1); - } - lua_pop(L, 1); - - lua_pushstring(L, "sni"); - lua_gettable(L, -2); - if (lua_type(L, -1) == LUA_TSTRING) { - tls_opts.sni = lua_tostring(L, -1); - } - lua_pop(L, 1); + /* TLS options (optional) */ + lua_pushstring(L, "ssl"); + lua_gettable(L, -2); + if (!!lua_toboolean(L, -1)) { + tls_opts.use_tls = true; + } + lua_pop(L, 1); + + lua_pushstring(L, "no_ssl_verify"); + lua_gettable(L, -2); + if (!!lua_toboolean(L, -1)) { + tls_opts.no_ssl_verify = true; + } + lua_pop(L, 1); + + /* Duplicate string options to avoid ephemeral Lua string pointers */ + lua_pushstring(L, "ssl_ca"); + lua_gettable(L, -2); + if (lua_type(L, -1) == LUA_TSTRING) { + dup_ca_file = g_strdup(lua_tostring(L, -1)); + tls_opts.ca_file = dup_ca_file; + } + lua_pop(L, 1); + + lua_pushstring(L, "ssl_ca_dir"); + lua_gettable(L, -2); + if (lua_type(L, -1) == LUA_TSTRING) { + dup_ca_dir = g_strdup(lua_tostring(L, -1)); + tls_opts.ca_dir = dup_ca_dir; + } + lua_pop(L, 1); + + lua_pushstring(L, "ssl_cert"); + lua_gettable(L, -2); + if (lua_type(L, -1) == LUA_TSTRING) { + dup_cert_file = g_strdup(lua_tostring(L, -1)); + tls_opts.cert_file = dup_cert_file; + } + lua_pop(L, 1); + + lua_pushstring(L, "ssl_key"); + lua_gettable(L, -2); + if (lua_type(L, -1) == LUA_TSTRING) { + dup_key_file = g_strdup(lua_tostring(L, -1)); + tls_opts.key_file = dup_key_file; + } + lua_pop(L, 1); + + lua_pushstring(L, "sni"); + lua_gettable(L, -2); + if (lua_type(L, -1) == LUA_TSTRING) { + dup_sni = g_strdup(lua_tostring(L, -1)); + tls_opts.sni = dup_sni; + } + lua_pop(L, 1); lua_pushstring(L, "no_pool"); lua_gettable(L, -2); @@ -1124,15 +1133,22 @@ rspamd_lua_redis_prepare_connection(lua_State *L, int *pcbref, gboolean is_async if (ret) { ud->terminated = 0; - ud->ctx = rspamd_redis_pool_connect_ext(ud->pool, - dbname, username, password, - rspamd_inet_address_to_string(addr->addr), - rspamd_inet_address_get_port(addr->addr), - &tls_opts); - - if (ip) { - rspamd_inet_address_free(ip); - } + ud->ctx = rspamd_redis_pool_connect_ext(ud->pool, + dbname, username, password, + rspamd_inet_address_to_string(addr->addr), + rspamd_inet_address_get_port(addr->addr), + &tls_opts); + + /* Free temporary TLS strings after they have been consumed */ + g_free(dup_ca_file); + g_free(dup_ca_dir); + g_free(dup_cert_file); + g_free(dup_key_file); + g_free(dup_sni); + + if (ip) { + rspamd_inet_address_free(ip); + } if (ud->ctx == NULL || ud->ctx->err) { if (ud->ctx) { @@ -1155,14 +1171,21 @@ rspamd_lua_redis_prepare_connection(lua_State *L, int *pcbref, gboolean is_async msg_debug_lua_redis("opened redis connection host=%s; lua_ctx=%p; redis_ctx=%p; ud=%p", host, ctx, ud->ctx, ud); - return ctx; - } + return ctx; + } - if (ip) { - rspamd_inet_address_free(ip); - } + if (ip) { + rspamd_inet_address_free(ip); + } + + /* Free any duplicated TLS strings on error path */ + g_free(dup_ca_file); + g_free(dup_ca_dir); + g_free(dup_cert_file); + g_free(dup_key_file); + g_free(dup_sni); - return NULL; + return NULL; } /*** diff --git a/test/lua/unit/redis_tls.lua b/test/lua/unit/redis_tls.lua deleted file mode 100644 index 9358992cf3..0000000000 --- a/test/lua/unit/redis_tls.lua +++ /dev/null @@ -1,36 +0,0 @@ --- Simple unit test to validate TLS options propagation in lua_redis -local lua_redis = require "lua_redis" - --- Build a minimal options table; no network connections are made here -local opts = { - servers = '127.0.0.1:6379', - timeout = 1.0, - ssl = true, - no_ssl_verify = true, - ssl_ca = '/tmp/ca.crt', - ssl_ca_dir = '/tmp/ca', - ssl_cert = '/tmp/client.crt', - ssl_key = '/tmp/client.key', - sni = 'example.test', -} - -local params = lua_redis.try_load_redis_servers(opts, nil, true) - -assert(params, 'try_load_redis_servers returned nil') -assert(params.ssl == true, 'ssl flag not propagated') -assert(params.no_ssl_verify == true, 'no_ssl_verify flag not propagated') -assert(params.ssl_ca == '/tmp/ca.crt', 'ssl_ca not propagated') -assert(params.ssl_ca_dir == '/tmp/ca', 'ssl_ca_dir not propagated') -assert(params.ssl_cert == '/tmp/client.crt', 'ssl_cert not propagated') -assert(params.ssl_key == '/tmp/client.key', 'ssl_key not propagated') -assert(params.sni == 'example.test', 'sni not propagated') - --- Also ensure request helpers pass these options through (no execution) --- This part only checks that the table has values set that would be consumed --- by rspamd_redis.make_request/connect in runtime code paths. -local req_attrs = { task = nil, config = nil, ev_base = nil } -- not used here -local req_tbl = { 'PING' } - --- If we got here, options are present; actual network tests are covered by functional tests. -return true -