From 47b063a50f7cde08c236fcea206d5817adcd40ef Mon Sep 17 00:00:00 2001 From: Kristoffer Dalby Date: Fri, 12 Sep 2025 12:08:27 +0200 Subject: [PATCH] policy: no * proto, empty proto is tcp,udp Signed-off-by: Kristoffer Dalby --- hscontrol/policy/policy_test.go | 11 ++++++ hscontrol/policy/v2/filter_test.go | 6 +++ hscontrol/policy/v2/types.go | 47 ++++++++++++++++++----- hscontrol/policy/v2/types_test.go | 61 ++++++++++++++++++++++++++++-- 4 files changed, 113 insertions(+), 12 deletions(-) diff --git a/hscontrol/policy/policy_test.go b/hscontrol/policy/policy_test.go index 353fd2c1..55fe56a1 100644 --- a/hscontrol/policy/policy_test.go +++ b/hscontrol/policy/policy_test.go @@ -222,6 +222,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, { SrcIPs: []string{ @@ -236,6 +237,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, }, }, @@ -371,10 +373,12 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, { SrcIPs: []string{"100.64.0.1/32", "100.64.0.2/32", "fd7a:115c:a1e0::1/128", "fd7a:115c:a1e0::2/128"}, DstPorts: hsExitNodeDestForTest, + IPProto: []int{6, 17}, }, }, }, @@ -478,6 +482,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, { SrcIPs: []string{"100.64.0.1/32", "100.64.0.2/32", "fd7a:115c:a1e0::1/128", "fd7a:115c:a1e0::2/128"}, @@ -513,6 +518,7 @@ func TestReduceFilterRules(t *testing.T) { {IP: "200.0.0.0/5", Ports: tailcfg.PortRangeAny}, {IP: "208.0.0.0/4", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{6, 17}, }, }, }, @@ -588,6 +594,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, { SrcIPs: []string{"100.64.0.1/32", "100.64.0.2/32", "fd7a:115c:a1e0::1/128", "fd7a:115c:a1e0::2/128"}, @@ -601,6 +608,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, }, }, @@ -676,6 +684,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, { SrcIPs: []string{"100.64.0.1/32", "100.64.0.2/32", "fd7a:115c:a1e0::1/128", "fd7a:115c:a1e0::2/128"}, @@ -689,6 +698,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, }, }, @@ -756,6 +766,7 @@ func TestReduceFilterRules(t *testing.T) { Ports: tailcfg.PortRangeAny, }, }, + IPProto: []int{6, 17}, }, }, }, diff --git a/hscontrol/policy/v2/filter_test.go b/hscontrol/policy/v2/filter_test.go index 12c60fbb..cc29e1cd 100644 --- a/hscontrol/policy/v2/filter_test.go +++ b/hscontrol/policy/v2/filter_test.go @@ -86,6 +86,7 @@ func TestParsing(t *testing.T) { {IP: "::/0", Ports: tailcfg.PortRange{First: 3389, Last: 3389}}, {IP: "100.100.100.100/32", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, @@ -187,6 +188,7 @@ func TestParsing(t *testing.T) { DstPorts: []tailcfg.NetPortRange{ {IP: "100.100.100.100/32", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, @@ -223,6 +225,7 @@ func TestParsing(t *testing.T) { Ports: tailcfg.PortRange{First: 5400, Last: 5500}, }, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, @@ -262,6 +265,7 @@ func TestParsing(t *testing.T) { DstPorts: []tailcfg.NetPortRange{ {IP: "100.100.100.100/32", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, @@ -295,6 +299,7 @@ func TestParsing(t *testing.T) { DstPorts: []tailcfg.NetPortRange{ {IP: "100.100.100.100/32", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, @@ -328,6 +333,7 @@ func TestParsing(t *testing.T) { DstPorts: []tailcfg.NetPortRange{ {IP: "100.100.100.100/32", Ports: tailcfg.PortRangeAny}, }, + IPProto: []int{protocolTCP, protocolUDP}, }, }, wantErr: false, diff --git a/hscontrol/policy/v2/types.go b/hscontrol/policy/v2/types.go index e840c5c6..69c4edf3 100644 --- a/hscontrol/policy/v2/types.go +++ b/hscontrol/policy/v2/types.go @@ -994,18 +994,46 @@ func (g Groups) Contains(group *Group) error { // that all group names conform to the expected format, which is always prefixed // with "group:". If any group name is invalid, an error is returned. func (g *Groups) UnmarshalJSON(b []byte) error { - var rawGroups map[string][]string - if err := json.Unmarshal(b, &rawGroups, policyJSONOpts...); err != nil { + // First unmarshal as a generic map to validate group names first + var rawMap map[string]interface{} + if err := json.Unmarshal(b, &rawMap); err != nil { return err } + // Validate group names first before checking data types + for key := range rawMap { + group := Group(key) + if err := group.Validate(); err != nil { + return err + } + } + + // Then validate each field can be converted to []string + rawGroups := make(map[string][]string) + for key, value := range rawMap { + switch v := value.(type) { + case []interface{}: + // Convert []interface{} to []string + var stringSlice []string + for _, item := range v { + if str, ok := item.(string); ok { + stringSlice = append(stringSlice, str) + } else { + return fmt.Errorf(`Group "%s" contains invalid member type, expected string but got %T`, key, item) + } + } + rawGroups[key] = stringSlice + case string: + return fmt.Errorf(`Group "%s" value must be an array of users, got string: "%s"`, key, v) + default: + return fmt.Errorf(`Group "%s" value must be an array of users, got %T`, key, v) + } + } + *g = make(Groups) for key, value := range rawGroups { group := Group(key) - if err := group.Validate(); err != nil { - return err - } - + // Group name already validated above var usernames Usernames for _, u := range value { @@ -1359,7 +1387,7 @@ func (p Protocol) Description() string { case ProtocolFC: return "Fibre Channel" case ProtocolWildcard: - return "Wildcard (all protocols)" + return "Wildcard (not supported - use specific protocol)" default: return "Unknown Protocol" } @@ -1370,9 +1398,10 @@ func (p Protocol) Description() string { func (p Protocol) parseProtocol() ([]int, bool) { switch p { case "": - return nil, false + // Empty protocol applies to TCP and UDP traffic only + return []int{protocolTCP, protocolUDP}, false case ProtocolWildcard: - // Wildcard protocol - allows all protocols like empty string + // Wildcard protocol - defensive handling (should not reach here due to validation) return nil, false case ProtocolIGMP: return []int{protocolIGMP}, true diff --git a/hscontrol/policy/v2/types_test.go b/hscontrol/policy/v2/types_test.go index 0d8c5059..449fe803 100644 --- a/hscontrol/policy/v2/types_test.go +++ b/hscontrol/policy/v2/types_test.go @@ -996,10 +996,34 @@ func TestUnmarshalPolicy(t *testing.T) { ] } `, - wantErr: `unknown field "BAD"`, + wantErr: `unknown field`, }, { - name: "disallow-unsupported-fields-groups-level", + name: "invalid-group-name", + input: ` +{ + "groups": { + "group:test": ["user@example.com"], + "INVALID_GROUP_FIELD": ["user@example.com"] + } +} +`, + wantErr: `Group has to start with "group:", got: "INVALID_GROUP_FIELD"`, + }, + { + name: "invalid-group-datatype", + input: ` +{ + "groups": { + "group:test": ["user@example.com"], + "group:invalid": "should fail" + } +} +`, + wantErr: `Group "group:invalid" value must be an array of users, got string: "should fail"`, + }, + { + name: "invalid-group-name-and-datatype-fails-on-name-first", input: ` { "groups": { @@ -1008,7 +1032,7 @@ func TestUnmarshalPolicy(t *testing.T) { } } `, - wantErr: `cannot unmarshal JSON string into Go []string`, + wantErr: `Group has to start with "group:", got: "INVALID_GROUP_FIELD"`, }, { name: "disallow-unsupported-fields-hosts-level", @@ -1255,6 +1279,37 @@ func TestUnmarshalPolicy(t *testing.T) { `, wantErr: `leading 0 not permitted in protocol number "0"`, }, + { + name: "protocol-empty-applies-to-tcp-udp-only", + input: ` +{ + "acls": [ + { + "action": "accept", + "src": ["*"], + "dst": ["*:80"] + } + ] +} +`, + want: &Policy{ + ACLs: []ACL{ + { + Action: "accept", + Protocol: "", + Sources: Aliases{ + Wildcard, + }, + Destinations: []AliasWithPorts{ + { + Alias: Wildcard, + Ports: []tailcfg.PortRange{{First: 80, Last: 80}}, + }, + }, + }, + }, + }, + }, { name: "protocol-icmp-with-specific-port-not-allowed", input: `