Validate NodeNames, Address and Tags fields (#612)

cmd/serf/command/agent/config.go

@@ -227,6 +227,11 @@ type Config struct {
 	// 5 seconds.
 	BroadcastTimeoutRaw string        `mapstructure:"broadcast_timeout"`
 	BroadcastTimeout    time.Duration `mapstructure:"-"`
+
+	// ValidateNodeNames controls whether nodenames only
+	// contain alphanumeric, dashes and '.'characters
+	// and sets maximum length to 128 characters
+	ValidateNodeNames bool `mapstructure:"validate_node_names"`
 }
 
 // BindAddrParts returns the parts of the BindAddr that should be
@@ -363,7 +368,6 @@ func containsKey(keys []string, key string) bool {
 func MergeConfig(a, b *Config) *Config {
 	var result Config = *a
 
-	// Copy the strings if they're set
 	if b.NodeName != "" {
 		result.NodeName = b.NodeName
 	}

serf/config.go

@@ -253,6 +253,11 @@ type Config struct {
 	//
 	// WARNING: this should ONLY be used in tests
 	messageDropper func(typ messageType) bool
+
+	// ValidateNodeNames controls whether nodenames only
+	// contain alphanumeric, dashes and '.'characters
+	// and sets maximum length to 128 characters
+	ValidateNodeNames bool
 }
 
 // Init allocates the subdata structures
@@ -298,6 +303,7 @@ func DefaultConfig() *Config {
 		QuerySizeLimit:               1024,
 		EnableNameConflictResolution: true,
 		DisableCoordinates:           false,
+		ValidateNodeNames:            false,
 		UserEventSizeLimit:           512,
 	}
 }

serf/delegate_test.go

@@ -223,3 +223,26 @@ func TestDelegate_MergeRemoteState(t *testing.T) {
 		t.Fatalf("bad query clock")
 	}
 }
+
+func TestDelegate_BadData(t *testing.T) {
+	ip1, returnFn1 := testutil.TakeIP()
+	defer returnFn1()
+
+	c := testConfig(t, ip1)
+	c.ProtocolVersion = 3
+	c.Tags["role"] = "test"
+	d := &delegate{&Serf{config: c}}
+	meta := d.NodeMeta(32)
+
+	out := d.serf.decodeTags(meta)
+	if out["role"] != "test" {
+		t.Fatalf("bad meta data: %v", meta)
+	}
+
+	defer func() {
+		if r := recover(); r == nil {
+			t.Fatalf("expected panic")
+		}
+	}()
+	d.NodeMeta(1)
+}

serf/merge_delegate.go

@@ -1,7 +1,9 @@
 package serf
 
 import (
+	"fmt"
 	"net"
+	"strconv"
 
 	"github.com/hashicorp/memberlist"
 )
@@ -17,22 +19,31 @@ type mergeDelegate struct {
 func (m *mergeDelegate) NotifyMerge(nodes []*memberlist.Node) error {
 	members := make([]*Member, len(nodes))
 	for idx, n := range nodes {
-		members[idx] = m.nodeToMember(n)
+		var err error
+		members[idx], err = m.nodeToMember(n)
+		if err != nil {
+			return err
+		}
 	}
 	return m.serf.config.Merge.NotifyMerge(members)
 }
 
 func (m *mergeDelegate) NotifyAlive(peer *memberlist.Node) error {
-	member := m.nodeToMember(peer)
+	member, err := m.nodeToMember(peer)
+	if err != nil {
+		return err
+	}
 	return m.serf.config.Merge.NotifyMerge([]*Member{member})
 }
 
-func (m *mergeDelegate) nodeToMember(n *memberlist.Node) *Member {
+func (m *mergeDelegate) nodeToMember(n *memberlist.Node) (*Member, error) {
 	status := StatusNone
 	if n.State == memberlist.StateLeft {
 		status = StatusLeft
 	}
-
+	if err := m.validiateMemberInfo(n); err != nil {
+		return nil, err
+	}
 	return &Member{
 		Name:        n.Name,
 		Addr:        net.IP(n.Addr),
@@ -45,5 +56,36 @@ func (m *mergeDelegate) nodeToMember(n *memberlist.Node) *Member {
 		DelegateMin: n.DMin,
 		DelegateMax: n.DMax,
 		DelegateCur: n.DCur,
+	}, nil
+}
+
+// validateMemberInfo checks that the data we are sending is valid
+func (m *mergeDelegate) validiateMemberInfo(n *memberlist.Node) error {
+	if err := m.serf.ValidateNodeNames(); err != nil {
+		return err
+	}
+
+	host, port, err := net.SplitHostPort(string(n.Addr))
+	if err != nil {
+		return err
+	}
+
+	ip := net.ParseIP(host)
+	if ip == nil || (ip.To4() == nil && ip.To16() == nil) {
+		return fmt.Errorf("%v is not a valid IPv4 or 1Pv6 address\n", ip)
+	}
+
+	p, err := strconv.Atoi(port)
+	if err != nil {
+		return err
+	}
+	if p < 0 || p > 65535 {
+		return fmt.Errorf("invalid port %v , port must be a valid number from 0-65535", p)
+	}
+
+	if len(n.Meta) > memberlist.MetaMaxSize {
+		return fmt.Errorf("Encoded length of tags exceeds limit of %d bytes",
+			memberlist.MetaMaxSize)
 	}
+	return nil
 }

serf/serf.go

@@ -11,6 +11,7 @@ import (
 	"math/rand"
 	"net"
 	"os"
+	"regexp"
 	"strconv"
 	"sync"
 	"sync/atomic"
@@ -36,6 +37,8 @@ const (
 	tagMagicByte uint8 = 255
 )
 
+const MaxNodeNameLength int = 128
+
 var (
 	// FeatureNotSupported is returned if a feature cannot be used
 	// due to an older protocol version being used.
@@ -269,6 +272,9 @@ func Create(conf *Config) (*Serf, error) {
 	if len(serf.encodeTags(conf.Tags)) > memberlist.MetaMaxSize {
 		return nil, fmt.Errorf("Encoded length of tags exceeds limit of %d bytes", memberlist.MetaMaxSize)
 	}
+	if err := serf.ValidateNodeNames(); err != nil {
+		return nil, err
+	}
 
 	// Check if serf member event coalescing is enabled
 	if conf.CoalescePeriod > 0 && conf.QuiescentPeriod > 0 && conf.EventCh != nil {
@@ -1884,3 +1890,20 @@ func (s *Serf) NumNodes() (numNodes int) {
 
 	return numNodes
 }
+
+// ValidateNodeNames verifies the NodeName contains
+// only alphanumeric, -, or . and is under 128 chracters
+func (s *Serf) ValidateNodeNames() error {
+	if s.config.ValidateNodeNames {
+		var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\-\.]+`)
+		if InvalidNameRe.MatchString(s.config.NodeName) {
+			return fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+
+				"all alpha-numerics and dashes and '.' ", s.config.NodeName)
+		}
+		if len(s.config.NodeName) > MaxNodeNameLength {
+			return fmt.Errorf("NodeName is %v characters. "+
+				"Valid length is between 1 and 128 characters", len(s.config.NodeName))
+		}
+	}
+	return nil
+}

serf/serf_test.go

@@ -2971,3 +2971,31 @@ func waitUntilIntentQueueLen(t *testing.T, desiredLen int, serfs ...*Serf) {
 		}
 	})
 }
+
+func TestSerf_ValidateNodeName(t *testing.T) {
+	type test struct {
+		nodename string
+		want     string
+	}
+
+	tests := []test{
+		{
+			nodename: "!BadChars&^*",
+			want:     "invalid characters",
+		},
+		{
+			nodename: "thisisonehundredandtwentyeightcharacterslongnodenametestaswehavetotesteachcaseeventheoonesonehundredandtwentyeightcharacterslong1",
+			want:     "Valid length",
+		},
+	}
+	for _, tc := range tests {
+		agentConfig := DefaultConfig()
+		agentConfig.NodeName = tc.nodename
+		agentConfig.ValidateNodeNames = true
+		_, err := Create(agentConfig)
+		if !strings.Contains(err.Error(), tc.want) {
+			t.Fatalf("expected: %v, got: %v", tc.want, err)
+		}
+	}
+
+}