Reject null, array, and map group values instead of reading them as strings

CHANGELOG.md

@@ -14,8 +14,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
   pass attached one, and `create-tunnel -address` set the remote before that. The list is now attached when
   the handshake starts, so a pending hostinfo never exists without one.
 - A firewall rule with `group: []`, `groups:` with no value, or a `groups` entry that isn't a string, such as
-  `groups: [1, 2]`, no longer crashes Nebula on startup or reload. An empty `group` array is rejected, a null
-  `groups` means no groups, and non-string entries are read as strings.
+  `groups: [1, 2]`, no longer crashes Nebula on startup or reload. A null `group` or `groups` means none was
+  provided, numbers and other simple values are read as strings, and an empty array, a null entry, a nested
+  array, or a map is rejected with an error.
 
 ## [1.11.1] - 2026-08-21
 

firewall.go

@@ -967,38 +967,51 @@ func convertRule(l *slog.Logger, p any, table string, i int) (rule, error) {
 	r.CAName = toString("ca_name", m)
 	r.CASha = toString("ca_sha", m)
 
-	// Make sure group isn't an array
-	if v, ok := m["group"].([]any); ok {
-		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")
+	// A null value, `group:` with nothing after it, is the same as no group
+	var singleGroup string
+	if g, ok := m["group"]; ok && g != nil {
+		// Make sure group isn't an array
+		if v, ok := g.([]any); ok {
+			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,
+				"rule", i,
+			)
+			g = v[0]
 		}
 
-		l.Warn("group was an array with a single value, converting to simple value",
-			"table", table,
-			"rule", i,
-		)
-		m["group"] = v[0]
+		var err error
+		singleGroup, err = groupName(g)
+		if err != nil {
+			return r, fmt.Errorf("group should contain a single value, %w", err)
+		}
 	}
 
-	singleGroup := toString("group", m)
-
 	// 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] = fmt.Sprintf("%v", v.Index(i).Interface())
+			for gi := 0; gi < v.Len(); gi++ {
+				name, err := groupName(v.Index(gi).Interface())
+				if err != nil {
+					return r, fmt.Errorf("groups entry #%d should contain a single value, %w", gi, err)
+				}
+				r.Groups[gi] = name
 			}
-		case reflect.String:
-			r.Groups = []string{rg.(string)}
 		default:
-			r.Groups = []string{fmt.Sprintf("%v", rg)}
+			name, err := groupName(rg)
+			if err != nil {
+				return r, fmt.Errorf("groups should contain an array or a single value, %w", err)
+			}
+			r.Groups = []string{name}
 		}
 	}
 
@@ -1014,6 +1027,22 @@ func convertRule(l *slog.Logger, p any, table string, i int) (rule, error) {
 	return r, nil
 }
 
+// groupName reads a group name from config. A scalar is read as a string, as the other rule fields are, so `1` is the
+// group "1". A null, array, or map would only become a name that matches nothing, like "<nil>" or "map[a:b]", so
+// those are rejected. A group really named one of those can be quoted.
+func groupName(v any) (string, error) {
+	if v == nil {
+		return "", errors.New("a null was provided")
+	}
+	switch reflect.TypeOf(v).Kind() {
+	case reflect.Slice, reflect.Array:
+		return "", errors.New("an array was provided")
+	case reflect.Map:
+		return "", errors.New("a map was provided")
+	}
+	return fmt.Sprintf("%v", v), nil
+}
+
 // sanity returns an error if the rule would be evaluated in a way that would short-circuit a configured check on a wildcard value
 // rules are evaluated as "port AND proto AND (ca_sha OR ca_name) AND (host OR group OR groups OR cidr) AND local_cidr"
 func (r *rule) sanity() error {

firewall_test.go

@@ -1439,25 +1439,51 @@ func TestAddFirewallRulesFromConfig(t *testing.T) {
 	require.NoError(t, AddFirewallRulesFromConfig(l, true, conf, mf))
 	assert.Equal(t, addRuleCall{incoming: true, proto: firewall.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, AddFirewallRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; group should contain a single value, an empty array was provided")
-
+	// Test group and groups values that used to crash or become a group that matches nothing: an empty array,
+	// a null, an array, or a map is rejected
+	badGroups := []struct {
+		key   string
+		value any
+		err   string
+	}{
+		{"group", []any{}, "group should contain a single value, an empty array was provided"},
+		{"group", []any{nil}, "group should contain a single value, a null was provided"},
+		{"group", []any{[]any{"a"}}, "group should contain a single value, an array was provided"},
+		{"group", map[string]any{"a": "b"}, "group should contain a single value, a map was provided"},
+		{"groups", []any{nil}, "groups entry #0 should contain a single value, a null was provided"},
+		{"groups", []any{"a", []any{"b"}}, "groups entry #1 should contain a single value, an array was provided"},
+		{"groups", []any{map[string]any{"a": "b"}}, "groups entry #0 should contain a single value, a map was provided"},
+		{"groups", map[string]any{"a": "b"}, "groups should contain an array or a single value, a map was provided"},
+	}
+	for _, tc := range badGroups {
+		conf = config.NewC(test.NewLogger())
+		mf = &mockFirewall{}
+		conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", tc.key: tc.value}}}
+		require.EqualError(t, AddFirewallRulesFromConfig(l, true, conf, mf), "firewall.inbound rule #0; "+tc.err, "%s: %v", tc.key, tc.value)
+	}
+
+	// Test scalar groups entries that aren't strings are read as strings
 	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}}}}
+	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "groups": []any{1, true}}}}
 	require.NoError(t, AddFirewallRulesFromConfig(l, true, conf, mf))
-	assert.Equal(t, []string{"1", "2"}, mf.lastCall.groups)
+	assert.Equal(t, []string{"1", "true"}, 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"}}}
+	conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", "group": 1}}}
 	require.NoError(t, AddFirewallRulesFromConfig(l, true, conf, mf))
-	assert.Empty(t, mf.lastCall.groups)
-	assert.Equal(t, "a", mf.lastCall.host)
+	assert.Equal(t, []string{"1"}, mf.lastCall.groups)
+
+	// Test a null group or groups is the same as not providing it
+	for _, key := range []string{"group", "groups"} {
+		conf = config.NewC(test.NewLogger())
+		mf = &mockFirewall{}
+		conf.Settings["firewall"] = map[string]any{"inbound": []any{map[string]any{"port": "1", "proto": "any", key: nil, "host": "a"}}}
+		require.NoError(t, AddFirewallRulesFromConfig(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())