diff --git a/CHANGELOG.md b/CHANGELOG.md index 3539a3ab..f075aea5 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/firewall.go b/firewall.go index b1950123..63ff3bfd 100644 --- a/firewall.go +++ b/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 "" 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 { diff --git a/firewall_test.go b/firewall_test.go index 5915e6e5..26556203 100644 --- a/firewall_test.go +++ b/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 + // 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", "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") + 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", "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": []any{1, 2}}}} + 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.Equal(t, []string{"1", "2"}, mf.lastCall.groups) + assert.Equal(t, []string{"1"}, 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, AddFirewallRulesFromConfig(l, true, conf, mf)) - assert.Empty(t, mf.lastCall.groups) - assert.Equal(t, "a", mf.lastCall.host) + // 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())