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.

CHANGELOG.md

@@ -172,3 +172,4 @@
 * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGauges` will no longer return 0 if some tenant doesn't have the gauge in their registry, and other tenants have negative values only. #368
 * [BUGFIX] `MetricFamiliesPerTenant.SendMaxOfGaugesPerTenant` no longer includes tenants, which do not have specified gauge. #368
 * [BUGFIX] Correctly format `host:port` addresses when using IPv6. #388
+* [BUGFIX] TCPTransport will reject BindAddr entries that are not an IP address, like "localhost" (previously, these would listen on 0.0.0.0) #396

kv/memberlist/memberlist_client_test.go

@@ -234,14 +234,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}
 
@@ -543,8 +557,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
 	}
 
@@ -826,6 +841,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) {
 				}
 			}()
 
+			localhostBindAddrsOnce.Do(setLocalhostBindAddrs)
 			for i, port := range ports {
 				id := fmt.Sprintf("Member-%d", i)
 				var cfg KVConfig
@@ -842,7 +858,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) {
 				cfg.AbortIfJoinFails = true
 
 				cfg.TCPTransport = TCPTransportConfig{
-					BindAddrs: []string{"localhost"},
+					BindAddrs: localhostBindAddrs,
 					BindPort:  port,
 				}
 
@@ -926,12 +942,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{net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))}
+	cfg.JoinMembers = []string{net.JoinHostPort(localhostIP.String(), strconv.Itoa(ports[0]))}
 
 	cfg.Codecs = []codec.Codec{c}
 
@@ -950,8 +967,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
 		}
@@ -990,14 +1008,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{net.JoinHostPort("127.0.0.1", strconv.Itoa(portToConnect))})
+				_, err := kv.kv.JoinMembers([]string{net.JoinHostPort(localhostIP.String(), strconv.Itoa(portToConnect))})
 				if err != nil {
 					return fmt.Errorf("%s failed to join the cluster: %f", name, err)
 				}
@@ -1098,8 +1117,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
 	}
 
@@ -1189,8 +1209,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],
 	}
 
@@ -1408,8 +1429,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

@@ -39,7 +39,7 @@ const colonColon = "::"
 
 // TCPTransportConfig is a configuration structure for creating new TCPTransport.
 type TCPTransportConfig struct {
-	// BindAddrs is a list of addresses to bind to.
+	// BindAddrs is a list of IP addresses to bind to.
 	BindAddrs flagext.StringSlice `yaml:"bind_addr"`
 
 	// BindPort is the port to listen on, for each address above.
@@ -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,
 		},
 	}