Просмотр исходного кода

Warn on int and bool group names, reject floats and dates

JackDoan 1 день назад
Родитель
Сommit
aa880d85b6
3 измененных файлов с 36 добавлено и 17 удалено
  1. 3 2
      CHANGELOG.md
  2. 28 14
      firewall.go
  3. 5 1
      firewall_test.go

+ 3 - 2
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
 

+ 28 - 14
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 "<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) {
+// 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 "<nil>" 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

+ 5 - 1
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}}}}