enable TCP keepalive on Redis connections by default

and make it configurable in `OIDCRedisCacheConnectTimeout
<connect-timeout> [0|<keep-alive-interval>]`; refactor Redis connect
routines

Signed-off-by: Hans Zandbelt <[email protected]>

ChangeLog

@@ -1,5 +1,10 @@
+02/22/2024
+- enable TCP keepalive on Redis connections by default
+  and make it configurable in `OIDCRedisCacheConnectTimeout <connect-timeout> [0|<keep-alive-interval>]`
+- refactor Redis connect routines
+
 02/21/2024
-- OIDCProviderSignedJwksUri - change to make exp claim optional in signed jwks
+- OIDCProviderSignedJwksUri: make exp claim optional in signed jwks; see #1182; thanks @psteniusubi
   interop with OpenID Federation specification https://openid.net/specs/openid-federation-1_0-32.html#section-5.2.1 
 
 02/15/2024

auth_openidc.conf

@@ -692,9 +692,13 @@
 # When not defined the default database 0 is used.
 #OIDCRedisCacheDatabase <number>
 
-# Timeout for connecting to the Redis servers.
-# When not defined the default connect timeout is 5 seconds.
-#OIDCRedisCacheConnectTimeout <seconds>
+# Timeout (in seconds) for connecting to the Redis servers.
+# An optional 2nd parameter can be supplied to set the keepalive interval (in seconds) on the
+# TCP connection to the Redis server. 0 disables keepalive.
+# NB: the interval setting only works when compiled and running with hiredis >= 1.2.0
+#     when compiled and running with hiredis < 1.2.0 any value > 0 will apply the default interval 
+# When not defined the default connect timeout is 5 seconds and the default hiredis keepalive (15s) is applied.
+#OIDCRedisCacheConnectTimeout <connect-timeout> [0|<keep-alive-interval>]
 
 # Timeout waiting for a response of the Redis servers after a request was sent.
 # When not defined the default timeout is 5 seconds.

src/cache/redis.c

@@ -51,6 +51,7 @@ extern module AP_MODULE_DECLARE_DATA auth_openidc_module;
 
 #define REDIS_CONNECT_TIMEOUT_DEFAULT 5
 #define REDIS_TIMEOUT_DEFAULT 5
