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

@@ -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);
     }

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. */
+    /* Compare server.bindaddr with CONFIG_DEFAULT_BINDADDR */
-    if (server.bindaddr_count == 0) {
+    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 <addr1> <addr2> ... <addrN> */
+    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);

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))
     {

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 we have no bind address, we don't listen on a TCP socket */
-    if (server.bindaddr_count == 0) {
+    if (server.bindaddr_count == 0) return C_OK;
-        bindaddr_count = 2;
-        bindaddr = default_bindaddr;
-    }
 
-    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++;

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 */

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
+}

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
         }
     }
+    if {$reconnect} {
         reconnect $level
     }
+}

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]
+        }
+    }
+}