kv/memberlist: fix incorrect TCP transport host parsing (#396)

* 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 #381.

* kv/memberlist: add test for IP address rejection

* Update CHANGELOG.md

Co-authored-by: Charles Korn <[email protected]>

* fix more tests

* rework sync.Once usage

* use require instead of manual tests

* require.EqualError

* require.Error no longer necessary

---------

Co-authored-by: Charles Korn <[email protected]>

CHANGELOG.md

@@ -173,3 +173,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] Memberlist's TCP transport will now reject bind addresses that are not IP addresses, such as "localhost", rather than silently converting these to 0.0.0.0 and therefore listening on all addresses. #396

kv/memberlist/http_status_handler_test.go

@@ -11,6 +11,7 @@ import (
 
 func TestPage(t *testing.T) {
 	conf := memberlist.DefaultLANConfig()
+	conf.BindAddr = getLocalhostAddr()
 	ml, err := memberlist.Create(conf)
 	require.NoError(t, err)
 

kv/memberlist/memberlist_client_test.go

@@ -234,14 +234,32 @@ func casWithErr(ctx context.Context, kv *Client, key string, updateFn func(*data
 	return kv.CAS(ctx, key, fn)
 }
 
+func getLocalhostAddr() string {
+	return getLocalhostAddrs()[0]
+}
+
+var addrsOnce sync.Once
+var localhostIP string
+
+func getLocalhostAddrs() []string {
+	addrsOnce.Do(func() {
+		ip, err := net.ResolveIPAddr("ip4", "localhost")
+		if err != nil {
+			localhostIP = "127.0.0.1" // this is the most common answer, try it
+		}
+		localhostIP = ip.String()
+	})
+	return []string{localhostIP}
+}
+
 func TestBasicGetAndCas(t *testing.T) {
 	c := dataCodec{}
 
 	name := "Ing 1"
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 	}
 	cfg.Codecs = []codec.Codec{c}
 
@@ -302,7 +320,9 @@ func withFixtures(t *testing.T, testFN func(t *testing.T, kv *Client)) {
 
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
-	cfg.TCPTransport = TCPTransportConfig{}
+	cfg.TCPTransport = TCPTransportConfig{
+		BindAddrs: getLocalhostAddrs(),
+	}
 	cfg.Codecs = []codec.Codec{c}
 
 	mkv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry())
@@ -456,6 +476,9 @@ func TestMultipleCAS(t *testing.T) {
 
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
+	cfg.TCPTransport = TCPTransportConfig{
+		BindAddrs: getLocalhostAddrs(),
+	}
 	cfg.Codecs = []codec.Codec{c}
 
 	mkv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry())
@@ -544,7 +567,7 @@ func defaultKVConfig(i int) KVConfig {
 	cfg.PushPullInterval = 5 * time.Second
 
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 		BindPort:  0, // randomize ports
 	}
 
@@ -842,7 +865,7 @@ func TestJoinMembersWithRetryBackoff(t *testing.T) {
 				cfg.AbortIfJoinFails = true
 
 				cfg.TCPTransport = TCPTransportConfig{
-					BindAddrs: []string{"localhost"},
+					BindAddrs: getLocalhostAddrs(),
 					BindPort:  port,
 				}
 
@@ -927,11 +950,11 @@ func TestMemberlistFailsToJoin(t *testing.T) {
 	cfg.AbortIfJoinFails = true
 
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 		BindPort:  0,
 	}
 
-	cfg.JoinMembers = []string{net.JoinHostPort("127.0.0.1", strconv.Itoa(ports[0]))}
+	cfg.JoinMembers = []string{net.JoinHostPort(getLocalhostAddr(), strconv.Itoa(ports[0]))}
 
 	cfg.Codecs = []codec.Codec{c}
 
@@ -951,7 +974,7 @@ func TestMemberlistFailsToJoin(t *testing.T) {
 func getFreePorts(count int) ([]int, error) {
 	var ports []int
 	for i := 0; i < count; i++ {
-		addr, err := net.ResolveTCPAddr("tcp", "127.0.0.1:0")
+		addr, err := net.ResolveTCPAddr("tcp", net.JoinHostPort(getLocalhostAddr(), "0"))
 		if err != nil {
 			return nil, err
 		}
@@ -997,7 +1020,7 @@ func runClient(kv *Client, name string, ringKey string, portToConnect int, casIn
 
 			// 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(getLocalhostAddr(), strconv.Itoa(portToConnect))})
 				if err != nil {
 					return fmt.Errorf("%s failed to join the cluster: %f", name, err)
 				}
@@ -1099,7 +1122,7 @@ func TestMultipleCodecs(t *testing.T) {
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 		BindPort:  0, // randomize
 	}
 
@@ -1190,7 +1213,7 @@ func TestRejoin(t *testing.T) {
 	var cfg1 KVConfig
 	flagext.DefaultValues(&cfg1)
 	cfg1.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 		BindPort:  ports[0],
 	}
 
@@ -1251,7 +1274,11 @@ func TestMessageBuffer(t *testing.T) {
 func TestNotifyMsgResendsOnlyChanges(t *testing.T) {
 	codec := dataCodec{}
 
-	cfg := KVConfig{}
+	cfg := KVConfig{
+		TCPTransport: TCPTransportConfig{
+			BindAddrs: getLocalhostAddrs(),
+		},
+	}
 	// We will be checking for number of messages in the broadcast queue, so make sure to use known retransmit factor.
 	cfg.RetransmitMult = 1
 	cfg.Codecs = append(cfg.Codecs, codec)
@@ -1316,7 +1343,11 @@ func TestNotifyMsgResendsOnlyChanges(t *testing.T) {
 func TestSendingOldTombstoneShouldNotForwardMessage(t *testing.T) {
 	codec := dataCodec{}
 
-	cfg := KVConfig{}
+	cfg := KVConfig{
+		TCPTransport: TCPTransportConfig{
+			BindAddrs: getLocalhostAddrs(),
+		},
+	}
 	// We will be checking for number of messages in the broadcast queue, so make sure to use known retransmit factor.
 	cfg.RetransmitMult = 1
 	cfg.LeftIngestersTimeout = 5 * time.Minute
@@ -1409,7 +1440,7 @@ func TestFastJoin(t *testing.T) {
 	var cfg KVConfig
 	flagext.DefaultValues(&cfg)
 	cfg.TCPTransport = TCPTransportConfig{
-		BindAddrs: []string{"localhost"},
+		BindAddrs: getLocalhostAddrs(),
 		BindPort:  0, // randomize
 	}
 
@@ -1459,6 +1490,9 @@ func TestDelegateMethodsDontCrashBeforeKVStarts(t *testing.T) {
 
 	cfg := KVConfig{}
 	cfg.Codecs = append(cfg.Codecs, codec)
+	cfg.TCPTransport = TCPTransportConfig{
+		BindAddrs: getLocalhostAddrs(),
+	}
 
 	kv := NewKV(cfg, log.NewNopLogger(), &dnsProviderMock{}, prometheus.NewPedanticRegistry())
 

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,
 		},
 	}
@@ -95,3 +95,9 @@ func TestFinalAdvertiseAddr(t *testing.T) {
 		})
 	}
 }
+
+func TestNonIPsAreRejected(t *testing.T) {
+	cfg := TCPTransportConfig{BindAddrs: flagext.StringSlice{"localhost"}}
+	_, err := NewTCPTransport(cfg, nil, nil)
+	require.EqualError(t, err, `could not parse bind addr "localhost" as IP address`)
+}