+#define REDIS_KEEPALIVE_DEFAULT -1
 
 /* create the cache context */
 static oidc_cache_cfg_redis_t *oidc_cache_redis_cfg_create(apr_pool_t *pool) {
@@ -61,6 +62,7 @@ static oidc_cache_cfg_redis_t *oidc_cache_redis_cfg_create(apr_pool_t *pool) {
 	context->database = -1;
 	context->connect_timeout.tv_sec = REDIS_CONNECT_TIMEOUT_DEFAULT;
 	context->connect_timeout.tv_usec = 0;
+	context->keepalive = REDIS_KEEPALIVE_DEFAULT;
 	context->timeout.tv_sec = REDIS_TIMEOUT_DEFAULT;
 	context->timeout.tv_usec = 0;
 	context->host_str = NULL;
@@ -93,6 +95,9 @@ int oidc_cache_redis_post_config(server_rec *s, oidc_cfg *cfg, const char *name)
 	if (cfg->cache_redis_connect_timeout != -1)
 		context->connect_timeout.tv_sec = cfg->cache_redis_connect_timeout;
 
+	if (cfg->cache_redis_keepalive != -1)
+		context->keepalive = cfg->cache_redis_keepalive;
+
 	if (cfg->cache_redis_timeout != -1)
 		context->timeout.tv_sec = cfg->cache_redis_timeout;
 
@@ -191,69 +196,140 @@ static void oidc_cache_redis_reply_free(redisReply **reply) {
 	}
 }
 
-/*
- * connect to Redis server
- */
-static apr_status_t oidc_cache_redis_connect(request_rec *r, oidc_cache_cfg_redis_t *context) {
+apr_byte_t oidc_cache_redis_set_keepalive(request_rec *r, redisContext *rctx, const int keepalive) {
+	apr_byte_t rv = TRUE;
+
+	// default is -1
+	if (keepalive == 0) {
+		oidc_debug(r, "not setting redisEnableKeepAlive");
+		goto end;
+	}
+
+#if HIREDIS_MAJOR >= 1 && HIREDIS_MINOR >= 2
+
+	if (keepalive == -1) {
+		oidc_debug(r, "setting redisEnableKeepAlive to the default interval");
+		if (redisEnableKeepAlive(rctx) != REDIS_OK) {
+			oidc_error(r, "redisEnableKeepAlive failed: %s", rctx->errstr);
+			rv = FALSE;
+		}
+		goto end;
+	}
+
+	oidc_debug(r, "setting redisEnableKeepAliveWithInterval: %d", keepalive);
+	if (redisEnableKeepAliveWithInterval(rctx, keepalive) != REDIS_OK) {
+		oidc_error(r, "redisEnableKeepAliveWithInterval failed: %s", rctx->errstr);
+		rv = FALSE;
+	}
+
+#else
 
+	// -1 or > 0
+	oidc_debug(r, "setting redisEnableKeepAlive to the default interval");
+	if (redisEnableKeepAlive(rctx) != REDIS_OK) {
+		oidc_error(r, "redisEnableKeepAlive failed: %s", rctx->errstr);
+		rv = FALSE;
+	}
+
+#endif
+
+end:
+
+	return rv;
+}
+
+apr_byte_t oidc_cache_redis_set_auth(request_rec *r, redisContext *rctx, const char *username, const char *password) {
+	apr_byte_t rv = TRUE;
 	redisReply *reply = NULL;
 
-	if (context->rctx != NULL)
+	if (password == NULL)
 		goto end;
 
-	/* no connection, connect to the configured Redis server */
-	oidc_debug(r, "calling redisConnectWithTimeout");
-	context->rctx = redisConnectWithTimeout(context->host_str, context->port, context->connect_timeout);
+	if (username != NULL)
+		reply = redisCommand(rctx, "AUTH %s %s", username, password);
+	else
+		reply = redisCommand(rctx, "AUTH %s", password);
 
-	/* check for errors */
-	if ((context->rctx == NULL) || (context->rctx->err != 0)) {
-		oidc_error(r, "failed to connect to Redis server (%s:%d): '%s'", context->host_str, context->port,
-			   context->rctx != NULL ? context->rctx->errstr : "");
-		context->disconnect(context);
+	if ((reply == NULL) || (reply->type == REDIS_REPLY_ERROR)) {
+		oidc_error(r, "Redis AUTH command failed: '%s' [%s]", rctx->errstr, reply ? reply->str : "<n/a>");
+		rv = FALSE;
 		goto end;
 	}
 
-	/* log the connection */
-	oidc_debug(r, "successfully connected to Redis server (%s:%d)", context->host_str, context->port);
+	oidc_debug(r, "successfully authenticated to the Redis server: %s", reply ? reply->str : "<n/a>");
 
-	if (redisSetTimeout(context->rctx, context->timeout) != REDIS_OK)
-		oidc_error(r, "redisSetTimeout failed: %s", context->rctx->errstr);
+end:
 
-	/* see if we need to authenticate to the Redis server */
-	if (context->passwd != NULL) {
-		if (context->username != NULL) {
-			reply = redisCommand(context->rctx, "AUTH %s %s", context->username, context->passwd);
-		} else {
-			reply = redisCommand(context->rctx, "AUTH %s", context->passwd);
-		}
-		if ((reply == NULL) || (reply->type == REDIS_REPLY_ERROR))
-			oidc_error(r, "Redis AUTH command (%s:%d) failed: '%s' [%s]", context->host_str, context->port,
-				   context->rctx->errstr, reply ? reply->str : "<n/a>");
-		else
-			oidc_debug(r, "successfully authenticated to the Redis server: %s",
-				   reply ? reply->str : "<n/a>");
-
-		/* free the auth answer */
-		oidc_cache_redis_reply_free(&reply);
-	}
+	oidc_cache_redis_reply_free(&reply);
 
-	/* see if we need to set the database */
-	if (context->database != -1) {
-		reply = redisCommand(context->rctx, "SELECT %d", context->database);
-		if ((reply == NULL) || (reply->type == REDIS_REPLY_ERROR))
-			oidc_error(r, "Redis SELECT command (%s:%d) failed: '%s' [%s]", context->host_str,
-				   context->port, context->rctx->errstr, reply ? reply->str : "<n/a>");
-		else
-			oidc_debug(r, "successfully selected database %d on the Redis server: %s", context->database,
-				   reply ? reply->str : "<n/a>");
-
-		/* free the database answer */
-		oidc_cache_redis_reply_free(&reply);
+	return rv;
+}
+
+apr_byte_t oidc_cache_redis_set_database(request_rec *r, redisContext *rctx, const int database) {
+	apr_byte_t rv = TRUE;
+	redisReply *reply = NULL;
+
+	if (database == -1)
+		goto end;
+
+	reply = redisCommand(rctx, "SELECT %d", database);
+	if ((reply == NULL) || (reply->type == REDIS_REPLY_ERROR)) {
+		oidc_error(r, "Redis SELECT command failed: '%s' [%s]", rctx->errstr, reply ? reply->str : "<n/a>");
+		rv = FALSE;
+		goto end;
 	}
 
+	oidc_debug(r, "successfully selected database %d on the Redis server: %s", database,
+		   reply ? reply->str : "<n/a>");
+
 end:
 
-	return (context->rctx != NULL) ? APR_SUCCESS : APR_EGENERAL;
+	oidc_cache_redis_reply_free(&reply);
+
+	return rv;
+}
+
+redisContext *oidc_cache_redis_connect_with_timeout(request_rec *r, const char *host, int port, struct timeval ct,
+						    struct timeval t, const char *msg) {
+	redisContext *rctx = NULL;
+
+	oidc_debug(r, "calling redisConnectWithTimeout: %d", (int)ct.tv_sec);
+	rctx = redisConnectWithTimeout(host, port, ct);
+
+	if ((rctx == NULL) || (rctx->err != 0)) {
+		oidc_error(r, "failed to connect to Redis server (%s%s%s:%d): '%s'", msg ? msg : "", msg ? ":" : "",
+			   host, port, rctx != NULL ? rctx->errstr : "");
+		if (rctx)
+			redisFree(rctx);
+		return NULL;
+	}
+
+	oidc_debug(r, "successfully connected to Redis server (%s%s%s:%d)", msg ? msg : "", msg ? ":" : "", host, port);
+
+	if (redisSetTimeout(rctx, t) != REDIS_OK)
+		oidc_error(r, "redisSetTimeout failed: %s", rctx->errstr);
+
+	return rctx;
+}
+
+/*
+ * connect to Redis server
+ */
+static apr_status_t oidc_cache_redis_connect(request_rec *r, oidc_cache_cfg_redis_t *context) {
+
+	if (context->rctx != NULL)
+		return APR_SUCCESS;
+
+	context->rctx = oidc_cache_redis_connect_with_timeout(r, context->host_str, context->port,
+							      context->connect_timeout, context->timeout, NULL);
+	if (context->rctx == NULL)
+		return APR_EGENERAL;
+
+	oidc_cache_redis_set_keepalive(r, context->rctx, context->keepalive);
+	oidc_cache_redis_set_auth(r, context->rctx, context->username, context->passwd);
+	oidc_cache_redis_set_database(r, context->rctx, context->database);
+
+	return APR_SUCCESS;
 }
 
 redisReply *oidc_cache_redis_command(request_rec *r, oidc_cache_cfg_redis_t *context, char **errstr, const char *format,

src/cache/redis.h

@@ -60,6 +60,7 @@ typedef struct oidc_cache_cfg_redis_t {
 	char *passwd;
 	int database;
 	struct timeval connect_timeout;
+	int keepalive;
 	struct timeval timeout;
 	char *host_str;
 	apr_port_t port;
@@ -77,3 +78,9 @@ apr_byte_t oidc_cache_redis_get(request_rec *r, const char *section, const char
 apr_byte_t oidc_cache_redis_set(request_rec *r, const char *section, const char *key, const char *value,
 				apr_time_t expiry);
 apr_status_t oidc_cache_redis_disconnect(oidc_cache_cfg_redis_t *context);
+
+apr_byte_t oidc_cache_redis_set_keepalive(request_rec *r, redisContext *rctx, const int keepalive);
+apr_byte_t oidc_cache_redis_set_auth(request_rec *r, redisContext *rctx, const char *username, const char *password);
+apr_byte_t oidc_cache_redis_set_database(request_rec *r, redisContext *rctx, const int database);
+redisContext *oidc_cache_redis_connect_with_timeout(request_rec *r, const char *host, int port, struct timeval ct,
+						    struct timeval t, const char *msg);

src/config.c

@@ -1334,6 +1334,17 @@ static const char *oidc_set_token_revocation_endpoint(cmd_parms *cmd, void *stru
 	return oidc_set_https_slot(cmd, struct_ptr, args);
 }
 
+static const char *oidc_set_redis_connect_timeout(cmd_parms *cmd, void *struct_ptr, const char *arg1,
+						  const char *arg2) {
+	oidc_cfg *cfg = (oidc_cfg *)ap_get_module_config(cmd->server->module_config, &auth_openidc_module);
+	const char *rv = NULL;
+	if (arg1)
+		rv = oidc_parse_redis_connect_timeout(cmd->pool, arg1, &cfg->cache_redis_connect_timeout);
+	if ((rv == NULL) && (arg2))
+		rv = oidc_parse_redis_keepalive(cmd->pool, arg2, &cfg->cache_redis_keepalive);
+	return OIDC_CONFIG_DIR_RV(cmd, rv);
+}
+
 int oidc_cfg_dir_refresh_access_token_before_expiry(request_rec *r) {
 	oidc_dir_cfg *dir_cfg = ap_get_module_config(r->per_dir_config, &auth_openidc_module);
 	if (dir_cfg->refresh_access_token_before_expiry == OIDC_CONFIG_POS_INT_UNSET)
@@ -1621,6 +1632,7 @@ void *oidc_create_server_config(apr_pool_t *pool, server_rec *svr) {
 	c->cache_redis_password = NULL;
 	c->cache_redis_database = -1;
 	c->cache_redis_connect_timeout = -1;
+	c->cache_redis_keepalive = -1;
 	c->cache_redis_timeout = -1;
 #endif
 
@@ -1860,6 +1872,8 @@ void *oidc_merge_server_config(apr_pool_t *pool, void *BASE, void *ADD) {
 	    add->cache_redis_database != -1 ? add->cache_redis_database : base->cache_redis_database;
 	c->cache_redis_connect_timeout = add->cache_redis_connect_timeout != -1 ? add->cache_redis_connect_timeout
 										: base->cache_redis_connect_timeout;
+	c->cache_redis_keepalive =
+	    add->cache_redis_keepalive != -1 ? add->cache_redis_keepalive : base->cache_redis_keepalive;
 	c->cache_redis_timeout = add->cache_redis_timeout != -1 ? add->cache_redis_timeout : base->cache_redis_timeout;
 #endif
 
@@ -3297,8 +3311,8 @@ const command_rec oidc_config_cmds[] = {
 				(void*)APR_OFFSETOF(oidc_cfg, cache_redis_database),
 				RSRC_CONF,
 				"Database for the Redis servers."),
-		AP_INIT_TAKE1(OIDCRedisCacheConnectTimeout,
-				oidc_set_int_slot,
+		AP_INIT_TAKE2(OIDCRedisCacheConnectTimeout,
+				oidc_set_redis_connect_timeout,
 				(void*)APR_OFFSETOF(oidc_cfg, cache_redis_connect_timeout),
 				RSRC_CONF,
 				"Timeout for connecting to the Redis servers."),

src/mod_auth_openidc.h

@@ -489,6 +489,7 @@ typedef struct oidc_cfg {
 	char *cache_redis_password;
 	int cache_redis_database;
 	int cache_redis_connect_timeout;
+	int cache_redis_keepalive;
 	int cache_redis_timeout;
 #endif
 	int cache_encrypt;

src/parse.c

@@ -1286,3 +1286,18 @@ const char *oidc_parse_outgoing_proxy_auth_type(apr_pool_t *pool, const char *ar
 
 	return NULL;
 }
+
+#define OIDC_REDIS_CONNECT_TIMEOUT_MIN 1
+#define OIDC_REDIS_CONNECT_TIMEOUT_MAX 3600
+
+const char *oidc_parse_redis_connect_timeout(apr_pool_t *pool, const char *arg, int *v) {
+	return oidc_parse_int_min_max(pool, arg, v, OIDC_REDIS_CONNECT_TIMEOUT_MIN, OIDC_REDIS_CONNECT_TIMEOUT_MAX);
+}
+
+// NB: zero for turning off TCP keepalive, which is enabled by default
+#define OIDC_REDIS_KEEPALIVE_TIMEOUT_MIN 0
+#define OIDC_REDIS_KEEPALIVE_TIMEOUT_MAX 3600
+
+const char *oidc_parse_redis_keepalive(apr_pool_t *pool, const char *arg, int *v) {
+	return oidc_parse_int_min_max(pool, arg, v, OIDC_REDIS_KEEPALIVE_TIMEOUT_MIN, OIDC_REDIS_KEEPALIVE_TIMEOUT_MAX);
+}

src/parse.h

@@ -130,6 +130,8 @@ const char *oidc_parse_set_state_input_headers_as(apr_pool_t *pool, const char *
 const char *oidc_parse_x_forwarded_headers(apr_pool_t *pool, const char *arg, apr_byte_t *x_forwarded_headers);
 const char *oidc_parse_outgoing_proxy_auth_type(apr_pool_t *pool, const char *arg, unsigned long *auth_type);
 const char *oidc_parse_trace_parent(apr_pool_t *pool, const char *arg, int *trace_parent);
+const char *oidc_parse_redis_connect_timeout(apr_pool_t *pool, const char *arg, int *int_value);
+const char *oidc_parse_redis_keepalive(apr_pool_t *pool, const char *arg, int *int_value);
 
 typedef const char *(*oidc_valid_int_function_t)(apr_pool_t *, int);
 typedef const char *(*oidc_valid_function_t)(apr_pool_t *, const char *);