From 07b0d144cee7e3ef0ed34fbf046c418d66099cea Mon Sep 17 00:00:00 2001 From: Yossi Gottlieb Date: Tue, 22 Jun 2021 12:50:17 +0300 Subject: [PATCH] Improve bind and protected-mode config handling. (#9034) * Specifying an empty `bind ""` configuration prevents Redis from listening on any TCP port. Before this commit, such configuration was not accepted. * Using `CONFIG GET bind` will always return an explicit configuration value. Before this commit, if a bind address was not specified the returned value was empty (which was an anomaly). Another behavior change is that modifying the `bind` configuration to a non-default value will NO LONGER DISABLE protected-mode implicitly. --- src/cluster.c | 4 ++ src/config.c | 27 +++++++++++-- src/networking.c | 1 - src/server.c | 16 ++++---- src/server.h | 2 + tests/support/cli.tcl | 17 ++++++++ tests/support/server.tcl | 6 ++- tests/unit/networking.tcl | 81 +++++++++++++++++++++++++++++++++++++++ 8 files changed, 138 insertions(+), 16 deletions(-) diff --git a/src/cluster.c b/src/cluster.c index 7612ab91d..16c2e4b56 100644 --- a/src/cluster.c +++ b/src/cluster.c @@ -561,6 +561,10 @@ void clusterInit(void) { "Your Redis port number must be 55535 or less."); exit(1); } + if (!server.bindaddr_count) { + serverLog(LL_WARNING, "No bind address is configured, but it is required for the Cluster bus."); + exit(1); + } if (listenToPort(port+CLUSTER_PORT_INCR, &server.cfd) == C_ERR) { exit(1); } diff --git a/src/config.c b/src/config.c index c3adab39a..45dc4046b 100644 --- a/src/config.c +++ b/src/config.c @@ -453,6 +453,10 @@ void loadServerConfigFromString(char *config) { if (addresses > CONFIG_BINDADDR_MAX) { err = "Too many bind addresses specified"; goto loaderr; } + + /* A single empty argument is treated as a zero bindaddr count */ + if (addresses == 1 && sdslen(argv[1]) == 0) addresses = 0; + /* Free old bind addresses */ for (j = 0; j < server.bindaddr_count; j++) { zfree(server.bindaddr[j]); @@ -785,7 +789,7 @@ void configSetCommand(client *c) { int vlen; sds *v = sdssplitlen(o->ptr,sdslen(o->ptr)," ",1,&vlen); - if (vlen < 1 || vlen > CONFIG_BINDADDR_MAX) { + if (vlen > CONFIG_BINDADDR_MAX) { addReplyError(c, "Too many bind addresses specified."); sdsfreesplitres(v, vlen); return; @@ -1564,15 +1568,30 @@ void rewriteConfigBindOption(struct rewriteConfigState *state) { int force = 1; sds line, addresses; char *option = "bind"; + int is_default = 0; - /* Nothing to rewrite if we don't have bind addresses. */ - if (server.bindaddr_count == 0) { + /* Compare server.bindaddr with CONFIG_DEFAULT_BINDADDR */ + if (server.bindaddr_count == CONFIG_DEFAULT_BINDADDR_COUNT) { + is_default = 1; + char *default_bindaddr[CONFIG_DEFAULT_BINDADDR_COUNT] = CONFIG_DEFAULT_BINDADDR; + for (int j = 0; j < CONFIG_DEFAULT_BINDADDR_COUNT; j++) { + if (strcmp(server.bindaddr[j], default_bindaddr[j]) != 0) { + is_default = 0; + break; + } + } + } + + if (is_default) { rewriteConfigMarkAsProcessed(state,option); return; } /* Rewrite as bind ... */ - addresses = sdsjoin(server.bindaddr,server.bindaddr_count," "); + if (server.bindaddr_count > 0) + addresses = sdsjoin(server.bindaddr,server.bindaddr_count," "); + else + addresses = sdsnew("\"\""); line = sdsnew(option); line = sdscatlen(line, " ", 1); line = sdscatsds(line, addresses); diff --git a/src/networking.c b/src/networking.c index b76ef7d08..a0cc3123b 100644 --- a/src/networking.c +++ b/src/networking.c @@ -992,7 +992,6 @@ void clientAcceptHandler(connection *conn) { * requests from non loopback interfaces. Instead we try to explain the * user what to do to fix it if needed. */ if (server.protected_mode && - server.bindaddr_count == 0 && DefaultUser->flags & USER_FLAG_NOPASS && !(c->flags & CLIENT_UNIX_SOCKET)) { diff --git a/src/server.c b/src/server.c index 85f79f914..2376c68fa 100644 --- a/src/server.c +++ b/src/server.c @@ -2622,6 +2622,7 @@ void createSharedObjects(void) { void initServerConfig(void) { int j; + char *default_bindaddr[CONFIG_DEFAULT_BINDADDR_COUNT] = CONFIG_DEFAULT_BINDADDR; updateCachedTime(1); getRandomHexChars(server.runid,CONFIG_RUN_ID_SIZE); @@ -2636,7 +2637,9 @@ void initServerConfig(void) { server.configfile = NULL; server.executable = NULL; server.arch_bits = (sizeof(long) == 8) ? 64 : 32; - server.bindaddr_count = 0; + server.bindaddr_count = CONFIG_DEFAULT_BINDADDR_COUNT; + for (j = 0; j < CONFIG_DEFAULT_BINDADDR_COUNT; j++) + server.bindaddr[j] = zstrdup(default_bindaddr[j]); server.unixsocketperm = CONFIG_DEFAULT_UNIX_SOCKET_PERM; server.ipfd.count = 0; server.tlsfd.count = 0; @@ -3028,16 +3031,11 @@ int createSocketAcceptHandler(socketFds *sfd, aeFileProc *accept_handler) { int listenToPort(int port, socketFds *sfd) { int j; char **bindaddr = server.bindaddr; - int bindaddr_count = server.bindaddr_count; - char *default_bindaddr[2] = {"*", "-::*"}; - /* Force binding of 0.0.0.0 if no bind address is specified. */ - if (server.bindaddr_count == 0) { - bindaddr_count = 2; - bindaddr = default_bindaddr; - } + /* If we have no bind address, we don't listen on a TCP socket */ + if (server.bindaddr_count == 0) return C_OK; - for (j = 0; j < bindaddr_count; j++) { + for (j = 0; j < server.bindaddr_count; j++) { char* addr = bindaddr[j]; int optional = *addr == '-'; if (optional) addr++; diff --git a/src/server.h b/src/server.h index 73db4dc20..8e5edd76a 100644 --- a/src/server.h +++ b/src/server.h @@ -111,6 +111,8 @@ typedef long long ustime_t; /* microsecond time type. */ #define CONFIG_DEFAULT_CLUSTER_CONFIG_FILE "nodes.conf" #define CONFIG_DEFAULT_UNIX_SOCKET_PERM 0 #define CONFIG_DEFAULT_LOGFILE "" +#define CONFIG_DEFAULT_BINDADDR_COUNT 2 +#define CONFIG_DEFAULT_BINDADDR { "*", "-::*" } #define NET_HOST_STR_LEN 256 /* Longest valid hostname */ #define NET_IP_STR_LEN 46 /* INET6_ADDRSTRLEN is 46, but we need to be sure */ #define NET_ADDR_STR_LEN (NET_IP_STR_LEN+32) /* Must be enough for ip:port */ diff --git a/tests/support/cli.tcl b/tests/support/cli.tcl index 19e306e24..a080823eb 100644 --- a/tests/support/cli.tcl +++ b/tests/support/cli.tcl @@ -11,9 +11,26 @@ proc rediscli_tls_config {testsdir} { } } +# Returns command line for executing redis-cli proc rediscli {host port {opts {}}} { set cmd [list src/redis-cli -h $host -p $port] lappend cmd {*}[rediscli_tls_config "tests"] lappend cmd {*}$opts return $cmd } + +# Returns command line for executing redis-cli with a unix socket address +proc rediscli_unixsocket {unixsocket {opts {}}} { + return [list src/redis-cli -s $unixsocket {*}$opts] +} + +# Run redis-cli with specified args on the server of specified level. +# Returns output broken down into individual lines. +proc rediscli_exec {level args} { + set cmd [rediscli_unixsocket [srv $level unixsocket] $args] + set fd [open "|$cmd" "r"] + set ret [lrange [split [read $fd] "\n"] 0 end-1] + close $fd + + return $ret +} diff --git a/tests/support/server.tcl b/tests/support/server.tcl index 54b0e4ed0..39a0dd065 100644 --- a/tests/support/server.tcl +++ b/tests/support/server.tcl @@ -626,7 +626,7 @@ proc start_server {options {code undefined}} { } } -proc restart_server {level wait_ready rotate_logs} { +proc restart_server {level wait_ready rotate_logs {reconnect 1}} { set srv [lindex $::servers end+$level] kill_server $srv @@ -668,5 +668,7 @@ proc restart_server {level wait_ready rotate_logs} { after 10 } } - reconnect $level + if {$reconnect} { + reconnect $level + } } diff --git a/tests/unit/networking.tcl b/tests/unit/networking.tcl index afd72c56b..045824183 100644 --- a/tests/unit/networking.tcl +++ b/tests/unit/networking.tcl @@ -1,3 +1,5 @@ +source tests/support/cli.tcl + test {CONFIG SET port number} { start_server {} { if {$::tls} { set port_cfg tls-port} else { set port_cfg port } @@ -34,3 +36,82 @@ test {CONFIG SET bind address} { $rd close } } {} {external:skip} + +start_server {config "minimal.conf" tags {"external:skip"}} { + test {Default bind address configuration handling} { + # Default is explicit and sane + assert_equal "* -::*" [lindex [r CONFIG GET bind] 1] + + # CONFIG REWRITE acknowledges this as a default + r CONFIG REWRITE + assert_equal 0 [count_message_lines [srv 0 config_file] bind] + + # Removing the bind address works + r CONFIG SET bind "" + assert_equal "" [lindex [r CONFIG GET bind] 1] + + # No additional clients can connect + catch {redis_client} err + assert_match {*connection refused*} $err + + # CONFIG REWRITE handles empty bindaddr + r CONFIG REWRITE + assert_equal 1 [count_message_lines [srv 0 config_file] bind] + + # Make sure we're able to restart + restart_server 0 0 0 0 + + # Make sure bind parameter is as expected and server handles binding + # accordingly. + assert_equal {bind {}} [rediscli_exec 0 config get bind] + catch {reconnect 0} err + assert_match {*connection refused*} $err + + assert_equal {OK} [rediscli_exec 0 config set bind *] + reconnect 0 + r ping + } {PONG} + + proc get_nonloopback_addr {} { + set addrlist [list {}] + catch { set addrlist [exec hostname -I] } + return [lindex $addrlist 0] + } + + proc get_nonloopback_client {} { + return [redis [get_nonloopback_addr] [srv 0 "port"] 0 $::tls] + } + + test {Protected mode works as expected} { + # Get a non-loopback address of this instance for this test. + set myaddr [get_nonloopback_addr] + if {$myaddr != "" && ![string match {127.*} $myaddr]} { + # Non-loopback client shoudl fail by default + set r2 [get_nonloopback_client] + catch {$r2 ping} err + assert_match {*DENIED*} $err + + # Bind configuration should not matter + assert_equal {OK} [r config set bind "*"] + set r2 [get_nonloopback_client] + catch {$r2 ping} err + assert_match {*DENIED*} $err + + # Setting a password should disable protected mode + assert_equal {OK} [r config set requirepass "secret"] + set r2 [redis $myaddr [srv 0 "port"] 0 $::tls] + assert_equal {OK} [$r2 auth secret] + assert_equal {PONG} [$r2 ping] + + # Clearing the password re-enables protected mode + assert_equal {OK} [r config set requirepass ""] + set r2 [redis $myaddr [srv 0 "port"] 0 $::tls] + assert_match {*DENIED*} $err + + # Explicitly disabling protected-mode works + assert_equal {OK} [r config set protected-mode no] + set r2 [redis $myaddr [srv 0 "port"] 0 $::tls] + assert_equal {PONG} [$r2 ping] + } + } +}