From d41443bd3d87adfb32dc28ee02d3aa81e515e255 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 16 Sep 2019 17:15:34 +0300 Subject: [PATCH 1/3] Fix: redisReconnect() should clear SSL context. We should not attempt to keep the context and re-establish the TLS connection for several reasons: 1. Maintain symmetry between redisConnect() and redisReconnect(), so in both cases an extra step is required to initiate SSL. 2. The caller may also wish to reconfigure the SSL session and needs a chance to do that. 3. It is not a practical thing to do on an async non-blocking connection context. --- hiredis.c | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/hiredis.c b/hiredis.c index 0658e34..c461131 100644 --- a/hiredis.c +++ b/hiredis.c @@ -708,6 +708,11 @@ int redisReconnect(redisContext *c) { c->err = 0; memset(c->errstr, '\0', strlen(c->errstr)); + if (c->privdata && c->funcs->free_privdata) { + c->funcs->free_privdata(c->privdata); + c->privdata = NULL; + } + redisNetClose(c); sdsfree(c->obuf); From a1e538092d79425232588c249f636c4c9d810ec4 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 16 Sep 2019 17:16:44 +0300 Subject: [PATCH 2/3] Make SSL timeout error compatible with rest. --- ssl.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/ssl.c b/ssl.c index 418ece6..78ab9e4 100644 --- a/ssl.c +++ b/ssl.c @@ -337,7 +337,7 @@ static int redisSSLRead(redisContext *c, char *buf, size_t bufcap) { } else { const char *msg = NULL; if (errno == EAGAIN) { - msg = "Timed out"; + msg = "Resource temporarily unavailable"; } __redisSetError(c, REDIS_ERR_IO, msg); return -1; From d952ed877ab1e515fcf5d16ec69576b87be61796 Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Mon, 16 Sep 2019 17:27:43 +0300 Subject: [PATCH 3/3] Add SSL mode tests. This repeats all existing tests in SSL mode, but does not yet provide SSL-specific tests. --- Makefile | 19 +++++++++----- test.c | 80 +++++++++++++++++++++++++++++++++++++++++++++++++++++--- test.sh | 49 ++++++++++++++++++++++++++++++++-- 3 files changed, 136 insertions(+), 12 deletions(-) diff --git a/Makefile b/Makefile index f992639..25ac154 100644 --- a/Makefile +++ b/Makefile @@ -65,6 +65,12 @@ STLIB_MAKE_CMD=$(AR) rcs uname_S := $(shell sh -c 'uname -s 2>/dev/null || echo not') USE_SSL?=0 + +# This is required for test.c only +ifeq ($(USE_SSL),1) + CFLAGS+=-DHIREDIS_TEST_SSL +endif + ifeq ($(uname_S),Linux) SSL_LDFLAGS=-lssl -lcrypto else @@ -176,19 +182,20 @@ hiredis-example: examples/example.c $(STLIBNAME) examples: $(EXAMPLES) -hiredis-test: test.o $(STLIBNAME) +TEST_LIBS = $(STLIBNAME) +ifeq ($(USE_SSL),1) + TEST_LIBS += $(SSL_STLIBNAME) -lssl -lcrypto -lpthread +endif +hiredis-test: test.o $(TEST_LIBS) hiredis-%: %.o $(STLIBNAME) - $(CC) $(REAL_CFLAGS) -o $@ $< $(STLIBNAME) $(REAL_LDFLAGS) + $(CC) $(REAL_CFLAGS) -o $@ $< $(TEST_LIBS) $(REAL_LDFLAGS) test: hiredis-test ./hiredis-test check: hiredis-test - @echo "$$REDIS_TEST_CONFIG" | $(REDIS_SERVER) - - $(PRE) ./hiredis-test -h 127.0.0.1 -p $(REDIS_PORT) -s /tmp/hiredis-test-redis.sock || \ - ( kill `cat /tmp/hiredis-test-redis.pid` && false ) - kill `cat /tmp/hiredis-test-redis.pid` + TEST_SSL=$(USE_SSL) ./test.sh .c.o: $(CC) -std=c99 -pedantic -c $(REAL_CFLAGS) $< diff --git a/test.c b/test.c index a35b984..8668e18 100644 --- a/test.c +++ b/test.c @@ -13,12 +13,16 @@ #include #include "hiredis.h" +#ifdef HIREDIS_TEST_SSL +#include "hiredis_ssl.h" +#endif #include "net.h" enum connection_type { CONN_TCP, CONN_UNIX, - CONN_FD + CONN_FD, + CONN_SSL }; struct config { @@ -33,6 +37,14 @@ struct config { struct { const char *path; } unix_sock; + + struct { + const char *host; + int port; + const char *ca_cert; + const char *cert; + const char *key; + } ssl; }; /* The following lines make up our testing "framework" :) */ @@ -93,11 +105,27 @@ static int disconnect(redisContext *c, int keep_fd) { return -1; } +static void do_ssl_handshake(redisContext *c, struct config config) { +#ifdef HIREDIS_TEST_SSL + redisSecureConnection(c, config.ssl.ca_cert, config.ssl.cert, config.ssl.key, NULL); + if (c->err) { + printf("SSL error: %s\n", c->errstr); + redisFree(c); + exit(1); + } +#else + (void) c; + (void) config; +#endif +} + static redisContext *do_connect(struct config config) { redisContext *c = NULL; if (config.type == CONN_TCP) { c = redisConnect(config.tcp.host, config.tcp.port); + } else if (config.type == CONN_SSL) { + c = redisConnect(config.ssl.host, config.ssl.port); } else if (config.type == CONN_UNIX) { c = redisConnectUnix(config.unix_sock.path); } else if (config.type == CONN_FD) { @@ -121,9 +149,21 @@ static redisContext *do_connect(struct config config) { exit(1); } + if (config.type == CONN_SSL) { + do_ssl_handshake(c, config); + } + return select_database(c); } +static void do_reconnect(redisContext *c, struct config config) { + redisReconnect(c); + + if (config.type == CONN_SSL) { + do_ssl_handshake(c, config); + } +} + static void test_format_commands(void) { char *cmd; int len; @@ -575,7 +615,8 @@ static void test_blocking_connection_timeouts(struct config config) { c = do_connect(config); test("Does not return a reply when the command times out: "); - s = write(c->fd, cmd, strlen(cmd)); + redisAppendFormattedCommand(c, cmd, strlen(cmd)); + s = c->funcs->write(c); tv.tv_sec = 0; tv.tv_usec = 10000; redisSetTimeout(c, tv); @@ -584,7 +625,7 @@ static void test_blocking_connection_timeouts(struct config config) { freeReplyObject(reply); test("Reconnect properly reconnects after a timeout: "); - redisReconnect(c); + do_reconnect(c, config); reply = redisCommand(c, "PING"); test_cond(reply != NULL && reply->type == REDIS_REPLY_STATUS && strcmp(reply->str, "PONG") == 0); freeReplyObject(reply); @@ -592,7 +633,7 @@ static void test_blocking_connection_timeouts(struct config config) { test("Reconnect properly uses owned parameters: "); config.tcp.host = "foo"; config.unix_sock.path = "foo"; - redisReconnect(c); + do_reconnect(c, config); reply = redisCommand(c, "PING"); test_cond(reply != NULL && reply->type == REDIS_REPLY_STATUS && strcmp(reply->str, "PONG") == 0); freeReplyObject(reply); @@ -895,6 +936,23 @@ int main(int argc, char **argv) { throughput = 0; } else if (argc >= 1 && !strcmp(argv[0],"--skip-inherit-fd")) { test_inherit_fd = 0; +#ifdef HIREDIS_TEST_SSL + } else if (argc >= 2 && !strcmp(argv[0],"--ssl-port")) { + argv++; argc--; + cfg.ssl.port = atoi(argv[0]); + } else if (argc >= 2 && !strcmp(argv[0],"--ssl-host")) { + argv++; argc--; + cfg.ssl.host = argv[0]; + } else if (argc >= 2 && !strcmp(argv[0],"--ssl-ca-cert")) { + argv++; argc--; + cfg.ssl.ca_cert = argv[0]; + } else if (argc >= 2 && !strcmp(argv[0],"--ssl-cert")) { + argv++; argc--; + cfg.ssl.cert = argv[0]; + } else if (argc >= 2 && !strcmp(argv[0],"--ssl-key")) { + argv++; argc--; + cfg.ssl.key = argv[0]; +#endif } else { fprintf(stderr, "Invalid argument: %s\n", argv[0]); exit(1); @@ -923,6 +981,20 @@ int main(int argc, char **argv) { test_blocking_io_errors(cfg); if (throughput) test_throughput(cfg); +#ifdef HIREDIS_TEST_SSL + if (cfg.ssl.port && cfg.ssl.host) { + printf("\nTesting against SSL connection (%s:%d):\n", cfg.ssl.host, cfg.ssl.port); + cfg.type = CONN_SSL; + + test_blocking_connection(cfg); + test_blocking_connection_timeouts(cfg); + test_blocking_io_errors(cfg); + test_invalid_timeout_errors(cfg); + test_append_formatted_commands(cfg); + if (throughput) test_throughput(cfg); + } +#endif + if (test_inherit_fd) { printf("\nTesting against inherited fd (%s):\n", cfg.unix_sock.path); cfg.type = CONN_FD; diff --git a/test.sh b/test.sh index 9a507ae..2cab9e6 100755 --- a/test.sh +++ b/test.sh @@ -2,11 +2,44 @@ REDIS_SERVER=${REDIS_SERVER:-redis-server} REDIS_PORT=${REDIS_PORT:-56379} +REDIS_SSL_PORT=${REDIS_SSL_PORT:-56443} +TEST_SSL=${TEST_SSL:-0} +SSL_TEST_ARGS= tmpdir=$(mktemp -d) PID_FILE=${tmpdir}/hiredis-test-redis.pid SOCK_FILE=${tmpdir}/hiredis-test-redis.sock +if [ "$TEST_SSL" = "1" ]; then + SSL_CA_CERT=${tmpdir}/ca.crt + SSL_CA_KEY=${tmpdir}/ca.key + SSL_CERT=${tmpdir}/redis.crt + SSL_KEY=${tmpdir}/redis.key + + openssl genrsa -out ${tmpdir}/ca.key 4096 + openssl req \ + -x509 -new -nodes -sha256 \ + -key ${SSL_CA_KEY} \ + -days 3650 \ + -subj '/CN=Hiredis Test CA' \ + -out ${SSL_CA_CERT} + openssl genrsa -out ${SSL_KEY} 2048 + openssl req \ + -new -sha256 \ + -key ${SSL_KEY} \ + -subj '/CN=Hiredis Test Cert' | \ + openssl x509 \ + -req -sha256 \ + -CA ${SSL_CA_CERT} \ + -CAkey ${SSL_CA_KEY} \ + -CAserial ${tmpdir}/ca.txt \ + -CAcreateserial \ + -days 365 \ + -out ${SSL_CERT} + + SSL_TEST_ARGS="--ssl-host 127.0.0.1 --ssl-port ${REDIS_SSL_PORT} --ssl-ca-cert ${SSL_CA_CERT} --ssl-cert ${SSL_CERT} --ssl-key ${SSL_KEY}" +fi + cleanup() { set +e kill $(cat ${PID_FILE}) @@ -14,7 +47,7 @@ cleanup() { } trap cleanup INT TERM EXIT -${REDIS_SERVER} - < ${tmpdir}/redis.conf <> ${tmpdir}/redis.conf <