diff --git a/CHANGELOG.md b/CHANGELOG.md index ed089185..f08d7431 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,23 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- Firewall rules are held as a flat list with a bitset of rule ids per port, instead of a tree of maps copied + into every port of a range. A rule like `port: 1024-65535` now costs kilobytes rather than tens of megabytes, + and matching is faster on every path. +- A firewall rule's `proto` can be `udplite`, `dccp`, `sctp`, `icmpv6`, or any IP protocol number. A rule with + a port or port range on a protocol that has no ports, such as `proto: 47`, is rejected rather than silently + never matching. `port: any` and `port: fragment` apply to every protocol. +- An ICMP non-first fragment is matched against `port: fragment` rules like a fragment of any other protocol. + It was only matched against `port: any` rules before. + +### Fixed + +- 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. + ## [1.11.1] - 2026-08-21 See the [v1.11.1](https://github.com/slackhq/nebula/milestone/30?closed=1) milestone for a complete list of changes. diff --git a/firewall/config.go b/firewall/config.go index 9e5b76e9..92ba7468 100644 --- a/firewall/config.go +++ b/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)} diff --git a/firewall/config_test.go b/firewall/config_test.go index f22e7226..8ce7a63c 100644 --- a/firewall/config_test.go +++ b/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{} diff --git a/firewall/rules_builder.go b/firewall/rules_builder.go index 1bc624c2..fe186583 100644 --- a/firewall/rules_builder.go +++ b/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) } diff --git a/firewall/rules_builder_test.go b/firewall/rules_builder_test.go index 6e413693..6da7d4dd 100644 --- a/firewall/rules_builder_test.go +++ b/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 diff --git a/firewall/table.go b/firewall/table.go index f3e1e0f8..317ea98c 100644 --- a/firewall/table.go +++ b/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