From aa880d85b666988f265024599e17f5bd5a9fe61b Mon Sep 17 00:00:00 2001 From: JackDoan Date: Mon, 19 Oct 2026 15:24:24 -0500 Subject: [PATCH] Warn on int and bool group names, reject floats and dates --- CHANGELOG.md | 5 +++-- firewall.go | 42 ++++++++++++++++++++++++++++-------------- firewall_test.go | 6 +++++- 3 files changed, 36 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f075aea5..83ba6022 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,8 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 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. 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. + provided. An unquoted int or bool group, such as `group: 7`, is still read as a string but logs a warning to + quote it, since YAML has already turned a value like `007` into `7`. A float, a date, 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 63ff3bfd..9626dc7d 100644 --- a/firewall.go +++ b/firewall.go @@ -971,26 +971,29 @@ func convertRule(l *slog.Logger, p any, table string, i int) (rule, error) { var singleGroup string if g, ok := m["group"]; ok && g != nil { // Make sure group isn't an array - if v, ok := g.([]any); ok { + v, isArray := g.([]any) + if isArray { 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] } var err error - singleGroup, err = groupName(g) + singleGroup, err = groupName(l, g, table, i) if err != nil { return r, fmt.Errorf("group should contain a single value, %w", err) } + + if isArray { + l.Warn("group was an array with a single value, converting to simple value", + "table", table, + "rule", i, + ) + } } // A null value, `groups:` with nothing after it, is the same as no groups @@ -1000,14 +1003,14 @@ func convertRule(l *slog.Logger, p any, table string, i int) (rule, error) { v := reflect.ValueOf(rg) r.Groups = make([]string, v.Len()) for gi := 0; gi < v.Len(); gi++ { - name, err := groupName(v.Index(gi).Interface()) + name, err := groupName(l, v.Index(gi).Interface(), table, i) if err != nil { return r, fmt.Errorf("groups entry #%d should contain a single value, %w", gi, err) } r.Groups[gi] = name } default: - name, err := groupName(rg) + name, err := groupName(l, rg, table, i) if err != nil { return r, fmt.Errorf("groups should contain an array or a single value, %w", err) } @@ -1027,20 +1030,31 @@ 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) { +// groupName reads a group name from config. YAML has already parsed an unquoted value by the time we see it, so `007` +// arrives as 7, `1.10` as 1.1, and a date as a time.Time, and the text that was written is gone. Strings are used as +// is. Ints and bools are converted so `group: 7` keeps working, with a warning to quote them. Anything else would +// become a name that may not match what was written, or matches nothing like "" or "map[a:b]", so it is rejected. +func groupName(l *slog.Logger, v any, table string, i int) (string, error) { if v == nil { return "", errors.New("a null was provided") } switch reflect.TypeOf(v).Kind() { + case reflect.String: + return reflect.ValueOf(v).String(), nil + case reflect.Int, reflect.Int8, reflect.Int16, reflect.Int32, reflect.Int64, + reflect.Uint, reflect.Uint8, reflect.Uint16, reflect.Uint32, reflect.Uint64, reflect.Bool: + l.Warn("group was not a string and should be quoted to match as written", + "table", table, + "rule", i, + "group", v, + ) + return fmt.Sprintf("%v", v), nil 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 + return "", fmt.Errorf("%v was read as a %T, quote it to use it as a group name", v, v) } // sanity returns an error if the rule would be evaluated in a way that would short-circuit a configured check on a wildcard value diff --git a/firewall_test.go b/firewall_test.go index 26556203..f4a1df1b 100644 --- a/firewall_test.go +++ b/firewall_test.go @@ -1454,6 +1454,10 @@ func TestAddFirewallRulesFromConfig(t *testing.T) { {"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"}, + {"group", 1.1, "group should contain a single value, 1.1 was read as a float64, quote it to use it as a group name"}, + {"group", []any{1.1}, "group should contain a single value, 1.1 was read as a float64, quote it to use it as a group name"}, + {"groups", []any{"a", 1.1}, "groups entry #1 should contain a single value, 1.1 was read as a float64, quote it to use it as a group name"}, + {"groups", time.Date(2001, 12, 14, 0, 0, 0, 0, time.UTC), "groups should contain an array or a single value, 2001-12-14 00:00:00 +0000 UTC was read as a time.Time, quote it to use it as a group name"}, } for _, tc := range badGroups { conf = config.NewC(test.NewLogger()) @@ -1462,7 +1466,7 @@ func TestAddFirewallRulesFromConfig(t *testing.T) { 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 + // Test int and bool groups entries 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, true}}}}