kv/memberlist: fix incorrect TCP transport host parsing

Previously, if you passed "localhost" as the bind address for a
TCPTransport, we would attempt to parse this as an IP address,
fail, and then begin listening on 0.0.0.0.

This is a security issue since 0.0.0.0 binds to all interfaces,
including public ones, while "localhost" is a shortcut for an IP
address that is typically is only accessible from the local machine,
not the wider Internet.

Fix this by returning an error if you attempt to pass a BindAddr to
TCPTransport that is not actually an IP address. Also, fix the tests
to resolve "localhost" to an IP address before proceeding - typically,
but not always, this is 127.0.0.1, which is why we try to parse the
loopback address instead of hardcoding it.

I can confirm that on a Mac, with this patch applied I no longer get
dialog boxes warning me that the tests are attempting to listen on
0.0.0.0.

Updates grafana#381.

kv/memberlist/memberlist_client_test.go

@@ -233,14 +233,28 @@ func casWithErr(ctx context.Context, kv *Client, key string, updateFn func(*data
 	return kv.CAS(ctx, key, fn)
 }
 
+var localhostIP *net.IPAddr
+var localhostBindAddrs []string
+var localhostBindAddrsOnce sync.Once
+
+func setLocalhostBindAddrs() {
+	var err error
+	localhostIP, err = net.ResolveIPAddr("ip4", "localhost")
+	if err != nil {
+		localhostBindAddrs = []string{"127.0.0.1"}
+	}
+	localhostBindAddrs = []string{localhostIP.String()}
+}
+
 func TestBasicGetAndCas(t *testing.T) {
 	c := dataCodec{}
 
 	name := "Ing 1"
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 	}
 	cfg.Codecs = []codec.Codec{c}
 
@@ -542,8 +556,9 @@ func defaultKVConfig(i int) KVConfig {
 	cfg.GossipNodes = 10
 	cfg.PushPullInterval = 5 * time.Second
 
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 		BindPort:  0, // randomize ports
 	}
 
@@ -825,6 +840,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) {
 				}
 			}()
 
+			localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 			for i, port := range ports {
 				id := fmt.Sprintf("Member-%d", i)
 				var cfg KVConfig
@@ -841,7 +857,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) {
 				cfg.AbortIfJoinFails = true
 
 				cfg.TCPTransport = TCPTransportConfig{
-					BindAddrs: []string{"localhost"},
+					BindAddrs: localhostBindAddrs,
 					BindPort:  port,
 				}
 
@@ -925,12 +941,13 @@ func TestMemberlistFailsToJoin(t *testing.T) {
 	cfg.MaxJoinRetries = 2
 	cfg.AbortIfJoinFails = true
 
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 		BindPort:  0,
 	}
 
-	cfg.JoinMembers = []string{fmt.Sprintf("127.0.0.1:%d", ports[0])}
+	cfg.JoinMembers = []string{localhostIP.String() + fmt.Sprintf("%d", ports[0])}
 
 	cfg.Codecs = []codec.Codec{c}
 
@@ -949,8 +966,9 @@ func TestMemberlistFailsToJoin(t *testing.T) {
 
 func getFreePorts(count int) ([]int, error) {
 	var ports []int
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	for i := 0; i < count; i++ {
-		addr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0")
+		addr, err := net.ResolveTCPAddr("tcp", localhostIP.String()+":0")
 		if err != nil {
 			return nil, err
 		}
@@ -989,14 +1007,15 @@ func runClient(kv *Client, name string, ringKey string, portToConnect int, casIn
 	// stop gossipping about the ring(s)
 	defer services.StopAndAwaitTerminated(context.Background(), kv.kv) //nolint:errcheck
 
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	for {
 		select {
 		case <-start:
 			start = nil
 
 			// let's join the first member
 			if portToConnect > 0 {
-				_, err := kv.kv.JoinMembers([]string{fmt.Sprintf("127.0.0.1:%d", portToConnect)})
+				_, err := kv.kv.JoinMembers([]string{localhostIP.String() + fmt.Sprintf(":%d", portToConnect)})
 				if err != nil {
 					return fmt.Errorf("%s failed to join the cluster: %f", name, err)
 				}
@@ -1097,8 +1116,9 @@ var _ codec.Codec = &distributedCounterCodec{}
 func TestMultipleCodecs(t *testing.T) {
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 		BindPort:  0, // randomize
 	}
 
@@ -1188,8 +1208,9 @@ func TestRejoin(t *testing.T) {
 
 	var cfg1 KVConfig
 	flagext.DefaultValues(&cfg1)
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg1.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 		BindPort:  ports[0],
 	}
 
@@ -1407,8 +1428,9 @@ func TestSendingOldTombstoneShouldNotForwardMessage(t *testing.T) {
 func TestFastJoin(t *testing.T) {
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
+	localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: localhostBindAddrs,
 		BindPort:  0, // randomize
 	}
 

kv/memberlist/tcp_transport.go

@@ -147,6 +147,9 @@ func NewTCPTransport(config TCPTransportConfig, logger log.Logger, registerer pr
 	port := config.BindPort
 	for _, addr := range config.BindAddrs {
 		ip := net.ParseIP(addr)
+		if ip == nil {
+			return nil, fmt.Errorf("could not parse bind addr %q as IP address", addr)
+		}
 
 		tcpAddr := &net.TCPAddr{IP: ip, Port: port}
 

kv/memberlist/tcp_transport_test.go

@@ -20,14 +20,14 @@ func TestTCPTransport_WriteTo_ShouldNotLogAsWarningExpectedFailures(t *testing.T
 		unexpectedLogs string
 	}{
 		"should not log 'connection refused' by default": {
-			remoteAddr:     "localhost:12345",
+			remoteAddr:     "127.0.0.1:12345",
 			unexpectedLogs: "connection refused",
 		},
 		"should log 'connection refused' if debug log level is enabled": {
 			setup: func(t *testing.T, cfg *TCPTransportConfig) {
 				cfg.TransportDebug = true
 			},
-			remoteAddr:   "localhost:12345",
+			remoteAddr:   "127.0.0.1:12345",
 			expectedLogs: "connection refused",
 		},
 	}
@@ -39,7 +39,7 @@ func TestTCPTransport_WriteTo_ShouldNotLogAsWarningExpectedFailures(t *testing.T
 
 			cfg := TCPTransportConfig{}
 			flagext.DefaultValues(&cfg)
-			cfg.BindAddrs = []string{"localhost"}
+			cfg.BindAddrs = []string{"127.0.0.1"}
 			cfg.BindPort = 0
 			if testData.setup != nil {
 				testData.setup(t, &cfg)
@@ -69,7 +69,7 @@ func TestFinalAdvertiseAddr(t *testing.T) {
 	}{
 		"should not fail with local address specified": {
 			advertiseAddr: "127.0.0.1",
-			bindAddrs:     []string{"localhost"},
+			bindAddrs:     []string{"127.0.0.1"},
 			bindPort:      0,
 		},
 	}