improvements to config parsing

firewall/config.go

@@ -159,6 +159,9 @@ func yamlToConfigRule(l *slog.Logger, p any, table string, i int) (configRule, e
 		if len(v) > 1 {
 			return r, errors.New("group should contain a single value, an array with more than one entry was provided")
 		}
+		if len(v) == 0 {
+			return r, errors.New("group should contain a single value, an empty array was provided")
+		}
 
 		l.Warn("group was an array with a single value, converting to simple value",
 			"table", table,
@@ -169,13 +172,15 @@ func yamlToConfigRule(l *slog.Logger, p any, table string, i int) (configRule, e
 
 	singleGroup := toString("group", m)
 
-	if rg, ok := m["groups"]; ok {
+	// A null value, `groups:` with nothing after it, is the same as no groups
+	if rg, ok := m["groups"]; ok && rg != nil {
 		switch reflect.TypeOf(rg).Kind() {
 		case reflect.Slice:
+			// Every entry is read as a string, as the other fields are, so `groups: [1, 2]` is the groups "1" and "2"
 			v := reflect.ValueOf(rg)
 			r.Groups = make([]string, v.Len())
 			for i := 0; i < v.Len(); i++ {
-				r.Groups[i] = v.Index(i).Interface().(string)
+				r.Groups[i] = fmt.Sprintf("%v", v.Index(i).Interface())
 			}
 		case reflect.String:
 			r.Groups = []string{rg.(string)}

firewall/config_test.go

@@ -291,6 +291,26 @@ func TestAddRulesFromConfig(t *testing.T) {
 	require.NoError(t, addRulesFromConfig(l, true, conf, mf))
 	assert.Equal(t, addRuleCall{incoming: true, proto: ProtoAny, startPort: 1, endPort: 1, groups: []string{"a", "b"}, ip: "", localIp: ""}, mf.lastCall)
 
+	// Test group and groups values that used to crash: an empty group array is rejected, groups entries that
+	// aren't strings are read as strings, and a null groups is no groups
+	conf = config.NewC(test.NewLogger())
+	mf = &mockFirewall{}
+	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "group": []any{}}}}
+	require.EqualError(t, addRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; group should contain a single value, an empty array was provided")
+
+	conf = config.NewC(test.NewLogger())
+	mf = &mockFirewall{}
+	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "groups": []any{1, 2}}}}
+	require.NoError(t, addRulesFromConfig(l, true, conf, mf))
+	assert.Equal(t, []string{"1", "2"}, mf.lastCall.groups)
+
+	conf = config.NewC(test.NewLogger())
+	mf = &mockFirewall{}
+	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "groups": nil, "host": "a"}}}
+	require.NoError(t, addRulesFromConfig(l, true, conf, mf))
+	assert.Empty(t, mf.lastCall.groups)
+	assert.Equal(t, "a", mf.lastCall.host)
+
 	// Test Add error
 	conf = config.NewC(test.NewLogger())
 	mf = &mockFirewall{}

firewall/rules_builder.go

@@ -110,6 +110,11 @@ func parseRule(proto uint8, startPort, endPort int32, groups []string, host, cid
 	if startPort <= PortAny && PortAny <= endPort {
 		startPort, endPort = PortAny, PortAny
 	}
+	// A port rule on a protocol without ports could never match. ICMP rules were coerced to `any` by AddRule,
+	// and a proto `any` rule with a port applies to the protocols that have them.
+	if startPort > PortAny && proto != ProtoAny && !iputil.HasPorts(proto) {
+		return rule{}, fmt.Errorf("protocol %d has no ports, port must be any or fragment", proto)
+	}
 
 	r := rule{
 		proto:     proto,
@@ -214,7 +219,8 @@ func (pi *protoIndex) add(r *rule, id int) {
 	case PortFragment:
 		pi.fragment.add(id)
 	default:
-		// A range only applies to a protocol with ports.
+		// A proto `any` rule with a port is indexed for the protocols that have ports, parseRule rejects a port on
+		// a protocol without them.
 		if pi.hasPorts {
 			pi.addPorts(int(r.startPort), int(r.endPort), id)
 		}

firewall/rules_builder_test.go

@@ -223,8 +223,9 @@ func TestRulesBuilder_protoIndex(t *testing.T) {
 	require.NoError(t, rb.AddRule(true, iputil.IPProtocolTCP, PortFragment, PortFragment, nil, "fragment", "", "", "", ""))
 	// A range that includes port 0 is port `any`.
 	require.NoError(t, rb.AddRule(true, iputil.IPProtocolUDP, 0, 100, nil, "zero", "", "", "", ""))
-	// GRE has no ports, so a port rule on it never applies.
-	require.NoError(t, rb.AddRule(true, 47, 22, 22, nil, "gre", "", "", "", ""))
+	// GRE has no ports, so a port rule on it is rejected rather than never applying, and `any` and `fragment` apply
+	require.Error(t, rb.AddRule(true, 47, 22, 22, nil, "gre", "", "", "", ""))
+	require.NoError(t, rb.AddRule(true, 47, PortFragment, PortFragment, nil, "gre", "", "", "", ""))
 	rules := rb.Build(nil, nil)
 
 	hosts := func(proto uint8, key int32) []string {
@@ -252,7 +253,7 @@ func TestRulesBuilder_protoIndex(t *testing.T) {
 	assert.False(t, rules.In.protos[47].hasPorts)
 	assert.Equal(t, []string{"any"}, hosts(47, PortAny))
 	assert.Equal(t, []string{"any"}, hosts(47, 22))
-	assert.Equal(t, []string{"any"}, hosts(47, PortFragment))
+	assert.Equal(t, []string{"any", "gre"}, hosts(47, PortFragment))
 	assert.Equal(t, []string{"any"}, hosts(iputil.IPProtocolICMP, PortFragment))
 
 	// Protocols without rules of their own share an index of the proto `any` rules: one for protocols with

firewall/table.go

@@ -32,7 +32,9 @@ type Table struct {
 	protos [256]*protoIndex
 }
 
-// protoIndex holds the rules that can apply to one protocol, in a set for each kind of packet.
+// protoIndex holds the rules that can apply to one protocol, in a set for each kind of packet. Rule ids are
+// indexes into the direction's rules, so every set is sized by the direction's rule count, not the protocol's:
+// byPort costs 64 KB per 8 rules in the direction.
 type protoIndex struct {
 	// hasPorts reports whether the protocol has ports; see iputil.HasPorts.
 	hasPorts bool