From 73ccaa4c53ed8bf33b17ecbf093891acf59c7991 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Thu, 20 Aug 2020 12:39:12 -0500 Subject: [PATCH 01/12] wip 1 --- cmd/serf/command/agent/config.go | 20 ++++++++++++++++++++ cmd/serf/command/agent/config_test.go | 13 +++++++++++++ serf/config.go | 12 ++++++++++++ 3 files changed, 45 insertions(+) diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index 5be733a06..2d3db441d 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -8,6 +8,7 @@ import ( "net" "os" "path/filepath" + "regexp" "sort" "strings" "time" @@ -344,6 +345,23 @@ func DecodeConfig(r io.Reader) (*Config, error) { result.BroadcastTimeout = dur } + //TODO(schristoff): this will prolly break some stuff, add config flag for this check? + if result.NodeName != "" { + var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + if InvalidNameRe.MatchString(result.NodeName) { + err = fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ + "all alpha-numerics and dashes.", result.NodeName) + return nil, err + } + + MaxLength := 63 + if len(result.NodeName) > MaxLength { + err = fmt.Errorf("NodeName is %v characters. "+ + "Valid length is between 1 and 63 characters", len(result.NodeName)) + return nil, err + } + } + return &result, nil } @@ -364,6 +382,8 @@ func MergeConfig(a, b *Config) *Config { var result Config = *a // Copy the strings if they're set + //TODO(schristoff): do i need to check nodename + //validatity here? if b.NodeName != "" { result.NodeName = b.NodeName } diff --git a/cmd/serf/command/agent/config_test.go b/cmd/serf/command/agent/config_test.go index 7aa62064f..b9e894834 100644 --- a/cmd/serf/command/agent/config_test.go +++ b/cmd/serf/command/agent/config_test.go @@ -585,3 +585,16 @@ func TestReadConfigPaths_dir(t *testing.T) { t.Fatalf("bad: %#v", config) } } + +func TestBadNodeName(t *testing.T) { + input := `{"node_name": "9b07b1d05b25d78a3f55d0ccde714f02 + 96230786636dd72ded4c731746517331 + 394b395a0524b495d40fdd20ad9e536c + d15c8a7872ca8617226f02c175dfc0ff + dff8012a7c25e608e4775107fd2a3aeb"}` + _, err := DecodeConfig(bytes.NewReader([]byte(input))) + if err == nil { + t.Fatalf("should have err") + } + +} diff --git a/serf/config.go b/serf/config.go index 2875e1d60..b33a38f13 100644 --- a/serf/config.go +++ b/serf/config.go @@ -1,9 +1,11 @@ package serf import ( + "fmt" "io" "log" "os" + "regexp" "time" "github.com/hashicorp/memberlist" @@ -271,6 +273,16 @@ func (c *Config) Init() { // for most of the configurations. func DefaultConfig() *Config { hostname, err := os.Hostname() + //TODO(schristoff): is it safe to add this here? also should be flag + if len(hostname) > 63 { + err = fmt.Errorf("Hostname is %v characters. Valid length is between 1 and 63 characters", len(hostname)) + } + + var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + if InvalidNameRe.MatchString(hostname) { + err = fmt.Errorf("Hostname contains invalid characters %v , Valid characters include all "+ + "all alpha-numerics and dashes.", hostname) + } if err != nil { panic(err) } From e9d7462c9e24b9fbcf75417df3f99e4bc2cb661a Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Thu, 20 Aug 2020 15:21:47 -0500 Subject: [PATCH 02/12] wip validateMemberInfo --- cmd/serf/command/agent/config.go | 11 ++++++--- serf/config.go | 4 +-- serf/merge_delegate.go | 42 +++++++++++++++++++++++++++++--- 3 files changed, 48 insertions(+), 9 deletions(-) diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index 2d3db441d..816165b0a 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -228,6 +228,11 @@ type Config struct { // 5 seconds. BroadcastTimeoutRaw string `mapstructure:"broadcast_timeout"` BroadcastTimeout time.Duration `mapstructure:"-"` + + //TODO(schristoff): better var name? :grimance: + //NodeNameValid specifies whether or not nodenames should + // be alphanumeric and within 128 characters + NodeNameValid bool `mapstructure:NodeNamevalid` } // BindAddrParts returns the parts of the BindAddr that should be @@ -354,10 +359,10 @@ func DecodeConfig(r io.Reader) (*Config, error) { return nil, err } - MaxLength := 63 + MaxLength := 128 if len(result.NodeName) > MaxLength { err = fmt.Errorf("NodeName is %v characters. "+ - "Valid length is between 1 and 63 characters", len(result.NodeName)) + "Valid length is between 1 and 128 characters", len(result.NodeName)) return nil, err } } @@ -381,9 +386,9 @@ func containsKey(keys []string, key string) bool { func MergeConfig(a, b *Config) *Config { var result Config = *a - // Copy the strings if they're set //TODO(schristoff): do i need to check nodename //validatity here? + if b.NodeName != "" { result.NodeName = b.NodeName } diff --git a/serf/config.go b/serf/config.go index b33a38f13..127528aec 100644 --- a/serf/config.go +++ b/serf/config.go @@ -274,8 +274,8 @@ func (c *Config) Init() { func DefaultConfig() *Config { hostname, err := os.Hostname() //TODO(schristoff): is it safe to add this here? also should be flag - if len(hostname) > 63 { - err = fmt.Errorf("Hostname is %v characters. Valid length is between 1 and 63 characters", len(hostname)) + if len(hostname) > 128 { + err = fmt.Errorf("Hostname is %v characters. Valid length is between 1 and 128 characters", len(hostname)) } var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) diff --git a/serf/merge_delegate.go b/serf/merge_delegate.go index 36eb52f7d..b2ab166d9 100644 --- a/serf/merge_delegate.go +++ b/serf/merge_delegate.go @@ -1,7 +1,9 @@ package serf import ( + "fmt" "net" + "regexp" "github.com/hashicorp/memberlist" ) @@ -15,24 +17,33 @@ type mergeDelegate struct { } func (m *mergeDelegate) NotifyMerge(nodes []*memberlist.Node) error { + var err error members := make([]*Member, len(nodes)) for idx, n := range nodes { - members[idx] = m.nodeToMember(n) + 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,28 @@ 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 { + var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + + if len(n.Name) > 128 { + return fmt.Errorf("NodeName length is %v characters. Valid length is between "+ + "1 and 128 characters.", len(n.Name)) + } + if InvalidNameRe.MatchString(n.Name) { + return fmt.Errorf("Nodename contains invalid characters %v , Valid characters include "+ + "all alpha-numerics and dashes", n.Name) + } + + if net.ParseIP(string(n.Addr)) == nil { + return fmt.Errorf("Address is %v . Must be a valid representation of an IP address. ", n.Addr) + } + if len(n.Meta) > memberlist.MetaMaxSize { + return fmt.Errorf("Encoded length of tags exceeds limit of %d bytes", + memberlist.MetaMaxSize) } + return nil } From e40dcb98dc436c7ca96fa5d1ca57ab2ec0c51a3f Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Thu, 20 Aug 2020 15:53:29 -0500 Subject: [PATCH 03/12] minor updates --- cmd/serf/command/agent/config.go | 12 +++++------- serf/config.go | 12 ------------ serf/merge_delegate.go | 2 +- 3 files changed, 6 insertions(+), 20 deletions(-) diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index 816165b0a..36fb89754 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -19,6 +19,7 @@ import ( // This is the default port that we use for Serf communication const DefaultBindPort int = 7946 +const MaxNodeName int = 128 // DefaultConfig contains the defaults for configurations. func DefaultConfig() *Config { @@ -229,10 +230,9 @@ type Config struct { BroadcastTimeoutRaw string `mapstructure:"broadcast_timeout"` BroadcastTimeout time.Duration `mapstructure:"-"` - //TODO(schristoff): better var name? :grimance: - //NodeNameValid specifies whether or not nodenames should + //ValidateNodeNames specifies whether or not nodenames should // be alphanumeric and within 128 characters - NodeNameValid bool `mapstructure:NodeNamevalid` + ValidateNodeNames bool `mapstructure:validate_node_names` } // BindAddrParts returns the parts of the BindAddr that should be @@ -350,8 +350,7 @@ func DecodeConfig(r io.Reader) (*Config, error) { result.BroadcastTimeout = dur } - //TODO(schristoff): this will prolly break some stuff, add config flag for this check? - if result.NodeName != "" { + if result.NodeName != "" && result.ValidateNodeNames { var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) if InvalidNameRe.MatchString(result.NodeName) { err = fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ @@ -359,8 +358,7 @@ func DecodeConfig(r io.Reader) (*Config, error) { return nil, err } - MaxLength := 128 - if len(result.NodeName) > MaxLength { + if len(result.NodeName) > MaxNodeName { err = fmt.Errorf("NodeName is %v characters. "+ "Valid length is between 1 and 128 characters", len(result.NodeName)) return nil, err diff --git a/serf/config.go b/serf/config.go index 127528aec..2875e1d60 100644 --- a/serf/config.go +++ b/serf/config.go @@ -1,11 +1,9 @@ package serf import ( - "fmt" "io" "log" "os" - "regexp" "time" "github.com/hashicorp/memberlist" @@ -273,16 +271,6 @@ func (c *Config) Init() { // for most of the configurations. func DefaultConfig() *Config { hostname, err := os.Hostname() - //TODO(schristoff): is it safe to add this here? also should be flag - if len(hostname) > 128 { - err = fmt.Errorf("Hostname is %v characters. Valid length is between 1 and 128 characters", len(hostname)) - } - - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - if InvalidNameRe.MatchString(hostname) { - err = fmt.Errorf("Hostname contains invalid characters %v , Valid characters include all "+ - "all alpha-numerics and dashes.", hostname) - } if err != nil { panic(err) } diff --git a/serf/merge_delegate.go b/serf/merge_delegate.go index b2ab166d9..e5802aa66 100644 --- a/serf/merge_delegate.go +++ b/serf/merge_delegate.go @@ -17,9 +17,9 @@ type mergeDelegate struct { } func (m *mergeDelegate) NotifyMerge(nodes []*memberlist.Node) error { - var err error members := make([]*Member, len(nodes)) for idx, n := range nodes { + var err error members[idx], err = m.nodeToMember(n) if err != nil { return err From e91297a5dcb5db72ff4ba093cccf78bac69be693 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Thu, 20 Aug 2020 17:17:15 -0500 Subject: [PATCH 04/12] more config update and questions --- serf/config.go | 22 ++++++++++++++++++++++ serf/merge_delegate.go | 16 +++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/serf/config.go b/serf/config.go index 2875e1d60..62c4423b5 100644 --- a/serf/config.go +++ b/serf/config.go @@ -1,9 +1,11 @@ package serf import ( + "fmt" "io" "log" "os" + "regexp" "time" "github.com/hashicorp/memberlist" @@ -14,6 +16,8 @@ import ( // our own protocol version. var ProtocolVersionMap map[uint8]uint8 +const MaxNodeName int = 128 + func init() { ProtocolVersionMap = map[uint8]uint8{ 5: 2, @@ -253,6 +257,11 @@ type Config struct { // // WARNING: this should ONLY be used in tests messageDropper func(typ messageType) bool + + //ValidateNodeNames specifies whether or not nodenames should + // be alphanumeric and within 128 characters + //TODO(schristoff): should this be here? + ValidateNodeNames bool } // Init allocates the subdata structures @@ -275,6 +284,18 @@ func DefaultConfig() *Config { panic(err) } + //Do we need to do this here if it is set to true in the config? + if len(hostname) > MaxNodeName { + //TODO(schristoff): panic seems a little harsh here? + panic(fmt.Errorf("NodeName is %v characters. "+ + "Valid length is between 1 and 128 characters", len(hostname))) + } + var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + if InvalidNameRe.MatchString(hostname) { + panic(fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ + "all alpha-numerics and dashes.", hostname)) + } + return &Config{ NodeName: hostname, BroadcastTimeout: 5 * time.Second, @@ -298,6 +319,7 @@ func DefaultConfig() *Config { QuerySizeLimit: 1024, EnableNameConflictResolution: true, DisableCoordinates: false, + ValidateNodeNames: true, UserEventSizeLimit: 512, } } diff --git a/serf/merge_delegate.go b/serf/merge_delegate.go index e5802aa66..cbd8d2ad8 100644 --- a/serf/merge_delegate.go +++ b/serf/merge_delegate.go @@ -63,13 +63,15 @@ func (m *mergeDelegate) nodeToMember(n *memberlist.Node) (*Member, error) { func (m *mergeDelegate) validiateMemberInfo(n *memberlist.Node) error { var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - if len(n.Name) > 128 { - return fmt.Errorf("NodeName length is %v characters. Valid length is between "+ - "1 and 128 characters.", len(n.Name)) - } - if InvalidNameRe.MatchString(n.Name) { - return fmt.Errorf("Nodename contains invalid characters %v , Valid characters include "+ - "all alpha-numerics and dashes", n.Name) + if m.serf.config.ValidateNodeNames { + if len(n.Name) > 128 { + return fmt.Errorf("NodeName length is %v characters. Valid length is between "+ + "1 and 128 characters.", len(n.Name)) + } + if InvalidNameRe.MatchString(n.Name) { + return fmt.Errorf("Nodename contains invalid characters %v , Valid characters include "+ + "all alpha-numerics and dashes", n.Name) + } } if net.ParseIP(string(n.Addr)) == nil { From 5fcfacf9215152b5a8996314018205c8fccc883a Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Sun, 23 Aug 2020 21:45:54 -0500 Subject: [PATCH 05/12] add regex for ip:port add tests --- cmd/serf/command/agent/agent.go | 13 +++++++++++++ cmd/serf/command/agent/agent_test.go | 27 +++++++++++++++++++++++++++ cmd/serf/command/agent/config.go | 3 +-- serf/config.go | 19 ++----------------- serf/merge_delegate.go | 5 +++-- 5 files changed, 46 insertions(+), 21 deletions(-) diff --git a/cmd/serf/command/agent/agent.go b/cmd/serf/command/agent/agent.go index c35d13e36..ca16c2322 100644 --- a/cmd/serf/command/agent/agent.go +++ b/cmd/serf/command/agent/agent.go @@ -8,6 +8,7 @@ import ( "io/ioutil" "log" "os" + "regexp" "strings" "sync" @@ -85,6 +86,18 @@ func Create(agentConf *Config, conf *serf.Config, logOutput io.Writer) (*Agent, } } + if agentConf.ValidateNodeNames { + var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + if InvalidNameRe.MatchString(agentConf.NodeName) { + return nil, fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ + "all alpha-numerics and dashes.", agentConf.NodeName) + } + if len(agentConf.NodeName) > serf.MaxNodeNameLength { + return nil, fmt.Errorf("NodeName is %v characters. "+ + "Valid length is between 1 and 128 characters", len(agentConf.NodeName)) + } + } + return agent, nil } diff --git a/cmd/serf/command/agent/agent_test.go b/cmd/serf/command/agent/agent_test.go index 7f6e65558..e1dd198f1 100644 --- a/cmd/serf/command/agent/agent_test.go +++ b/cmd/serf/command/agent/agent_test.go @@ -318,3 +318,30 @@ func TestAgentKeyringFile_NoKeys(t *testing.T) { t.Fatalf("bad: %s", err) } } +func TestAgentCreate_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, serf.DefaultConfig(), nil) + if !strings.Contains(err.Error(), tc.want) { + t.Fatalf("expected: %v, got: %v", tc.want, err) + } + } + +} diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index 36fb89754..c4649fc7d 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -19,7 +19,6 @@ import ( // This is the default port that we use for Serf communication const DefaultBindPort int = 7946 -const MaxNodeName int = 128 // DefaultConfig contains the defaults for configurations. func DefaultConfig() *Config { @@ -358,7 +357,7 @@ func DecodeConfig(r io.Reader) (*Config, error) { return nil, err } - if len(result.NodeName) > MaxNodeName { + if len(result.NodeName) > serf.MaxNodeNameLength { err = fmt.Errorf("NodeName is %v characters. "+ "Valid length is between 1 and 128 characters", len(result.NodeName)) return nil, err diff --git a/serf/config.go b/serf/config.go index 62c4423b5..dc397cfbb 100644 --- a/serf/config.go +++ b/serf/config.go @@ -1,23 +1,21 @@ package serf import ( - "fmt" "io" "log" "os" - "regexp" "time" "github.com/hashicorp/memberlist" ) +const MaxNodeNameLength int = 128 + // ProtocolVersionMap is the mapping of Serf delegate protocol versions // to memberlist protocol versions. We mask the memberlist protocols using // our own protocol version. var ProtocolVersionMap map[uint8]uint8 -const MaxNodeName int = 128 - func init() { ProtocolVersionMap = map[uint8]uint8{ 5: 2, @@ -260,7 +258,6 @@ type Config struct { //ValidateNodeNames specifies whether or not nodenames should // be alphanumeric and within 128 characters - //TODO(schristoff): should this be here? ValidateNodeNames bool } @@ -284,18 +281,6 @@ func DefaultConfig() *Config { panic(err) } - //Do we need to do this here if it is set to true in the config? - if len(hostname) > MaxNodeName { - //TODO(schristoff): panic seems a little harsh here? - panic(fmt.Errorf("NodeName is %v characters. "+ - "Valid length is between 1 and 128 characters", len(hostname))) - } - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - if InvalidNameRe.MatchString(hostname) { - panic(fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ - "all alpha-numerics and dashes.", hostname)) - } - return &Config{ NodeName: hostname, BroadcastTimeout: 5 * time.Second, diff --git a/serf/merge_delegate.go b/serf/merge_delegate.go index cbd8d2ad8..17a0ef323 100644 --- a/serf/merge_delegate.go +++ b/serf/merge_delegate.go @@ -62,7 +62,8 @@ func (m *mergeDelegate) nodeToMember(n *memberlist.Node) (*Member, error) { // validateMemberInfo checks that the data we are sending is valid func (m *mergeDelegate) validiateMemberInfo(n *memberlist.Node) error { var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - + var InvalidIPv4Re = regexp.MustCompile(`([0-9]{1,3}\.){3}[0-9]{1,3}:[0-9]+`) + var InvalidIPv6Re = regexp.MustCompile(`([0-9a-f]{1,4}:){7}[0-9a-f]{1,4}`) if m.serf.config.ValidateNodeNames { if len(n.Name) > 128 { return fmt.Errorf("NodeName length is %v characters. Valid length is between "+ @@ -74,7 +75,7 @@ func (m *mergeDelegate) validiateMemberInfo(n *memberlist.Node) error { } } - if net.ParseIP(string(n.Addr)) == nil { + if InvalidIPv4Re.MatchString(string(n.Addr)) || InvalidIPv6Re.MatchString(string(n.Addr)) { return fmt.Errorf("Address is %v . Must be a valid representation of an IP address. ", n.Addr) } if len(n.Meta) > memberlist.MetaMaxSize { From dbbe2ca83e3b94c1d107d9ed8c97ed8289400746 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Sun, 23 Aug 2020 21:54:19 -0500 Subject: [PATCH 06/12] appease go vet --- cmd/serf/command/agent/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index c4649fc7d..7d2e57254 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -231,7 +231,7 @@ type Config struct { //ValidateNodeNames specifies whether or not nodenames should // be alphanumeric and within 128 characters - ValidateNodeNames bool `mapstructure:validate_node_names` + ValidateNodeNames bool `mapstructure:"validate_node_names"` } // BindAddrParts returns the parts of the BindAddr that should be From ffd9c1e081aedab4a30bf764c592f6e68aba7740 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 14:37:57 -0500 Subject: [PATCH 07/12] make validatenodenames() --- cmd/serf/command/agent/agent.go | 13 ---------- cmd/serf/command/agent/config.go | 16 ------------ cmd/serf/command/agent/config_test.go | 13 ---------- serf/config.go | 2 -- serf/delegate_test.go | 23 ++++++++++++++++++ serf/merge_delegate.go | 35 +++++++++++++++------------ serf/serf.go | 23 ++++++++++++++++++ 7 files changed, 66 insertions(+), 59 deletions(-) diff --git a/cmd/serf/command/agent/agent.go b/cmd/serf/command/agent/agent.go index ca16c2322..c35d13e36 100644 --- a/cmd/serf/command/agent/agent.go +++ b/cmd/serf/command/agent/agent.go @@ -8,7 +8,6 @@ import ( "io/ioutil" "log" "os" - "regexp" "strings" "sync" @@ -86,18 +85,6 @@ func Create(agentConf *Config, conf *serf.Config, logOutput io.Writer) (*Agent, } } - if agentConf.ValidateNodeNames { - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - if InvalidNameRe.MatchString(agentConf.NodeName) { - return nil, fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ - "all alpha-numerics and dashes.", agentConf.NodeName) - } - if len(agentConf.NodeName) > serf.MaxNodeNameLength { - return nil, fmt.Errorf("NodeName is %v characters. "+ - "Valid length is between 1 and 128 characters", len(agentConf.NodeName)) - } - } - return agent, nil } diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index 7d2e57254..d995c92f9 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -8,7 +8,6 @@ import ( "net" "os" "path/filepath" - "regexp" "sort" "strings" "time" @@ -349,21 +348,6 @@ func DecodeConfig(r io.Reader) (*Config, error) { result.BroadcastTimeout = dur } - if result.NodeName != "" && result.ValidateNodeNames { - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - if InvalidNameRe.MatchString(result.NodeName) { - err = fmt.Errorf("NodeName contains invalid characters %v , Valid characters include "+ - "all alpha-numerics and dashes.", result.NodeName) - return nil, err - } - - if len(result.NodeName) > serf.MaxNodeNameLength { - err = fmt.Errorf("NodeName is %v characters. "+ - "Valid length is between 1 and 128 characters", len(result.NodeName)) - return nil, err - } - } - return &result, nil } diff --git a/cmd/serf/command/agent/config_test.go b/cmd/serf/command/agent/config_test.go index b9e894834..7aa62064f 100644 --- a/cmd/serf/command/agent/config_test.go +++ b/cmd/serf/command/agent/config_test.go @@ -585,16 +585,3 @@ func TestReadConfigPaths_dir(t *testing.T) { t.Fatalf("bad: %#v", config) } } - -func TestBadNodeName(t *testing.T) { - input := `{"node_name": "9b07b1d05b25d78a3f55d0ccde714f02 - 96230786636dd72ded4c731746517331 - 394b395a0524b495d40fdd20ad9e536c - d15c8a7872ca8617226f02c175dfc0ff - dff8012a7c25e608e4775107fd2a3aeb"}` - _, err := DecodeConfig(bytes.NewReader([]byte(input))) - if err == nil { - t.Fatalf("should have err") - } - -} diff --git a/serf/config.go b/serf/config.go index dc397cfbb..ad4f4c1a2 100644 --- a/serf/config.go +++ b/serf/config.go @@ -9,8 +9,6 @@ import ( "github.com/hashicorp/memberlist" ) -const MaxNodeNameLength int = 128 - // ProtocolVersionMap is the mapping of Serf delegate protocol versions // to memberlist protocol versions. We mask the memberlist protocols using // our own protocol version. diff --git a/serf/delegate_test.go b/serf/delegate_test.go index 641df4be3..8c73b3209 100644 --- a/serf/delegate_test.go +++ b/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) +} diff --git a/serf/merge_delegate.go b/serf/merge_delegate.go index 17a0ef323..dbcf00241 100644 --- a/serf/merge_delegate.go +++ b/serf/merge_delegate.go @@ -3,7 +3,7 @@ package serf import ( "fmt" "net" - "regexp" + "strconv" "github.com/hashicorp/memberlist" ) @@ -61,23 +61,28 @@ func (m *mergeDelegate) nodeToMember(n *memberlist.Node) (*Member, error) { // validateMemberInfo checks that the data we are sending is valid func (m *mergeDelegate) validiateMemberInfo(n *memberlist.Node) error { - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) - var InvalidIPv4Re = regexp.MustCompile(`([0-9]{1,3}\.){3}[0-9]{1,3}:[0-9]+`) - var InvalidIPv6Re = regexp.MustCompile(`([0-9a-f]{1,4}:){7}[0-9a-f]{1,4}`) - if m.serf.config.ValidateNodeNames { - if len(n.Name) > 128 { - return fmt.Errorf("NodeName length is %v characters. Valid length is between "+ - "1 and 128 characters.", len(n.Name)) - } - if InvalidNameRe.MatchString(n.Name) { - return fmt.Errorf("Nodename contains invalid characters %v , Valid characters include "+ - "all alpha-numerics and dashes", n.Name) - } + if err := m.serf.ValidateNodeNames(); err != nil { + return err + } + + host, port, err := net.SplitHostPort(string(n.Addr)) + if err != nil { + return err } - if InvalidIPv4Re.MatchString(string(n.Addr)) || InvalidIPv6Re.MatchString(string(n.Addr)) { - return fmt.Errorf("Address is %v . Must be a valid representation of an IP address. ", n.Addr) + 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) diff --git a/serf/serf.go b/serf/serf.go index c04719082..ab443bba5 100644 --- a/serf/serf.go +++ b/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 checks the ValidateNodeNames config flag to be true +// then runs validations +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.", 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 +} From 3f452221319e59a82b12e3ef37c88f34415c68a3 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 14:55:36 -0500 Subject: [PATCH 08/12] fix regex, move test --- serf/serf.go | 4 ++-- serf/serf_test.go | 28 ++++++++++++++++++++++++++++ 2 files changed, 30 insertions(+), 2 deletions(-) diff --git a/serf/serf.go b/serf/serf.go index ab443bba5..0c5a00c27 100644 --- a/serf/serf.go +++ b/serf/serf.go @@ -1891,11 +1891,11 @@ func (s *Serf) NumNodes() (numNodes int) { return numNodes } -// ValidateNodeNames checks the ValidateNodeNames config flag to be true +// ValidateNodeNames checks the ValidateNodeNames flag to be true // then runs validations func (s *Serf) ValidateNodeNames() error { if s.config.ValidateNodeNames { - var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\\-]+`) + 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.", s.config.NodeName) diff --git a/serf/serf_test.go b/serf/serf_test.go index cb5676078..2a4159313 100644 --- a/serf/serf_test.go +++ b/serf/serf_test.go @@ -2971,3 +2971,31 @@ func waitUntilIntentQueueLen(t *testing.T, desiredLen int, serfs ...*Serf) { } }) } + +func TestAgentCreate_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) + } + } + +} From 47b6615ec549c49a9048e93f1cc0095c860a6a2a Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 15:13:31 -0500 Subject: [PATCH 09/12] failing test? just remove it --- cmd/serf/command/agent/agent_test.go | 27 --------------------------- 1 file changed, 27 deletions(-) diff --git a/cmd/serf/command/agent/agent_test.go b/cmd/serf/command/agent/agent_test.go index e1dd198f1..7f6e65558 100644 --- a/cmd/serf/command/agent/agent_test.go +++ b/cmd/serf/command/agent/agent_test.go @@ -318,30 +318,3 @@ func TestAgentKeyringFile_NoKeys(t *testing.T) { t.Fatalf("bad: %s", err) } } -func TestAgentCreate_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, serf.DefaultConfig(), nil) - if !strings.Contains(err.Error(), tc.want) { - t.Fatalf("expected: %v, got: %v", tc.want, err) - } - } - -} From 00e98d97528871d3f517f4fa17b570bbca8087aa Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 15:18:36 -0500 Subject: [PATCH 10/12] cleanupcomments --- serf/serf.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/serf/serf.go b/serf/serf.go index 0c5a00c27..1a62b3cab 100644 --- a/serf/serf.go +++ b/serf/serf.go @@ -1891,8 +1891,8 @@ func (s *Serf) NumNodes() (numNodes int) { return numNodes } -// ValidateNodeNames checks the ValidateNodeNames flag to be true -// then runs validations +// ValidateNodeNames checks the similarly named flag and does +// alphanumeric and length checks on NodeNames func (s *Serf) ValidateNodeNames() error { if s.config.ValidateNodeNames { var InvalidNameRe = regexp.MustCompile(`[^A-Za-z0-9\-\.\\]+`) From ae7dc03af573c98f909ed6a8db49cd749f4535d7 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 15:35:23 -0500 Subject: [PATCH 11/12] clean up comments and test func --- cmd/serf/command/agent/config.go | 8 +++----- serf/config.go | 5 +++-- serf/serf.go | 8 ++++---- serf/serf_test.go | 2 +- 4 files changed, 11 insertions(+), 12 deletions(-) diff --git a/cmd/serf/command/agent/config.go b/cmd/serf/command/agent/config.go index d995c92f9..f7f95922b 100644 --- a/cmd/serf/command/agent/config.go +++ b/cmd/serf/command/agent/config.go @@ -228,8 +228,9 @@ type Config struct { BroadcastTimeoutRaw string `mapstructure:"broadcast_timeout"` BroadcastTimeout time.Duration `mapstructure:"-"` - //ValidateNodeNames specifies whether or not nodenames should - // be alphanumeric and within 128 characters + // ValidateNodeNames controls whether nodenames only + // contain alphanumeric, dashes and '.'characters + // and sets maximum length to 128 characters ValidateNodeNames bool `mapstructure:"validate_node_names"` } @@ -367,9 +368,6 @@ func containsKey(keys []string, key string) bool { func MergeConfig(a, b *Config) *Config { var result Config = *a - //TODO(schristoff): do i need to check nodename - //validatity here? - if b.NodeName != "" { result.NodeName = b.NodeName } diff --git a/serf/config.go b/serf/config.go index ad4f4c1a2..a5a633423 100644 --- a/serf/config.go +++ b/serf/config.go @@ -254,8 +254,9 @@ type Config struct { // WARNING: this should ONLY be used in tests messageDropper func(typ messageType) bool - //ValidateNodeNames specifies whether or not nodenames should - // be alphanumeric and within 128 characters + // ValidateNodeNames controls whether nodenames only + // contain alphanumeric, dashes and '.'characters + // and sets maximum length to 128 characters ValidateNodeNames bool } diff --git a/serf/serf.go b/serf/serf.go index 1a62b3cab..9a9d98346 100644 --- a/serf/serf.go +++ b/serf/serf.go @@ -1891,14 +1891,14 @@ func (s *Serf) NumNodes() (numNodes int) { return numNodes } -// ValidateNodeNames checks the similarly named flag and does -// alphanumeric and length checks on NodeNames +// 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\-\.\\]+`) + 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.", s.config.NodeName) + "all alpha-numerics and dashes and '.' ", s.config.NodeName) } if len(s.config.NodeName) > MaxNodeNameLength { return fmt.Errorf("NodeName is %v characters. "+ diff --git a/serf/serf_test.go b/serf/serf_test.go index 2a4159313..aae7f7f2d 100644 --- a/serf/serf_test.go +++ b/serf/serf_test.go @@ -2972,7 +2972,7 @@ func waitUntilIntentQueueLen(t *testing.T, desiredLen int, serfs ...*Serf) { }) } -func TestAgentCreate_ValidateNodeName(t *testing.T) { +func TestSerf_ValidateNodeName(t *testing.T) { type test struct { nodename string want string From 345766712bcec56148021d94e42de64795e74ec7 Mon Sep 17 00:00:00 2001 From: Sarah Christoff Date: Mon, 24 Aug 2020 16:00:48 -0500 Subject: [PATCH 12/12] set flag to default to false --- serf/config.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/serf/config.go b/serf/config.go index a5a633423..37b55f74f 100644 --- a/serf/config.go +++ b/serf/config.go @@ -303,7 +303,7 @@ func DefaultConfig() *Config { QuerySizeLimit: 1024, EnableNameConflictResolution: true, DisableCoordinates: false, - ValidateNodeNames: true, + ValidateNodeNames: false, UserEventSizeLimit: 512, } }